Implement MegaETH Bridge [MegaETHBridgeFacet v1.0.0] #1429
Conversation
WalkthroughThis PR introduces a new MegaETH Bridge facet implementation enabling token bridging via the MegaETH Bridge. The changes include a core Solidity contract, deployment and update scripts, configuration file, documentation, and comprehensive test coverage. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The review demands careful analysis of the core contract's bridging logic, multiple execution pathways (direct vs. swap-and-bridge), initialization patterns with config parsing, integration with external bridge interfaces, script logic, and test coverage. The heterogeneous changes across contract, deployment, and test files require separate reasoning for each area. Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
lifi-action-bot
changed the title
Test Coverage ReportLine Coverage: 86.24% (2746 / 3184 lines) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (8)
docs/MegaETHBridgeFacet.md (2)
5-11: Hyphenate compound modifiers and tighten wording.Use “Bridge‑specific” and “DELEGATECALL → MegaETHBridgeFacet; MegaETHBridgeFacet → MegaETHBridge” to improve clarity. Also fix “contract” typo if present.
57-58: Use descriptive link text and correct relative paths.Replace “here” with the target names and ensure paths resolve from docs/:
-The swap library can be found [here](../src/Libraries/LibSwap.sol). +See LibSwap.sol (swap library) at ../src/Libraries/LibSwap.sol. -`BridgeData` and the events we can emit can be found [here](../src/Interfaces/ILiFi.sol). +See ILiFi.sol (BridgeData and events) at ../src/Interfaces/ILiFi.sol. -A detailed explanation ... can be found [here](https://apidocs.li.finance/reference/how-to-transfer-tokens). +See LiFi API guide: How to transfer tokens — https://apidocs.li.finance/reference/how-to-transfer-tokensAlso applies to: 63-64, 94-94
test/solidity/Facets/MegaETHBridgeFacet.t.sol (3)
47-66: Label key addresses in setUp() for traceability.Add vm.label for tokens, holders, bridges, router, and diamond to ease debugging.
function setUp() public { customBlockNumberForForking = 15876510; initTestBase(); + vm.label(USDC_ADDRESS, "USDC"); + vm.label(USDC_HOLDER, "USDC_HOLDER"); + vm.label(DAI_L1_ADDRESS, "DAI_L1"); + vm.label(DAI_L1_HOLDER, "DAI_L1_HOLDER"); + vm.label(DAI_L2_ADDRESS, "DAI_L2"); + vm.label(STANDARD_BRIDGE, "STANDARD_BRIDGE"); + vm.label(DAI_BRIDGE, "DAI_L1_BRIDGE"); + vm.label(UNISWAP_V2_ROUTER, "UNISWAP_V2_ROUTER"); + vm.label(address(diamond), "LiFiDiamond");Also applies to: 86-109
206-218: Assert expected event(s) on successful ERC20 bridge.Add vm.expectEmit(...) to verify the MegaETH/bridge event and parameters before calling startBridgeTokensViaMegaETHBridge. This ensures we don’t silently pass if nothing happens.
Would you like me to draft the exact expectEmit with the event signature from MegaETHBridgeFacet or IL1StandardBridge?
220-266: Swap+bridge test: assert swap outcome and emitted events.
- Capture balances before/after or assert emitted swap/bridge events.
- Consider a tighter deadline and slippage for determinism on forked block 15,876,510.
script/deploy/facets/UpdateMegaETHBridgeFacet.s.sol (1)
30-51: Confirm stdJson decoding and network key exist; add basic sanity checks.
- Ensure config contains the current network key (e.g., “mainnet”) before decoding.
- Optionally assert configs.length > 0 and standardBridge != address(0).
function getCallData() internal override returns (bytes memory) { path = string.concat(root, "/config/megaeth.json"); json = vm.readFile(path); + require(bytes(network).length != 0, "network not set"); + require(json.parseRaw(string.concat(".", network)).length != 0, "network missing in config"); address standardBridge = _getConfigContractAddress( path, string.concat(".", network, ".standardBridge") ); + require(standardBridge != address(0), "standardBridge=0"); bytes memory rawConfig = json.parseRaw( string.concat(".", network, ".tokens") ); Config[] memory configs = abi.decode(rawConfig, (Config[])); + require(configs.length != 0, "empty tokens config");If this pattern differs from other facets’ update scripts in-repo, please align. Want me to scan for them and match style?
src/Facets/MegaETHBridgeFacet.sol (2)
63-71: Optional gas micro-optimization in loop.Use unchecked increment to save gas.
Example:
for (uint256 i; i < _configs.length; ) { // body unchecked { ++i; } }
80-97: registerMegaETHBridge: underscore params; consider deregistration path.
- Follow underscore naming for params.
- Consider allowing clearing an override (or add a deregister function) so assets can fall back to standardBridge.
Apply this diff for naming:
-/// @param assetId Address of token -/// @param bridge Address of bridge for asset -function registerMegaETHBridge(address assetId, address bridge) external { +/// @param _assetId Address of token +/// @param _bridge Address of bridge for asset +function registerMegaETHBridge(address _assetId, address _bridge) external { LibDiamond.enforceIsContractOwner(); Storage storage s = getStorage(); if (!s.initialized) revert NotInitialized(); - if (bridge == address(0)) { + if (_bridge == address(0)) { revert InvalidConfig(); } - s.bridges[assetId] = IL1StandardBridge(bridge); + s.bridges[_assetId] = IL1StandardBridge(_bridge); - emit MegaETHBridgeRegistered(assetId, bridge); + emit MegaETHBridgeRegistered(_assetId, _bridge); }Based on coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
config/megaeth.json(1 hunks)docs/MegaETHBridgeFacet.md(1 hunks)script/deploy/facets/DeployMegaETHBridgeFacet.s.sol(1 hunks)script/deploy/facets/UpdateMegaETHBridgeFacet.s.sol(1 hunks)src/Facets/MegaETHBridgeFacet.sol(1 hunks)test/solidity/Facets/MegaETHBridgeFacet.t.sol(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
{src,test/solidity,script}/**/*.sol
📄 CodeRabbit inference engine (conventions.md)
{src,test/solidity,script}/**/*.sol: All Solidity files must start with a license identifier, followed by a blank line, then the pragma statement
Use SPDX license identifier // SPDX-License-Identifier: LGPL-3.0-only for all first-party Solidity files
Error names must be descriptive PascalCase, use custom errors instead of revert() strings, and omit error messages for gas optimization
Solidity variable/function naming: state variables camelCase; function params camelCase with leading underscore; constants and immutables in CONSTANT_CASE; private/internal variables camelCase; function names camelCase
Files:
script/deploy/facets/DeployMegaETHBridgeFacet.s.solsrc/Facets/MegaETHBridgeFacet.solscript/deploy/facets/UpdateMegaETHBridgeFacet.s.soltest/solidity/Facets/MegaETHBridgeFacet.t.sol
**/*.sol
📄 CodeRabbit inference engine (conventions.md)
Follow solhint rules as configured in .solhint.json
Files:
script/deploy/facets/DeployMegaETHBridgeFacet.s.solsrc/Facets/MegaETHBridgeFacet.solscript/deploy/facets/UpdateMegaETHBridgeFacet.s.soltest/solidity/Facets/MegaETHBridgeFacet.t.sol
script/deploy/facets/Deploy*.s.sol
📄 CodeRabbit inference engine (conventions.md)
script/deploy/facets/Deploy*.s.sol: Deployment scripts for facets must be named Deploy{ContractName}.s.sol and inherit DeployScriptBase; deploy using deploy(type(ContractName).creationCode); omit getConstructorArgs() when not needed
When constructor args are required, implement getConstructorArgs() using stdJson, then call deploy(type(FacetName).creationCode) and return (deployed, constructorArgs)
Files:
script/deploy/facets/DeployMegaETHBridgeFacet.s.sol
{README.md,Deploy.md,docs/**}
📄 CodeRabbit inference engine (conventions.md)
Keep primary documentation up to date: README.md (overview/setup), Deploy.md (deployment), and docs/ (technical docs)
Files:
docs/MegaETHBridgeFacet.md
src/**/*.sol
📄 CodeRabbit inference engine (conventions.md)
src/**/*.sol: Every contract must include NatSpec header in this order: @title, @author LI.FI (https://li.fi), @notice, @Custom:version X.Y.Z
All public/external functions must have NatSpec comments documenting purpose, params, and returns
Add inline comments for complex logic, optimizations, gas-saving techniques, and math steps
Emit GenericSwapCompleted after successful same-chain swap completion
Files:
src/Facets/MegaETHBridgeFacet.sol
src/Facets/**/*Facet.sol
📄 CodeRabbit inference engine (conventions.md)
src/Facets/**/*Facet.sol: Facet contracts must be located in src/Facets and include "Facet" in the contract name
Facet contracts must implement: internal _startBridge, swapAndStartBridgeTokensVia{FacetName}, and startBridgeTokensVia{FacetName}
Facet contracts must import/use: ILiFi, LibAsset, LibSwap, LibAllowList; and use security mixins ReentrancyGuard, SwapperV2, Validatable; ECDSA optional
Do not use msg.sender as refund address; require an explicit parameter for refund address
In {facetName}Data, receiverAddress must be the first field and be validated against bridgeData.receiver; verify targetChainId equals bridgeData.destinationChain for EVM-to-EVM
Apply required modifiers where appropriate: nonReentrant, refundExcessNative, validateBridgeData, doesNotContainSourceSwaps/doesContainSourceSwaps, doesNotContainDestinationCalls/doesContainDestinationCalls
Emit LiFiTransferStarted at the start of a transaction before external calls; do not emit LiFiTransferCompleted or LiFiTransferRecovered in facets
For native fees in facets, use the _depositAndSwap variant and reserve required fees before execution
For non-EVM chains: receiverAddress must be bytes (not bytes32), must not be zero, and bridgeData.receiver must equal NON_EVM_ADDRESS
Files:
src/Facets/MegaETHBridgeFacet.sol
script/deploy/facets/Update*.s.sol
📄 CodeRabbit inference engine (conventions.md)
Update scripts must be named Update{ContractName}.s.sol, inherit UpdateScriptBase, call update("{ContractName}") and use getExcludes() to return excluded selectors
Files:
script/deploy/facets/UpdateMegaETHBridgeFacet.s.sol
test/solidity/**/*.t.sol
📄 CodeRabbit inference engine (conventions.md)
test/solidity/**/*.t.sol: Solidity test files must use .t.sol extension; organize imports with system libraries first, then project files
Test naming: successful tests prefixed test_, failure tests prefixed testRevert_, base/inherited tests prefixed testBase_; negative tests must check specific revert reasons
Each test contract must implement setUp() that configures block numbers, initializes base contracts, sets up facets, and uses vm.label; contracts inheriting TestBase.sol must call initTestBase() in setUp()
Use vm.startPrank()/vm.stopPrank() for user simulation; maintain blank-line formatting rules for tests (expectRevert separation, assertion grouping, spacing)
Use assertEq for equality, custom assertion modifiers for balance changes, vm.expectRevert with precise reasons, and vm.expectEmit(true,true,true,true,) to verify events and parameters
Files:
test/solidity/Facets/MegaETHBridgeFacet.t.sol
🧠 Learnings (9)
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-09-12T10:17:51.686Z
Learning: Applies to script/deploy/facets/Deploy*.s.sol : Deployment scripts for facets must be named Deploy{ContractName}.s.sol and inherit DeployScriptBase; deploy using deploy(type(ContractName).creationCode); omit getConstructorArgs() when not needed
Applied to files:
script/deploy/facets/DeployMegaETHBridgeFacet.s.solscript/deploy/facets/UpdateMegaETHBridgeFacet.s.sol
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-09-12T10:17:51.686Z
Learning: Applies to script/deploy/facets/Deploy*.s.sol : When constructor args are required, implement getConstructorArgs() using stdJson, then call deploy(type(FacetName).creationCode) and return (deployed, constructorArgs)
Applied to files:
script/deploy/facets/DeployMegaETHBridgeFacet.s.sol
📚 Learning: 2025-08-26T15:19:07.800Z
Learnt from: mirooon
PR: lifinance/contracts#1328
File: script/deploy/facets/LDA/DeployAlgebraFacet.s.sol:4-4
Timestamp: 2025-08-26T15:19:07.800Z
Learning: DeployScriptBase.sol is located at script/deploy/facets/utils/DeployScriptBase.sol, not script/deploy/utils/DeployScriptBase.sol. Import paths from script/deploy/facets/LDA/ should use "../utils/DeployScriptBase.sol" to reference it correctly.
Applied to files:
script/deploy/facets/DeployMegaETHBridgeFacet.s.solscript/deploy/facets/UpdateMegaETHBridgeFacet.s.sol
📚 Learning: 2025-09-25T00:04:16.437Z
Learnt from: 0xDEnYO
PR: lifinance/contracts#1395
File: script/deploy/zksync/DeployFeeForwarder.zksync.s.sol:4-4
Timestamp: 2025-09-25T00:04:16.437Z
Learning: ZkSync deployment scripts have their own DeployScriptBase.sol located at script/deploy/zksync/utils/DeployScriptBase.sol, and all ZkSync deployment scripts use the relative import path "./utils/DeployScriptBase.sol" to reference it, not "../facets/utils/DeployScriptBase.sol".
Applied to files:
script/deploy/facets/DeployMegaETHBridgeFacet.s.solscript/deploy/facets/UpdateMegaETHBridgeFacet.s.sol
📚 Learning: 2025-01-28T14:27:50.689Z
Learnt from: ezynda3
PR: lifinance/contracts#924
File: script/deploy/zksync/DeployReceiverStargateV2.s.sol:19-21
Timestamp: 2025-01-28T14:27:50.689Z
Learning: In LiFi's deployment scripts, the `deploy` method in `DeployScriptBase` handles the concatenation of constructor arguments with the contract's creation code, so child contracts don't need to concatenate the arguments explicitly.
Applied to files:
script/deploy/facets/DeployMegaETHBridgeFacet.s.sol
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-09-12T10:17:51.686Z
Learning: Applies to src/Facets/**/*Facet.sol : Facet contracts must implement: internal _startBridge, swapAndStartBridgeTokensVia{FacetName}, and startBridgeTokensVia{FacetName}
Applied to files:
docs/MegaETHBridgeFacet.mdsrc/Facets/MegaETHBridgeFacet.soltest/solidity/Facets/MegaETHBridgeFacet.t.sol
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-09-12T10:17:51.686Z
Learning: Applies to script/deploy/facets/Update*.s.sol : Update scripts must be named Update{ContractName}.s.sol, inherit UpdateScriptBase, call update("{ContractName}") and use getExcludes() to return excluded selectors
Applied to files:
script/deploy/facets/UpdateMegaETHBridgeFacet.s.sol
📚 Learning: 2025-09-12T10:17:51.686Z
Learnt from: CR
PR: lifinance/contracts#0
File: conventions.md:0-0
Timestamp: 2025-09-12T10:17:51.686Z
Learning: Applies to src/Facets/**/*Facet.sol : Apply required modifiers where appropriate: nonReentrant, refundExcessNative, validateBridgeData, doesNotContainSourceSwaps/doesContainSourceSwaps, doesNotContainDestinationCalls/doesContainDestinationCalls
Applied to files:
test/solidity/Facets/MegaETHBridgeFacet.t.sol
📚 Learning: 2025-01-22T12:36:12.699Z
Learnt from: mirooon
PR: lifinance/contracts#945
File: test/solidity/Facets/GlacisFacet.t.sol:214-262
Timestamp: 2025-01-22T12:36:12.699Z
Learning: The GlacisFacet test suite inherits from TestBaseFacet which already covers various failure scenarios including invalid receiver address, invalid amounts, same chain bridging, and insufficient funds, making additional failure scenario tests redundant.
Applied to files:
test/solidity/Facets/MegaETHBridgeFacet.t.sol
🪛 LanguageTool
docs/MegaETHBridgeFacet.md
[grammar] ~5-~5: Use a hyphen to join words.
Context: ...Facet works by forwarding MegaETH Bridge specific calls to MegaETH Bridge [contra...
(QB_NEW_EN_HYPHEN)
[grammar] ~49-~49: Ensure spelling is correct
Context: ... sure to pass enough gas if the MegaETH facet is used directly. ## Swap Data Some m...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~55-~55: Use a hyphen to join words.
Context: ...ameter. Swapping is performed by a swap specific library that expects an array o...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
docs/MegaETHBridgeFacet.md
57-57: Link text should be descriptive
(MD059, descriptive-link-text)
63-63: Link text should be descriptive
(MD059, descriptive-link-text)
94-94: Link text should be descriptive
(MD059, descriptive-link-text)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: enforce-min-test-coverage
- GitHub Check: run-unit-tests
🔇 Additional comments (7)
script/deploy/facets/DeployMegaETHBridgeFacet.s.sol (1)
10-14: LGTM: deploy via DeployScriptBase with creationCode is correct.script/deploy/facets/UpdateMegaETHBridgeFacet.s.sol (1)
23-28: Initializer excluded from update: good.Excluding initMegaETH avoids accidental re‑init during cuts.
config/megaeth.json (1)
1-186: Fix 52 assetId addresses to use proper EIP-55 checksums.Verification found 52 assetId values that are lowercase and fail EIP-55 checksum validation. The script output above provides the corrected checksummed versions for each (e.g.,
0xc011a73ee8576fb46f5e1c5751ca3b9fe0af2a6f→0xC011a73ee8576Fb46F5E1c5751cA3B9Fe0af2a6F). All bridge addresses are already properly checksummed. No duplicate assetId conflicts detected.Update all 52 assetId entries to their EIP-55 checksummed format as shown in the verification output above.
⛔ Skipped due to learnings
Learnt from: 0xDEnYO PR: lifinance/contracts#819 File: deployments/polygon.json:0-0 Timestamp: 2024-10-04T09:21:59.708Z Learning: Ensure that scripts used for codebase verification produce valid and accurate results before reporting issues, especially when checking Ethereum address checksums in `deployments/polygon.json`.Learnt from: 0xDEnYO PR: lifinance/contracts#819 File: deployments/polygon.json:0-0 Timestamp: 2024-10-09T03:47:21.269Z Learning: Ensure that scripts used for codebase verification produce valid and accurate results before reporting issues, especially when checking Ethereum address checksums in `deployments/polygon.json`.Learnt from: 0xDEnYO PR: lifinance/contracts#1275 File: src/Facets/AllBridgeFacet.sol:164-164 Timestamp: 2025-07-16T01:03:08.106Z Learning: In src/Facets/AllBridgeFacet.sol, the team has decided that explicit validation for address downcasting from `_bridgeData.sendingAssetId` to `bytes32(uint256(uint160(_bridgeData.sendingAssetId)))` is not required, accepting the potential risk of silent overflow from unsafe downcasting.src/Facets/MegaETHBridgeFacet.sol (4)
4-13: Import/use LibAllowList to restrict tokens/bridges.Guidelines require facets to import/use LibAllowList. Add allowlist checks for sendingAssetId and chosen bridge before approvals/external calls.
If you share the project’s LibAllowList API, I can propose exact calls. As per coding guidelines.
⛔ Skipped due to learnings
Learnt from: mirooon PR: lifinance/contracts#1413 File: src/Facets/EverclearFacet.sol:4-13 Timestamp: 2025-10-10T10:56:04.861Z Learning: LibAllowList is only required for facets that make arbitrary external calls to DEX aggregators (e.g., GenericSwapFacetV3). Bridge facets that call specific protocol contracts (like EverclearFacet calling IEverclearFeeAdapter) do not need to import LibAllowList.
1-3: Fix license/pragma order (license, blank line, pragma).Guideline requires a blank line after the SPDX header and before pragma. Add an empty line between Lines 1 and 2.
Apply this diff:
-// SPDX-License-Identifier: LGPL-3.0-only -pragma solidity ^0.8.17; +// SPDX-License-Identifier: LGPL-3.0-only + +pragma solidity ^0.8.17;⛔ Skipped due to learnings
Learnt from: CR PR: lifinance/contracts#0 File: conventions.md:0-0 Timestamp: 2025-09-12T10:17:51.686Z Learning: Applies to src/Facets/**/*Facet.sol : Facet contracts must implement: internal _startBridge, swapAndStartBridgeTokensVia{FacetName}, and startBridgeTokensVia{FacetName}Learnt from: CR PR: lifinance/contracts#0 File: conventions.md:0-0 Timestamp: 2025-09-12T10:17:51.686Z Learning: Applies to src/Facets/**/*Facet.sol : Apply required modifiers where appropriate: nonReentrant, refundExcessNative, validateBridgeData, doesNotContainSourceSwaps/doesContainSourceSwaps, doesNotContainDestinationCalls/doesContainDestinationCallsLearnt from: mirooon PR: lifinance/contracts#1406 File: src/Facets/UnitFacet.sol:75-81 Timestamp: 2025-10-02T18:10:09.934Z Learning: In UnitFacet.sol and similar facet contracts in src/Facets/, the LiFiTransferStarted event emission should ALWAYS be at the end of the _startBridge internal function, after all validations and asset transfers have been completed successfully.Learnt from: mirooon PR: lifinance/contracts#1328 File: src/Periphery/LDA/Facets/NativeWrapperFacet.sol:1-2 Timestamp: 2025-08-29T11:18:56.656Z Learning: For src/Periphery/LDA/Facets/NativeWrapperFacet.sol, the SPDX license identifier should be immediately followed by the pragma statement without a blank line in between, as confirmed by mirooon.Learnt from: CR PR: lifinance/contracts#0 File: conventions.md:0-0 Timestamp: 2025-09-12T10:17:51.686Z Learning: Applies to src/Facets/**/*Facet.sol : In {facetName}Data, receiverAddress must be the first field and be validated against bridgeData.receiver; verify targetChainId equals bridgeData.destinationChain for EVM-to-EVMLearnt from: mirooon PR: lifinance/contracts#1406 File: src/Facets/UnitFacet.sol:1-2 Timestamp: 2025-10-02T18:14:51.385Z Learning: For facet contracts in src/Facets/, the SPDX license identifier should be immediately followed by the pragma statement without a blank line in between, as confirmed by mirooon for src/Facets/UnitFacet.sol.Learnt from: 0xDEnYO PR: lifinance/contracts#1275 File: src/Facets/AllBridgeFacet.sol:164-164 Timestamp: 2025-07-16T01:03:08.106Z Learning: In src/Facets/AllBridgeFacet.sol, the team has decided that explicit validation for address downcasting from `_bridgeData.sendingAssetId` to `bytes32(uint256(uint160(_bridgeData.sendingAssetId)))` is not required, accepting the potential risk of silent overflow from unsafe downcasting.
166-191: Move LiFiTransferStarted out of _startBridge (must emit before external calls).Event is currently emitted after bridge calls. Move emission to the external entrypoints (done in suggested diffs) and remove it here.
Apply this diff:
- if (LibAsset.isNativeAsset(_bridgeData.sendingAssetId)) { + if (LibAsset.isNativeAsset(_bridgeData.sendingAssetId)) { bridge.depositETHTo{ value: _bridgeData.minAmount }( _bridgeData.receiver, _megaETHData.l2Gas, "" ); } else { LibAsset.maxApproveERC20( IERC20(_bridgeData.sendingAssetId), address(bridge), _bridgeData.minAmount ); if (_megaETHData.isSynthetix) { bridge.depositTo(_bridgeData.receiver, _bridgeData.minAmount); } else { bridge.depositERC20To( _bridgeData.sendingAssetId, _megaETHData.assetIdOnL2, _bridgeData.receiver, _bridgeData.minAmount, _megaETHData.l2Gas, "" ); } } - - emit LiFiTransferStarted(_bridgeData);As per coding guidelines.
Also applies to: 193-193
⛔ Skipped due to learnings
Learnt from: mirooon PR: lifinance/contracts#1406 File: src/Facets/UnitFacet.sol:75-81 Timestamp: 2025-10-02T18:10:09.934Z Learning: In UnitFacet.sol and similar facet contracts in src/Facets/, the LiFiTransferStarted event emission should ALWAYS be at the end of the _startBridge internal function, after all validations and asset transfers have been completed successfully.Learnt from: CR PR: lifinance/contracts#0 File: conventions.md:0-0 Timestamp: 2025-09-12T10:17:51.686Z Learning: Applies to src/Facets/**/*Facet.sol : Emit LiFiTransferStarted at the start of a transaction before external calls; do not emit LiFiTransferCompleted or LiFiTransferRecovered in facetsLearnt from: 0xDEnYO PR: lifinance/contracts#1275 File: src/Facets/AllBridgeFacet.sol:186-188 Timestamp: 2025-07-16T01:04:55.857Z Learning: In LiFi facet contracts, when public entry point functions have `nonReentrant` modifier protection, internal functions like `_startBridge` that they call benefit from this reentrancy protection, making event emission order after external calls acceptable from a security perspective.Learnt from: mirooon PR: lifinance/contracts#1328 File: test/solidity/Periphery/GasZipPeriphery.t.sol:223-233 Timestamp: 2025-08-27T23:36:40.773Z Learning: In bridge facet swap flows with requiresDeposit=false, tokens are already held by the diamond contract from previous swaps. When subsequent contracts like GasZipPeriphery call LibAsset.depositAsset, they pull tokens from msg.sender (the diamond) to themselves via transferFrom, requiring no additional approvals since the diamond already owns the tokens.
99-119: Emit LiFiTransferStarted at start; fix docs; explicit refund recipient.
- Emit LiFiTransferStarted before any external calls.
- Don’t use msg.sender as refund address; require explicit parameter.
- Fix duplicated/typo NatSpec (@param _megaETHData and “containing”).
Apply this diff:
-/// @param _bridgeData Data contaning core information for bridging -/// @param _bridgeData Data specific to MegaETH Bridge +/// @param _bridgeData Data containing core information for bridging +/// @param _megaETHData Data specific to MegaETH Bridge +/// @param _refundRecipient Address to refund excess native value function startBridgeTokensViaMegaETHBridge( - ILiFi.BridgeData memory _bridgeData, - MegaETHData calldata _megaETHData + ILiFi.BridgeData memory _bridgeData, + MegaETHData calldata _megaETHData, + address payable _refundRecipient ) external payable nonReentrant - refundExcessNative(payable(msg.sender)) + refundExcessNative(_refundRecipient) doesNotContainSourceSwaps(_bridgeData) doesNotContainDestinationCalls(_bridgeData) validateBridgeData(_bridgeData) { + emit LiFiTransferStarted(_bridgeData); LibAsset.depositAsset( _bridgeData.sendingAssetId, _bridgeData.minAmount ); _startBridge(_bridgeData, _megaETHData); }As per coding guidelines.
⛔ Skipped due to learnings
Learnt from: mirooon PR: lifinance/contracts#1406 File: src/Facets/UnitFacet.sol:75-81 Timestamp: 2025-10-02T18:10:09.934Z Learning: In UnitFacet.sol and similar facet contracts in src/Facets/, the LiFiTransferStarted event emission should ALWAYS be at the end of the _startBridge internal function, after all validations and asset transfers have been completed successfully.Learnt from: CR PR: lifinance/contracts#0 File: conventions.md:0-0 Timestamp: 2025-09-12T10:17:51.686Z Learning: Applies to src/Facets/**/*Facet.sol : Emit LiFiTransferStarted at the start of a transaction before external calls; do not emit LiFiTransferCompleted or LiFiTransferRecovered in facetsLearnt from: mirooon PR: lifinance/contracts#1413 File: src/Facets/EverclearV2Facet.sol:75-96 Timestamp: 2025-10-13T11:13:48.847Z Learning: In EverclearV2Facet (and all LiFi facets), the team standard is to use msg.sender as the refund address with the refundExcessNative modifier, not requiring an explicit refund address parameter.
| **WARNING**: If the l2Gas is underpaid, the `finalizeDeposit` internal function call will fail and the funds will be lost! | ||
| Please make sure to pass enough gas if the MegaETH facet is used directly. | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Reword unsafe claim: underpaid L2 gas does not automatically “lose funds.”
The statement “funds will be lost” is inaccurate and risky. Typically, underpaying l2Gas causes the message to fail/fallback or require retry, not irrevocably burn funds. Recommend neutral, actionable guidance.
Apply:
-**WARNING**: If the l2Gas is underpaid, the `finalizeDeposit` internal function call will fail and the funds will be lost!
-Please make sure to pass enough gas if the MegaETH facet is used directly.
+**WARNING**: If `l2Gas` is under-estimated, `finalizeDeposit` on L2 may not complete and the message can remain unfinalized until retried with sufficient gas.
+Ensure `l2Gas` is provisioned conservatively or implement a retry path when calling the MegaETH facet directly.🌐 Web query:
Confirm Optimism-style L1StandardBridge deposit behavior on under-specified L2 gas: does it become retryable/unfinalized rather than irrevocably lost?
💡 Result:
Short answer: It does not irrevocably lose the deposit. Optimism’s Bedrock / L1StandardBridge flow encodes an L2 gas limit into the deposit and, if the L2 execution fails for lack of gas (or other L2-state reasons), the message/deposit can be replayed / relayed on L2 (or replayed with a larger gas limit) rather than the funds being permanently lost. Cite: Optimism docs (deposit flow and “Replaying messages”) and the OP‑Stack spec/Bedrock upgrade notes explaining the CrossDomainMessenger replayability; independent comparisons also describe the difference vs Arbitrum’s retryable‑ticket model. [1][2][3]
Sources
- Optimism docs — Deposit flow / “Replaying messages”. [Optimism Documentation]. (docs.optimism.io)
- OP‑Stack / Standard Bridges spec and Bedrock upgrade note describing CrossDomainMessenger replay guarantees. (specs.optimism.io)
- Bedrock vs Nitro comparisons (explain Optimism uses replay via CrossDomainMessenger; Arbitrum uses retryable tickets). (scalability.guide)
Citations:
- 1: https://docs.optimism.io/stack/transactions?utm_source=openai
- 2: https://specs.optimism.io/protocol/bridges.html?utm_source=openai
- 3: https://scalability.guide/posts/bedrock_vs_nitro/deposits/?utm_source=openai
Update documentation with accurate L2 gas behavior: funds do not become "lost" but remain retryable
The verification confirms that Optimism's deposit flow supports message replay/relay when L2 execution fails due to insufficient gas. The message/deposit can be replayed or relayed on L2 (or replayed with a larger gas limit) rather than the funds being permanently lost. The original documentation's claim that funds "will be lost" is inaccurate and should be corrected to reflect this retryable behavior.
Apply the suggested fix:
-**WARNING**: If the l2Gas is underpaid, the `finalizeDeposit` internal function call will fail and the funds will be lost!
-Please make sure to pass enough gas if the MegaETH facet is used directly.
+**WARNING**: If `l2Gas` is under-estimated, `finalizeDeposit` on L2 may not complete and the message can remain unfinalized until retried with sufficient gas.
+Ensure `l2Gas` is provisioned conservatively or implement a retry path when calling the MegaETH facet directly.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| **WARNING**: If the l2Gas is underpaid, the `finalizeDeposit` internal function call will fail and the funds will be lost! | |
| Please make sure to pass enough gas if the MegaETH facet is used directly. | |
| **WARNING**: If `l2Gas` is under-estimated, `finalizeDeposit` on L2 may not complete and the message can remain unfinalized until retried with sufficient gas. | |
| Ensure `l2Gas` is provisioned conservatively or implement a retry path when calling the MegaETH facet directly. | |
🧰 Tools
🪛 LanguageTool
[grammar] ~49-~49: Ensure spelling is correct
Context: ... sure to pass enough gas if the MegaETH facet is used directly. ## Swap Data Some m...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
In docs/MegaETHBridgeFacet.md around lines 48 to 50, update the warning text so
it no longer claims funds "will be lost" when l2Gas is underpaid; instead
describe that L2 execution can fail due to insufficient gas but the
deposit/message remains retryable/relayable on Optimism and can be replayed with
a higher gas limit or relayed on L2 to complete finalization; replace the
existing alarming sentence with a concise note instructing callers to ensure
sufficient l2Gas to avoid needing a replay and include a short recovery hint
about replaying/relaying the message on L2 rather than implying permanent loss.
| // SPDX-License-Identifier: UNLICENSED | ||
| pragma solidity ^0.8.17; |
There was a problem hiding this comment.
Fix SPDX and formatting to match project policy.
Per coding guidelines: use // SPDX-License-Identifier: LGPL-3.0-only and insert a blank line before pragma.
-// SPDX-License-Identifier: UNLICENSED
-pragma solidity ^0.8.17;
+// SPDX-License-Identifier: LGPL-3.0-only
+
+pragma solidity ^0.8.17;🤖 Prompt for AI Agents
In script/deploy/facets/DeployMegaETHBridgeFacet.s.sol around lines 1 to 2, the
SPDX header and formatting don't match project policy; replace the existing SPDX
line with "// SPDX-License-Identifier: LGPL-3.0-only" and ensure there is a
single blank line between the SPDX comment and the "pragma solidity ^0.8.17;"
line so the file conforms to the project's license and formatting guidelines.
| // SPDX-License-Identifier: UNLICENSED | ||
| pragma solidity ^0.8.17; |
There was a problem hiding this comment.
Fix SPDX and formatting to match project policy.
Use LGPL‑3.0‑only and insert a blank line after SPDX.
-// SPDX-License-Identifier: UNLICENSED
-pragma solidity ^0.8.17;
+// SPDX-License-Identifier: LGPL-3.0-only
+
+pragma solidity ^0.8.17;As per coding guidelines.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // SPDX-License-Identifier: UNLICENSED | |
| pragma solidity ^0.8.17; | |
| // SPDX-License-Identifier: LGPL-3.0-only | |
| pragma solidity ^0.8.17; |
🤖 Prompt for AI Agents
In script/deploy/facets/UpdateMegaETHBridgeFacet.s.sol around lines 1 to 2, the
file uses an incorrect SPDX identifier and lacks the required blank line;
replace the SPDX header "UNLICENSED" with "LGPL-3.0-only" and insert a single
blank line after the SPDX comment, keeping the pragma solidity ^0.8.17; line
intact and ensuring file formatting matches the project policy.
| /// @title MegaETH Bridge Facet | ||
| /// @author Li.Finance (https://li.finance) | ||
| /// @notice Provides functionality for bridging through MegaETH Bridge | ||
| /// @custom:version 1.0.0 |
There was a problem hiding this comment.
Correct NatSpec author to match guidelines.
Use “LI.FI (https://li.fi)”.
Apply this diff:
-/// @author Li.Finance (https://li.finance)
+/// @author LI.FI (https://li.fi)As per coding guidelines.
🤖 Prompt for AI Agents
In src/Facets/MegaETHBridgeFacet.sol around lines 14 to 17, the NatSpec author
tag uses "Li.Finance (https://li.finance)"; update it to follow guidelines by
replacing the author with "LI.FI (https://li.fi)". Ensure the @author line is
exactly: LI.FI (https://li.fi) and keep surrounding NatSpec formatting intact.
| struct MegaETHData { | ||
| address assetIdOnL2; | ||
| uint32 l2Gas; | ||
| bool isSynthetix; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Facet data must contain receiverAddress first and validate chain.
Guidelines require:
- receiverAddress as the first field in {facetName}Data, validated against bridgeData.receiver.
- Verify targetChainId equals bridgeData.destinationChain (EVM→EVM).
Currently MegaETHData lacks both. Please extend MegaETHData and validate in the flow.
I can draft the struct and validation once you confirm BridgeData field names (destinationChain or destinationChainId). As per coding guidelines.
| /// @notice Initialize local variables for the MegaETH Bridge Facet | ||
| /// @param configs Bridge configuration data | ||
| function initMegaETH( | ||
| Config[] calldata configs, | ||
| IL1StandardBridge standardBridge | ||
| ) external { | ||
| LibDiamond.enforceIsContractOwner(); | ||
|
|
||
| Storage storage s = getStorage(); | ||
|
|
||
| if (s.initialized) { | ||
| revert AlreadyInitialized(); | ||
| } | ||
|
|
||
| for (uint256 i = 0; i < configs.length; i++) { | ||
| if (configs[i].bridge == address(0)) { | ||
| revert InvalidConfig(); | ||
| } | ||
| s.bridges[configs[i].assetId] = IL1StandardBridge( | ||
| configs[i].bridge | ||
| ); | ||
| } | ||
|
|
||
| s.standardBridge = standardBridge; | ||
| s.initialized = true; | ||
|
|
||
| emit MegaETHInitialized(configs); | ||
| } |
There was a problem hiding this comment.
Init: enforce non-zero standardBridge; underscore params; complete NatSpec.
- Validate standardBridge != address(0) to prevent later calls to address(0).
- Follow underscore naming for function params.
- Add missing @param for standardBridge.
Apply this diff:
-/// @param configs Bridge configuration data
+/// @param configs Bridge configuration data
+/// @param standardBridge Global default L1 bridge
function initMegaETH(
- Config[] calldata configs,
- IL1StandardBridge standardBridge
+ Config[] calldata _configs,
+ IL1StandardBridge _standardBridge
) external {
LibDiamond.enforceIsContractOwner();
Storage storage s = getStorage();
if (s.initialized) {
revert AlreadyInitialized();
}
- for (uint256 i = 0; i < configs.length; i++) {
- if (configs[i].bridge == address(0)) {
+ for (uint256 i = 0; i < _configs.length; i++) {
+ if (_configs[i].bridge == address(0)) {
revert InvalidConfig();
}
- s.bridges[configs[i].assetId] = IL1StandardBridge(
- configs[i].bridge
+ s.bridges[_configs[i].assetId] = IL1StandardBridge(
+ _configs[i].bridge
);
}
- s.standardBridge = standardBridge;
+ if (address(_standardBridge) == address(0)) {
+ revert InvalidConfig();
+ }
+ s.standardBridge = _standardBridge;
s.initialized = true;
- emit MegaETHInitialized(configs);
+ emit MegaETHInitialized(_configs);
}🤖 Prompt for AI Agents
In src/Facets/MegaETHBridgeFacet.sol around lines 49 to 76, the initMegaETH
function must validate the standardBridge parameter, follow the underscore
naming convention for inputs, and complete NatSpec: rename parameters to
_configs and _standardBridge (update all usages to the underscored names), add a
require/revert that _standardBridge != address(0) before assignment to prevent
later calls to address(0), update s.standardBridge = _standardBridge, and add a
NatSpec @param for standardBridge describing the IL1StandardBridge instance
being set.
| /// @notice Performs a swap before bridging via MegaETH Bridge | ||
| /// @param _bridgeData Data contaning core information for bridging | ||
| /// @param _swapData An array of swap related data for performing swaps before bridging | ||
| /// @param _bridgeData Data specific to MegaETH Bridge | ||
| function swapAndStartBridgeTokensViaMegaETHBridge( | ||
| ILiFi.BridgeData memory _bridgeData, | ||
| LibSwap.SwapData[] calldata _swapData, | ||
| MegaETHData calldata _megaETHData | ||
| ) | ||
| external | ||
| payable | ||
| nonReentrant | ||
| refundExcessNative(payable(msg.sender)) | ||
| containsSourceSwaps(_bridgeData) | ||
| doesNotContainDestinationCalls(_bridgeData) | ||
| validateBridgeData(_bridgeData) | ||
| { | ||
| _bridgeData.minAmount = _depositAndSwap( | ||
| _bridgeData.transactionId, | ||
| _bridgeData.minAmount, | ||
| _swapData, | ||
| payable(msg.sender) | ||
| ); | ||
| _startBridge(_bridgeData, _megaETHData); | ||
| } | ||
|
|
There was a problem hiding this comment.
Swap variant: correct modifier name; emit early; explicit refund recipient; docs.
- Use doesContainSourceSwaps per guidelines.
- Emit LiFiTransferStarted at the start.
- Don’t use msg.sender as refund address; accept explicit parameter and pass through to _depositAndSwap.
- Fix duplicated/typo NatSpec.
Apply this diff:
-/// @param _bridgeData Data contaning core information for bridging
+/// @param _bridgeData Data containing core information for bridging
/// @param _swapData An array of swap related data for performing swaps before bridging
-/// @param _bridgeData Data specific to MegaETH Bridge
+/// @param _megaETHData Data specific to MegaETH Bridge
+/// @param _refundRecipient Address to refund excess native value
function swapAndStartBridgeTokensViaMegaETHBridge(
- ILiFi.BridgeData memory _bridgeData,
- LibSwap.SwapData[] calldata _swapData,
- MegaETHData calldata _megaETHData
+ ILiFi.BridgeData memory _bridgeData,
+ LibSwap.SwapData[] calldata _swapData,
+ MegaETHData calldata _megaETHData,
+ address payable _refundRecipient
)
external
payable
nonReentrant
- refundExcessNative(payable(msg.sender))
- containsSourceSwaps(_bridgeData)
+ refundExcessNative(_refundRecipient)
+ doesContainSourceSwaps(_bridgeData)
doesNotContainDestinationCalls(_bridgeData)
validateBridgeData(_bridgeData)
{
+ emit LiFiTransferStarted(_bridgeData);
_bridgeData.minAmount = _depositAndSwap(
_bridgeData.transactionId,
_bridgeData.minAmount,
_swapData,
- payable(msg.sender)
+ _refundRecipient
);
_startBridge(_bridgeData, _megaETHData);
}As per coding guidelines.
🤖 Prompt for AI Agents
In src/Facets/MegaETHBridgeFacet.sol around lines 121 to 146, update the
swapAndStartBridgeTokensViaMegaETHBridge function: rename the
containsSourceSwaps modifier to doesContainSourceSwaps, add an explicit refund
recipient parameter (e.g., address payable _refundAddress) to the function
signature and pass that into _depositAndSwap instead of payable(msg.sender),
emit the LiFiTransferStarted event immediately at the start of the function
(before swaps/bridging), and fix the NatSpec by removing the duplicated @param
_bridgeData line and adding a correct @param for _megaETHData and for the new
refund parameter.
| /// @dev Contains the business logic for the bridge via MegaETH Bridge | ||
| /// @param _bridgeData Data contaning core information for bridging | ||
| /// @param _bridgeData Data specific to MegaETH Bridge | ||
| function _startBridge( | ||
| ILiFi.BridgeData memory _bridgeData, | ||
| MegaETHData calldata _megaETHData | ||
| ) private { | ||
| Storage storage s = getStorage(); | ||
| IL1StandardBridge nonStandardBridge = s.bridges[ | ||
| _bridgeData.sendingAssetId | ||
| ]; | ||
| IL1StandardBridge bridge = LibUtil.isZeroAddress( | ||
| address(nonStandardBridge) | ||
| ) | ||
| ? s.standardBridge | ||
| : nonStandardBridge; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add validations required by guidelines (receiver/chain/allowlist).
Before performing approvals/calls in _startBridge:
- Validate _megaETHData.receiverAddress == _bridgeData.receiver (and non-zero).
- Validate targetChainId equals _bridgeData.destinationChain (EVM→EVM).
- Enforce LibAllowList for the chosen bridge and sendingAssetId.
Share exact BridgeData field names and LibAllowList API to draft precise code. As per coding guidelines.
| function _startBridge( | ||
| ILiFi.BridgeData memory _bridgeData, | ||
| MegaETHData calldata _megaETHData | ||
| ) private { | ||
| Storage storage s = getStorage(); | ||
| IL1StandardBridge nonStandardBridge = s.bridges[ | ||
| _bridgeData.sendingAssetId | ||
| ]; | ||
| IL1StandardBridge bridge = LibUtil.isZeroAddress( | ||
| address(nonStandardBridge) | ||
| ) | ||
| ? s.standardBridge | ||
| : nonStandardBridge; | ||
|
|
||
| if (LibAsset.isNativeAsset(_bridgeData.sendingAssetId)) { |
There was a problem hiding this comment.
Add NotInitialized guard before selecting bridge.
Without initialization, this can call address(0) and lose funds.
Apply this diff:
function _startBridge(
ILiFi.BridgeData memory _bridgeData,
MegaETHData calldata _megaETHData
) private {
Storage storage s = getStorage();
+ if (!s.initialized) revert NotInitialized();
IL1StandardBridge nonStandardBridge = s.bridges[
_bridgeData.sendingAssetId
];Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Facets/MegaETHBridgeFacet.sol around lines 152-166, add a NotInitialized
guard before selecting the bridge: check that either
s.bridges[_bridgeData.sendingAssetId] or s.standardBridge is non-zero and revert
with NotInitialized if both are address(0) to avoid selecting address(0) and
losing funds; place this guard immediately before computing
nonStandardBridge/bridge so the function fails early when bridges aren't
configured.
| // SPDX-License-Identifier: Unlicense | ||
| pragma solidity ^0.8.17; |
There was a problem hiding this comment.
Fix SPDX and formatting to match project policy.
Tests are first‑party Solidity files; use LGPL‑3.0‑only and insert a blank line after SPDX.
-// SPDX-License-Identifier: Unlicense
-pragma solidity ^0.8.17;
+// SPDX-License-Identifier: LGPL-3.0-only
+
+pragma solidity ^0.8.17;As per coding guidelines.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // SPDX-License-Identifier: Unlicense | |
| pragma solidity ^0.8.17; | |
| // SPDX-License-Identifier: LGPL-3.0-only | |
| pragma solidity ^0.8.17; |
🤖 Prompt for AI Agents
In test/solidity/Facets/MegaETHBridgeFacet.t.sol around lines 1 to 2, the SPDX
identifier and file formatting do not follow project policy: change the SPDX
line to "SPDX-License-Identifier: LGPL-3.0-only" and insert a single blank line
immediately after that SPDX line so the file begins with the correct license tag
followed by an empty line, then the pragma solidity declaration.
Which Jira task belongs to this PR?
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)