-
Notifications
You must be signed in to change notification settings - Fork 108
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test(e2e): add gateway upgrade in upgrade test #2932
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request primarily focus on enhancing the upgrade process for the Zeta Chain node, including updates to the Docker build system, versioning, and end-to-end testing capabilities. Key modifications include adjusting version references in the Makefile, adding new methods for upgrading gateway contracts, and introducing command-line flags for test configurations. The changelog has been updated to reflect significant new features, refactoring, and fixes, ensuring comprehensive documentation of the changes. Changes
Assessment against linked issues
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (7)
cmd/zetae2e/local/local.go (2)
415-418
: Appropriate implementation of gateway upgrade logic.The gateway upgrade logic is correctly implemented and guarded by the
upgradeGateways
flag. To improve clarity and maintainability, consider adding a brief comment explaining the purpose of this upgrade step.Add a comment to provide context:
// upgrade gateways if upgradeGateways { + // Upgrade gateways to the latest version used in dependencies deployerRunner.UpgradeGateways() }
Line range hint
1-518
: Summary: Gateway upgrade functionality added, setup process needs further refinement.The changes introduce gateway upgrade functionality and make progress towards merging v1 and v2 setups. While the implementations are generally sound, there are opportunities for improvement:
- Unify the v1 and v2 setup processes to reduce complexity and improve maintainability.
- Update TODO comments with more specific action items and timelines.
- Consider adding more comprehensive error handling and logging for the new gateway upgrade process.
To improve the overall architecture and maintainability of the E2E testing setup:
- Create a separate configuration package to manage all flag definitions and their corresponding variables.
- Implement a unified setup function that handles both v1 and v2 configurations.
- Consider introducing a more modular approach to test execution, allowing for easier addition and removal of test cases.
These changes will make the codebase more scalable and easier to maintain as the project evolves.
changelog.md (5)
Line range hint
1-39
: Version v12.2.4 changes appear appropriate, but consider improving documentation.The changes in version v12.2.4 address several important fixes and improvements. However, it would be beneficial to provide more context for some of the changes, especially for external contributors or users.
Consider adding brief explanations for the following changes:
- The reason for the 50% increase in base gas price (line 4).
- The rationale behind changing the WhitelistERC20 authorization from group1 to group2 (line 5).
- The purpose of adding the pending outtx hash to the tracker after 10 minutes (line 6).
This additional context would help users and contributors better understand the motivations behind these changes.
Line range hint
41-114
: Version v12.1.0 introduces significant changes, but some improvements can be made.The changes in version v12.1.0 include important features, fixes, and refactoring. However, there are a few areas that could be improved for clarity and consistency.
In the Features section, the description for item fix: modify emissions to follow fixed rewards per block #1658 (line 52) is vague. Consider providing more details about the modification to emission distribution and its implications.
In the Fixes section, the description for item
MsgRemoveForeignCoin
: renameName
field intoZRC20Address
#1650 (line 59) uses asterisks for emphasis, which is inconsistent with the formatting in the rest of the document. Consider using a different method for emphasis, such as backticks for code-like terms.In the Refactoring section, items refactor: Optimize return #1628 and refactor: zetaclient re-organization #1640 (lines 75-76) lack specific details about what was optimized or reorganized. Consider adding brief explanations of the changes and their benefits.
The addition of EVM fee calculation to TSS migration of EVM chains (line 78) seems to be a significant change. Consider adding a brief explanation of how this impacts the system architecture and any potential performance implications.
Line range hint
1-385
: Overall, the changelog is informative but could benefit from some improvements.The changelog provides a detailed record of changes across multiple versions, including important features, fixes, and breaking changes. However, there are several areas where it could be enhanced:
- Consistency: Ensure all entries follow a consistent format, including pull request links and detailed descriptions.
- Context: Provide more background information for significant changes, especially those that impact system architecture or user interactions.
- Breaking Changes: Clearly highlight and explain all breaking changes, including migration steps for users.
- Testing: Include more information about new or updated tests related to significant changes.
- Performance Impacts: When possible, mention any performance implications of major changes.
Consider reorganizing the changelog to group similar types of changes together within each version (e.g., all breaking changes, all new features, all fixes). This could improve readability and make it easier for users to understand the impact of each release.
Add a brief summary at the beginning of each version's changes, highlighting the most significant updates. This would help users quickly grasp the importance of each release.
By implementing these suggestions, the changelog could become an even more valuable resource for users, contributors, and maintainers of the project.
Missing Test Case for Paused zrc20 Deposit Prevention
The test case to verify that deposits to a paused zrc20 are rejected has not been implemented. Please ensure that a comprehensive test case is added to confirm this behavior.
🔗 Analysis chain
Line range hint
241-385
: Versions v11.0.0 and v10.1.2 introduce important features and fixes, but some improvements can be made.These versions include significant changes such as HSM capability for zetaclient hot key, tss funds migration, and various fixes. However, there are areas where documentation and consistency can be improved.
The addition of HSM capability for zetaclient hot key (line 244) is a valuable security enhancement.
Some feature descriptions lack detail, such as the "new thread to zetaclient" (lines 245-246). Consider providing more context about the purpose and benefits of these additions.
In the Fixes section of v11.0.0, some items lack pull request links (e.g., lines 254-255). For consistency and traceability, consider adding these links.
The addition of Bitcoin block header and merkle proof functionality (line 339) seems to be a significant architectural change. Consider documenting the implications of this change on the system's overall design and any potential performance impacts.
The fix for preventing deposits for paused zrc20 (line 257) is important for system integrity. It would be beneficial to add a test case to verify this behavior:
The fix for the Athens-3 issue regarding Bitcoin outtx inclusion (line 259) lacks details about the specific problem addressed. It's important to provide more information about the issue and how the fix resolves it.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Test: Attempt to deposit to a paused zrc20 # Expected: Deposit should be rejected # TODO: Implement test case to verify paused zrc20 deposit prevention echo "Implement test case for paused zrc20 deposit prevention"Length of output: 120
Outdated Query Paths Found in changelog.md
The verification process identified remaining references to old query paths in
changelog.md
(Lines: 36-42). Please update these paths to the new endpoints to ensure documentation accuracy and prevent potential integration issues.🔗 Analysis chain
Line range hint
116-239
: Version v12.0.0 introduces breaking changes and significant features, but requires careful consideration.This version includes several breaking changes, new features, and important fixes. The breaking changes, particularly those related to module reorganization and query modifications, need to be clearly communicated to users and may require updates to dependent systems.
The breaking changes related to TSS and chain validation queries (lines 119-126) may impact existing integrations. To ensure a smooth transition, it's crucial to verify that all affected systems are updated accordingly.
The merging of observer params and core params into chain params (lines 134-137) represents a significant architectural change. Consider documenting the rationale behind this decision and any potential impacts on system performance or maintainability.
In the Features section, some items lack pull request links (e.g., lines 245-247). For consistency and traceability, consider adding these links.
The fix for Code4rena issue (line 263) lacks details about the specific problem addressed. It's important to provide more information about the issue and how the fix resolves it.
The addition of pagination to queries iterating over large data sets (lines 279-280) is a good performance improvement.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining references to old query paths rg -i '/zeta-chain/crosschain/(pendingNonces|chainNonces)' rg -i '/zeta-chain/observer/(observers_by_chain|all_observer_mappers)' rg -i '/zeta-chain/observer/(params|get_core_params|get_core_params_by_chain)'Length of output: 1858
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- Makefile (1 hunks)
- changelog.md (1 hunks)
- cmd/zetae2e/local/local.go (5 hunks)
- contrib/localnet/orchestrator/start-zetae2e.sh (1 hunks)
- e2e/runner/v2_gateway.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
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_gateway.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (5)
Makefile (2)
302-302
: Version update for source build approved.The update of
OLD_VERSION
from 'release/v19' to 'release/v20' aligns with the PR objectives to update the upgrade reference to version 20. This change ensures that the correct version is used when building from source.
302-307
: Changes align with PR objectives and upgrade process.The updates to the
zetanode-upgrade
target in the Makefile accurately reflect the intention to upgrade to version 20, as stated in the PR objectives. Both the source build and binary build options have been updated consistently. These changes support the enhancement of the end-to-end (E2E) upgrade testing workflow by ensuring the correct version is used during the upgrade process.No further changes appear necessary in this file to meet the stated objectives of the PR.
cmd/zetae2e/local/local.go (3)
49-49
: Appropriate addition of new flag constant.The
flagUpgradeGateways
constant is well-named and consistent with the existing flag naming convention.
87-87
: Proper integration of new flag into command.The
flagUpgradeGateways
flag is correctly added to the command with a clear and concise description.
117-117
: Consistent addition of new flag variable.The
upgradeGateways
variable is properly initialized from the flag, maintaining consistency with other flag variables.
* add gateway upgrade * change reference * add v2 setup for all tests * test v2 in light upgrade * refactor setup to use custody v2 directly
…guration (#2953) * update artillery config * more fixes * feat: integrate authenticated calls smart contract functionality into protocol (#2904) * e2e tests and modifications for authenticated call * extend test with sender check and revert case * separate tests into separate files * cleanup * withdraw and call support and tests * bump protocol contracts * split tests into separate files * small cleanup * fmt * generate * lint * changelog * PR comments * fix case in proto * bump vote inbound gas limit in zetaclient * fix test * generate * fixing tests * call options non empty * generate * test fix * rename gateway caller * pr comments rename tests * PR comment * generate * tests * update tests fixes * tests fixes * fix * test fix * feat!: bank precompile (#2860) * feat: bank precompile * feat: add deposit * feat: extend deposit * PoC: spend amount on behalf of EOA * feat: expand deposit with transferFrom * use CallEVM instead on ZRC20 bindings * divide the contract into different files * initialize e2e testing * remove duplicated funding * add codecov * expand e2e * fix: wait for deposit tx to be mined * apply first round of reviews * cover al error types test * fixes using time.Since * Include CallContract interface * fix eth events in deposit precompile method * emit Deposit event * add withdraw function * finalize withdraw * pack event arguments generically * add high level event function * first round of review fixes * second round of reviews * create bank account when instantiating bank * e2e: add good and bad scenarios * modify fmt * chore: group input into eventData struct * docs: document bank's methods * chore: generate files with suffix .gen.go * chore: assert errors with errorIs * chore: reset e2e test by resetting allowance * test: add first batch of unit test * test: cover all cases * test: complete unit test cases * include review suggestions * include e2e through contract * test: add e2e through contract complete * test: revert balance between tests * Update precompiles/bank/const.go Co-authored-by: Lucas Bertrand <[email protected]> * fix: changed coin denom --------- Co-authored-by: skosito <[email protected]> Co-authored-by: Lucas Bertrand <[email protected]> * feat: add sender to revert context (#2919) * e2e tests and modifications for authenticated call * extend test with sender check and revert case * separate tests into separate files * cleanup * withdraw and call support and tests * bump protocol contracts * split tests into separate files * small cleanup * fmt * generate * lint * changelog * PR comments * fix case in proto * bump vote inbound gas limit in zetaclient * fix test * generate * fixing tests * call options non empty * generate * test fix * rename gateway caller * pr comments rename tests * PR comment * generate * tests * add sender in test contract * extend e2e tests * generate * changelog * PR comment * generate * update tests fixes * tests fixes * fix * test fix * gas limit fixes * PR comment fix * fix bad merge * ci: add option to enable monitoring stack (#2927) * ci: add option to enable monitoring stack * start prometheus faster * update * ci: add rpcimportable test (#2817) * ci: add rpcimportable test * add ci * fmt * use github.com/btcsuite/btcd/btcutil in pkg/chains * remove app imports types tests * use standalone sdkconfig package * fix policies test * move crosschain keeper tests from types to keeper * never seal config in tests * use zeta-chain/ethermint#126 * add some comments * use merged ethermint hash * show resulting go.mod * ci: Add SARIF upload to GitHub Security Dashboard (#2929) * add semgrep sarif upload to GHAS * added comment to clairfy the usage of the utility script * use ghcr.io instead * add tag to image * bad org name --------- Co-authored-by: jkan2 <[email protected]> * fix: add recover to InitChainer to diplay informative message when starting a node from block 1 (#2925) * add recover to InitChainer * generate files * add docs link to error message * move InitChainErrorMessage to app.go * Update app/app.go Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * use const for message --------- Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * test: add wait for block to tss migration test (#2931) * add wait for block to tss migration test * add comments * refactor identifiers * rename checkNumberOfTssGenerated to checkNumberOfTSSGenerated * chore: allow full zetaclient config overlay (#2945) * test(e2e): add gateway upgrade in upgrade test (#2932) * add gateway upgrade * change reference * add v2 setup for all tests * test v2 in light upgrade * refactor setup to use custody v2 directly * chore: improve localnet build performance (#2928) * chore: improve localnet build performance * propagate NODE_VERSION and NODE_COMMIT * update hashes --------- Co-authored-by: skosito <[email protected]> Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> Co-authored-by: Lucas Bertrand <[email protected]> Co-authored-by: Alex Gartner <[email protected]> Co-authored-by: jkan2 <[email protected]> Co-authored-by: jkan2 <[email protected]> Co-authored-by: Tanmay <[email protected]>
Description
Checking the change
If you comment out the following line
node/cmd/zetae2e/local/local.go
Line 417 in 30d60d1
Closes: #2921 #2926
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests