feat: erc20 auth calls #3012
feat: erc20 auth calls #3012
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3012 +/- ##
========================================
Coverage 64.90% 64.91%
========================================
Files 408 408
Lines 28406 28407 +1
========================================
+ Hits 18438 18441 +3
+ Misses 9172 9170 -2
Partials 796 796
|
skosito
requested review from
brewmaster012,
kingpinXD,
ws4charlie,
swift1337 and
fbac
as code owners
📝 WalkthroughWalkthroughThis pull request introduces significant updates to the ZetaChain node, including new features, refactoring, tests, fixes, and CI improvements. Key enhancements involve support for stateful precompiled contracts, a common RPC package, and improvements to staking contracts. The integration of authenticated calls for ERC20 and ZRC20 contracts is notable, alongside support for multiple Bitcoin chains. The changelog reflects these changes, while various tests have been expanded to cover new functionalities. Additionally, several methods and variables have been added or updated across multiple files to streamline contract interactions and improve overall code quality. 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: 15
🧹 Outside diff range and nitpick comments (25)
e2e/e2etests/test_v2_zevm_to_evm_arbitrary_call.go (1)
Line range hint
1-45: Suggest minor improvements for error handling and documentation.The test function is well-structured and covers the essential aspects of setup, execution, and verification. To further enhance its robustness and maintainability, consider the following suggestions:
Error Handling: Implement more granular error checking, particularly for the transaction execution and CCTX mining process. This will help identify specific failure points in case of test failures.
Documentation: Add a brief comment at the beginning of the test function explaining its purpose and the specific scenario it's testing. This will improve the test's self-documentation and make it easier for other developers to understand its intent.
Here's an example of how you might implement these suggestions:
// TestV2ZEVMToEVMArbitraryCall verifies the ability to make arbitrary calls // from ZEVM to EVM using the V2 gateway contract. It tests the successful // transmission of a payload and the correct updating of contract state. func TestV2ZEVMToEVMArbitraryCall(r *runner.E2ERunner, args []string) { require.Len(r, args, 0) // ... (existing setup code) // Perform the call tx, err := r.V2ZEVMToEMVArbitraryCall( r.TestDAppV2EVMAddr, r.EncodeSimpleCall(payloadMessageEVMCall), gatewayzevm.RevertOptions{ OnRevertGasLimit: big.NewInt(0), }, ) require.NoError(r, err, "Failed to execute V2ZEVMToEMVArbitraryCall") // Wait for the cctx to be mined cctx, err := utils.WaitCctxMinedByInboundHash(r.Ctx, tx.Hash().Hex(), r.CctxClient, r.Logger, r.CctxTimeout) require.NoError(r, err, "Failed to wait for CCTX to be mined") r.Logger.CCTX(*cctx, "call") require.Equal(r, crosschaintypes.CctxStatus_OutboundMined, cctx.CctxStatus.Status, "Unexpected CCTX status") // ... (existing assertion code) }These changes will improve the test's resilience and self-documentation, making it easier to maintain and debug in the future.
e2e/e2etests/test_v2_eth_withdraw_and_arbitrary_call.go (1)
Incomplete Method Renaming Detected
The renaming of
V2ETHWithdrawAndCalltoV2ETHWithdrawAndArbitraryCallis not fully applied across the codebase. The old method name is still present in the following files:
e2e/runner/v2_zevm.goe2e/e2etests/test_v2_eth_withdraw_and_call_revert_with_call.goe2e/e2etests/test_v2_eth_withdraw_and_call.gocmd/zetae2e/local/v2.goPlease update these instances to ensure consistency and prevent potential issues.
🔗 Analysis chain
Line range hint
27-31: Approve method renaming and verify consistency.The renaming of
V2ETHWithdrawAndCalltoV2ETHWithdrawAndArbitraryCallenhances code clarity by explicitly indicating the arbitrary nature of the call. This change aligns well with the function nameTestV2ETHWithdrawAndArbitraryCall.To ensure consistency across the codebase, please run the following script to verify that all occurrences of this method have been updated:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all occurrences of the renamed method. # Test: Search for any remaining instances of the old method name. # Expect: No results, indicating all instances have been updated. rg --type go 'V2ETHWithdrawAndCall' # Test: Confirm the presence of the new method name. # Expect: At least one result, showing the updated method name. rg --type go 'V2ETHWithdrawAndArbitraryCall'Length of output: 4095
e2e/e2etests/test_v2_zevm_to_evm_call.go (1)
Action Required: Implement
V2ZEVMToEMVCallMethodThe new method
V2ZEVMToEMVCalldoes not appear to be implemented in the codebase. Please ensure that the method is defined to prevent potential runtime errors. Additionally, confirm that there are no remaining references toV2ZEVMToEMVAuthenticatedCallto maintain code consistency.
- Implement the
V2ZEVMToEMVCallmethod with the required functionality.- Ensure all references to the old method
V2ZEVMToEMVAuthenticatedCallare removed.🔗 Analysis chain
Line range hint
26-32: Verify the implications of the method name change.The method call has been renamed from
V2ZEVMToEMVAuthenticatedCalltoV2ZEVMToEMVCall. This change may have broader implications:
- Ensure that the new method
V2ZEVMToEMVCallmaintains the same functionality as the previousV2ZEVMToEMVAuthenticatedCall, particularly regarding authentication.- Verify that this change is consistent with the PR objectives, specifically the implementation of ERC20 authorization calls.
- Check if any documentation or other test cases need to be updated to reflect this method name change.
To ensure consistency across the codebase, please run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to the old method name and verify the new method's implementation. # Search for any remaining references to the old method name echo "Searching for references to V2ZEVMToEMVAuthenticatedCall:" rg "V2ZEVMToEMVAuthenticatedCall" --type go # Check the implementation of the new method echo "Checking implementation of V2ZEVMToEMVCall:" ast-grep --lang go --pattern 'func $_ V2ZEVMToEMVCall($_) $_'Length of output: 329
zetaclient/chains/evm/signer/v2_signer.go (1)
26-26: Approve change with minor suggestion for consistency.The modification to include
cctx.InboundParams.Senderas an argument tosignERC20CustodyWithdrawAndCallis appropriate and aligns with the PR objectives. This change enhances the context provided to the signing function, which is crucial for proper authentication and logging in ERC20 authorization calls.For improved consistency with other case handlers in this function, consider extracting the sender as a variable at the beginning of the function. This approach would enhance readability and maintain a uniform structure across all cases.
Consider applying the following refactor:
func (signer *Signer) SignOutboundFromCCTXV2( ctx context.Context, cctx *types.CrossChainTx, outboundData *OutboundData, ) (*ethtypes.Transaction, error) { + sender := cctx.InboundParams.Sender outboundType := evm.ParseOutboundTypeFromCCTX(*cctx) switch outboundType { // ... (other cases remain unchanged) case evm.OutboundTypeERC20WithdrawAndCall: - return signer.signERC20CustodyWithdrawAndCall(ctx, cctx.InboundParams.Sender, outboundData) + return signer.signERC20CustodyWithdrawAndCall(ctx, sender, outboundData) // ... (subsequent cases would use 'sender' instead of 'cctx.InboundParams.Sender') } return nil, fmt.Errorf("unsupported outbound type %d", outboundType) }This refactoring enhances code readability and maintains consistency across all cases in the switch statement.
e2e/e2etests/test_v2_erc20_withdraw_and_call_revert.go (1)
Incomplete Function Name Update Detected
The function name
V2ERC20WithdrawAndCallremains in several locations across the codebase:
cmd/zetae2e/local/v2.goe2e/e2etests/test_v2_erc20_withdraw_and_call.go- Various test files
This incomplete update can lead to inconsistencies and potential issues in functionality and testing.
Recommendations:
Update Function Names:
- Replace all remaining instances of
V2ERC20WithdrawAndCallwithV2ERC20WithdrawAndArbitraryCallto maintain consistency.Rename Test Functions:
- Modify test function names from
TestV2ERC20WithdrawAndCall*toTestV2ERC20WithdrawAndArbitraryCall*to accurately reflect the changes.Review Documentation and Comments:
- Ensure that all documentation and inline comments referencing the old function name are updated accordingly.
Run Comprehensive Tests:
- After making the updates, execute the test suite to confirm that all tests pass and that there are no residual references causing failures.
🔗 Analysis chain
Line range hint
32-40: Approve function name change and suggest test name updateThe change from
V2ERC20WithdrawAndCalltoV2ERC20WithdrawAndArbitraryCallappropriately reflects the expanded functionality to support arbitrary calls. This modification aligns with the reported changes and enhances the flexibility of the system.To maintain consistency and clarity:
Consider updating the test function name from
TestV2ERC20WithdrawAndCallReverttoTestV2ERC20WithdrawAndArbitraryCallRevertto accurately reflect the new functionality being tested.Ensure that all other occurrences of this function call throughout the codebase have been updated to use the new name
V2ERC20WithdrawAndArbitraryCall.To verify that all occurrences of the function call have been updated, you can run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining occurrences of the old function name # Test: Search for the old function name. Expect: No results rg --type go "V2ERC20WithdrawAndCall" # Test: Search for the new function name. Expect: At least one result rg --type go "V2ERC20WithdrawAndArbitraryCall"Length of output: 3093
e2e/e2etests/test_v2_eth_withdraw_and_call_revert_with_call.go (1)
Update Remaining References to
V2ETHWithdrawAndArbitraryCallThe search identified multiple instances of
V2ETHWithdrawAndCallthat need to be updated toV2ETHWithdrawAndArbitraryCallto ensure consistency across the codebase:
- e2e/runner/v2_zevm.go
- Function definitions and comments referencing the old method name.
- e2e/e2etests/e2etests.go
- Test names and associated references using the old method name.
- e2e/e2etests/test_v2_eth_withdraw_and_call_revert_with_call.go
- Function calls still utilizing
V2ETHWithdrawAndCall.- e2e/e2etests/test_v2_eth_withdraw_and_call.go
- Existing function calls with the outdated method name.
- e2e/e2etests/test_v2_eth_withdraw_and_call_through_contract.go
- References in test functions needing renaming.
- cmd/zetae2e/local/v2.go
- Test name references that require updating.
Please ensure all these instances are revised to
V2ETHWithdrawAndArbitraryCallto maintain consistency and prevent potential issues.🔗 Analysis chain
Line range hint
28-38: Approve method name change and suggest updates for consistencyThe change from
V2ETHWithdrawAndCalltoV2ETHWithdrawAndArbitraryCallis approved as it better reflects the expanded functionality. However, to maintain consistency and clarity, please consider the following recommendations:
Update the test function name to
TestV2ETHWithdrawAndArbitraryCallRevertWithCallto align with the new method name.Modify the comment above the method call to accurately describe the new functionality:
// Perform the withdraw and arbitrary call
- Verify if any other parts of the codebase, such as documentation or related tests, need updates to reflect this change in functionality.
To ensure all occurrences of the old method name have been updated, please run the following command:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining instances of V2ETHWithdrawAndCall rg --type go "V2ETHWithdrawAndCall"Length of output: 3240
e2e/e2etests/test_v2_erc20_withdraw_and_call_revert_with_call.go (2)
Line range hint
29-39: Approve function name change with a minor suggestion.The renaming of
V2ERC20WithdrawAndCalltoV2ERC20WithdrawAndArbitraryCallappropriately reflects the enhanced functionality allowing for arbitrary calls. This change aligns well with the PR objectives of implementing support for ERC20 authorization calls.To further improve clarity, consider renaming the function to
V2ERC20WithdrawAndExecuteArbitraryCall. This name more explicitly conveys that the function performs both a withdrawal and an arbitrary call execution.
Line range hint
1-61: Enhance test function organization and maintainability.While the test function covers important scenarios, consider the following improvements to enhance its organization and maintainability:
Extract the hard-coded string constant
payloadMessageWithdrawOnRevertERC20to a package-level constant for better maintainability.Consider breaking down the test function into smaller, more focused test cases. This would improve readability and make it easier to maintain and debug individual scenarios.
Implement a helper function for common setup operations like ERC20 approvals. This would reduce duplication and improve the focus of each test case.
Example refactoring:
const ( PayloadMessageWithdrawOnRevertERC20 = "this is a test ERC20 withdraw and call on revert" ) func TestV2ERC20WithdrawAndArbitraryCall(r *runner.E2ERunner, args []string) { // ... (existing setup code) r.Run("WithdrawAndCallRevert", func() { testWithdrawAndCallRevert(r, amount) }) r.Run("AssertTestDAppZEVMCalled", func() { testAssertTestDAppZEVMCalled(r, amount) }) // ... (other sub-tests) } func testWithdrawAndCallRevert(r *runner.E2ERunner, amount *big.Int) { setupERC20Approvals(r) tx := r.V2ERC20WithdrawAndArbitraryCall( // ... (existing parameters) ) cctx := utils.WaitCctxMinedByInboundHash(r.Ctx, tx.Hash().Hex(), r.CctxClient, r.Logger, r.CctxTimeout) r.Logger.CCTX(*cctx, "withdraw") require.Equal(r, crosschaintypes.CctxStatus_Reverted, cctx.CctxStatus.Status) } func setupERC20Approvals(r *runner.E2ERunner) { r.ApproveERC20ZRC20(r.GatewayZEVMAddr) r.ApproveETHZRC20(r.GatewayZEVMAddr) } // ... (other helper functions)This refactoring improves code organization, reduces duplication, and enhances maintainability while preserving the existing functionality.
e2e/e2etests/test_v2_erc20_withdraw_and_call.go (3)
17-22: Function signature improvement and gas limit management.The removal of the unused
argsparameter enhances code clarity. The use of an underscore for the unused parameter adheres to Go conventions.The addition of gas limit management is a positive change, ensuring proper test isolation. However, consider extracting this setup into a separate helper function for improved readability and reusability.
Consider refactoring the gas limit setup into a helper function:
func withCustomGasLimit(r *runner.E2ERunner, gasLimit uint64, testFunc func()) { previousGasLimit := r.ZEVMAuth.GasLimit r.ZEVMAuth.GasLimit = gasLimit defer func() { r.ZEVMAuth.GasLimit = previousGasLimit }() testFunc() } // Usage in test: func TestV2ERC20WithdrawAndCall(r *runner.E2ERunner, _ []string) { withCustomGasLimit(r, 10000000, func() { // Test body here }) }This approach improves code organization and makes the gas limit setup reusable across multiple tests if needed.
24-28: Test setup simplification and assertion update.The use of a fixed amount (
10000) simplifies the test setup and removes the dependency on external arguments. This change enhances test readability and maintainability.The update to
AssertTestDAppEVMCalledusing the new payload constant ensures consistency with the earlier changes.For improved clarity, consider defining the fixed amount as a named constant at the package level:
const testWithdrawAmount = 10000 // In the test function: amount := big.NewInt(testWithdrawAmount)This approach enhances code readability and makes it easier to modify the test amount if needed in the future.
48-54: Enhanced test coverage with sender verification.The addition of sender verification significantly improves the test coverage by ensuring the correct sender is used in authenticated calls. This is a crucial aspect of the feature being tested.
Consider enhancing the error handling to provide more context in case of a failure:
senderForMsg, err := r.TestDAppV2EVM.SenderWithMessage( &bind.CallOpts{}, []byte(payloadMessageWithdrawAuthenticatedCallERC20), ) require.NoError(r, err, "Failed to retrieve sender for message") require.Equal(r, r.ZEVMAuth.From, senderForMsg, "Unexpected sender for authenticated call")This approach will provide more informative error messages, facilitating easier debugging if the test fails.
e2e/e2etests/test_v2_zevm_to_evm_call_through_contract.go (2)
Line range hint
15-67: Enhance test structure and readabilityThe test has a clear structure, but its readability and maintainability could be improved. Consider the following suggestions:
- Add comments to delineate the main sections of the test (setup, action, verification) for improved clarity.
- Consider extracting repeated operations (e.g., waiting for transaction receipts) into helper functions to reduce duplication.
- Add brief comments explaining the purpose of key operations, especially those using custom types or methods from the E2ERunner.
Here's an example of how you might structure the test with improved comments:
func TestV2ZEVMToEVMCallThroughContract(r *runner.E2ERunner, args []string) { require.Len(r, args, 0) // Setup: Verify initial state r.AssertTestDAppEVMCalled(false, payloadMessageEVMAuthenticatedCallThroughContract, big.NewInt(0)) // Setup: Deploy caller contract and fund it gatewayCallerAddr, _, gatewayCaller := deployAndFundGatewayCaller(r) // Action: Perform the call through contract tx := r.V2ZEVMToEMVCallThroughContract( gatewayCaller, r.TestDAppV2EVMAddr, []byte(payloadMessageEVMAuthenticatedCallThroughContract), gatewayzevmcaller.RevertOptions{OnRevertGasLimit: big.NewInt(0)}, ) cctx := waitAndGetCctx(r, tx) // Verification: Check call results r.AssertTestDAppEVMCalled(true, payloadMessageEVMAuthenticatedCallThroughContract, big.NewInt(0)) // Verification: Verify sender verifySender(r, gatewayCallerAddr, payloadMessageEVMAuthenticatedCallThroughContract) } // Helper functions (implement these as needed) func deployAndFundGatewayCaller(r *runner.E2ERunner) (common.Address, *gatewayzevmcaller.GatewayZEVMCaller, error) { // ... implementation ... } func waitAndGetCctx(r *runner.E2ERunner, tx *types.Transaction) *crosschaintypes.CrossChainTx { // ... implementation ... } func verifySender(r *runner.E2ERunner, expectedSender common.Address, message string) { // ... implementation ... }This structure improves readability by clearly separating the test phases and extracting repeated operations into helper functions.
Clarify Removal of Authentication Checks
The modification from
V2ZEVMToEMVAuthenticatedCallThroughContracttoV2ZEVMToEMVCallThroughContractindicates the elimination of authentication-specific validations. Considering that other test files still reference authenticated calls, this change might lead to inconsistencies in test coverage and system behavior.Recommendations:
Consistency Across Tests:
- Ensure that the removal of authentication checks in this test aligns with the overall testing strategy. If authentication is a critical component, other tests should reflect similar validations to maintain comprehensive coverage.
Update Related Assertions:
- Review and update assertions to confirm that the core functionality is adequately tested without relying on authentication. This ensures that the removal does not inadvertently weaken the test's effectiveness.
Documentation:
- Add comments or documentation explaining the rationale behind removing authentication checks. This provides clarity for future maintainers and prevents misunderstandings regarding the test's purpose.
Impact Analysis:
- Assess the impact of this change on the system's security and functionality. Verify that the underlying implementation no longer requires authentication in this context or that alternative mechanisms have been appropriately integrated.
🔗 Analysis chain
Line range hint
37-45: Verify the implications of removing authentication checksThe change from
V2ZEVMToEMVAuthenticatedCallThroughContracttoV2ZEVMToEMVCallThroughContractsuggests a shift in the authentication mechanism or requirements. Please consider the following:
- Confirm that removing authentication-specific checks aligns with the current system design and requirements.
- Ensure that the test still adequately covers the intended functionality without these checks.
- Consider adding assertions or documentation to clarify the expected behavior regarding authentication, if applicable.
To verify the changes across the codebase, please run the following script:
This script will help identify any related changes or remaining references to authenticated calls that might need attention.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for references to authenticated calls and related changes # Search for references to authenticated calls echo "Searching for references to authenticated calls:" rg --type go "AuthenticatedCall|authentication" -g '!vendor/' # Check for other occurrences of V2ZEVMToEMVCallThroughContract echo "Checking for other occurrences of V2ZEVMToEMVCallThroughContract:" rg --type go "V2ZEVMToEMVCallThroughContract" -g '!vendor/' # Look for any TODO or FIXME comments related to authentication echo "Searching for TODO or FIXME comments related to authentication:" rg --type go "TODO|FIXME" -g '!vendor/' | rg "auth|Authentication|sender"Length of output: 3334
pkg/contracts/testdappv2/TestDAppV2.sol (2)
Line range hint
54-63: Remove obsolete function implementationThe
onCallfunction at lines 54-63 appears to be an obsolete implementation that is inconsistent with the newonCallfunction defined at lines 106-110. To maintain code clarity and prevent potential confusion:
- Remove this obsolete function implementation.
- Update any references to this function throughout the codebase.
- Ensure that all tests are updated to use the new
onCallfunction signature.Apply this diff to remove the obsolete function:
- function onCall( - zContext calldata _context, - address _zrc20, - uint256 amount, - bytes calldata message - ) - external - { - require(!isRevertMessage(string(message))); - - setCalledWithMessage(string(message)); - setAmountWithMessage(string(message), amount); - }
106-110: Approve changes with minor improvement suggestionThe new
onCallfunction implementation correctly aligns with the described changes and provides a more streamlined interface. The function appropriately handles theMessageContextand sets the necessary state variables.However, there's a minor inconsistency:
The function is declared to return
bytes memory, but the current implementation doesn't return any value. Consider either removing the return type if it's not needed or implementing a meaningful return value.If no return value is needed, apply this diff:
- function onCall(MessageContext calldata messageContext, bytes calldata message) external payable returns (bytes memory) { + function onCall(MessageContext calldata messageContext, bytes calldata message) external payable { setCalledWithMessage(string(message)); setAmountWithMessage(string(message), msg.value); senderWithMessage[message] = messageContext.sender; }Alternatively, if a return value is required, implement it based on your specific use case.
pkg/contracts/testdappv2/TestDAppV2.json (2)
160-190: Approve newonCallfunction with recommendations.The addition of this new
onCallfunction enhances the contract's flexibility in handling various types of calls. Thepayablemodifier andbytesreturn type provide increased versatility.Consider the following recommendations:
- Update the contract's documentation to clearly differentiate between the two
onCallfunctions and their use cases.- Implement access control mechanisms if this function should not be callable by any external actor.
- If this function is intended to replace the previous
onCall(formerlyonCrossChainCall), consider deprecating the old version to maintain a clean interface.Additionally, as this function is
payable, ensure proper handling of received Ether to prevent potential vulnerabilities.
Line range hint
1-263: Summary of ABI changes and their implications.The modifications to the TestDAppV2 contract ABI primarily focus on the
onCallfunctionality:
- Renaming of
onCrossChainCalltoonCall.- Addition of a new
payableonCallfunction with a different signature.These changes suggest a shift towards a more generalized and flexible approach to handling calls within the contract. However, they also introduce potential challenges:
- Backward Compatibility: Existing integrations may break due to the function rename.
- Interface Clarity: The presence of two
onCallfunctions with different signatures could lead to confusion for developers integrating with this contract.- Documentation: Comprehensive documentation updates are crucial to explain the differences and use cases for each
onCallfunction.To mitigate these challenges, consider:
- Implementing a clear deprecation strategy for the old function if it's being phased out.
- Providing migration guides for existing integrations.
- Enhancing function names or implementing function selectors to clearly differentiate between the two
onCallfunctions.These changes align with the PR objective of implementing support for ERC20 authorization calls, but careful consideration of their broader impact on the system is necessary.
go.mod (1)
Line range hint
374-393: Caution: Evaluate replace directives and maintain upstream alignment.The use of replace directives, particularly with ZetaChain-maintained forks, introduces potential risks:
- Divergence from upstream changes, potentially missing important updates or bug fixes.
- Increased maintenance burden to keep forks up-to-date.
- Possible incompatibilities with other dependencies expecting the original versions.
To mitigate these risks:
- Document the reasons for each replace directive, especially for ZetaChain forks.
- Implement a strategy to regularly sync forks with upstream repositories.
- Consider contributing necessary changes back to the original repositories when possible.
- Periodically re-evaluate the need for each replace directive and remove them when no longer necessary.
Implement a CI/CD process to automatically check for updates in the original repositories and notify developers of potential merge opportunities.
e2e/e2etests/e2etests.go (1)
846-851: Updated test description for ERC20 withdrawal with authenticated call.The description for
TestV2ERC20WithdrawAndCallNamehas been modified to specify an "authenticated call" instead of an "arbitrary call". This change aligns with the PR objectives of implementing support for ERC20 authorization calls.However, to maintain consistency with other test definitions, consider adding a default value for the amount parameter:
runner.NewE2ETest( TestV2ERC20WithdrawAndCallName, "withdraw ERC20 from ZEVM and authenticated call a contract using V2 contract", - []runner.ArgDefinition{}, + []runner.ArgDefinition{ + {Description: "amount", DefaultValue: "1000"}, + }, TestV2ERC20WithdrawAndCall, ),changelog.md (2)
Line range hint
1-321: Summary of changelog and suggestions for improvement.The changelog demonstrates significant development and improvement across multiple versions, addressing various aspects such as security, performance, and functionality. Key observations:
- Frequent introduction of new features and capabilities.
- Regular fixes and refactoring to improve system stability and performance.
- Several breaking changes, particularly in major version updates.
While the continuous improvement is commendable, consider the following suggestions for future versioning and changelog management:
- Aim for a more stable API design to reduce the frequency of breaking changes, especially in minor versions.
- Provide more detailed migration guides for versions with breaking changes.
- Consider grouping related changes under feature flags to allow for gradual adoption of new functionality.
- Implement a consistent format for changelog entries, including links to relevant issues or pull requests.
- Add a "Deprecated" section to highlight features or APIs that will be removed in future versions.
To improve changelog management, consider implementing a changelog generation tool that enforces a consistent format and links changes to their corresponding issues or pull requests. This could be achieved using a script that parses commit messages or pull request descriptions with specific tags or formats.
Example implementation:
#!/bin/bash # Generate changelog entries from git commits # Get the latest tag latest_tag=$(git describe --tags --abbrev=0) # Generate changelog entries git log ${latest_tag}..HEAD --pretty=format:'- %s' --reverse | while read -r line; do # Extract the type of change from the commit message type=$(echo "$line" | sed -n 's/^- \([a-z]\+\):.*/\1/p') # Categorize the change case "$type" in feat) echo "### Features" ;; fix) echo "### Fixes" ;; refactor) echo "### Refactoring" ;; chore) echo "### Chores" ;; test) echo "### Tests" ;; ci) echo "### CI" ;; *) echo "### Other Changes" ;; esac # Print the changelog entry echo "$line" doneThis script can be integrated into your release process to automatically generate consistent changelog entries.
Issues Found: Missing Changes in
changelog.mdThe verification process revealed that the expected removals or replacements in
changelog.md(lines 20-26) are not present. Specifically, the changes related to external chain height checks, gas price adjustments, WhitelistERC20 authorization, and Bitcoin-related modifications do not appear as intended.Please ensure that all relevant code changes are accurately reflected in the
changelog.mdto maintain clarity and traceability.🔗 Analysis chain
Line range hint
1-24: Approve changes in v12.2.4 with a recommendation for thorough testing.The fixes in version v12.2.4 address critical issues related to chain height, gas prices, authorization, and Bitcoin operations. These changes are essential for improving the stability and security of the system. However, given the nature of these fixes, it is crucial to ensure comprehensive testing has been performed, especially for the following changes:
- The additional check for external chain height increases.
- The 50% increase in base gas price for EIP1559.
- The change in WhitelistERC20 authorization from group1 to group2.
- The modifications to Bitcoin-related operations, including keysign scheduling and gas fee calculations.
To verify the impact of these changes, please run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation and test coverage of the v12.2.4 fixes # Test: Check for the presence of external chain height check rg --type go "external.*chain.*height.*increases" # Test: Verify the gas price adjustment for EIP1559 rg --type go "base.*gas.*price.*1\.5" # Test: Confirm the WhitelistERC20 authorization change rg --type go "WhitelistERC20.*group2" # Test: Check for Bitcoin-related changes rg --type go "bitcoin.*keysign.*schedule" rg --type go "SegWit.*tx.*size" rg --type go "ConfirmationCount.*bitcoin" # Test: Verify test coverage for these changes go test -v -cover ./... | grep -E "external.*chain|gas.*price|WhitelistERC20|bitcoin"Length of output: 2138
e2e/e2etests/test_v2_erc20_withdraw_and_arbitrary_call.go (1)
14-14: Use consistent naming for payload message constantsTo improve code readability and maintainability, consider renaming
payloadMessageWithdrawERC20topayloadMessageV2ERC20WithdrawAndArbitraryCallto match the test function name. This makes it clearer which test the payload message is associated with.Apply the following change:
-const payloadMessageWithdrawERC20 = "this is a test ERC20 withdraw and call payload" +const payloadMessageV2ERC20WithdrawAndArbitraryCall = "this is a test ERC20 withdraw and call payload"And update its usage throughout the function accordingly.
e2e/runner/v2_zevm.go (3)
Line range hint
35-53: Refactor to reduce code duplication between V2ETHWithdrawAndArbitraryCall and V2ETHWithdrawAndCallThe functions
V2ETHWithdrawAndArbitraryCallandV2ETHWithdrawAndCallshare similar logic, differing mainly in theIsArbitraryCallflag withingatewayzevm.CallOptions. Refactoring them into a single helper function can enhance maintainability and reduce redundancy.Suggested refactor:
func (r *E2ERunner) v2ETHWithdrawAndCallCommon( receiver ethcommon.Address, amount *big.Int, payload []byte, revertOptions gatewayzevm.RevertOptions, isArbitraryCall bool, ) *ethtypes.Transaction { tx, err := r.GatewayZEVM.WithdrawAndCall0( r.ZEVMAuth, receiver.Bytes(), amount, r.ETHZRC20Addr, payload, gatewayzevm.CallOptions{ GasLimit: gasLimit, IsArbitraryCall: isArbitraryCall, }, revertOptions, ) require.NoError(r, err) return tx } func (r *E2ERunner) V2ETHWithdrawAndArbitraryCall( receiver ethcommon.Address, amount *big.Int, payload []byte, revertOptions gatewayzevm.RevertOptions, ) *ethtypes.Transaction { return r.v2ETHWithdrawAndCallCommon(receiver, amount, payload, revertOptions, true) } func (r *E2ERunner) V2ETHWithdrawAndCall( receiver ethcommon.Address, amount *big.Int, payload []byte, revertOptions gatewayzevm.RevertOptions, ) *ethtypes.Transaction { return r.v2ETHWithdrawAndCallCommon(receiver, amount, payload, revertOptions, false) }Also applies to: 56-70
Line range hint
223-237: Refactor V2ZEVMToEMVCallThroughContract to use a common helper functionThe function
V2ZEVMToEMVCallThroughContractcan be refactored to use a helper function similar to other ZEVM to EMV call methods. This will enhance consistency across the codebase.Suggested refactor:
func (r *E2ERunner) v2ZEVMToEMVCallThroughContractCommon( gatewayZEVMCaller *gatewayzevmcaller.GatewayZEVMCaller, receiver ethcommon.Address, payload []byte, revertOptions gatewayzevmcaller.RevertOptions, isArbitraryCall bool, ) *ethtypes.Transaction { tx, err := gatewayZEVMCaller.CallGatewayZEVM( r.ZEVMAuth, receiver.Bytes(), r.ETHZRC20Addr, payload, gatewayzevmcaller.CallOptions{ GasLimit: gasLimit, IsArbitraryCall: isArbitraryCall, }, revertOptions, ) require.NoError(r, err) return tx } func (r *E2ERunner) V2ZEVMToEMVCallThroughContract( gatewayZEVMCaller *gatewayzevmcaller.GatewayZEVMCaller, receiver ethcommon.Address, payload []byte, revertOptions gatewayzevmcaller.RevertOptions, ) *ethtypes.Transaction { return r.v2ZEVMToEMVCallThroughContractCommon(gatewayZEVMCaller, receiver, payload, revertOptions, false) }
Line range hint
35-53: Ensure consistent error handling across all transaction functionsAll transaction functions use
require.NoError(r, err)for error handling. While this is appropriate for testing environments, consider returning the error to allow for more flexible handling in different contexts.Consider modifying the functions to return the error:
func (r *E2ERunner) v2ETHWithdrawAndCallCommon(...) (*ethtypes.Transaction, error) { tx, err := r.GatewayZEVM.WithdrawAndCall0(...) if err != nil { return nil, err } return tx, nil }This approach allows the caller to decide how to handle the error appropriately.
Also applies to: 62-74, 138-146, 167-174, 187-193
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumpkg/contracts/testdappv2/TestDAppV2.binis excluded by!**/*.bin
📒 Files selected for processing (23)
- changelog.md (1 hunks)
- cmd/zetae2e/local/v2.go (1 hunks)
- e2e/e2etests/e2etests.go (2 hunks)
- e2e/e2etests/test_v2_erc20_withdraw_and_arbitrary_call.go (1 hunks)
- e2e/e2etests/test_v2_erc20_withdraw_and_call.go (4 hunks)
- e2e/e2etests/test_v2_erc20_withdraw_and_call_revert.go (1 hunks)
- e2e/e2etests/test_v2_erc20_withdraw_and_call_revert_with_call.go (1 hunks)
- e2e/e2etests/test_v2_eth_withdraw_and_arbitrary_call.go (1 hunks)
- e2e/e2etests/test_v2_eth_withdraw_and_call.go (1 hunks)
- e2e/e2etests/test_v2_eth_withdraw_and_call_revert.go (1 hunks)
- e2e/e2etests/test_v2_eth_withdraw_and_call_revert_with_call.go (1 hunks)
- e2e/e2etests/test_v2_eth_withdraw_and_call_through_contract.go (1 hunks)
- e2e/e2etests/test_v2_zevm_to_evm_arbitrary_call.go (1 hunks)
- e2e/e2etests/test_v2_zevm_to_evm_call.go (1 hunks)
- e2e/e2etests/test_v2_zevm_to_evm_call_through_contract.go (1 hunks)
- e2e/runner/v2_zevm.go (6 hunks)
- go.mod (1 hunks)
- pkg/contracts/testdappv2/TestDAppV2.abi (1 hunks)
- pkg/contracts/testdappv2/TestDAppV2.go (2 hunks)
- pkg/contracts/testdappv2/TestDAppV2.json (2 hunks)
- pkg/contracts/testdappv2/TestDAppV2.sol (2 hunks)
- zetaclient/chains/evm/signer/v2_sign.go (2 hunks)
- zetaclient/chains/evm/signer/v2_signer.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (18)
cmd/zetae2e/local/v2.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/e2etests.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_v2_erc20_withdraw_and_arbitrary_call.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_v2_erc20_withdraw_and_call.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_v2_erc20_withdraw_and_call_revert.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_v2_erc20_withdraw_and_call_revert_with_call.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_v2_eth_withdraw_and_arbitrary_call.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_v2_eth_withdraw_and_call.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_v2_eth_withdraw_and_call_revert.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_v2_eth_withdraw_and_call_revert_with_call.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_v2_eth_withdraw_and_call_through_contract.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_v2_zevm_to_evm_arbitrary_call.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_v2_zevm_to_evm_call.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_v2_zevm_to_evm_call_through_contract.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/runner/v2_zevm.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.pkg/contracts/testdappv2/TestDAppV2.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.zetaclient/chains/evm/signer/v2_signer.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (18)
e2e/e2etests/test_v2_zevm_to_evm_arbitrary_call.go (1)
25-31: Approve function call update with verification suggestion.The update from
V2ZEVMToEMVCalltoV2ZEVMToEMVArbitraryCallappears to be a deliberate change to more accurately reflect the function's purpose. This aligns with the PR objectives of implementing support for ERC20 authorization calls.To ensure consistency across the codebase, please verify that the corresponding function in the
runnerpackage has been updated accordingly. Run the following script to check for any remaining occurrences of the old function name:✅ Verification successful
To accurately verify the implementation of
V2ZEVMToEMVArbitraryCall, please execute the following script:
Function
V2ZEVMToEMVArbitraryCallcorrectly implemented and updated.The new function
V2ZEVMToEMVArbitraryCallis present ine2e/runner/v2_zevm.go, and the update intest_v2_zevm_to_evm_arbitrary_call.golines 25-31 accurately reflects this change.
./e2e/runner/v2_zevm.go🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining occurrences of the old function name # Test: Search for the old function name. Expect: No results. rg --type go 'V2ZEVMToEMVCall' # Test: Verify the new function exists in the runner package. Expect: One or more occurrences. rg --type go 'func.*V2ZEVMToEMVArbitraryCall' pkg/runner/Length of output: 769
Script:
#!/bin/bash # Description: Search for the definition of V2ZEVMToEMVArbitraryCall across the entire codebase # Test: Verify the new function exists rg --type go 'func.*V2ZEVMToEMVArbitraryCall' .Length of output: 119
e2e/e2etests/test_v2_eth_withdraw_and_call.go (1)
Line range hint
34-38: Verify the impact of removing sender verification and changing the function name.The modifications to this test function warrant careful consideration:
The removal of the expected sender setup (
SetExpectedOnCallSender) may impact the test's ability to verify the correct sender. Ensure that this verification is still performed elsewhere or that it's no longer necessary due to changes in the underlying implementation.The function name change from
V2ETHWithdrawAndAuthenticatedCalltoV2ETHWithdrawAndCallsuggests a possible alteration in the underlying implementation or intent. Verify that this change aligns with the updated contract behavior and that it doesn't inadvertently remove any authentication checks.Consider adding a comment explaining the rationale behind these changes to improve code maintainability.
To ensure these changes don't introduce regressions, please run the following verification:
e2e/e2etests/test_v2_erc20_withdraw_and_call.go (2)
15-15: Constant name update enhances clarity.The renaming of the constant from
payloadMessageWithdrawERC20topayloadMessageWithdrawAuthenticatedCallERC20accurately reflects its updated purpose in the context of authenticated calls. This change improves code readability and maintainability.
Line range hint
37-46: Consistent update to withdraw operation and CCTX assertion.The use of the updated
payloadMessageWithdrawAuthenticatedCallERC20constant in theV2ERC20WithdrawAndCallmethod call maintains consistency with the earlier changes. The CCTX assertion logic remains unchanged, which is appropriate if the underlying behavior hasn't been modified.e2e/e2etests/test_v2_zevm_to_evm_call_through_contract.go (1)
Line range hint
61-66: Clarify the relevance of the sender assertionGiven the removal of earlier authentication checks, the presence of this sender assertion at the end of the test requires clarification:
senderForMsg, err := r.TestDAppV2EVM.SenderWithMessage( &bind.CallOpts{}, []byte(payloadMessageEVMAuthenticatedCallThroughContract), ) require.NoError(r, err) require.Equal(r, gatewayCallerAddr, senderForMsg)Please consider the following:
- Confirm that this assertion is still relevant and aligns with the current implementation of
V2ZEVMToEMVCallThroughContract.- If the sender identity is still important, consider adding a comment explaining its significance in the context of the simplified test.
- If this check is no longer necessary, consider removing it to maintain consistency with the removal of other authentication-related checks.
To verify the relevance of this assertion, please run the following script:
This script will help identify if similar sender assertions are used consistently across tests and if there's any documentation that explains their purpose.
✅ Verification successful
Confirm Sender Assertion Consistency Across Tests
The sender assertion present in
e2e/e2etests/test_v2_zevm_to_evm_call_through_contract.gois consistently used across multiple test files, ensuring that sender identity verification remains robust even after the removal of specific authentication checks.
- Consistency: Similar sender assertions (
SenderWithMessage,gatewayCallerAddr) are found in other test files, indicating a standardized approach.- Documentation: Some tests include comments regarding sender verification, which supports understanding and maintaining the verification process.
Recommendations:
- Documentation: If not already present, add comments explaining the importance of sender assertions in the context of the simplified test to enhance clarity.
- Review Purpose: Ensure that the sender assertion aligns with the current functionality of
V2ZEVMToEVMCallThroughContract, maintaining its relevance.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for similar sender assertions in other tests # Search for similar sender assertions in test files echo "Searching for similar sender assertions in test files:" rg --type go "SenderWithMessage|gatewayCallerAddr" -g '*test*.go' -g '!vendor/' # Check for any comments or documentation related to sender verification echo "Checking for comments or documentation related to sender verification:" rg --type go "sender verification|check sender" -g '!vendor/'Length of output: 2506
cmd/zetae2e/local/v2.go (1)
37-37: Approved: Enhanced test coverage for ERC20 operations.The addition of
TestV2ERC20WithdrawAndArbitraryCallNameto the ERC20 token workflow tests is a positive enhancement, improving the coverage of ERC20 transactions, particularly for withdraw and arbitrary call operations.To ensure the new test case is properly implemented and doesn't conflict with existing tests, please run the following verification:
✅ Verification successful
Verification Successful: The new ERC20 test case is correctly implemented without conflicts.
The
TestV2ERC20WithdrawAndArbitraryCallfunction has been properly added to the codebase, and no naming conflicts were detected during verification.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the new ERC20 test case # Test: Check if the new test function exists and review its implementation ast-grep --lang go --pattern 'func TestV2ERC20WithdrawAndArbitraryCall($$$) { $$$ }' # Test: Ensure no conflicts with existing test names rg --type go 'func TestV2ERC20WithdrawAndArbitraryCall'Length of output: 2825
pkg/contracts/testdappv2/TestDAppV2.json (1)
155-159: Verify impact of function name change fromonCrossChainCalltoonCall.The renaming of this function from
onCrossChainCalltoonCallsuggests a more generalized approach to handling calls. However, this change may impact existing integrations or external contracts that rely on the previous function name.To ensure this change doesn't introduce breaking changes, please run the following verification script:
Review the results to identify any potential conflicts or necessary updates in other parts of the codebase.
go.mod (2)
62-62: Justify new dependencies and conduct security audit.The update to
github.com/zeta-chain/protocol-contractsand the addition of new dependencies (github.com/bnb-chain/tss-lib,github.com/showa-93/go-mask, andgithub.com/tonkeeper/tongo) may introduce new features or resolve issues. However, it is crucial to:
- Provide justification for each new dependency to ensure they are necessary.
- Conduct a security audit of these dependencies to mitigate potential vulnerabilities.
Additionally, verify that these changes do not introduce conflicts with existing dependencies.
#!/bin/bash # Check for potential conflicts and vulnerabilities go mod verify go list -m all | grep -E "github.com/bnb-chain/tss-lib|github.com/showa-93/go-mask|github.com/tonkeeper/tongo" go vet ./...Also applies to: 364-368
Line range hint
5-5: Toolchain update approved, verify compatibility.The update to Go toolchain 1.22.5 is a positive change, incorporating bug fixes and potential performance enhancements. To ensure seamless integration, it is advisable to verify compatibility with your project's codebase and dependencies.
e2e/e2etests/e2etests.go (2)
839-845: New test added for ERC20 withdrawal with arbitrary call.The addition of
TestV2ERC20WithdrawAndArbitraryCallNameenhances the test coverage for V2 smart contract functionality. This test specifically targets the withdrawal of ERC20 tokens from ZEVM with an arbitrary contract call using the V2 contract.
839-851: Verify the implementation of new and modified tests.To ensure that the new tests and modifications are correctly implemented, it would be prudent to verify the corresponding test functions.
Run the following script to check the implementation of the new and modified test functions:
✅ Verification successful
✅ Verified: The new test functions
TestV2ERC20WithdrawAndArbitraryCallandTestV2ERC20WithdrawAndCallare correctly implemented in the codebase.
- Implementations found in:
e2e/e2etests/test_v2_erc20_withdraw_and_arbitrary_call.goe2e/e2etests/test_v2_erc20_withdraw_and_call.go🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of new and modified test functions # Test: Search for the implementation of TestV2ERC20WithdrawAndArbitraryCall echo "Searching for TestV2ERC20WithdrawAndArbitraryCall implementation:" rg --type go -A 10 'func TestV2ERC20WithdrawAndArbitraryCall' # Test: Search for the implementation of TestV2ERC20WithdrawAndCall echo "Searching for TestV2ERC20WithdrawAndCall implementation:" rg --type go -A 10 'func TestV2ERC20WithdrawAndCall'Length of output: 4559
changelog.md (4)
Line range hint
216-247: Approve changes in v11.0.0 with recommendations for thorough testing.The changes in version v11.0.0 introduce important features and improvements:
- HSM capability for zetaclient hot key, enhancing security.
- New thread for checking zeta supply across connected chains.
- Ability to update observers, improving network management.
These additions, along with the fixes and refactoring, contribute to improved security, monitoring, and overall code quality.
To ensure the reliability and effectiveness of these changes, consider the following recommendations:
- Conduct thorough testing of the HSM capability, including various key management scenarios and potential failure modes.
- Implement comprehensive monitoring and alerting for the new zeta supply checking thread.
- Develop and execute test cases for the observer update functionality, including edge cases and potential conflicts.
To verify the implementation and test coverage of these new features, run the following script:
#!/bin/bash # Description: Verify the implementation and test coverage of v11.0.0 features # Test: Check for HSM capability implementation rg --type go "HSM.*zetaclient.*hot.*key" # Test: Verify the zeta supply checking thread rg --type go "thread.*checks.*zeta.*supply" # Test: Look for observer update functionality rg --type go "update.*observer" # Test: Check for tests related to these new features rg --type go "test.*HSM.*capability" rg --type go "test.*zeta.*supply.*check" rg --type go "test.*update.*observer"
Line range hint
91-214: Highlight breaking changes in v12.0.0 and suggest a migration guide.Version v12.0.0 introduces several breaking changes that will require updates to systems and applications interacting with the affected endpoints. The main changes include:
- Moving TSS and chain validation queries from
crosschaintoobservermodule.- Consolidation of observer sets for all chains.
- Merging of observer params and core params into chain params.
- Changes to the
GetTssAddressendpoint for Bitcoin.To ensure a smooth transition for users, consider the following actions:
- Create a comprehensive migration guide detailing all breaking changes and their implications.
- Provide code examples for updating affected API calls and queries.
- Develop and share scripts to help users identify and update affected parts of their codebase.
- Consider implementing temporary backwards compatibility or redirects for the most critical endpoints to allow for a gradual migration.
To verify the implementation of these changes and the presence of migration documentation, run the following script:
#!/bin/bash # Description: Verify the implementation of breaking changes and presence of migration guide # Test: Check for the new observer module endpoints rg --type go "observer/pendingNonces" rg --type go "observer/chainNonces" rg --type go "observer/observer_set" # Test: Verify the consolidation of chain params rg --type go "observer/get_chain_params" # Test: Check for the updated GetTssAddress endpoint rg --type go "observer/getTssAddress/.*bitcoin_chain_id" # Test: Look for migration guide or breaking changes documentation rg --type md "migration.*guide.*v12\.0\.0" rg --type md "breaking.*changes.*v12\.0\.0"Would you like assistance in creating a detailed migration guide for these breaking changes?
Line range hint
249-321: Highlight important features in v10.1.2 and recommend thorough testing and documentation.Version v10.1.2 introduces several significant features and improvements:
- External stress testing capability.
- Liquidity cap setting for ZRC20.
- TSS funds migration capability.
- Bitcoin block header and merkle proof functionality.
- Various fixes and refactoring to improve system stability and performance.
These additions enhance the system's testability, flexibility, and security.
To ensure the effective implementation and utilization of these features, consider the following recommendations:
- Develop comprehensive documentation for the external stress testing framework, including setup instructions and best practices for use.
- Create detailed guides for setting and managing liquidity caps for ZRC20 tokens.
- Implement thorough testing scenarios for the TSS funds migration capability, covering various edge cases and potential failure modes.
- Ensure the Bitcoin block header and merkle proof functionality is well-tested and documented, particularly focusing on its integration with existing systems.
To verify the implementation, test coverage, and documentation of these new features, run the following script:
#!/bin/bash # Description: Verify the implementation, testing, and documentation of v10.1.2 features # Test: Check for stress testing implementation rg --type go "external.*stress.*testing" # Test: Verify ZRC20 liquidity cap functionality rg --type go "liquidity.*cap.*ZRC20" # Test: Look for TSS funds migration capability rg --type go "tss.*funds.*migration" # Test: Check for Bitcoin block header and merkle proof functionality rg --type go "Bitcoin.*block.*header" rg --type go "merkle.*proof" # Test: Verify test coverage for these features rg --type go "test.*stress.*testing" rg --type go "test.*liquidity.*cap" rg --type go "test.*tss.*funds.*migration" rg --type go "test.*Bitcoin.*block.*header" # Test: Look for documentation of new features rg --type md "stress.*testing.*documentation" rg --type md "ZRC20.*liquidity.*cap" rg --type md "tss.*funds.*migration.*guide" rg --type md "Bitcoin.*block.*header.*merkle.*proof"Would you like assistance in creating comprehensive documentation or test plans for these new features?
Line range hint
26-89: Highlight the significance of emission distribution change in v12.1.0.The modification of emission distribution to use fixed block rewards in v12.1.0 is a significant change that could substantially impact the project's tokenomics. This change warrants special attention:
- Ensure that the rationale behind this change is well-documented.
- Verify that the new fixed block reward mechanism has been thoroughly tested and analyzed for potential long-term effects.
- Confirm that this change aligns with the project's overall economic model and goals.
To address this, consider the following actions:
- Create comprehensive documentation explaining the new emission distribution model, including its benefits and potential impacts.
- Develop and execute thorough tests to validate the new emission distribution mechanism under various scenarios.
- Prepare clear communication materials for stakeholders, explaining the change and its implications.
- Consider creating a simulation or economic analysis to demonstrate the long-term effects of this change on the token supply and distribution.
To verify the implementation and documentation of this change, run the following script:
pkg/contracts/testdappv2/TestDAppV2.abi (1)
159-189: UpdatedonCallFunction Correctly Implements the New InterfaceThe
onCallfunction defined in lines 159-189 has been appropriately updated to reflect the new contract interface:
- Inputs:
messageContextof typestruct TestDAppV2.MessageContext, which includes the sender's address.messageof typebytes.- Outputs:
- Returns a
bytestype, allowing for flexibility in response data.- State Mutability:
- Changed to
payable, enabling the function to receive Ether, which is suitable for handling authorized calls involving ERC20 tokens.This update aligns with the PR objective to support ERC20 authorization calls and ensures that the contract can handle the new message context structure effectively.
pkg/contracts/testdappv2/TestDAppV2.go (2)
54-55: Updated ABI and Binary Reflect Contract ChangesThe updated
ABIandBinvariables correctly synchronize the Go bindings with the Solidity contract's interface, ensuring seamless interaction between the Go code and the Ethereum contract.
422-426: 'OnCall' Method Signature Accurately Mirrors Solidity FunctionThe revisions to the
OnCallmethod signatures in the Go bindings precisely reflect the updated Solidity function:// Solidity: function onCall((bytes,address,uint256) _context, address _zrc20, uint256 amount, bytes message) returns()This alignment guarantees that the parameters
_context,_zrc20,amount, andmessageare correctly passed, facilitating proper contract execution.Also applies to: 429-433, 436-440
Description
Support erc20 auth call, in sync with: zeta-chain/protocol-contracts#401
How Has This Been Tested?
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation