fix: address warnings in build #399
Conversation
lumtis
changed the title
lumtis
requested review from
andresaiello,
brewmaster012,
CharlieMc0,
fadeev,
skosito and
fbac
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request includes modifications to several contracts and test files within the ZetaChain protocol. Key changes involve the removal of the 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 (10)
v2/test/fuzz/ERC20CustodyEchidnaTest.sol (1)
Entire test file is commented out intentionally but reduces test coverage.
The
ERC20CustodyEchidnaTest.solfile is fully commented out as per issue #384, indicating that the tests are currently broken and not in use. This action reduces the test coverage for theERC20Custodycontract, which may lead to undetected issues.🔗 Analysis chain
Line range hint
1-38: Entire file is commented out. Verify if this is intentional.The entire content of this file has been commented out. This appears to be related to the PR objective of addressing warnings in builds. However, commenting out an entire test file may lead to reduced test coverage.
Please confirm if commenting out this entire file is intentional and aligns with the PR objectives. If not, consider uncommenting the necessary parts of the file.
Additionally, let's investigate the referenced GitHub issue for more context:
If you need assistance in determining which parts of the file should be uncommented or how to address the warnings without commenting out the entire file, please let me know.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Fetch the content of the referenced GitHub issue gh issue view 384 --repo zeta-chain/protocol-contractsLength of output: 367
Script:
#!/bin/bash # Attempt to fetch the content of the referenced GitHub issue using curl curl -s https://api.github.com/repos/zeta-chain/protocol-contracts/issues/384 # Search for Solidity files by extension and look for 'withdrawAndCall' functions fd --extension=solidity --search-path=test | xargs rg 'function .*withdrawAndCall' # Search for contracts related to ERC20Custody in Solidity test files fd --extension=solidity --search-path=test | xargs rg 'contract .*ERC20Custody.*Test'Length of output: 21214
Script:
#!/bin/bash # Attempt to fetch the content of the referenced GitHub issue using curl curl -s https://api.github.com/repos/zeta-chain/protocol-contracts/issues/384 # Search for Solidity files by correct extension and look for 'withdrawAndCall' functions fd --extension=sol --search-path=v2/test | xargs rg 'function .*withdrawAndCall' # Search for contracts related to ERC20Custody in Solidity test files fd --extension=sol --search-path=v2/test | xargs rg 'contract .*ERC20Custody.*Test'Length of output: 5765
v2/test/utils/ReceiverEVM.sol (1)
76-78: Approve changes with minor suggestions for improvementThe modifications to the
onCallfunction address the PR's objective of resolving build warnings while maintaining the function's core behavior. However, I have a few suggestions:
Consider adding a comment explaining why the parameter names were removed and why an empty byte array is returned. This would improve code maintainability.
If the return value is not used, consider using
returns (bytes memory)instead of explicitly returning an empty string. This allows the EVM to optimize gas usage.Here's a suggested implementation incorporating these changes:
- function onCall(MessageContext calldata, bytes calldata) external payable returns (bytes memory) { + // Parameter names removed to address unused parameter warnings + // Returns empty byte array to satisfy interface requirements + function onCall(MessageContext calldata, bytes calldata) external payable returns (bytes memory) { emit ReceivedOnCall(); - return ""; + return ""; // Explicit return can be removed if the return value is not used }v2/test/utils/upgrades/ZetaConnectorNativeUpgradeTest.sol (4)
34-39: LGTM! Consider updating the function comment.The changes align with the PR objectives by commenting out the unused
internalSendHashparameter. This should help address build warnings. The event name change toWithdrawnV2is noted and assumed to be part of the upgrade strategy.Consider updating the function comment to reflect the removal of the
internalSendHashparameter:-/// @notice Withdraw tokens to a specified address. -/// @param to The address to withdraw tokens to. -/// @param amount The amount of tokens to withdraw. -//// @param internalSendHash A hash used for internal tracking of the transaction. -/// @dev This function can only be called by the TSS address. +/// @notice Withdraw tokens to a specified address. +/// @param to The address to withdraw tokens to. +/// @param amount The amount of tokens to withdraw. +/// @dev This function can only be called by the TSS address. +/// @dev The internalSendHash parameter has been removed in this version.Also applies to: 48-48
55-61: LGTM! Consider updating the function comment.The changes align with the PR objectives by commenting out the unused
internalSendHashparameter. This should help address build warnings.Consider updating the function comment to reflect the removal of the
internalSendHashparameter:-/// @notice Withdraw tokens and call a contract through Gateway. -/// @param to The address to withdraw tokens to. -/// @param amount The amount of tokens to withdraw. -/// @param data The calldata to pass to the contract call. -//// @param internalSendHash A hash used for internal tracking of the transaction. -/// @dev This function can only be called by the TSS address. +/// @notice Withdraw tokens and call a contract through Gateway. +/// @param to The address to withdraw tokens to. +/// @param amount The amount of tokens to withdraw. +/// @param data The calldata to pass to the contract call. +/// @dev This function can only be called by the TSS address. +/// @dev The internalSendHash parameter has been removed in this version.
82-89: LGTM! Consider updating the function comment.The changes align with the PR objectives by commenting out the unused
internalSendHashparameter. This should help address build warnings.Consider updating the function comment to reflect the removal of the
internalSendHashparameter:-/// @notice Withdraw tokens and call a contract with a revert callback through Gateway. -/// @param to The address to withdraw tokens to. -/// @param amount The amount of tokens to withdraw. -/// @param data The calldata to pass to the contract call. -//// @param internalSendHash A hash used for internal tracking of the transaction. -/// @dev This function can only be called by the TSS address. -/// @param revertContext Revert context to pass to onRevert. +/// @notice Withdraw tokens and call a contract with a revert callback through Gateway. +/// @param to The address to withdraw tokens to. +/// @param amount The amount of tokens to withdraw. +/// @param data The calldata to pass to the contract call. +/// @param revertContext Revert context to pass to onRevert. +/// @dev This function can only be called by the TSS address. +/// @dev The internalSendHash parameter has been removed in this version.
Inconsistent
WithdrawnV2event usage found across the codebase.
The
Withdrawnevent is still present in several contracts:
v1/contracts/evm/ERC20Custody.solv2/contracts/evm/interfaces/IZetaConnector.solv2/contracts/zevm/interfaces/IGatewayZEVM.solv2/contracts/evm/interfaces/IERC20Custody.solOnly specific test and upgrade contracts have the
WithdrawnV2event:
v2/test/utils/upgrades/ZetaConnectorNativeUpgradeTest.solv2/test/utils/upgrades/ERC20CustodyUpgradeTest.solv2/test/utils/upgrades/ZetaConnectorNonNativeUpgradeTest.solv2/test/utils/upgrades/GatewayZEVMUpgradeTest.solEnsure that the
Withdrawnevent is consistently renamed toWithdrawnV2across all relevant contracts to maintain uniformity and prevent potential issues.🔗 Analysis chain
Line range hint
1-114: Verify consistency of theWithdrawnV2event usage.The AI summary mentions that the
Withdrawnevent has been renamed toWithdrawnV2. While this change is not directly related to addressing build warnings, it appears to be part of the upgrade strategy.Please ensure that all references to this event have been updated consistently across the codebase. Run the following script to verify:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to the old 'Withdrawn' event and verify the usage of the new 'WithdrawnV2' event. echo "Checking for remaining 'Withdrawn' event references:" rg --type solidity 'event\s+Withdrawn' echo "Verifying 'WithdrawnV2' event usage:" rg --type solidity 'event\s+WithdrawnV2' rg --type solidity 'emit\s+WithdrawnV2'Length of output: 434
Script:
#!/bin/bash # Description: Check for any remaining references to the old 'Withdrawn' event and verify the usage of the new 'WithdrawnV2' event by searching '.sol' files directly. echo "Checking for remaining 'Withdrawn' event references:" rg 'event\s+Withdrawn' --glob '*.sol' echo "Verifying 'WithdrawnV2' event usage:" rg 'event\s+WithdrawnV2' --glob '*.sol' rg 'emit\s+WithdrawnV2' --glob '*.sol'Length of output: 3509
v2/contracts/evm/ZetaConnectorNative.sol (4)
30-31: Approved: Unused parameter removal improves code clarity.The removal of the unused
internalSendHashparameter addresses build warnings without affecting the function's behavior. The added comment provides valuable context.Consider moving the comment above the function declaration for better readability:
/// @notice Withdraw tokens to a specified address. /// @param to The address to withdraw tokens to. /// @param amount The amount of tokens to withdraw. /// @dev This function can only be called by the TSS address. /// @dev internalSendHash parameter removed (unused currently, see https://github.com/zeta-chain/protocol-contracts/issues/398) function withdraw( address to, uint256 amount ) external override nonReentrant onlyRole(WITHDRAWER_ROLE) whenNotPaused { // ... (rest of the function) }Also applies to: 36-36
52-53: Approved: Unused parameter removal enhances code quality.The removal of the unused
internalSendHashparameter effectively addresses build warnings without impacting the function's logic. The added comment provides valuable context.Similar to the previous function, consider moving the comment above the function declaration for improved readability:
/// @notice Withdraw tokens and call a contract through Gateway. /// @param to The address to withdraw tokens to. /// @param amount The amount of tokens to withdraw. /// @param data The calldata to pass to the contract call. /// @dev This function can only be called by the TSS address. /// @dev internalSendHash parameter removed (unused currently, see https://github.com/zeta-chain/protocol-contracts/issues/398) function withdrawAndCall( address to, uint256 amount, bytes calldata data ) external override nonReentrant onlyRole(WITHDRAWER_ROLE) whenNotPaused { // ... (rest of the function) }Also applies to: 59-59
80-81: Approved: Unused parameter removal improves code quality, but a minor adjustment is needed.The removal of the unused
internalSendHashparameter effectively addresses build warnings without impacting the function's logic. The added comment provides valuable context.
- Consider moving the comment above the function declaration for improved readability:
/// @notice Withdraw tokens and call a contract with a revert callback through Gateway. /// @param to The address to withdraw tokens to. /// @param amount The amount of tokens to withdraw. /// @param data The calldata to pass to the contract call. /// @param revertContext Revert context to pass to onRevert. /// @dev This function can only be called by the TSS address. /// @dev internalSendHash parameter removed (unused currently, see https://github.com/zeta-chain/protocol-contracts/issues/398) function withdrawAndRevert( address to, uint256 amount, bytes calldata data, RevertContext calldata revertContext ) external override nonReentrant onlyRole(WITHDRAWER_ROLE) whenNotPaused { // ... (rest of the function) }
- Remove the unnecessary comma after the commented-out parameter on line 88:
- bytes32, /*internalSendHash*/ + bytes32 /*internalSendHash*/Also applies to: 88-88
Line range hint
1-114: Overall: Changes effectively address build warnings and improve code quality.The modifications to the
withdraw,withdrawAndCall, andwithdrawAndRevertfunctions consistently remove the unusedinternalSendHashparameter, addressing build warnings as per the PR objective. The added comments provide valuable context for these changes.Consider the following suggestions for further improvement:
- Ensure that the removal of the
internalSendHashparameter doesn't impact any external systems or documentation that may reference these function signatures.- If the
internalSendHashfunctionality is planned for future use, consider adding a TODO comment or creating a separate issue to track its implementation.- Review other contracts in the system to ensure consistency in handling similar unused parameters.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (98)
v2/docs/src/contracts/evm/ZetaConnectorNative.sol/contract.ZetaConnectorNative.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/struct.MessageContext.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/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/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/types/ReceiverEVM.tsis excluded by!v2/types/**v2/types/TestUniversalContract.tsis excluded by!v2/types/**v2/types/ZetaConnectorNative.tsis excluded by!v2/types/**v2/types/ZetaConnectorNativeUpgradeTest.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/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/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/**
📒 Files selected for processing (10)
- v2/contracts/evm/ZetaConnectorNative.sol (3 hunks)
- v2/test/ERC20Custody.t.sol (0 hunks)
- v2/test/GatewayZEVM.t.sol (0 hunks)
- v2/test/ZRC20.t.sol (2 hunks)
- v2/test/fuzz/ERC20CustodyEchidnaTest.sol (1 hunks)
- v2/test/fuzz/GatewayEVMEchidnaTest.sol (1 hunks)
- v2/test/utils/ReceiverEVM.sol (1 hunks)
- v2/test/utils/SenderZEVM.sol (3 hunks)
- v2/test/utils/TestUniversalContract.sol (1 hunks)
- v2/test/utils/upgrades/ZetaConnectorNativeUpgradeTest.sol (3 hunks)
💤 Files with no reviewable changes (2)
- v2/test/ERC20Custody.t.sol
- v2/test/GatewayZEVM.t.sol
🧰 Additional context used
🔇 Additional comments (10)
v2/test/fuzz/GatewayEVMEchidnaTest.sol (1)
Line range hint
1-32: Entire file is commented out. Verify if this is intentional.The entire content of this file has been commented out, including import statements, contract definition, and test functions. This effectively removes all the fuzzing tests for the GatewayEVM contract using Echidna.
There's a TODO comment referencing an issue:
// TODO: https://github.com/zeta-chain/protocol-contracts/issues/384Please confirm if commenting out this entire file is intentional. If it is, consider the following:
- Impact on test coverage: Removing these tests may decrease the overall test coverage for the GatewayEVM contract.
- Fuzzing effectiveness: Echidna tests are valuable for finding edge cases and potential vulnerabilities. Removing them might leave the contract more susceptible to undiscovered issues.
If this change is temporary, consider adding a more descriptive comment explaining why the file is commented out and when it should be re-enabled.
To verify the status of the referenced issue, run the following script:
This will help determine if the TODO is still relevant or if the issue has been resolved.
✅ Verification successful
Confirmed: Commenting out is intentional due to open issue #384.
The entireGatewayEVMEchidnaTest.solfile is currently commented out because GitHub issue #384 is still open. Once the issue is resolved, the tests can be reinstated to maintain test coverage and fuzzing effectiveness.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the status of the referenced GitHub issue # Test: Fetch the issue status gh issue view 384 --json state,title # Expected output: The issue state (open/closed) and titleLength of output: 83
v2/test/fuzz/ERC20CustodyEchidnaTest.sol (4)
3-3: Address the TODO comment referencing issue #384There's a TODO comment referencing a GitHub issue. This suggests there might be ongoing work or known issues related to this file.
Let's check the status of the referenced issue:
#!/bin/bash # Fetch the status of the referenced GitHub issue gh issue view 384 --repo zeta-chain/protocol-contracts --json state,titleDepending on the status of the issue, we may need to address it before uncommenting this file. Would you like assistance in resolving the TODO or creating a follow-up task?
Line range hint
5-11: Verify the necessity of commented-out importsThe import statements are currently commented out. If this file is to be uncommented in the future, these imports will likely be necessary for the contract to function correctly.
Let's check if these imported files exist in the project:
#!/bin/bash # Check if the imported files exist echo "Checking ERC20Custody.sol:" fd --type f "ERC20Custody.sol" contracts/evm echo "Checking GatewayEVM.sol:" fd --type f "GatewayEVM.sol" contracts/evm echo "Checking TestERC20.sol:" fd --type f "TestERC20.sol" test/utilsIf you need help ensuring that these imports are correctly set up when uncommenting the file, please let me know.
Line range hint
13-38: Reconsider commenting out the entire ERC20CustodyEchidnaTest contractThis contract appears to be a comprehensive fuzz test for the ERC20Custody functionality, particularly focusing on the
withdrawAndCallmethod. Commenting out this entire test contract may significantly reduce test coverage for this critical functionality.Key points to consider:
- The contract sets up a test environment with a TestERC20 token and a GatewayEVM proxy.
- It includes a test function
testWithdrawAndCallwhich verifies the correct behavior of thewithdrawAndCallfunction.- The test ensures that no ERC20 tokens are left in the gateway contract after execution, which is an important security check.
Let's check if there are other tests covering similar functionality:
#!/bin/bash # Search for other tests that might cover similar functionality echo "Searching for other withdrawAndCall tests:" rg --type solidity "function test.*withdrawAndCall" test/ echo "Searching for other ERC20Custody tests:" rg --type solidity "contract.*ERC20Custody.*Test" test/If you need help in refactoring this test to address the build warnings without commenting it out entirely, please let me know. We could explore options like:
- Selectively commenting out parts of the test that cause warnings.
- Modifying the test to comply with the latest contract interfaces.
- Using mock contracts to isolate the test from changes in the main contracts.
Line range hint
1-38: Summary: Reconsider the approach of commenting out the entire test fileWhile commenting out this file may address immediate build warnings, it introduces several concerns:
- Reduced test coverage: This file contains a fuzz test for critical ERC20Custody functionality.
- Potential for overlooking issues: The
testWithdrawAndCallfunction tests important security aspects.- Unresolved TODO: There's a reference to an open issue that may need addressing.
Recommendations:
- Review the referenced issue Fix echidna tests #384 and determine if it's still relevant.
- Instead of commenting out the entire file, consider addressing the build warnings directly.
- If certain parts must remain commented out, clearly document the reason and create a follow-up task to re-enable them.
- Ensure that the functionality tested here is covered by other tests in the suite.
Let's check for any recent changes to the ERC20Custody contract that might have caused these tests to fail:
#!/bin/bash # Check recent changes to ERC20Custody.sol git log -n 5 --oneline -- contracts/evm/ERC20Custody.solI can help analyze the build warnings, suggest targeted fixes, or help create mock contracts to isolate these tests from changes in the main contracts. Would you like assistance with any of these tasks?
v2/test/utils/TestUniversalContract.sol (1)
Line range hint
1-62: Overall assessment of changesThe modifications to the
TestUniversalContractsuccessfully address the build warnings by removing unused parameter names. However, it's crucial to ensure that these changes don't introduce any unintended side effects or reduce code maintainability.To summarize:
- The changes align with the PR objective of addressing build warnings.
- The approach used (commenting out parameter names) could be improved for better code clarity and maintainability.
- Verification of the impact on tests and contract usage is necessary.
Please consider the suggestions provided in the previous comments to improve the implementation while maintaining the desired functionality.
v2/test/ZRC20.t.sol (1)
Line range hint
1-368: Overall, the test file is comprehensive and well-structured.While reviewing the changes, I noticed that this test file covers a wide range of scenarios for the ZRC20 contract, including basic information, transfers, approvals, burns, deposits, withdrawals, and various administrative functions. This comprehensive approach to testing is commendable and contributes to the robustness of the codebase.
v2/test/utils/SenderZEVM.sol (3)
5-5: Correct import of required structsThe import statement correctly includes
CallOptionsandRevertOptionsfrom theIGatewayZEVM.solinterface, which are necessary for the updated function calls.
48-48: Ensure proper handling ofcallOptionsin gateway functionsThe updated calls to
IGatewayZEVM(gateway).callandwithdrawAndCallnow includecallOptions, which is appropriate. Verify that thecallOptionsprovided meet the requirements of the gateway and that all necessary fields are correctly populated.You may run the following script to check the usage of
callOptionsin theIGatewayZEVMinterface and implementations to ensure compatibility:#!/bin/bash # Description: Verify that the IGatewayZEVM interface methods accept the callOptions as expected. # Search for the interface and implementation of IGatewayZEVM methods. rg --files-with-matches "interface IGatewayZEVM" --type solidity | xargs cat # Display the method signatures involving 'call' and 'withdrawAndCall' rg --type solidity "function (call|withdrawAndCall)\(" -A 3Also applies to: 86-86
83-84:⚠️ Potential issueHardcoded gas limit might lead to transaction failure
In this instance,
gasLimitis hardcoded to1, which may not be adequate for thewithdrawAndCallfunction to execute properly. It's important to set a gas limit that reflects the expected computational workload of the transaction.Run the following script to check for consistency and adequacy of
gasLimitvalues inCallOptions:#!/bin/bash # Description: Check gasLimit values in CallOptions across the codebase. # Find all CallOptions initializations and list their gasLimit values. rg --type solidity "CallOptions\s*memory\s*\w+\s*=\s*CallOptions\({.*gasLimit\s*:\s*\d+.*}\);" -A 2
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #399 +/- ##
=======================================
Coverage 84.23% 84.23%
=======================================
Files 8 8
Lines 387 387
Branches 121 121
=======================================
Hits 326 326
Misses 61 61 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.github/workflows/generated-files_v2.yaml (1)
53-55: LGTM! Consider adding a comment for clarity.The addition of the
forge --versionstep is a good practice. It helps verify the installed version of Foundry, which can be useful for debugging and ensuring consistency across different environments.Consider adding a brief comment explaining the purpose of this step, for example:
- name: Run forge --version run: forge --version # Verify Foundry version for debugging and consistency
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- .github/workflows/generated-files_v2.yaml (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
.github/workflows/generated-files_v2.yaml (1)
Line range hint
1-55: Overall, the change is beneficial and non-disruptive.The addition of the
forge --versionstep enhances the workflow by providing version information for Foundry. This change:
- Improves debugging capabilities.
- Ensures consistency across different environments.
- Does not alter the existing workflow structure or functionality.
The placement of the new step is logical, coming after Foundry installation and before any Foundry-related commands are used.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (5)
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/zevm/GatewayZEVM.sol/contract.GatewayZEVM.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/**
📒 Files selected for processing (1)
- v2/test/GatewayZEVM.t.sol (2 hunks)
🧰 Additional context used
| [[dependencies]] | ||
| name = "forge-std" | ||
| version = "1.9.2" | ||
| source = "https://soldeer-revisions.s3.amazonaws.com/forge-std/1_9_2_06-08-2024_17:31:25_forge-std-1.9.2.zip" | ||
| url = "https://soldeer-revisions.s3.amazonaws.com/forge-std/1_9_2_06-08-2024_17:31:25_forge-std-1.9.2.zip" |
There was a problem hiding this comment.
is this change because soldeer version is different?
There was a problem hiding this comment.
It's when I run soldeer upgrade, using same version as the CI
| uint256 ownerBalanceBefore = zetaToken.balanceOf(owner); | ||
| uint256 gatewayBalanceBefore = zetaToken.balanceOf(address(gateway)); | ||
| uint256 protocolAddressBalanceBefore = protocolAddress.balance; | ||
| // uint256 ownerBalanceBefore = zetaToken.balanceOf(owner); |
There was a problem hiding this comment.
i think we can just remove these commented lines, seems these are not used
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
v2/test/GatewayZEVM.t.sol (1)
Line range hint
1-1000: Overall changes look good, with some suggestions
The changes consistently reflect that ZETA token operations are not currently supported, which aligns with the PR objectives. All ZETA-related operations now throw a
ZETANotSupportederror, ensuring clear behavior until ZETA support is reintroduced.A new
WithdrawnV2event has been added to theGatewayZEVMInboundTestcontract. This suggests an upgrade in the withdrawal process, which is being properly tested in thetestUpgradeAndWithdrawZRC20function. This change prepares the codebase for future upgrades and improves test coverage.Consider adding TODO comments where ZETA support has been removed, indicating when and how these features should be restored in the future. This will help maintain context for future development.
Ensure that all tests related to ZETA operations are either updated to expect the
ZETANotSupportederror or are clearly marked as skipped until ZETA support is reintroduced.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (17)
v2/pkg/erc20custody.t.sol/erc20custodytest.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.t.sol/gatewayzevminboundtest.gois excluded by!v2/pkg/**v2/pkg/gatewayzevm.t.sol/gatewayzevmoutboundtest.gois excluded by!v2/pkg/**v2/pkg/receiverevm.sol/receiverevm.gois excluded by!v2/pkg/**v2/pkg/testuniversalcontract.sol/testuniversalcontract.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.t.sol/zetaconnectornonnativetest.gois excluded by!v2/pkg/**v2/pkg/zrc20.t.sol/zrc20test.gois excluded by!v2/pkg/**v2/types/factories/ReceiverEVM__factory.tsis excluded by!v2/types/**v2/types/factories/TestUniversalContract__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/**
📒 Files selected for processing (1)
- v2/test/GatewayZEVM.t.sol (2 hunks)
🧰 Additional context used
🔇 Additional comments (2)
v2/test/GatewayZEVM.t.sol (2)
422-424: Remove commented-out codeThese lines are no longer needed as ZETA token operations are currently not supported. To improve code cleanliness and maintainability, it's best to remove these commented lines entirely.
Apply this diff to remove the commented-out code:
- // uint256 gatewayBalanceBefore = zetaToken.balanceOf(address(gateway)); - // uint256 protocolBalanceBefore = protocolAddress.balance;If you plan to reintroduce ZETA support in the future, consider adding a TODO comment instead.
492-494: Remove commented-out codeThese commented lines are redundant and should be removed to maintain code cleanliness. This aligns with the previous reviewer's comment.
Apply this diff to remove the commented-out code:
- // uint256 ownerBalanceBefore = zetaToken.balanceOf(owner); - // uint256 gatewayBalanceBefore = zetaToken.balanceOf(address(gateway)); - // uint256 protocolAddressBalanceBefore = protocolAddress.balance;If ZETA support is planned to be reintroduced later, consider adding a clear TODO comment indicating when these checks should be restored.
Mainly comment out unused parameter or variables
NOTE: the test failure should be fixed once #393 is merged
Summary by CodeRabbit
Release Notes
New Features
WithdrawnV2for enhanced tracking of withdrawal operations.ERC20Custodycontract, ensuring functionality post-upgrade.ZRC20andZETAtoken withdrawal functionalities.Echidna, improving testing capabilities.Bug Fixes
Refactor
Tests
GatewayEVMandERC20Custodyfunctionalities, improving overall robustness.