fix: fix admin e2e tests and bump protocol contracts #3006
Conversation
skosito
added
ADMIN_TESTS
skosito
added
TSS_MIGRATION_TESTS
skosito
changed the title
skosito
requested review from
brewmaster012,
kingpinXD,
lumtis,
ws4charlie,
swift1337 and
fbac
as code owners
📝 WalkthroughWalkthroughThe changes in this pull request involve renaming command-line flags and updating functionalities related to contract upgrades across multiple files. Key modifications include renaming the Changes
Possibly related PRs
Suggested labels
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: 1
🧹 Outside diff range and nitpick comments (8)
e2e/runner/v2_upgrade.go (2)
13-18: Function renaming and addition of ERC20Custody upgrade are appropriate.The renaming of
UpgradeGatewaystoUpgradeContractsaccurately reflects the broader scope of contract upgrades. The addition ofr.UpgradeERC20Custody()is consistent with the function's purpose.Consider updating the function comment to be more specific:
// UpgradeContracts upgrades the GatewayZEVM, GatewayEVM, and ERC20Custody contracts // It deploys new contract implementations with the current imported artifacts and upgrades the contracts
59-76: Addition ofUpgradeERC20Custodyfunction is appropriate and well-implemented.The new
UpgradeERC20Custodyfunction correctly implements the upgrade process for the ERC20Custody contract, following the established pattern of other upgrade functions. It includes proper error handling and transaction receipt verification.For consistency with other upgrade functions, consider renaming the function to
UpgradeERC20CustodyEVM:func (r *E2ERunner) UpgradeERC20CustodyEVM() { // ... (rest of the function remains the same) }Also, update the call in the
UpgradeContractsfunction accordingly:func (r *E2ERunner) UpgradeContracts() { r.UpgradeGatewayZEVM() r.UpgradeGatewayEVM() r.UpgradeERC20CustodyEVM() }e2e/runner/v2_setup_evm.go (4)
51-52: Approve changes with minor suggestion for consistencyThe variable renaming improves code clarity. However, for consistency, consider updating the variable name
gatewayEVMAddrtogatewayImplAddrto clearly distinguish it from the proxy address.- gatewayEVMAddr, txGateway, _, err := gatewayevm.DeployGatewayEVM(r.EVMAuth, r.EVMClient) + gatewayImplAddr, txGateway, _, err := gatewayevm.DeployGatewayEVM(r.EVMAuth, r.EVMClient)Also applies to: 60-61
72-77: Approve changes with suggestion for improved error handlingThe updates to the initializer data packing for the ERC20Custody contract are correct and align with the contract's requirements. However, consider adding error checking for the ABI packing operation.
initializerData, err = erc20CustodyABI.Pack("initialize", r.GatewayEVMAddr, r.TSSAddress, r.Account.EVMAddress()) -require.NoError(r, err) +if err != nil { + return fmt.Errorf("failed to pack initializer data for ERC20Custody: %w", err) +}
79-84: Approve changes with suggestion for improved error handlingThe updates to the ERC20Custody proxy contract deployment logic, including the new variable names, enhance code clarity. Consider adding more robust error handling for the contract instantiation.
r.ERC20CustodyV2, err = erc20custodyv2.NewERC20Custody(erc20CustodyProxyAddress, r.EVMClient) -require.NoError(r, err) +if err != nil { + return fmt.Errorf("failed to instantiate ERC20CustodyV2 contract: %w", err) +}Also applies to: 88-90
93-93: Approve changes with suggestion for logging consistencyThe updates to the logging statement and transaction receipt checks are consistent with the earlier variable renaming. For improved logging consistency, consider updating the ERC20CustodyV2 log message to use the proxy address.
r.Logger.Info( "ERC20CustodyV2 contract address: %s, tx hash: %s", - erc20CustodyAddr.Hex(), + erc20CustodyProxyAddress.Hex(), txCustody.Hash().Hex(), )Also applies to: 113-114
cmd/zetae2e/local/local.go (2)
49-49: Approve flag renaming with a minor suggestion.The renaming of
flagUpgradeGatewaystoflagUpgradeContractsaccurately reflects the shift in functionality. This change enhances code clarity and maintainability.Consider updating the flag description to provide more context:
- flagUpgradeContracts = "upgrade-contracts" + flagUpgradeContracts = "upgrade-contracts" // Set to true to upgrade contracts during setup for ZEVM and EVM
117-117: Approve changes inlocalE2ETestwith a suggestion for error handling.The modifications in the
localE2ETestfunction are consistent with the flag renaming. The upgrade logic has been correctly adjusted to useUpgradeContracts()instead ofUpgradeGateways().Consider adding error handling for the
UpgradeContracts()call:if upgradeContracts { - deployerRunner.UpgradeContracts() + if err := deployerRunner.UpgradeContracts(); err != nil { + logger.Print("❌ Failed to upgrade contracts: %v", err) + os.Exit(1) + } }This addition would ensure that any errors during the contract upgrade process are properly logged and handled.
Also applies to: 416-419
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
- cmd/zetae2e/local/local.go (4 hunks)
- contrib/localnet/orchestrator/start-zetae2e.sh (1 hunks)
- e2e/runner/v2_setup_evm.go (2 hunks)
- e2e/runner/v2_upgrade.go (2 hunks)
- go.mod (1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
cmd/zetae2e/local/local.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.contrib/localnet/orchestrator/start-zetae2e.sh (1)
Pattern
**/*.sh: Review the shell scripts, point out issues relative to security, performance, and maintainability.e2e/runner/v2_setup_evm.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/runner/v2_upgrade.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (12)
e2e/runner/v2_upgrade.go (2)
6-6: Import addition is appropriate.The addition of the
erc20custody.solimport is consistent with the existing import pattern and is necessary for the newUpgradeERC20Custodyfunction.
Line range hint
1-76: Overall file structure and consistency are maintained.The changes to this file, including the addition of the
UpgradeERC20Custodyfunction and the renaming ofUpgradeGatewaystoUpgradeContracts, maintain the overall structure and consistency of the file. Error handling and logging patterns are consistent across all upgrade functions.e2e/runner/v2_setup_evm.go (1)
65-69: Approve changes for improved clarityThe updates to the ERC20Custody contract deployment logic, including the variable renaming, enhance code readability while maintaining the original functionality.
contrib/localnet/orchestrator/start-zetae2e.sh (4)
Line range hint
1-124: The initial setup and structure are well-organized and comprehensive.The script demonstrates good practices in shell scripting, including:
- Proper shebang and shellcheck disable
- Well-defined functions for version retrieval and config reading
- Comprehensive setup for funding various accounts on the local Ethereum network
These elements contribute to a robust foundation for the end-to-end testing process.
Line range hint
1-301: Summary of changes and recommendations forstart-zetae2e.shThe primary modification in this script is the transition from gateway upgrades to contract upgrades. This change, while seemingly straightforward, may have significant implications for the testing process and potentially the broader system.
Key points and recommendations:
- The change from
--upgrade-gatewaysto--upgrade-contractshas been consistently applied in the upgrade mode section.- The TSS migration mode remains unchanged, but its compatibility with the new contract upgrade process should be verified.
- It's crucial to ensure that all dependent systems and documentation are updated to reflect this change in the upgrade process.
- Consider adding comments to explain the rationale behind this change and its expected impact on the testing process.
- A thorough testing of the new contract upgrade process is recommended to ensure it behaves as expected and doesn't introduce any regressions.
To ensure a smooth transition, please verify the following:
- The
zetae2ecommand correctly handles the new--upgrade-contractsflag.- All relevant documentation has been updated to reflect this change.
- Any CI/CD pipelines or automated tests that rely on this script have been adjusted accordingly.
These verifications will help maintain the integrity of the testing process and ensure that the change is fully integrated into the existing infrastructure.
261-263: 🛠️ Refactor suggestionEnsure consistency and understand implications of switching to contract upgrades.
The script has been modified to use
--upgrade-contractsinstead of--upgrade-gateways. This change suggests a shift in the upgrade process from focusing on gateways to contracts.While the modification has been applied consistently in both scenarios (upgrade height < 100 and >= 100), it's crucial to verify the following:
- The
zetae2ecommand supports this new flag and behaves as expected.- The implications of upgrading contracts instead of gateways are fully understood and documented.
- Any dependent systems or scripts that may rely on the previous gateway upgrade process have been updated accordingly.
To ensure the change has been applied consistently throughout the codebase, please run the following command:
#!/bin/bash # Check for any remaining instances of gateway upgrades grep -R "upgrade.*gateway" . # Verify the usage of the new contract upgrade flag grep -R "upgrade.*contract" .Consider adding a comment explaining the rationale behind this change and its expected impact on the testing process. This will help future maintainers understand the context of this modification.
Line range hint
126-165: Verify TSS migration mode compatibility with contract upgrades.The TSS migration mode remains unchanged in this update. While this suggests that it may not be directly affected by the switch from gateway upgrades to contract upgrades, it's crucial to ensure its continued compatibility with the new upgrade process.
Please confirm that the TSS migration process is still valid and compatible with the new contract upgrade mechanism. Consider running the following commands to verify:
cmd/zetae2e/local/local.go (1)
87-87: Flag registration updated correctly.The flag registration has been properly updated to use
flagUpgradeContracts, maintaining consistency with the earlier renaming. The description accurately conveys the flag's purpose.go.mod (4)
Line range hint
366-385: Clarify the necessity of replace directives and document their purpose.The addition and modification of several replace directives, particularly those pointing to ZetaChain maintained forks, raises some concerns:
- While replace directives can be useful for using forked versions or specific commits, they can make the project harder to maintain and upgrade in the future.
- It's important to understand the specific reasons for each replacement and whether there's a plan to contribute these changes back to the original repositories.
Please provide clarification on the following:
- The specific reasons for each replacement, especially those pointing to ZetaChain forks.
- Any plans to contribute the changes in these forks back to the original repositories.
- The potential impact of these replacements on the project's maintainability and future upgrades.
Additionally, consider documenting the purpose and necessity of these replacements in the project's documentation to aid future maintenance decisions.
To assist in understanding the impact of these replacements, please run the following script:
#!/bin/bash # Description: Find usage of replaced packages in the codebase for package in $(grep "^replace" go.mod | awk '{print $2}'); do echo "Usage of $package:" rg --type go "$package" -A 5 done
62-62: Clarify the purpose of new dependencies and document their usage.The addition of new dependencies (
github.com/bnb-chain/tss-lib,github.com/showa-93/go-mask, andgithub.com/tonkeeper/tongo) may introduce new functionality, but it also increases the complexity of the project. Please provide clarification on the following:
- The specific use case for each new dependency in the project.
- Why these particular libraries were chosen over potential alternatives.
- Any potential impact on the project's performance or security.
Additionally, consider documenting the purpose and usage of these dependencies in the project's documentation to aid future maintenance.
To assist in understanding the usage of these new dependencies, please run the following script:
Also applies to: 353-357
✅ Verification successful
New dependencies are actively utilized within the codebase.
The dependencies
github.com/bnb-chain/tss-lib,github.com/showa-93/go-mask, andgithub.com/tonkeeper/tongoare extensively used across multiple components, including but not limited to:
github.com/bnb-chain/tss-lib
zetaclient/tss/tss_signer.gocmd/zetaclientd/main.gocmd/zetaclientd/gen_pre_params.gogithub.com/showa-93/go-mask
zetaclient/config/types.gogithub.com/tonkeeper/tongo
- Various files within
zetaclient/testutils/mocks/,zetaclient/orchestrator/,zetaclient/chains/ton/observer/, and others.This widespread integration confirms that the new dependencies are necessary for the project's functionality and complexity is justified.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find usage of new dependencies in the codebase echo "Usage of github.com/bnb-chain/tss-lib:" rg --type go "github.com/bnb-chain/tss-lib" -A 5 echo "Usage of github.com/showa-93/go-mask:" rg --type go "github.com/showa-93/go-mask" -A 5 echo "Usage of github.com/tonkeeper/tongo:" rg --type go "github.com/tonkeeper/tongo" -A 5Length of output: 675
Script:
#!/bin/bash # Description: Find usage of new dependencies in the codebase echo "Usage of github.com/bnb-chain/tss-lib:" rg --type go "github\\.com/bnb-chain/tss-lib" -A 5 echo "Usage of github.com/showa-93/go-mask:" rg --type go "github\\.com/showa-93/go-mask" -A 5 echo "Usage of github.com/tonkeeper/tongo:" rg --type go "github\\.com/tonkeeper/tongo" -A 5Length of output: 13501
Line range hint
3-5: Approve Go version update with compatibility verification.The update to Go 1.22.5 is a positive change, incorporating the latest language features and bug fixes. However, it is crucial to ensure that all dependencies and the codebase are compatible with this version.
To verify compatibility, please run the following script:
62-62: Approve dependency update with breaking changes verification.The update of
github.com/zeta-chain/protocol-contractsto versionv1.0.2-athens3.0.20241014093550-f7f6d9fd971ais noted. While updating dependencies is generally beneficial for bug fixes and new features, it's crucial to verify that this update doesn't introduce any breaking changes.To check for potential breaking changes, please run the following script:
skosito
removed
UPGRADE_LIGHT_TESTS
Description
How Has This Been Tested?
Summary by CodeRabbit
New Features
Bug Fixes
Chores