LF-12743 Create fix for gaszipfacet [GasZipFacet v2.0.3] #1058
Conversation
WalkthroughThis pull request adds support for the staging environment by introducing new entries and configurations for the GasZip facet. Deployment JSON files now include a staging section with the version "2.0.3" and relevant deployment details. A new TypeScript demo script facilitates bridging tokens via GasZip, and helper utilities have been updated for address conversion and balance checking. Additionally, the GasZipFacet smart contract and its tests have been updated with a version bump, enhanced error handling via a new Changes
Possibly related PRs
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
mirooon
changed the title
lifi-action-bot
changed the title
🤖 GitHub Action: Security Alerts Review 🔍🟢 Dismissed Security Alerts with Comments 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: ✅ No unresolved security alerts! 🎉 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/Facets/GasZipFacet.sol (1)
98-125:⚠️ Potential issueAddress potential reentrant issue highlighted in pipeline failure
The pipeline has identified a potential issue with reentrant functions emitting events after making external calls. The
_startBridgefunction makes an external call toGAS_ZIP_ROUTER.depositbefore emitting theLiFiTransferStartedevent, which could lead to out-of-order events if there's a reentrant call.Consider emitting events before making external calls:
// validate bridgeData // make sure destinationChainId is of a different network if (_bridgeData.destinationChainId == block.chainid) revert CannotBridgeToSameNetwork(); + emit LiFiTransferStarted(_bridgeData); // We are depositing to a new contract that supports deposits for EVM chains + Solana (therefore 'receiver' address is bytes32) GAS_ZIP_ROUTER.deposit{ value: _bridgeData.minAmount }( _gasZipData.destinationChains, _gasZipData.receiverAddress ); - emit LiFiTransferStarted(_bridgeData); }🧰 Tools
🪛 GitHub Actions: Security Alerts Review
[error] 98-98: Olympix Integrated Security: Reentrant functions which emit events after making an external call may lead to out-of-order events.
🧹 Nitpick comments (1)
script/demoScripts/demoGasZip.ts (1)
1-87: Clear and well-structured demo script for GasZip bridgingThe demo script is well organized with proper setup, configuration, and execution steps for the GasZip protocol. Good job providing clear console output for tracing the process.
Note that there's a commented-out line (#33) which appears to reference
addressToBytes32RightPaddedfor handling EVM addresses, which matches the function implemented in the helpers file. Consider including a comment explaining when this alternate approach would be used.- // const userReceiver = addressToBytes32RightPadded({evm_address}) // <== in case of evm address + // For EVM destination chains, convert the EVM address using right-padding + // const userReceiver = addressToBytes32RightPadded("0xYourEvmAddressHere") // <== in case of evm address
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
deployments/_deployments_log_file.json(1 hunks)deployments/arbitrum.diamond.staging.json(1 hunks)deployments/arbitrum.staging.json(1 hunks)lib/safe-smart-account(1 hunks)script/demoScripts/demoGasZip.ts(1 hunks)script/demoScripts/utils/demoScriptHelpers.ts(3 hunks)src/Facets/GasZipFacet.sol(4 hunks)test/solidity/Facets/GasZipFacet.t.sol(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: Security Alerts Review
src/Facets/GasZipFacet.sol
[error] 64-64: Olympix Integrated Security: The contract contains functions with inadequate validation of input parameters, potentially leading to unexpected behavior or vulnerabilities.
[error] 39-39: Olympix Integrated Security: Parameters passed to a constructor that are not validated for correct values may lead to contract creation in an undesired state.
[error] 31-31: Olympix Integrated Security: Reentrant functions which emit events after making an external call may lead to out-of-order events.
[error] 98-98: Olympix Integrated Security: Reentrant functions which emit events after making an external call may lead to out-of-order events.
🔇 Additional comments (12)
lib/safe-smart-account (1)
1-1: Subproject Commit Update VerificationThe update registering the subproject commit
009b8732a8d639379731d8cbce155d93ae2f6c66looks correct. Please ensure that this commit hash aligns with the intended state of thesafe-smart-accountlibrary and that it has undergone the necessary preliminary audit, considering its integration with the new GasZipFacet functionality.deployments/arbitrum.diamond.staging.json (1)
156-163: Addition of GasZipFacet Configuration
The new GasZipFacet entry with address"0x18Cd6D3A48f526b3BD140a26B0225C42969f0F63"and version"2.0.3"has been correctly added. Please verify that this address is consistent with the deployment logs and other environment configurations.deployments/arbitrum.staging.json (1)
54-55: New GasZipFacet Address Mapping Added
The GasZipFacet entry has been introduced with the same address"0x18Cd6D3A48f526b3BD140a26B0225C42969f0F63", aligning with the diamond staging configuration. This change appears consistent and maintains the overall JSON structure.deployments/_deployments_log_file.json (1)
28597-28609: New "staging" Deployment Log Entry Added
A new entry for the "staging" environment with version"2.0.3"has been successfully added. The deployment details—such as"ADDRESS","OPTIMIZER_RUNS","TIMESTAMP","CONSTRUCTOR_ARGS","SALT", and"VERIFIED"—appear to be correctly structured and consistent with similar entries in other environments. Please double-check that the"TIMESTAMP"and other data values accurately reflect the intended deployment metadata.script/demoScripts/utils/demoScriptHelpers.ts (2)
481-495: Well-implemented address conversion utility functionThis new utility function
addressToBytes32RightPaddedcorrectly implements right-padding for address conversion, which accurately mimics Solidity'sbytes32(bytes20(uint160(address)))conversion behavior. Good job including proper input validation and documenting the function's purpose.
681-697: Good refactoring of the ensureBalance functionThe updated
ensureBalancefunction now properly handles both native tokens and ERC20 tokens with a cleaner implementation. The parameter renaming fromtokenContracttoassetis more accurate and the optionalpublicClientparameter makes the function more flexible.script/demoScripts/demoGasZip.ts (1)
50-62:✅ Verification successful
Verify bridgeData configuration
The bridgeData setup uses
NON_EVM_ADDRESSfor the receiver, but the transaction is actually sending to the Solana address specified inuserReceiver. Ensure this is the intended behavior for the GasZip protocol.
🏁 Script executed:
#!/bin/bash # Check how the NON_EVM_ADDRESS is used in the GasZipFacet contract rg -n "NON_EVM_ADDRESS" src/Facets/GasZipFacet.solLength of output: 158
Let's search the repo for occurrences of
userReceiverto verify if it’s used elsewhere and how it relates toNON_EVM_ADDRESS.Once we see where and how
userReceiveris referenced, we can see if the GasZip protocol is intentionally usingNON_EVM_ADDRESSas a placeholder in the bridgeData while using the actual Solana address later.Please run this script and share the results, so we can finalize the review comment.
🏁 Script executed:
#!/bin/bash # Search for occurrences of userReceiver in the codebase rg -n "userReceiver" .Length of output: 788
GasZip bridgeData configuration is correct
After verifying the usage in GasZipFacet (which defines and checks for NON_EVM_ADDRESS at lines 25 and 108) and noting that the demo script passes the actual Solana address via
userReceiver(line 65), we confirm that using NON_EVM_ADDRESS as a placeholder in the bridgeData is intentional in the GasZip protocol. No changes are needed here.src/Facets/GasZipFacet.sol (3)
17-17: Version bump documented properlyThe version bump from 2.0.2 to 2.0.3 is properly documented with the
@custom:versiontag.
27-27: Improved naming convention for constantsGood job renaming
gasZipRoutertoGAS_ZIP_ROUTERfollowing constant naming conventions. This improves code readability and is consistent with other constants in the contract.Also applies to: 32-32, 119-119
110-110: Fixed address conversion methodThe updated address conversion from
bytes32(uint256(uint160(...)))tobytes32(bytes20(uint160(...)))is more accurate as it ensures proper right-padding of the address, which aligns with Solidity's type conversion behavior. This change fixes a potential issue with address representation.test/solidity/Facets/GasZipFacet.t.sol (2)
41-41: Properly updated address conversion in testsThe address conversion has been correctly updated from
bytes32(uint256(uint160(...)))tobytes32(bytes20(uint160(...)))to match the changes in the main contract. This ensures that tests accurately verify the contract's behavior.Also applies to: 96-96
137-140: Updated assertion to match refactored variable nameThe assertion has been properly updated to use the renamed variable
GAS_ZIP_ROUTERinstead ofgasZipRouter. This keeps the test in sync with the implementation.
Test Coverage ReportLine Coverage: 79.10% (2109 / 2666 lines) |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
script/demoScripts/utils/demoScriptHelpers.ts (1)
737-756: 🛠️ Refactor suggestionAdd null check for publicClient when handling native tokens.
When
assetequalszeroAddress, the code relies onpublicClientto fetch the native token balance. However, sincepublicClientis an optional parameter, this could lead to a runtime error if it's not provided.if (asset === zeroAddress) { + if (!publicClient) { + throw new Error('publicClient is required to check native token balance'); + } // Special case: asset represents the native token (e.g. ETH). // Retrieve the native balance using the public client. balance = await publicClient.getBalance({ address: walletAddress }) } else {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
deployments/_deployments_log_file.json(1 hunks)deployments/arbitrum.diamond.staging.json(1 hunks)deployments/arbitrum.staging.json(1 hunks)script/demoScripts/utils/demoScriptHelpers.ts(3 hunks)test/solidity/utils/TestBase.sol(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/solidity/utils/TestBase.sol
🚧 Files skipped from review as they are similar to previous changes (3)
- deployments/arbitrum.diamond.staging.json
- deployments/arbitrum.staging.json
- deployments/_deployments_log_file.json
🧰 Additional context used
🪛 Biome (1.9.4)
script/demoScripts/utils/demoScriptHelpers.ts
[error] 18-18: Shouldn't redeclare 'zeroAddress'. Consider to delete it or rename it.
'zeroAddress' is defined here:
(lint/suspicious/noRedeclare)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run-unit-tests
- GitHub Check: enforce-min-test-coverage
🔇 Additional comments (1)
script/demoScripts/utils/demoScriptHelpers.ts (1)
482-496: Good implementation of the right-padded address conversion.The new function is well-documented and correctly implements address conversion to 32-byte format with right padding, which replicates Solidity's behavior. The validation check for Ethereum address format adds robustness to the implementation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
script/demoScripts/demoGasZip.ts (2)
65-68: Using direct chain ID value instead of utility functionThe script directly sets
destinationChains: gasZipDestinationChainIdinstead of using thegetDestinationChainsValuefunction mentioned in the previous discussions. According to past comments, this function is provided as a convenience for third parties to calculate this parameter.- const gasZipData: IGasZip.GasZipDataStruct = { - receiverAddress: userReceiver, - destinationChains: gasZipDestinationChainId, - } + // Demonstrate usage of getDestinationChainsValue function + const destinationChainIds = [gasZipDestinationChainId]; + const destinationChains = await lifiDiamondContract.read.getDestinationChainsValue([destinationChainIds]); + + const gasZipData: IGasZip.GasZipDataStruct = { + receiverAddress: userReceiver, + destinationChains: destinationChains, + }
23-23: Add comment explaining NON_EVM_ADDRESS purposeThe purpose of this constant isn't clear. Consider adding a comment to explain its significance and why this specific format is used.
-const NON_EVM_ADDRESS = '0x11f111f111f111F111f111f111F111f111f111F1' +// Placeholder address for non-EVM chains in the bridgeData.receiver field +// The actual receiver address is set in the protocol-specific data (gasZipData) +const NON_EVM_ADDRESS = '0x11f111f111f111F111f111f111F111f111f111F1'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
deployments/_deployments_log_file.json(1 hunks)deployments/arbitrum.diamond.staging.json(1 hunks)deployments/arbitrum.staging.json(1 hunks)script/demoScripts/demoGasZip.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- deployments/_deployments_log_file.json
- deployments/arbitrum.staging.json
- deployments/arbitrum.diamond.staging.json
🧰 Additional context used
🧬 Code Definitions (1)
script/demoScripts/demoGasZip.ts (2)
script/demoScripts/utils/demoScriptChainConfig.ts (1) (1)
SupportedChain(4-4)script/demoScripts/utils/demoScriptHelpers.ts (4) (4)
setupEnvironment(579-620)addressToBytes32RightPadded(491-497)ensureBalance(741-766)executeTransaction(700-736)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: enforce-min-test-coverage
🔇 Additional comments (3)
script/demoScripts/demoGasZip.ts (3)
34-39: Consider using getDestinationChainsValue functionBased on previous discussions, there was a convenience function
getDestinationChainsValueavailable in the GasZip contracts that could be used to calculate the destination chains parameter. Consider using this function to demonstrate its usage as mentioned in past review comments.
51-63: LGTM: Bridge data structure looks properly configuredThe bridge data structure is well-formed with all required fields, including a randomly generated transaction ID for uniqueness.
28-29:❓ Verification inconclusive
Ensure destination chain consistency
There's a discrepancy between the comment in line 29 indicating berachain and the log message in line 46 that mentions Solana. This could lead to confusion for users of this demo script.
🏁 Script executed:
#!/bin/bash # Check for inconsistencies in chain ID references echo "Checking for chain ID references in the codebase..." rg -A 1 "berachain.*143" --type ts rg -A 1 "Solana.*chain.*id" --type tsLength of output: 189
Action Required: Confirm Destination Chain Label Consistency
The automated searches did not reveal any references to “Solana” near the destination chain IDs, so it’s unclear whether the discrepancy noted in the log message (at line 46) still exists. Please manually verify that the log output in
script/demoScripts/demoGasZip.tsat line 46 is accurate. In particular:
- Ensure that the destination chain label in the comment on line 29 (“berachain”) matches what’s actually logged on line 46.
- If the log message still references “Solana” while the code clearly intends “berachain,” update either the log or the comment to maintain consistency.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
script/demoScripts/demoGasZip.ts (2)
23-23: Document the purpose of NON_EVM_ADDRESS constant.This hardcoded address is used as a receiver in the bridge data but its purpose is unclear. Is this a placeholder, a testing address, or something else? Consider adding a comment explaining what this address represents and why it's being used.
-const NON_EVM_ADDRESS = '0x11f111f111f111F111f111f111F111f111f111F1' +// Placeholder address used for the bridge transaction's receiver field +// The actual recipient is specified in the gasZipData.receiverAddress +const NON_EVM_ADDRESS = '0x11f111f111f111F111f111f111F111f111f111F1'
65-68: Consider using the getDestinationChainsValue function for destinationChains.According to the past review comments, there is a
getDestinationChainsValuefunction that could be used to construct the destinationChains parameter correctly. This would demonstrate the function's usage as suggested in the comments.- const gasZipData: IGasZip.GasZipDataStruct = { - receiverAddress: userReceiver, - destinationChains: gasZipDestinationChainId, - } + // Use getDestinationChainsValue for demonstration purposes + const chainIds = [gasZipDestinationChainId] + const destinationChainsValue = await lifiDiamondContract.read.getDestinationChainsValue([chainIds]) + + const gasZipData: IGasZip.GasZipDataStruct = { + receiverAddress: userReceiver, + destinationChains: destinationChainsValue, + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
script/demoScripts/demoGasZip.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
script/demoScripts/demoGasZip.ts (2)
script/demoScripts/utils/demoScriptChainConfig.ts (1)
SupportedChain(4-4)script/demoScripts/utils/demoScriptHelpers.ts (4)
setupEnvironment(579-620)addressToBytes32RightPadded(491-497)ensureBalance(741-766)executeTransaction(700-736)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: enforce-min-test-coverage
🔇 Additional comments (8)
script/demoScripts/demoGasZip.ts (8)
1-14: Import statements look good and necessary dependencies are included.The script imports all necessary modules for bridging tokens via GasZip, including the required ABIs, helper functions, and type definitions. Good usage of specific imports rather than wildcard imports.
28-29: Good documentation of chain IDs with source references.The comments clearly explain the purpose of each chain ID and provide a reference to the Gas.zip documentation for the custom destination chain ID. This helps future developers understand the value source and meaning.
34-39: Good demonstration of address conversion options.The code includes both an active implementation for EVM addresses and a commented implementation for Solana addresses, providing good examples for different blockchain types. This adds educational value to the demo script.
46-46: Correct destination in log message.The log message now correctly states "Berachain" as the destination, addressing the inconsistency highlighted in a previous review comment where it incorrectly mentioned Solana.
51-63: Bridge data structure is properly configured.The bridgeData object is well-structured with all required fields. Using random bytes for the transaction ID ensures uniqueness. The integration name "ACME Devs" is set, along with appropriate values for all fields needed for the bridge operation.
71-80: Well-structured transaction execution with proper validation.The transaction execution uses the helper function
executeTransactionwith appropriate parameters, including validation of the transaction receipt (trueas the last parameter). This ensures the transaction is properly completed before the script concludes.
83-88: Proper error handling with informative error messages.The main function execution includes appropriate error handling that logs errors and sets the process exit code accordingly, making it suitable for both manual execution and integration into automated workflows.
57-58:✅ Verification successful
Ensure NON_EVM_ADDRESS is correctly used in the bridge flow.
The
bridgeData.receiveruses the hardcodedNON_EVM_ADDRESSwhile the actual recipient address is specified ingasZipData.receiverAddress. Verify that this is the correct pattern for GasZip bridging, as it might be confusing having two different receiver addresses.
🌐 Web query:
How does GasZip handle receiver addresses in bridging transactions?💡 Result:
Gas.zip handles receiver addresses in bridging transactions by allowing users to either default to their own address or explicitly specify a different recipient through the transaction parameters. The system supports both scenarios across multiple chains simultaneously.
Key aspects of address handling:
Direct recipient specification
Users can manually input any valid blockchain address as the recipient during deposit creation. The recipient address is passed via therecipientparameter in contract calls likedepositToGasZipNative[2][6].Default sender address
If no recipient is explicitly provided, Gas.zip defaults to using the sender's wallet address as the receiver on destination chains [4][5].Multi-chain addressing
A single transaction can route funds to different recipient addresses across multiple chains using LayerZero'sadapterParamssystem:createOptimizedAdapterParams(BigInt(selection.v2LZid), parseEther(selection.valueInEther))This encodes both destination chain ID and recipient address information [5][9].
Address validation
Contracts verify recipient addresses through:
- Native address format checks
- Chain-specific compatibility validation
- Limits checks to prevent invalid transactions [6][8]
The system supports sending to external addresses different from the sender through its calldata structure, enabling use cases like corporate gas funding or cross-protocol interactions. All address operations are executed atomically in the same transaction that initiates the bridging process [2][4][6].
Citations:
- 1: https://www.gas.zip/faq
- 2: https://etherscan.io/address/0xf28a352377663ca134bd27b582b1a9a4dad7e534
- 3: https://www.fuse.io/ecosystem-project/gaz-zip
- 4: https://news.fuse.io/fueling-the-future-gas-zip-bridge-integrates-with-fuse-network/
- 5: https://github.com/gasdotzip/gas-lz-contracts/blob/main/README.md
- 6: https://dev.gas.zip/gas/overview
- 7: https://docs.eclipse.xyz/users/getting-started/2.-bridge-assets-for-gas-and-transactions
- 8: https://docs.optimism.io/app-developers/bridging/standard-bridge
- 9: https://lz.gas.zip
- 10: https://etherscan.io/address/0x9a21e33f1a78b17dad32010cedb9fd2f071c17d3
- 11: https://aurora.dev/ecosystem/gas-zip
GasZip bridging receiver usage verified
Based on updated GasZip documentation, the system intentionally separates the placeholder value from the actual recipient address. In this bridge flow, using a hardcoded
NON_EVM_ADDRESSforbridgeData.receiveris acceptable since the true recipient is supplied viagasZipData.receiverAddress(and passed as the explicitrecipientparameter in the deposit call). Please double-check that the deposit logic (e.g., indepositToGasZipNative) correctly extracts and uses the recipient fromgasZipData.receiverAddress. This approach is consistent with GasZip’s design for multi-chain address handling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
script/demoScripts/demoGasZip.ts (3)
23-23: Clarify purpose of NON_EVM_ADDRESS constant.This hardcoded address appears to be a placeholder value, but it's used as the receiver in
bridgeDatawhileuserReceiveris used ingasZipData. This inconsistency may cause confusion.If this is intentionally different from the actual receiver used in
gasZipData, consider adding a comment explaining its purpose. Otherwise, consider usinguserReceiverconsistently in both places.
46-46: Log message doesn't accurately reflect all configured destination chains.The console log only mentions bridging to Berachain, but the code configures multiple destination chains (berachain, base, zksync).
Since you're setting up multiple destination chains, consider updating the log to reflect this:
- console.info(`Bridge ${amount} native from ${srcChain} --> Berachain`) + console.info(`Bridge ${amount} native from ${srcChain} --> Multiple chains (Berachain, Base, ZkSync)`)
65-68: Consider using the existing helper function for calculating destination chains.Based on previous review comments, there's a
getDestinationChainsValuefunction that could be used instead of manually calculating the destination chains value.According to previous reviewer comments, consider using the
getDestinationChainsValuefunction mentioned in the past review:- const dstChains = gasZipDestinationChainId.reduce( - (p, c) => (p << BigInt(16)) + BigInt(c), - BigInt(0) - ) + const dstChains = await lifiDiamondContract.read.getDestinationChainsValue([ + gasZipDestinationChainId + ])This would demonstrate the usage of the convenience function as suggested in previous reviews.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
script/demoScripts/demoGasZip.ts(1 hunks)src/Facets/GasZipFacet.sol(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Facets/GasZipFacet.sol
🧰 Additional context used
🧬 Code Definitions (1)
script/demoScripts/demoGasZip.ts (2)
script/demoScripts/utils/demoScriptChainConfig.ts (1)
SupportedChain(4-4)script/demoScripts/utils/demoScriptHelpers.ts (4)
setupEnvironment(579-620)addressToBytes32RightPadded(491-497)ensureBalance(741-766)executeTransaction(700-736)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run-unit-tests
- GitHub Check: enforce-min-test-coverage
🔇 Additional comments (3)
script/demoScripts/demoGasZip.ts (3)
28-29: LGTM! Clear distinction between native and GasZip chain IDs.The code correctly distinguishes between native destination chain IDs and GasZip-specific chain IDs with helpful comments, addressing previous review feedback.
70-73:❓ Verification inconclusive
Verify that receiverAddress is properly formatted for the destination chain.
The script is using
addressToBytes32RightPaddedto format an EVM address, but it's not clear if this is the correct format for the destination chain (Berachain).
🌐 Web query:
What address format does Berachain use? Is it compatible with Ethereum addresses?💡 Result:
Berachain uses the same address format as Ethereum, featuring a 0x-prefixed 40-character hexadecimal string (e.g.,
0xb794f5ea0ba39494ce839613fffba74279579268). This compatibility stems from Berachain’s EVM-identical architecture, which allows seamless interaction with Ethereum-based tools, wallets, and smart contracts.Key Details:
Address Format:
Identical to Ethereum’s format, ensuring interoperability with all EVM-compatible systems[4][5][8]. User wallets like MetaMask automatically support Berachain addresses after configuring the network[4].EVM Compatibility:
Berachain’s execution layer mirrors Ethereum’s Virtual Machine (EVM), enabling:
- Direct use of Ethereum addresses on Berachain[11][14].
- Deployment of unmodified Ethereum smart contracts[8][15].
- Native integration with Ethereum wallets (e.g., MetaMask, Gem Wallet)[4][5].
Practical Implications:
Developers can port Ethereum dApps to Berachain without changing address-handling logic[2][10]. Users retain their existing Ethereum addresses when interacting with Berachain’s ecosystem[5][14].For users and developers, this design eliminates friction when migrating assets or applications between Ethereum and Berachain.
Citations:
- 1: https://rocketreach.co/berachain-email-format_b728c8fbc43bfd3a
- 2: https://www.jetking.com/blog/driving-decentralized-app-growth
- 3: https://daic.capital/blog/berachain-in-depth-analysis-technical-architecture
- 4: https://docs.berachain.com/learn/connect-to-berachain
- 5: https://docs.gemwallet.com/blockchains/berachain/
- 6: https://beosin.com/ko/resources/exploring-berachain-analysis-of-its-native-defi-protocols-and-technical-key-points
- 7: https://docs.berachain.com/nodes/quickstart
- 8: https://www.ledger.com/academy/topics/crypto/what-is-berachain-and-how-does-it-work
- 9: https://blog.berachain.com/blog/how-to-deploy-and-verify-contracts-on-berachain
- 10: https://docs.berachain.com/developers/quickstart/
- 11: https://docs.berachain.com/learn/
- 12: https://www.imperator.co/resources/blog/berachain-presentation
- 13: https://blog.berachain.com/blog/your-berachain-rpc-guide
- 14: https://www.berachain.com
- 15: https://atomicwallet.io/academy/articles/berachain-crypto
- 16: https://ethplorer.io/address/0x82c6cff50b06617b48d4ae16747bedc93f8ec417
- 17: https://docs.alchemy.com/reference/berachain-api-faq
- 18: https://www.youtube.com/watch?v=fJ8y7Y15vT0
- 19: https://docs.berachain.com/developers/contracts/governance
- 20: https://docs.berachain.com/developers/contracts/honey-token
Action Required: Verify Receiver Address Format for Berachain
Based on the web query, Berachain uses the standard Ethereum address format (a 0x-prefixed 40-character hexadecimal string). Please confirm that theaddressToBytes32RightPaddedconversion applied touserReceiverproduces a value that is compatible with this requirement. In particular:
- Verify that the conversion does not inadvertently alter the conventional format (e.g. by producing an unexpected bytes32 representation when a 20-byte address is expected).
- If the destination contract on Berachain expects a standard Ethereum address (or its ABI-encoded 32-byte word with left-padding), adjust the conversion function accordingly.
58-59:❓ Verification inconclusive
Inconsistent receiver address usage.
NON_EVM_ADDRESSis used as the receiver inbridgeData, butuserReceiver(derived from the signer's address) is used as the receiver ingasZipData. This inconsistency may lead to unexpected behavior.
🏁 Script executed:
#!/bin/bash # Check how GasZipFacet contract uses the receiver addresses from both structs rg -A 5 -B 5 "startBridgeTokensViaGasZip" --type solidityLength of output: 90
I'll generate a new verification shell script that searches for the usage of
startBridgeTokensViaGasZipand the receiver address patterns (NON_EVM_ADDRESSanduserReceiver) in Solidity files without relying on the unrecognized file type. This should help us determine how both receiver addresses are used in the GasZipFacet contract.Please run the above script so we can manually verify whether the inconsistency mentioned in the review comment is present. Once you provide the output, we'll be able to update the review comment accordingly.
🏁 Script executed:
#!/bin/bash # Re-run search for the usage of startBridgeTokensViaGasZip across Solidity files rg -g "*.sol" -A 5 -B 5 "startBridgeTokensViaGasZip" # Additionally, search for occurrences of NON_EVM_ADDRESS and userReceiver in Solidity files rg -g "*.sol" -A 5 -B 5 "NON_EVM_ADDRESS" rg -g "*.sol" -A 5 -B 5 "userReceiver"Length of output: 34690
Subject: Verify Consistent Receiver Address Handling Between bridgeData and gasZipData
After reviewing the GasZipFacet and associated test files, it appears that the use of
NON_EVM_ADDRESSin thebridgeDatastruct is a deliberate marker for non-EVM flows. In GasZipFacet, when_bridgeData.receiverequalsNON_EVM_ADDRESS, the contract bypasses the check that would otherwise enforce that the gasZip data’s receiver address matches a user-derived (i.e. EVM) address. In contrast, for EVM flows the logic expects that the receiver ingasZipDatais the padded version of the signer’s address.The demo script in
script/demoScripts/demoGasZip.ts(lines 58-59) sets the receiver toNON_EVM_ADDRESSforbridgeData, yet it seems that elsewhere in the system (as seen in other facets or tests) a user’s EVM address (referred to asuserReceiver) might be used. This discrepancy could be intentional to differentiate between native (non-EVM) and EVM flows. However, if the design intent was for both structs to consistently represent the same receiver information, this mismatch might lead to unintended behavior.
- Action: Verify that when
bridgeData.receiveris set toNON_EVM_ADDRESS, the corresponding handling ofgasZipData.receiverAddress(possibly derived from a signed user address) aligns with the intended flow.- Action: If the design should enforce consistency between both structs in all contexts, consider aligning the receiver assignment logic to use the same source of truth.
Please review the design and verify that this behavior is intentional and that the documentation clearly describes the differences between EVM and non-EVM flows.
Which Jira task belongs to this PR?
LF-12743
Why did I implement it this way?
Example bridge transactions
Arbitrum to EVM Berachain - https://www.gas.zip/scan/tx/0xcafb1cfe25f4698e38707d2df9078913557e88b50fb09c42151b2581e9693ada
Arbitrum to non-EVM Solana - https://www.gas.zip/scan/tx/0x645f7c773c30e7c48b882d4f0aecbce967e460a41f5706a5881e760fb9a8e3bf
Arbitrum to non-EVM SUI - https://www.gas.zip/scan/tx/0xf722df97dfaa10aaf90ef36fedb37af6583a42627e5d1b9ebb626900c1757fb8
Arbitrum to EVM Berachain and Polygon
https://www.gas.zip/scan/tx/0x0b106f4705312d0a4100b9076ce2018471ec84f7a6048a79ceac9c7bbda3569d
Arbitrum to EVM Base, Berachain, ZkSync
https://www.gas.zip/scan/tx/0xe86ef5623e76f0821e6f46d8693646a8359ea43586ee3fad0f4dbbb2047623fd
Added documentation:
https://www.notion.so/lifi/GasZip-V1-1c2f0ff14ac780a3b67fd8f424454316
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)