refactor: disable ZETA related functions and remove call functions not using callOptions
#393
Conversation
lumtis
requested review from
andresaiello,
brewmaster012,
CharlieMc0,
fadeev,
skosito and
fbac
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughA new interface Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (11)
v2/contracts/Errors.sol (3)
4-6: Approve interface declaration with minor documentation fix.The interface declaration and its purpose are clear. However, there's a small typo in the documentation.
Please update the documentation comment as follows:
- /// @notice Interface for contracts that with non supported methods. + /// @notice Interface for contracts with non-supported methods.
7-7: Approve error declaration with suggestion for consideration.The
ZETANotSupportederror declaration is clear and aligns with the PR objective of disabling ZETA-related functions.Consider adding an optional message parameter to the error for more informative error reporting:
error ZETANotSupported(string message);This would allow for more context when the error is thrown, potentially easing debugging and improving user experience.
1-9: Summary: New interface aligns with PR objectives.The introduction of the
INotSupportedMethodsinterface with theZETANotSupportederror aligns well with the PR objective of disabling ZETA-related functions. This provides a standardized way to handle unsupported methods across the codebase, which should improve consistency and maintainability.As you implement this interface in other contracts, ensure consistent usage of the
ZETANotSupportederror for all ZETA-related functions that are being disabled. This will provide a uniform error handling mechanism throughout the codebase.v2/test/GatewayEVMZEVM.t.sol (2)
127-131: LGTM. Consider adjusting thegasLimitvalue.The initialization of
callOptionsis consistent with its addition as a state variable. However, thegasLimitvalue of 1 seems unusually low. Consider setting a more realistic gas limit for testing purposes, as this low value might not accurately represent real-world scenarios.
159-159: LGTM. Consider extractingCallOptionsfor improved readability.The update to use
CallOptionsin thegatewayZEVM.callfunction is consistent with the PR objectives. However, creating theCallOptionsinline might reduce readability. Consider extracting this to a separate variable or using the state variablecallOptionsif appropriate.v2/test/GatewayEVM.t.sol (2)
506-510: Reminder to address the TODO comment for future implementationThe TODO comment indicates that this test should be updated once ZETA support is re-enabled (see issue #394). Consider tracking this with a GitHub issue to ensure it is revisited.
Would you like me to open a new GitHub issue to track this TODO?
514-515: Consider removing commented out code for clarityThe lines checking the owner's ZETA balance after deposit are commented out. Since the deposit of ZETA is currently not supported and the test expects a revert, consider removing these lines to keep the codebase clean.
v2/test/GatewayZEVM.t.sol (4)
309-313: Reminder to update tests when ZETA support is restoredThe test currently expects a revert with
ZETANotSupportedand includes a TODO to replace the error check once ZETA support returns. Ensure that these tests are revisited and updated accordingly when ZETA functionality is reintroduced.Would you like me to open a GitHub issue to track this task?
329-333: Reminder to update tests for ZETA withdrawal failuresSimilar to earlier, this test is adjusted to expect
ZETANotSupporteddue to the temporary disabling of ZETA. Please remember to update these tests when re-enabling ZETA support.Would you like assistance in updating these tests once ZETA is supported again?
Line range hint
378-395: Commented out assertions may hide potential issuesIn the
testWithdrawZETAfunction, several assertions are commented out. This may prevent detection of issues in future changes.Consider uncommenting the assertions or updating the test to reflect the current state.
467-467: Ensure test reflects current ZETA support statusThe test expects a revert with
ZETANotSupported. Verify that this is the correct behavior and update the test as necessary when ZETA support is reinstated.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
v2/soldeer.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
- v2/contracts/Errors.sol (1 hunks)
- v2/contracts/evm/GatewayEVM.sol (3 hunks)
- v2/contracts/zevm/GatewayZEVM.sol (3 hunks)
- v2/contracts/zevm/interfaces/IGatewayZEVM.sol (0 hunks)
- v2/test/GatewayEVM.t.sol (2 hunks)
- v2/test/GatewayEVMZEVM.t.sol (5 hunks)
- v2/test/GatewayZEVM.t.sol (10 hunks)
- v2/test/utils/SenderZEVM.sol (3 hunks)
💤 Files with no reviewable changes (1)
- v2/contracts/zevm/interfaces/IGatewayZEVM.sol
🧰 Additional context used
🔇 Additional comments (18)
v2/contracts/Errors.sol (1)
1-2: LGTM: Appropriate license and Solidity version.The MIT license is suitable for open-source projects, and using a specific, recent Solidity version (0.8.26) is good for consistency and security.
v2/test/utils/SenderZEVM.sol (4)
5-5: LGTM: Import statement for CallOptions and RevertOptions added.The new import statement is necessary and consistent with the changes made in the functions below. It correctly imports the required structures from the IGatewayZEVM interface.
5-5: Summary: Consistent implementation of CallOptions with a concern.The changes in this file consistently implement the use of
CallOptionsin bothcallReceiverandwithdrawAndCallReceiverfunctions. The new import statement and the updates to the function calls are correct and align with the PR objectives.However, there's a consistent use of a very low
gasLimit(1) in both functions. While this consistency suggests it might be intentional, it's important to verify if this value is appropriate for the intended use case. Such a low gas limit could potentially cause issues in execution.Please confirm if the
gasLimitof 1 is intentional and suitable for these test functions. If not, consider adjusting it to a more practical value.Also applies to: 42-46, 50-50, 85-89, 91-91
85-89: Consistent use of CallOptions, but verify gas limit.The changes to use
CallOptionsinwithdrawAndCallReceiverare correct and consistent with thecallReceiverfunction. However, thegasLimitof 1 is still present and might be too low for practical use. Please verify if this is intentional or if it should be adjusted.#!/bin/bash # Description: Check for other occurrences of CallOptions in the codebase to compare gas limit values echo "Searching for CallOptions usage in the codebase:" rg --type solidity 'CallOptions\s*\{[^}]*gasLimit:' .Also applies to: 91-91
42-46: Verify the gas limit in CallOptions.The changes to use
CallOptionsare correct and align with the updated interface. However, thegasLimitof 1 seems unusually low and might cause issues in execution. Please verify if this is intentional or if it should be set to a more reasonable value.Also applies to: 50-50
✅ Verification successful
Gas Limit Configuration Verified
The usage of
CallOptionswithgasLimit: 1is consistent across multiple test files in the codebase. This indicates that the low gas limit is intentional and standard for testing purposes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other occurrences of gasLimit in test files to compare values echo "Searching for gasLimit usage in test files:" rg --type solidity 'gasLimit:' test/Length of output: 195
Script:
#!/bin/bash # Description: Check for other occurrences of gasLimit in Solidity test files echo "Searching for gasLimit usage in Solidity test files:" rg -g "*.sol" 'gasLimit:' test/Length of output: 192
Script:
#!/bin/bash # Description: Check for other occurrences of gasLimit in all Solidity files echo "Searching for gasLimit usage in all Solidity files:" rg -g "*.sol" 'gasLimit:' .Length of output: 3561
v2/test/GatewayEVMZEVM.t.sol (4)
62-63: LGTM. Please clarify the purpose ofRevertOptions.The addition of the
callOptionsvariable aligns with the PR objectives. However, theRevertOptionsvariable wasn't mentioned in the summary. Could you please provide more context on its purpose and how it relates to the overall changes?
178-182: LGTM. Consistent use ofCallOptions.The update to the function signature in the encoded data correctly incorporates the
CallOptionsstructure, aligning with the PR objectives. The use of thecallOptionsvariable (presumably the state variable) maintains consistency and improves the test's alignment with the updated function signature.
255-260: LGTM. Consistent implementation ofCallOptions.The update to the function signature in the encoded data correctly incorporates the
CallOptionsstructure, aligning with the PR objectives. This change is consistent with the similar modification made in thetestCallReceiverEVMFromSenderZEVMfunction, maintaining a uniform approach throughout the test file.
Line range hint
1-283: Overall LGTM. Successfully incorporatedCallOptions.The changes in this file successfully incorporate the
CallOptionsstructure as intended, aligning well with the PR objectives. The implementation is consistent across different test functions, improving the contract's functionality and testability. While there are a few minor suggestions for improvement (such as clarifying the purpose ofRevertOptionsand considering a more realisticgasLimit), the overall changes are well-implemented and maintain the existing test coverage.v2/contracts/zevm/GatewayZEVM.sol (2)
6-6: ImportingINotSupportedMethodsInterfaceThe import statement correctly includes the
INotSupportedMethodsinterface, allowing the contract to implement necessary error handling for unsupported methods.
27-28: AddingPausableUpgradeableandINotSupportedMethodsto InheritanceThe contract now inherits from
PausableUpgradeableandINotSupportedMethods, providing pausing functionality and implementing the interface for unsupported methods. This enhances the contract's capabilities and aligns with best practices.v2/contracts/evm/GatewayEVM.sol (2)
4-4: ImportingINotSupportedMethodsInterfaceThe addition of the
INotSupportedMethodsimport allows the contract to implement standardized error handling for unsupported methods.
27-28: Updated Inheritance withPausableUpgradeableandINotSupportedMethodsThe contract now inherits from
PausableUpgradeableandINotSupportedMethods. Ensure that the pause functionality is correctly initialized and that all required functions from the interface are properly implemented.v2/test/GatewayEVM.t.sol (2)
415-415: Definition ofZETANotSupportederror is appropriateThe error declaration correctly defines
ZETANotSupported, which is used to handle unsupported ZETA operations.
506-510: Update to test reflects current unsupported ZETA operationsThe test now correctly expects a revert with
ZETANotSupportedwhen attempting to deposit ZETA, which aligns with the current functionality.v2/test/GatewayZEVM.t.sol (3)
35-35: Appropriate addition ofZETANotSupportederrorThe addition of the
ZETANotSupportederror correctly captures unsupported ZETA operations and aligns with the changes in handling unsupported methods.
340-343: Ensure consistent error handling for ZETA operationsThe expected revert is changed to
ZETANotSupported, replacingInsufficientZetaAmount. Confirm that this aligns with the intended behavior for unsupported ZETA operations.
Line range hint
173-206: Ensure updated function signatures are used correctlyThe function
gateway.withdrawAndCallis updated to accept aCallOptionsstruct. Please verify that all calls to this function correctly utilize the newCallOptionsparameter and that test cases reflect the updated functionality.Run the following script to check for correct usage:
✅ Verification successful
All
withdrawAndCallfunction calls correctly utilize the updatedCallOptionsparameter.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check all calls to `withdrawAndCall` to ensure they use the updated `CallOptions` struct. # Test: Search for `withdrawAndCall` function calls. # Expect: All calls should include `CallOptions` parameter. rg --type solidity -A2 'gateway\.withdrawAndCall\('Length of output: 86
Script:
#!/bin/bash # Description: Check all calls to `withdrawAndCall` in Solidity files to ensure they use the updated `CallOptions` struct. # Find all .sol files and search for `withdrawAndCall` function calls. fd --extension sol --type file | xargs rg 'gateway\.withdrawAndCall\(' -A2Length of output: 4253
There was a problem hiding this comment.
looks good, regarding your comment in description, i also did a bit of cleaning up those duplicate methods here couple days ago, i think i removed correct tests and tests were passing, maybe it can help: 752dde4#diff-6b934f508cf9d7ef1dbd0d3a31087c6a8dc330fa646dbb07d7477fcb024cc253
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
v2/contracts/zevm/GatewayZEVM.sol (5)
6-6: LGTM: Addition ofINotSupportedMethodsinterfaceThe addition of the
INotSupportedMethodsinterface and its implementation in theGatewayZEVMcontract aligns with the PR objective of disabling ZETA-related functions. This change provides a standardized way to handle unsupported methods.Consider updating the contract's documentation to reflect this change and explain which methods are no longer supported and why.
Also applies to: 28-28
205-239: LGTM: Disabling ZETA withdrawal functionalityThe changes to the
withdrawfunction effectively disable ZETA withdrawals, which aligns with the PR objectives. The addition of theZETANotSupportederror provides a clear indication to users that this functionality is currently unavailable.Consider removing the commented-out code to improve code readability. The original implementation can be retrieved from version control if needed in the future. If you prefer to keep it for now, add a comment explaining why it's being retained and when it might be removed or re-enabled.
243-274: LGTM: Disabling ZETA withdrawAndCall functionalityThe changes to the
withdrawAndCallfunction effectively disable ZETA withdrawals and subsequent contract calls, which aligns with the PR objectives. The addition of theZETANotSupportederror provides a clear indication to users that this functionality is currently unavailable.As with the
withdrawfunction, consider removing the commented-out code to improve code readability. The original implementation can be retrieved from version control if needed in the future. If you prefer to keep it for now, add a comment explaining why it's being retained and when it might be removed or re-enabled.
Line range hint
282-293: LGTM: Updatedcallfunction to useCallOptionsThe modification of the
callfunction to useCallOptionsinstead of a separategasLimitparameter is a good improvement. This change simplifies the function signature and provides more flexibility for future extensions of call options.Consider updating the function's documentation to reflect the new
CallOptionsparameter and explain what options are available within this structure.
Line range hint
1-465: Summary: Disabling ZETA functions and updating call optionsThe changes in this file successfully achieve the PR objectives:
- ZETA-related withdrawal functions (
withdrawandwithdrawAndCall) have been disabled and now revert with aZETANotSupportederror.- The
callfunction has been updated to useCallOptions, improving flexibility and consistency.These modifications enhance the contract's alignment with current requirements while maintaining a clear path for future updates through TODO comments and linked issues.
Consider the following suggestions for future improvements:
- Once the ZETA functionality is ready to be re-enabled, ensure that all disabled functions are thoroughly tested and updated.
- If the commented-out code for ZETA functions is kept long-term, consider moving it to a separate file or document to maintain cleaner code in the main contract.
- Update the contract's documentation to clearly indicate which functions are currently disabled and provide information on alternative methods users should use in the meantime.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (144)
v2/docs/src/SUMMARY.mdis excluded by!v2/docs/**v2/docs/src/contracts/Errors.sol/interface.INotSupportedMethods.mdis excluded by!v2/docs/**v2/docs/src/contracts/README.mdis excluded by!v2/docs/**v2/docs/src/contracts/Revert.sol/interface.Revertable.mdis excluded by!v2/docs/**v2/docs/src/contracts/Revert.sol/struct.RevertContext.mdis excluded by!v2/docs/**v2/docs/src/contracts/Revert.sol/struct.RevertOptions.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ERC20Custody.sol/contract.ERC20Custody.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/GatewayEVM.sol/contract.GatewayEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ZetaConnectorBase.sol/abstract.ZetaConnectorBase.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ZetaConnectorNative.sol/contract.ZetaConnectorNative.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ZetaConnectorNonNative.sol/contract.ZetaConnectorNonNative.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IERC20Custody.sol/interface.IERC20Custody.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IERC20Custody.sol/interface.IERC20CustodyErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IERC20Custody.sol/interface.IERC20CustodyEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.Callable.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.IGatewayEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.IGatewayEVMErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.IGatewayEVMEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/struct.MessageContext.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IZetaConnector.sol/interface.IZetaConnectorEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IZetaNonEthNew.sol/interface.IZetaNonEthNew.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/GatewayZEVM.sol/contract.GatewayZEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/SystemContract.sol/contract.SystemContract.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/SystemContract.sol/interface.SystemContractErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/ZRC20.sol/contract.ZRC20.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/ZRC20.sol/interface.ZRC20Errors.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/interface.IGatewayZEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/interface.IGatewayZEVMErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/interface.IGatewayZEVMEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/struct.CallOptions.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/ISystem.sol/interface.ISystem.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IWZETA.sol/interface.IWETH9.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/enum.CoinType.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/interface.IZRC20.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/interface.IZRC20Metadata.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/interface.ZRC20Events.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/interface.UniversalContract.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/interface.zContract.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/struct.zContext.mdis excluded by!v2/docs/**v2/docs/src/index.mdis excluded by!v2/docs/**v2/pkg/address.sol/address.gois excluded by!v2/pkg/**v2/pkg/beaconproxy.sol/beaconproxy.gois excluded by!v2/pkg/**v2/pkg/console.sol/console.gois excluded by!v2/pkg/**v2/pkg/core.sol/core.gois excluded by!v2/pkg/**v2/pkg/defender.sol/defender.gois excluded by!v2/pkg/**v2/pkg/defenderdeploy.sol/defenderdeploy.gois excluded by!v2/pkg/**v2/pkg/erc1967proxy.sol/erc1967proxy.gois excluded by!v2/pkg/**v2/pkg/erc1967utils.sol/erc1967utils.gois excluded by!v2/pkg/**v2/pkg/erc20custody.sol/erc20custody.gois excluded by!v2/pkg/**v2/pkg/erc20custody.t.sol/erc20custodytest.gois excluded by!v2/pkg/**v2/pkg/erc20custodyupgradetest.sol/erc20custodyupgradetest.gois excluded by!v2/pkg/**v2/pkg/errors.sol/inotsupportedmethods.gois excluded by!v2/pkg/**v2/pkg/gatewayevm.sol/gatewayevm.gois excluded by!v2/pkg/**v2/pkg/gatewayevm.t.sol/gatewayevminboundtest.gois excluded by!v2/pkg/**v2/pkg/gatewayevm.t.sol/gatewayevmtest.gois excluded by!v2/pkg/**v2/pkg/gatewayevmupgradetest.sol/gatewayevmupgradetest.gois excluded by!v2/pkg/**v2/pkg/gatewayevmzevm.t.sol/gatewayevmzevmtest.gois excluded by!v2/pkg/**v2/pkg/gatewayzevm.sol/gatewayzevm.gois excluded by!v2/pkg/**v2/pkg/gatewayzevm.t.sol/gatewayzevminboundtest.gois excluded by!v2/pkg/**v2/pkg/gatewayzevm.t.sol/gatewayzevmoutboundtest.gois excluded by!v2/pkg/**v2/pkg/gatewayzevmupgradetest.sol/gatewayzevmupgradetest.gois excluded by!v2/pkg/**v2/pkg/igatewayzevm.sol/igatewayzevm.gois excluded by!v2/pkg/**v2/pkg/math.sol/math.gois excluded by!v2/pkg/**v2/pkg/mockerc20.sol/mockerc20.gois excluded by!v2/pkg/**v2/pkg/mockerc721.sol/mockerc721.gois excluded by!v2/pkg/**v2/pkg/proxyadmin.sol/proxyadmin.gois excluded by!v2/pkg/**v2/pkg/receiverevm.sol/receiverevm.gois excluded by!v2/pkg/**v2/pkg/safeconsole.sol/safeconsole.gois excluded by!v2/pkg/**v2/pkg/safeerc20.sol/safeerc20.gois excluded by!v2/pkg/**v2/pkg/senderzevm.sol/senderzevm.gois excluded by!v2/pkg/**v2/pkg/signedmath.sol/signedmath.gois excluded by!v2/pkg/**v2/pkg/src/strings.sol/strings.gois excluded by!v2/pkg/**v2/pkg/stderror.sol/stderror.gois excluded by!v2/pkg/**v2/pkg/stdjson.sol/stdjson.gois excluded by!v2/pkg/**v2/pkg/stdmath.sol/stdmath.gois excluded by!v2/pkg/**v2/pkg/stdstorage.sol/stdstorage.gois excluded by!v2/pkg/**v2/pkg/stdstorage.sol/stdstoragesafe.gois excluded by!v2/pkg/**v2/pkg/stdstyle.sol/stdstyle.gois excluded by!v2/pkg/**v2/pkg/stdtoml.sol/stdtoml.gois excluded by!v2/pkg/**v2/pkg/storageslot.sol/storageslot.gois excluded by!v2/pkg/**v2/pkg/strings.sol/strings.gois excluded by!v2/pkg/**v2/pkg/systemcontract.sol/systemcontract.gois excluded by!v2/pkg/**v2/pkg/systemcontractmock.sol/systemcontractmock.gois excluded by!v2/pkg/**v2/pkg/testerc20.sol/testerc20.gois excluded by!v2/pkg/**v2/pkg/testuniversalcontract.sol/testuniversalcontract.gois excluded by!v2/pkg/**v2/pkg/transparentupgradeableproxy.sol/transparentupgradeableproxy.gois excluded by!v2/pkg/**v2/pkg/upgradeablebeacon.sol/upgradeablebeacon.gois excluded by!v2/pkg/**v2/pkg/upgrades.sol/unsafeupgrades.gois excluded by!v2/pkg/**v2/pkg/upgrades.sol/upgrades.gois excluded by!v2/pkg/**v2/pkg/utils.sol/utils.gois excluded by!v2/pkg/**v2/pkg/versions.sol/versions.gois excluded by!v2/pkg/**v2/pkg/wzeta.sol/weth9.gois excluded by!v2/pkg/**v2/pkg/zeta.non-eth.sol/zetanoneth.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornative.sol/zetaconnectornative.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornative.t.sol/zetaconnectornativetest.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornativeupgradetest.sol/zetaconnectornativeupgradetest.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornonnative.sol/zetaconnectornonnative.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornonnative.t.sol/zetaconnectornonnativetest.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornonnativeupgradetest.sol/zetaconnectornonnativeupgradetest.gois excluded by!v2/pkg/**v2/pkg/zrc20.sol/zrc20.gois excluded by!v2/pkg/**v2/pkg/zrc20.t.sol/zrc20test.gois excluded by!v2/pkg/**v2/soldeer.lockis excluded by!**/*.lockv2/types/Errors.sol/INotSupportedMethods.tsis excluded by!v2/types/**v2/types/Errors.sol/index.tsis excluded by!v2/types/**v2/types/GatewayZEVM.tsis excluded by!v2/types/**v2/types/IGatewayZEVM.sol/IGatewayZEVM.tsis excluded by!v2/types/**v2/types/factories/Address__factory.tsis excluded by!v2/types/**v2/types/factories/BeaconProxy__factory.tsis excluded by!v2/types/**v2/types/factories/ERC1967Proxy__factory.tsis excluded by!v2/types/**v2/types/factories/ERC1967Utils__factory.tsis excluded by!v2/types/**v2/types/factories/ERC20CustodyUpgradeTest__factory.tsis excluded by!v2/types/**v2/types/factories/ERC20Custody__factory.tsis excluded by!v2/types/**v2/types/factories/Errors.sol/INotSupportedMethods__factory.tsis excluded by!v2/types/**v2/types/factories/Errors.sol/index.tsis excluded by!v2/types/**v2/types/factories/GatewayEVMUpgradeTest__factory.tsis excluded by!v2/types/**v2/types/factories/GatewayEVM__factory.tsis excluded by!v2/types/**v2/types/factories/GatewayZEVMUpgradeTest__factory.tsis excluded by!v2/types/**v2/types/factories/GatewayZEVM__factory.tsis excluded by!v2/types/**v2/types/factories/IGatewayZEVM.sol/IGatewayZEVM__factory.tsis excluded by!v2/types/**v2/types/factories/Math__factory.tsis excluded by!v2/types/**v2/types/factories/MockERC20__factory.tsis excluded by!v2/types/**v2/types/factories/MockERC721__factory.tsis excluded by!v2/types/**v2/types/factories/ProxyAdmin__factory.tsis excluded by!v2/types/**v2/types/factories/ReceiverEVM__factory.tsis excluded by!v2/types/**v2/types/factories/SafeERC20__factory.tsis excluded by!v2/types/**v2/types/factories/SenderZEVM__factory.tsis excluded by!v2/types/**v2/types/factories/StdError.sol/StdError__factory.tsis excluded by!v2/types/**v2/types/factories/StdStorage.sol/StdStorageSafe__factory.tsis excluded by!v2/types/**v2/types/factories/Strings__factory.tsis excluded by!v2/types/**v2/types/factories/SystemContract.sol/SystemContract__factory.tsis excluded by!v2/types/**v2/types/factories/SystemContractMock.sol/SystemContractMock__factory.tsis excluded by!v2/types/**v2/types/factories/TestERC20__factory.tsis excluded by!v2/types/**v2/types/factories/TestUniversalContract__factory.tsis excluded by!v2/types/**v2/types/factories/TransparentUpgradeableProxy.sol/TransparentUpgradeableProxy__factory.tsis excluded by!v2/types/**v2/types/factories/UpgradeableBeacon__factory.tsis excluded by!v2/types/**v2/types/factories/WZETA.sol/WETH9__factory.tsis excluded by!v2/types/**v2/types/factories/ZRC20.sol/ZRC20__factory.tsis excluded by!v2/types/**v2/types/factories/Zeta.non-eth.sol/ZetaNonEth__factory.tsis excluded by!v2/types/**v2/types/factories/ZetaConnectorNativeUpgradeTest__factory.tsis excluded by!v2/types/**v2/types/factories/ZetaConnectorNative__factory.tsis excluded by!v2/types/**v2/types/factories/ZetaConnectorNonNativeUpgradeTest__factory.tsis excluded by!v2/types/**v2/types/factories/ZetaConnectorNonNative__factory.tsis excluded by!v2/types/**v2/types/factories/index.tsis excluded by!v2/types/**v2/types/index.tsis excluded by!v2/types/**
📒 Files selected for processing (6)
- v2/contracts/Errors.sol (1 hunks)
- v2/contracts/evm/GatewayEVM.sol (3 hunks)
- v2/contracts/zevm/GatewayZEVM.sol (3 hunks)
- v2/test/GatewayEVMZEVM.t.sol (5 hunks)
- v2/test/GatewayZEVM.t.sol (10 hunks)
- v2/test/utils/SenderZEVM.sol (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- v2/contracts/Errors.sol
- v2/contracts/evm/GatewayEVM.sol
- v2/test/GatewayZEVM.t.sol
- v2/test/utils/SenderZEVM.sol
🧰 Additional context used
🔇 Additional comments (4)
v2/test/GatewayEVMZEVM.t.sol (4)
62-63: LGTMThe declaration of
callOptionsandrevertOptionsis appropriate and aligns with the integration of theCallOptionsstructure into the tests.
156-162: Verify sufficientgasLimitforgatewayZEVM.callThe
CallOptionsused here setsgasLimitto1. Similar to earlier, this value may not provide enough gas for the call to complete, especially withisArbitraryCallset totrue. Please ensure that thegasLimitis adequate for the complexity of the transaction.
181-185: EnsuregasLimitincallOptionsis adequateThe
callOptionspassed in this function invocation have agasLimitof1. As previously mentioned, this may not be sufficient for the transaction to execute without issues. Verify that thegasLimitis set appropriately for the function being called.
258-263: ConfirmgasLimitsufficiency inwithdrawAndCallThe
callOptionsforwithdrawAndCallalso use agasLimitof1. Consistent with earlier comments, please verify that thisgasLimitis sufficient for the function to execute as expected.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #393 +/- ##
==========================================
- Coverage 84.95% 84.23% -0.72%
==========================================
Files 8 8
Lines 412 387 -25
Branches 138 121 -17
==========================================
- Hits 350 326 -24
+ Misses 62 61 -1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
v2/contracts/zevm/GatewayZEVM.sol (2)
205-239: Approved: ZETA withdrawal disabled as per PR objectivesThe changes to the
withdrawfunction for ZETA tokens effectively disable this functionality, which aligns with the PR objectives. The addition of theZETANotSupportederror provides clear feedback to callers.However, consider cleaning up the commented-out code in a future PR to improve code maintainability. The TODO comment and issue link provide sufficient context for future reference.
In a future PR, consider removing the commented-out code to keep the codebase clean. The version control system can be used to retrieve the previous implementation if needed.
Line range hint
1-461: Summary: Changes align well with PR objectivesThe modifications to the
GatewayZEVMcontract successfully achieve the PR objectives:
- ZETA-related functions (
withdrawandwithdrawAndCall) have been disabled and now revert with a clear error message.- The introduction of the
INotSupportedMethodsinterface provides a structured way to handle unsupported operations.- The
callfunction has been updated to useCallOptions, improving consistency across the contract.These changes effectively streamline the codebase by disabling certain functions and removing outdated methods.
Consider creating a follow-up task to clean up the commented-out code in the
withdrawandwithdrawAndCallfunctions. While this code provides useful context for the recent changes, it may impact code readability and maintainability in the long term. The version control system can be used to retrieve previous implementations if needed in the future.v2/test/GatewayZEVM.t.sol (2)
358-362: LGTM: Updated error handling for ZETA withdrawal.The error expectation has been changed from
InsufficientZetaAmounttoZETANotSupported, aligning with the PR objective of disabling ZETA-related functions. The TODO comment provides good context for this change.Would you like me to create a GitHub issue to track the TODO for replacing this error check once ZETA support is restored?
Line range hint
427-454: LGTM: Disabled ZETA withdrawal test.The ZETA withdrawal test has been significantly modified, with most of the logic commented out and a new expectation for the
ZETANotSupportederror added. This aligns with the PR objective of disabling ZETA-related functions.Would you like me to create a GitHub issue to track the TODO for restoring this test once ZETA support is back?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (67)
v2/docs/src/SUMMARY.mdis excluded by!v2/docs/**v2/docs/src/contracts/Errors.sol/interface.INotSupportedMethods.mdis excluded by!v2/docs/**v2/docs/src/contracts/Revert.sol/interface.Revertable.mdis excluded by!v2/docs/**v2/docs/src/contracts/Revert.sol/struct.RevertContext.mdis excluded by!v2/docs/**v2/docs/src/contracts/Revert.sol/struct.RevertOptions.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ERC20Custody.sol/contract.ERC20Custody.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/GatewayEVM.sol/contract.GatewayEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ZetaConnectorBase.sol/abstract.ZetaConnectorBase.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ZetaConnectorNative.sol/contract.ZetaConnectorNative.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/ZetaConnectorNonNative.sol/contract.ZetaConnectorNonNative.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IERC20Custody.sol/interface.IERC20Custody.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IERC20Custody.sol/interface.IERC20CustodyErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IERC20Custody.sol/interface.IERC20CustodyEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.Callable.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.IGatewayEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.IGatewayEVMErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.IGatewayEVMEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/struct.MessageContext.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IZetaConnector.sol/interface.IZetaConnectorEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IZetaNonEthNew.sol/interface.IZetaNonEthNew.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/GatewayZEVM.sol/contract.GatewayZEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/SystemContract.sol/contract.SystemContract.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/SystemContract.sol/interface.SystemContractErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/ZRC20.sol/contract.ZRC20.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/ZRC20.sol/interface.ZRC20Errors.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/interface.IGatewayZEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/interface.IGatewayZEVMErrors.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/interface.IGatewayZEVMEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/struct.CallOptions.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/ISystem.sol/interface.ISystem.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IWZETA.sol/interface.IWETH9.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/enum.CoinType.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/interface.IZRC20.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/interface.IZRC20Metadata.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IZRC20.sol/interface.ZRC20Events.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/interface.UniversalContract.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/interface.zContract.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/struct.MessageContext.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/struct.zContext.mdis excluded by!v2/docs/**v2/docs/src/index.mdis excluded by!v2/docs/**v2/pkg/erc20custody.t.sol/erc20custodytest.gois excluded by!v2/pkg/**v2/pkg/gatewayevm.sol/gatewayevm.gois excluded by!v2/pkg/**v2/pkg/gatewayevm.t.sol/gatewayevminboundtest.gois excluded by!v2/pkg/**v2/pkg/gatewayevm.t.sol/gatewayevmtest.gois excluded by!v2/pkg/**v2/pkg/gatewayevmzevm.t.sol/gatewayevmzevmtest.gois excluded by!v2/pkg/**v2/pkg/gatewayzevm.sol/gatewayzevm.gois excluded by!v2/pkg/**v2/pkg/gatewayzevm.t.sol/gatewayzevminboundtest.gois excluded by!v2/pkg/**v2/pkg/gatewayzevm.t.sol/gatewayzevmoutboundtest.gois excluded by!v2/pkg/**v2/pkg/gatewayzevmupgradetest.sol/gatewayzevmupgradetest.gois excluded by!v2/pkg/**v2/pkg/igatewayzevm.sol/igatewayzevm.gois excluded by!v2/pkg/**v2/pkg/senderzevm.sol/senderzevm.gois excluded by!v2/pkg/**v2/pkg/systemcontract.sol/systemcontract.gois excluded by!v2/pkg/**v2/pkg/systemcontractmock.sol/systemcontractmock.gois excluded by!v2/pkg/**v2/pkg/testuniversalcontract.sol/testuniversalcontract.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornative.t.sol/zetaconnectornativetest.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornonnative.t.sol/zetaconnectornonnativetest.gois excluded by!v2/pkg/**v2/pkg/zrc20.t.sol/zrc20test.gois excluded by!v2/pkg/**v2/types/GatewayZEVM.tsis excluded by!v2/types/**v2/types/IGatewayZEVM.sol/IGatewayZEVM.tsis excluded by!v2/types/**v2/types/factories/GatewayEVM__factory.tsis excluded by!v2/types/**v2/types/factories/GatewayZEVMUpgradeTest__factory.tsis excluded by!v2/types/**v2/types/factories/GatewayZEVM__factory.tsis excluded by!v2/types/**v2/types/factories/IGatewayZEVM.sol/IGatewayZEVM__factory.tsis excluded by!v2/types/**v2/types/factories/SenderZEVM__factory.tsis excluded by!v2/types/**v2/types/factories/SystemContract.sol/SystemContract__factory.tsis excluded by!v2/types/**v2/types/factories/SystemContractMock.sol/SystemContractMock__factory.tsis excluded by!v2/types/**v2/types/factories/TestUniversalContract__factory.tsis excluded by!v2/types/**
📒 Files selected for processing (3)
- v2/contracts/zevm/GatewayZEVM.sol (3 hunks)
- v2/contracts/zevm/interfaces/IGatewayZEVM.sol (0 hunks)
- v2/test/GatewayZEVM.t.sol (10 hunks)
💤 Files with no reviewable changes (1)
- v2/contracts/zevm/interfaces/IGatewayZEVM.sol
🧰 Additional context used
🔇 Additional comments (12)
v2/contracts/zevm/GatewayZEVM.sol (3)
6-6: LGTM: New interface implementation aligns with PR objectivesThe addition of the
INotSupportedMethodsinterface and its implementation in theGatewayZEVMcontract is in line with the PR's goal of disabling ZETA-related functions. This change provides a structured way to handle unsupported methods.Also applies to: 28-28
243-274: Approved: ZETA withdrawAndCall disabled as per PR objectivesThe changes to the
withdrawAndCallfunction for ZETA tokens mirror those made to thewithdrawfunction, effectively disabling this functionality as intended. This aligns with the PR objectives.As with the
withdrawfunction, consider cleaning up the commented-out code in a future PR to improve code maintainability. The TODO comment and issue link provide sufficient context for future reference.The suggestion to clean up commented-out code in a future PR applies here as well, for the same reasons mentioned in the previous comment.
Line range hint
284-289: LGTM: Improved consistency with CallOptionsThe update to the
callfunction signature, replacing the separategasLimitparameter with theCallOptionsstructure, improves consistency across the contract. This change aligns well with the use ofCallOptionsin other parts of the contract and simplifies the function interface.v2/test/GatewayZEVM.t.sol (9)
35-35: LGTM: New error for ZETA functionality.The addition of the
ZETANotSupportederror aligns with the PR objective of disabling ZETA-related functions.
173-180: LGTM: Updated function call using CallOptions struct.The
withdrawAndCallfunction now uses aCallOptionsstruct, which aligns with the PR objective of updating function calls to usecallOptions. This change improves code structure and readability.
188-195: LGTM: Consistent use of CallOptions struct.This
withdrawAndCallfunction call has been updated to use theCallOptionsstruct, maintaining consistency with the previous change and aligning with the PR objectives.
201-208: LGTM: Consistent update to use CallOptions struct.This
withdrawAndCallfunction call has been updated to use theCallOptionsstruct, maintaining consistency with the previous changes and aligning with the PR objectives.
214-221: LGTM: Consistent use of CallOptions struct.This
withdrawAndCallfunction call has been updated to use theCallOptionsstruct, maintaining consistency with the previous changes and aligning with the PR objectives.
234-241: LGTM: Consistent update to use CallOptions struct.This
withdrawAndCallfunction call has been updated to use theCallOptionsstruct, maintaining consistency with the previous changes and aligning with the PR objectives.
268-275: LGTM: Consistent use of CallOptions struct.This
withdrawAndCallfunction call has been updated to use theCallOptionsstruct, maintaining consistency with the previous changes and aligning with the PR objectives.
368-373: LGTM: Consistent update to error handling for ZETA withdrawal.The error expectation has been changed from
MessageSizeExceededtoZETANotSupported, maintaining consistency with the previous change and aligning with the PR objective of disabling ZETA-related functions. The TODO comment provides good context for this change.
378-382: LGTM: Consistent update to error handling for ZETA withdrawal.The error expectation has been changed from
ZeroAddresstoZETANotSupported, maintaining consistency with the previous changes and aligning with the PR objective of disabling ZETA-related functions. The TODO comment provides good context for this change.
Not running generate now to simplify review.
Opening for review now, there is a test error happening in these lines:
https://github.com/zeta-chain/protocol-contracts/pull/393/files#diff-2605cea5bc2def7c46e3d80e4eaef42116a8c377202c5645d231f1bd7daf4726R178
https://github.com/zeta-chain/protocol-contracts/pull/393/files#diff-2605cea5bc2def7c46e3d80e4eaef42116a8c377202c5645d231f1bd7daf4726R255
-> replacing the gasLimit with callOptions for withdraw tests. Gonna investigate further later, maybe you have an idea @skosito or are these tests still needed at all?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests