fix!: tolerate v2 contracts in tss migration tests #3002
Conversation
gartnera
added
no-changelog
gartnera
force-pushed
the
fix-tss-migration-tests
branch
from
abdeba9 to
c6addea
Compare
gartnera
force-pushed
the
fix-tss-migration-tests
branch
from
c6addea to
85f55b7
Compare
lumtis
requested review from
brewmaster012,
kingpinXD,
ws4charlie,
swift1337 and
fbac
as code owners
📝 WalkthroughWalkthroughThe pull request introduces several updates across multiple files, primarily focusing on enhancing transaction handling and error reporting. Key changes include modifications to 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 (5)
e2e/runner/admin_evm.go (1)
29-38: Approve changes with minor suggestion for consistencyThe modifications to use
ERC20CustodyV2are appropriate and align with the PR objectives. The added comment provides valuable clarity regarding the naming convention differences between v1 and v2.To maintain consistency with the
UpdateTssAddressForConnectormethod, consider updating the log message to include "V2" for clarity:-r.Logger.Info(fmt.Sprintf("TSS ERC20 Address Update Tx: %s", tx.Hash().String())) +r.Logger.Info(fmt.Sprintf("TSS ERC20V2 Address Update Tx: %s", tx.Hash().String()))This minor change would make it explicit that the V2 contract is being updated, enhancing code readability and maintaining consistency with the changes made.
e2e/utils/require.go (2)
28-35: Approve changes with a minor suggestion for improvement.The inclusion of the transaction hash in the error message is a valuable addition, providing more context for debugging failed tests. However, there's a minor inconsistency in the error message format.
Consider updating the error message format for clarity:
- msg := "receipt status is not successful: %s" + msg := "receipt status is not successful (tx hash: %s)"This change makes it explicit that the additional information is the transaction hash, improving readability of the error message.
Line range hint
1-48: Summary: Improved error reporting with minor suggestions for consistency.The changes to
RequireTxSuccessfulandRequireTxFailed(formerlyRequiredTxFailed) functions enhance error reporting by including transaction hashes. This addition provides valuable context for debugging failed tests.To further improve these utility functions:
- Implement the suggested changes for consistent and clear error messaging.
- Consider reviewing other similar utility functions in this file or package to ensure they follow the same pattern of including relevant identifiers (like transaction hashes) in error messages.
Maintaining consistency in error reporting across test utilities is crucial for efficient debugging and test maintenance.
zetaclient/chains/evm/signer/v2_sign.go (1)
204-204: Approval: Consistent improvement in Amount field handlingThe modification in the
signERC20CustodyWithdrawRevertfunction mirrors the improvement made insignGatewayExecuteRevert. This change maintains consistency across the codebase and offers the same benefits of preserving precision for large numerical values.Given the similarity of these changes, consider refactoring the
RevertContextcreation into a separate function to reduce code duplication and enhance maintainability. For example:func createRevertContext(sender string, asset common.Address, amount *big.Int, revertMessage string) revert.RevertContext { return revert.RevertContext{ Sender: common.HexToAddress(sender), Asset: asset, Amount: amount, RevertMessage: revertMessage, } }This function could then be used in both
signGatewayExecuteRevertandsignERC20CustodyWithdrawRevert, promoting code reuse and consistency.go.mod (1)
Inaccessible Replace Directives Identified
The replace directives in your
go.modfile reference repositories that are currently inaccessible:
github.com/zeta-chain/tss-libgithub.aaakk.us.kg/zeta-chain/go-ethereumgithub.aaakk.us.kg/zeta-chain/go-libp2pgithub.aaakk.us.kg/zeta-chain/go-tssThis may lead to build issues or prevent dependency updates. Please verify that these replace directives are necessary and update their repository URLs accordingly.
🔗 Analysis chain
Line range hint
3-5: Approve Go version update and verify development environment.The update to Go 1.22.5 is commendable, as it likely includes important security patches and bug fixes. However, it is crucial to ensure that all development environments and CI/CD pipelines are aligned with this version to maintain consistency across the project.
To verify the Go version across the project, please run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check Go version in CI/CD configuration files echo "Checking CI/CD configurations for Go version:" grep -r "go-version" .github/workflows/*.yml .gitlab-ci.yml 2>/dev/null || echo "No CI/CD configs found with go-version specified." # Check Go version in Dockerfiles echo -e "\nChecking Dockerfiles for Go version:" grep -r "FROM golang:" Dockerfile* 2>/dev/null || echo "No Dockerfiles found with golang base image." # Reminder for local development environments echo -e "\nReminder: Ensure all developers update their local Go version to 1.22.5"Length of output: 1970
📜 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)
- e2e/runner/admin_evm.go (1 hunks)
- e2e/utils/require.go (1 hunks)
- go.mod (1 hunks)
- x/fungible/keeper/v2_evm.go (2 hunks)
- zetaclient/chains/evm/signer/v2_sign.go (2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
e2e/runner/admin_evm.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/utils/require.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/fungible/keeper/v2_evm.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/evm/signer/v2_sign.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (6)
x/fungible/keeper/v2_evm.go (3)
190-190: Improved precision in Amount handlingThe modification to directly assign the
*big.Intvalue to theAmountfield in theRevertContextstruct is a commendable improvement. This change eliminates the potential loss of precision that could occur when converting from*big.Inttouint64, ensuring accurate representation of large numbers which is crucial in blockchain contexts.
244-244: Consistent improvement in Amount handlingThe modification in
CallDepositAndRevertmirrors the change made inCallExecuteRevert, demonstrating a consistent approach to handling theAmountfield. This change maintains precision by directly using the*big.Inttype and ensures uniformity across the codebase, which is crucial for maintainability and reducing potential errors.
190-190: Summary of improvementsThe modifications to
CallExecuteRevertandCallDepositAndRevertfunctions represent a significant enhancement in handling large numbers within the EVM context. By directly using*big.Intfor theAmountfield inRevertContext, these changes:
- Eliminate potential precision loss.
- Ensure consistency across the codebase.
- Align with best practices for blockchain development.
These improvements contribute to the overall robustness of the system and support the PR's objective of addressing issues in tss migration tests.
Also applies to: 244-244
zetaclient/chains/evm/signer/v2_sign.go (1)
84-84: Approval: Enhanced precision in Amount field assignmentThe modification to directly assign
txData.amountto theAmountfield inRevertContextis a commendable improvement. This change preserves the original type ofamount, likely a*big.Int, thus maintaining full precision for large numerical values. This adjustment mitigates potential issues related to integer overflow that could have occurred with the previousuint64conversion, especially when dealing with substantial amounts in blockchain transactions.go.mod (2)
62-62: Verify impact of dependency updates and additions.The update of
github.com/zeta-chain/protocol-contractsand the addition of new dependencies (github.com/bnb-chain/tss-lib,github.com/showa-93/go-mask,github.com/tonkeeper/tongo) may introduce new features or security improvements. However, it is essential to assess their impact on the project.Please run the following script to analyze the usage of these dependencies:
#!/bin/bash echo "Analyzing usage of updated and new dependencies:" # Check usage of updated zeta-chain/protocol-contracts echo "Usage of updated zeta-chain/protocol-contracts:" grep -R "github.com/zeta-chain/protocol-contracts" . --include="*.go" || echo "No direct usage found." # Check usage of new dependencies for dep in "github.com/bnb-chain/tss-lib" "github.com/showa-93/go-mask" "github.com/tonkeeper/tongo"; do echo -e "\nUsage of $dep:" grep -R "$dep" . --include="*.go" || echo "No usage found." done echo -e "\nPlease review the output and ensure these dependencies are necessary and used appropriately."Also applies to: 283-287
Line range hint
290-306: Review and validate replace directives.The use of replace directives, particularly for ZetaChain maintained forks, is noted. While these can be beneficial for applying custom patches or using specific versions, it's crucial to regularly review their necessity and ensure they're up-to-date.
Please run the following script to analyze the replace directives:
#!/bin/bash echo "Analyzing replace directives:" # Check if the replaced repositories exist and are accessible for repo in "github.com/zeta-chain/tss-lib" "github.com/zeta-chain/go-ethereum" "github.com/zeta-chain/go-libp2p" "github.com/zeta-chain/go-tss"; do echo -e "\nChecking $repo:" git ls-remote $repo > /dev/null 2>&1 if [ $? -eq 0 ]; then echo "Repository is accessible." echo "Latest commit:" git ls-remote $repo HEAD | cut -f1 else echo "Repository is not accessible or doesn't exist." fi done echo -e "\nPlease review the output and ensure all replace directives are necessary and point to the correct versions."
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3002 +/- ##
===========================================
- Coverage 66.05% 66.02% -0.04%
===========================================
Files 397 397
Lines 22313 22337 +24
===========================================
+ Hits 14739 14747 +8
- Misses 6806 6816 +10
- Partials 768 774 +6
|
Description
#2932 broke the tss migration tests because the V2 ERC20 custody contract did not have
UpdateTSSAddresson the current protocol-contracts version (related to zeta-chain/protocol-contracts#363).Use the latest version of protocol-contracts. This includes a consensus breaking change from zeta-chain/protocol-contracts#377.
How Has This Been Tested?
Summary by CodeRabbit
New Features
Bug Fixes
Chores