feat: implement MsgMigrateERC20CustodyFunds to migrate the funds from the ERC20Custody to a new contracts
#2630
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughWalkthroughThe recent changes introduce a new message type, Changes
Assessment against linked issues
These updates enhance the system's ability to manage ERC20 custody funds, aligning with the requirements outlined in the linked issue. The changes were implemented with thorough testing and clear documentation. 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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
lumtis
requested review from
brewmaster012,
kingpinXD,
ws4charlie,
skosito,
swift1337 and
fbac
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2630 +/- ##
===========================================
+ Coverage 70.60% 70.85% +0.24%
===========================================
Files 340 344 +4
Lines 18533 18677 +144
===========================================
+ Hits 13086 13234 +148
+ Misses 4857 4855 -2
+ Partials 590 588 -2
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Outside diff range, codebase verification and nitpick comments (4)
zetaclient/chains/evm/signer/admin_cmd.go (2)
35-35: Lack of test coverage for error case.The error case for unknown commands in the
SignCommandTxfunction is not covered by tests. Ensure to add test cases to verify this scenario.
- File:
zetaclient/chains/evm/signer/admin_cmd.go- Line: 35
Analysis chain
Lack of test coverage for error case.
The error case for unknown commands is not covered by tests. Ensure to add test cases to verify this scenario.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify test coverage for the error case in SignCommandTx function. # Test: Search for test cases covering the error scenario. Expect: At least one test case covering the error scenario. rg --type go -A 5 'SignCommandTx.*unknown command'Length of output: 507
Tools
GitHub Check: codecov/patch
[warning] 35-35: zetaclient/chains/evm/signer/admin_cmd.go#L35
Added line #L35 was not covered by tests
87-87: Lack of test coverage for "invalid amount" error case.The
SignMigrateERC20CustodyFundsCmdfunction has test cases for successful signing, invalid parameters, and keysign failure. However, there is no test case for the "invalid amount" error scenario. Ensure to add a test case to verify this specific error handling path.
- File:
zetaclient/chains/evm/signer/admin_cmd_test.go- Relevant lines: Around the existing test cases for
SignMigrateERC20CustodyFundsCmdAnalysis chain
Lack of test coverage for error cases.
The error cases for invalid amount, ABI retrieval, and ABI packing errors are not covered by tests. Ensure to add test cases to verify these scenarios.
Also applies to: 92-92, 96-96
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify test coverage for error cases in SignMigrateERC20CustodyFundsCmd function. # Test: Search for test cases covering the error scenarios. Expect: At least one test case covering each error scenario. rg --type go -A 5 'SignMigrateERC20CustodyFundsCmd.*invalid amount' rg --type go -A 5 'SignMigrateERC20CustodyFundsCmd.*GetAbi' rg --type go -A 5 'SignMigrateERC20CustodyFundsCmd.*withdraw pack error'Length of output: 655
Script:
#!/bin/bash # Description: Verify if the SignMigrateERC20CustodyFundsCmd function is covered by any test cases. # Search for the SignMigrateERC20CustodyFundsCmd function in the test files. rg --type go 'SignMigrateERC20CustodyFundsCmd' --glob '*_test.go'Length of output: 964
Script:
#!/bin/bash # Description: Extract test cases for the SignMigrateERC20CustodyFundsCmd function from the admin_cmd_test.go file to verify coverage of specific error scenarios. # Extract the relevant test cases from the admin_cmd_test.go file. rg -A 20 'func TestSigner_SignMigrateERC20CustodyFundsCmd' zetaclient/chains/evm/signer/admin_cmd_test.goLength of output: 714
Script:
#!/bin/bash # Description: Extract additional lines from the admin_cmd_test.go file to capture the complete test cases for SignMigrateERC20CustodyFundsCmd. # Extract more lines from the admin_cmd_test.go file to capture the complete test cases. rg -A 50 'func TestSigner_SignMigrateERC20CustodyFundsCmd' zetaclient/chains/evm/signer/admin_cmd_test.goLength of output: 1650
Tools
GitHub Check: codecov/patch
[warning] 87-87: zetaclient/chains/evm/signer/admin_cmd.go#L87
Added line #L87 was not covered by testsx/crosschain/keeper/msg_server_migrate_tss_funds.go (1)
91-91: Lack of test coverage for error case.The error case for the
GetMedianGasValuescall within theinitiateMigrateTSSFundsCCTXfunction is not covered by tests. Ensure to add test cases to verify this scenario.
- File:
x/crosschain/keeper/msg_server_migrate_tss_funds.go- Function:
initiateMigrateTSSFundsCCTXAnalysis chain
Lack of test coverage for error case.
The error case for the
GetMedianGasValuescall is not covered by tests. Ensure to add test cases to verify this scenario.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify test coverage for the error case in initiateMigrateTSSFundsCCTX function. # Test: Search for test cases covering the error scenario. Expect: At least one test case covering the error scenario. rg --type go -A 5 'initiateMigrateTSSFundsCCTX.*GetMedianGasValues'Length of output: 69
Script:
#!/bin/bash # Description: Verify test coverage for the error case in initiateMigrateTSSFundsCCTX function. # Test: Search for test cases covering the error scenario. Expect: At least one test case covering the error scenario. rg --type go -A 10 'func initiateMigrateTSSFundsCCTX' | rg 'GetMedianGasValues' -B 10Length of output: 85
Script:
#!/bin/bash # Description: Verify test coverage for the error case in initiateMigrateTSSFundsCCTX function. # Step 1: Locate the definition of the initiateMigrateTSSFundsCCTX function. echo "Searching for the definition of the initiateMigrateTSSFundsCCTX function..." rg --type go 'func initiateMigrateTSSFundsCCTX' # Step 2: Locate test files that might cover the initiateMigrateTSSFundsCCTX function. echo "Searching for test files covering the initiateMigrateTSSFundsCCTX function..." rg --type go 'initiateMigrateTSSFundsCCTX' --testsLength of output: 858
Script:
#!/bin/bash # Description: Verify test coverage for the error case in initiateMigrateTSSFundsCCTX function. # Step 1: Locate the definition of the initiateMigrateTSSFundsCCTX function. echo "Searching for the definition of the initiateMigrateTSSFundsCCTX function..." rg --type go 'func initiateMigrateTSSFundsCCTX' # Step 2: Locate test files and functions that might cover the initiateMigrateTSSFundsCCTX function. echo "Searching for test files and functions covering the initiateMigrateTSSFundsCCTX function..." rg --type go 'initiateMigrateTSSFundsCCTX'Length of output: 848
zetaclient/chains/evm/signer/admin_cmd_test.go (1)
196-234: Ensure comprehensive coverage of edge cases.The function
TestSigner_SignMigrateTssFundsCmdcovers two primary scenarios: successful signing and failure due to key signing issues. However, additional edge cases should be tested for comprehensive coverage:
- Invalid
txDatainputs.- Network issues or client errors.
- Handling of unexpected exceptions.
Consider adding test cases for these scenarios to enhance the robustness of the tests.
Analysis chain
Ensure comprehensive coverage of edge cases.
The function
TestSigner_SignMigrateTssFundsCmdappears to be comprehensive, but ensure that all edge cases are covered and that the test assertions are correct.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the coverage of edge cases in the test function. # Test: Search for the test function and verify the coverage of edge cases. rg --type go -A 20 $'TestSigner_SignMigrateTssFundsCmd'Length of output: 1737
Script:
#!/bin/bash # Extract the full implementation of the TestSigner_SignMigrateTssFundsCmd function to verify edge case coverage. ast-grep --lang go --pattern $'func TestSigner_SignMigrateTssFundsCmd(t *testing.T) { $$$ }'Length of output: 3372
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (4)
typescript/zetachain/zetacore/crosschain/events_pb.d.tsis excluded by!**/*_pb.d.tstypescript/zetachain/zetacore/crosschain/tx_pb.d.tsis excluded by!**/*_pb.d.tsx/crosschain/types/events.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/crosschain/types/tx.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
Files selected for processing (26)
- changelog.md (1 hunks)
- cmd/zetae2e/local/local.go (1 hunks)
- docs/openapi/openapi.swagger.yaml (1 hunks)
- docs/spec/crosschain/messages.md (1 hunks)
- e2e/e2etests/e2etests.go (2 hunks)
- e2e/e2etests/test_migrate_erc20_custody_funds.go (1 hunks)
- pkg/constant/constant.go (1 hunks)
- proto/zetachain/zetacore/crosschain/events.proto (1 hunks)
- proto/zetachain/zetacore/crosschain/tx.proto (2 hunks)
- testutil/sample/crosschain.go (1 hunks)
- x/authority/types/authorization_list.go (1 hunks)
- x/authority/types/authorization_list_test.go (1 hunks)
- x/crosschain/keeper/cctx_utils.go (1 hunks)
- x/crosschain/keeper/msg_server_migrate_erc20_custody_funds.go (1 hunks)
- x/crosschain/keeper/msg_server_migrate_erc20_custody_funds_test.go (1 hunks)
- x/crosschain/keeper/msg_server_migrate_tss_funds.go (5 hunks)
- x/crosschain/keeper/msg_server_migrate_tss_funds_test.go (2 hunks)
- x/crosschain/keeper/msg_server_whitelist_erc20.go (5 hunks)
- x/crosschain/types/cmd_cctxs.go (1 hunks)
- x/crosschain/types/cmd_cctxs_test.go (1 hunks)
- x/crosschain/types/message_migrate_erc20_custody_funds.go (1 hunks)
- x/crosschain/types/message_migrate_erc20_custody_funds_test.go (1 hunks)
- zetaclient/chains/evm/signer/admin_cmd.go (1 hunks)
- zetaclient/chains/evm/signer/admin_cmd_test.go (1 hunks)
- zetaclient/chains/evm/signer/signer.go (3 hunks)
- zetaclient/chains/evm/signer/signer_test.go (3 hunks)
Files skipped from review due to trivial changes (1)
- x/crosschain/keeper/cctx_utils.go
Additional context used
Path-based instructions (22)
pkg/constant/constant.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.proto/zetachain/zetacore/crosschain/events.proto (1)
Pattern
**/*.proto: Review the Protobuf definitions, point out issues relative to compatibility, and expressiveness.x/crosschain/types/message_migrate_erc20_custody_funds.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_migrate_erc20_custody_funds.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/crosschain/keeper/msg_server_migrate_erc20_custody_funds.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/evm/signer/admin_cmd.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/crosschain/keeper/msg_server_migrate_tss_funds.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/crosschain/keeper/msg_server_whitelist_erc20.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/authority/types/authorization_list.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/crosschain/types/message_migrate_erc20_custody_funds_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.proto/zetachain/zetacore/crosschain/tx.proto (1)
Pattern
**/*.proto: Review the Protobuf definitions, point out issues relative to compatibility, and expressiveness.x/crosschain/types/cmd_cctxs.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/evm/signer/admin_cmd_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/crosschain/keeper/msg_server_migrate_erc20_custody_funds_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/evm/signer/signer_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/authority/types/authorization_list_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.cmd/zetae2e/local/local.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.x/crosschain/types/cmd_cctxs_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/crosschain/keeper/msg_server_migrate_tss_funds_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/evm/signer/signer.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.testutil/sample/crosschain.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
Learnings (2)
zetaclient/chains/evm/signer/admin_cmd_test.go (3)
Learnt from: ws4charlie PR: zeta-chain/node#2411 File: zetaclient/orchestrator/chain_activate.go:116-181 Timestamp: 2024-07-05T00:02:36.493Z Learning: The `CreateSignerObserverEVM` function in `zetaclient/orchestrator/chain_activate.go` is covered by unit tests in `zetaclient/orchestrator/chain_activate_test.go`.Learnt from: ws4charlie PR: zeta-chain/node#2411 File: zetaclient/orchestrator/orchestrator.go:192-217 Timestamp: 2024-07-04T23:46:38.428Z Learning: The `GetUpdatedSigner` method in `zetaclient/orchestrator/orchestrator.go` is covered by unit tests in `zetaclient/orchestrator/chain_activate_test.go` and `zetaclient/orchestrator/orchestrator_test.go`.Learnt from: ws4charlie PR: zeta-chain/node#2411 File: zetaclient/orchestrator/chain_activate.go:184-247 Timestamp: 2024-07-05T00:02:31.446Z Learning: The `CreateSignerObserverBTC` function in `zetaclient/orchestrator/chain_activate.go` is covered by unit tests in `zetaclient/orchestrator/chain_activate_test.go`.zetaclient/chains/evm/signer/signer_test.go (2)
Learnt from: ws4charlie PR: zeta-chain/node#2411 File: zetaclient/orchestrator/chain_activate.go:116-181 Timestamp: 2024-07-05T00:02:36.493Z Learning: The `CreateSignerObserverEVM` function in `zetaclient/orchestrator/chain_activate.go` is covered by unit tests in `zetaclient/orchestrator/chain_activate_test.go`.Learnt from: ws4charlie PR: zeta-chain/node#2411 File: zetaclient/orchestrator/orchestrator.go:192-217 Timestamp: 2024-07-04T23:46:38.428Z Learning: The `GetUpdatedSigner` method in `zetaclient/orchestrator/orchestrator.go` is covered by unit tests in `zetaclient/orchestrator/chain_activate_test.go` and `zetaclient/orchestrator/orchestrator_test.go`.
GitHub Check: codecov/patch
x/crosschain/keeper/msg_server_migrate_erc20_custody_funds.go
[warning] 98-98: x/crosschain/keeper/msg_server_migrate_erc20_custody_funds.go#L98
Added line #L98 was not covered by testszetaclient/chains/evm/signer/admin_cmd.go
[warning] 35-35: zetaclient/chains/evm/signer/admin_cmd.go#L35
Added line #L35 was not covered by tests
[warning] 50-50: zetaclient/chains/evm/signer/admin_cmd.go#L50
Added line #L50 was not covered by tests
[warning] 54-54: zetaclient/chains/evm/signer/admin_cmd.go#L54
Added line #L54 was not covered by tests
[warning] 87-87: zetaclient/chains/evm/signer/admin_cmd.go#L87
Added line #L87 was not covered by tests
[warning] 92-92: zetaclient/chains/evm/signer/admin_cmd.go#L92
Added line #L92 was not covered by tests
[warning] 96-96: zetaclient/chains/evm/signer/admin_cmd.go#L96
Added line #L96 was not covered by testsx/crosschain/types/cmd_cctxs.go
[warning] 134-134: x/crosschain/types/cmd_cctxs.go#L134
Added line #L134 was not covered by tests
[warning] 155-155: x/crosschain/types/cmd_cctxs.go#L155
Added line #L155 was not covered by tests
Markdownlint
docs/spec/crosschain/messages.md
281-281: Column: 1
Hard tabs(MD010, no-hard-tabs)
282-282: Column: 1
Hard tabs(MD010, no-hard-tabs)
283-283: Column: 1
Hard tabs(MD010, no-hard-tabs)
284-284: Column: 1
Hard tabs(MD010, no-hard-tabs)
285-285: Column: 1
Hard tabs(MD010, no-hard-tabs)
LanguageTool
changelog.md
[grammar] ~8-~8: The plural noun “contracts” cannot be used with the article “a”. Did you mean “a new contract” or “new contracts”?
Context: ...rate the funds from the ERC20Custody to a new contracts ## v19.0.0 ### Breaking Changes * [2...(A_NNS)
Additional comments not posted (75)
docs/openapi/openapi.swagger.yaml (1)
Line range hint
1-1:
LGTM!The new object definition
crosschainMsgMigrateERC20CustodyFundsResponsewith propertycctx_indexis well-defined and expressive.pkg/constant/constant.go (1)
11-13: LGTM!The new constant
CmdMigrateERC20CustodyFundsis well-named and consistent with existing constants. It enhances the clarity and capabilities of the command set.proto/zetachain/zetacore/crosschain/events.proto (1)
72-77: LGTM!The new message definition
EventERC20CustodyFundsMigrationwith fieldsnew_custody_address,erc20_address,amount, andcctx_indexis well-defined and expressive.x/crosschain/types/message_migrate_erc20_custody_funds.go (8)
3-9: Imports look good.The necessary packages for errors, math, and SDK types are correctly imported.
11-13: Constants and variable declarations look good.The constant and variable declarations are appropriate and necessary.
15-29: FunctionNewMsgMigrateERC20CustodyFundslooks good.The function correctly initializes the message with the provided parameters.
31-33: FunctionRoutelooks good.The function correctly returns the router key.
35-37: FunctionTypelooks good.The function correctly returns the message type.
39-45: FunctionGetSignerslooks good.The function correctly converts the creator address and returns it as the signer.
47-50: FunctionGetSignByteslooks good.The function correctly marshals the message to JSON and sorts it.
52-71: FunctionValidateBasiclooks good.The function performs necessary checks for the creator address, new custody address, ERC20 address, and amount.
e2e/e2etests/test_migrate_erc20_custody_funds.go (1)
3-13: Imports look good.The necessary packages for math, Ethereum bindings, test utilities, and crosschain types are correctly imported.
x/crosschain/keeper/msg_server_migrate_erc20_custody_funds.go (1)
3-11: Imports look good.The necessary packages for context, errors, SDK types, authority types, and crosschain types are correctly imported.
zetaclient/chains/evm/signer/admin_cmd.go (2)
115-131: LGTM!The function
SignMigrateTssFundsCmdis well-structured and there are no immediate issues.
50-50: Lack of test coverage for error cases.The error cases for invalid ERC20 address and ABI packing errors are not covered by tests. Ensure to add test cases to verify these scenarios.
Also applies to: 54-54
Tools
GitHub Check: codecov/patch
[warning] 50-50: zetaclient/chains/evm/signer/admin_cmd.go#L50
Added line #L50 was not covered by testsx/crosschain/keeper/msg_server_migrate_tss_funds.go (1)
68-68: Lack of test coverage for error case.The error case for the
initiateMigrateTSSFundsCCTXcall is not covered by tests. Ensure to add test cases to verify this scenario.Verification successful
Test coverage for error cases in
MigrateTssFundsfunction.The error cases for the
MigrateTssFundsfunction are adequately covered in the following test file:
x/crosschain/keeper/msg_server_migrate_tss_funds_test.goThis file includes multiple test scenarios that handle various error conditions, ensuring comprehensive test coverage.
- File:
x/crosschain/keeper/msg_server_migrate_tss_funds_test.go
- Tests for authorization errors
- Tests for inbound enabled errors
- Tests for TSS not found
- Tests for empty TSS history
- Tests for no new TSS generated
- Tests for current TSS being the latest
- Tests for pending nonces not found
- Tests for pending migrations
No further action is required as the error cases are well-covered by the existing tests.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify test coverage for the error case in MigrateTssFunds function. # Test: Search for test cases covering the error scenario. Expect: At least one test case covering the error scenario. rg --type go -A 5 'MigrateTssFunds.*initiateMigrateTSSFundsCCTX'Length of output: 66
Script:
#!/bin/bash # Description: Verify test coverage for the error case in MigrateTssFunds function. # Step 1: Find all test files that call the MigrateTssFunds function. test_files=$(rg --files-with-matches 'MigrateTssFunds' --type go --glob '*_test.go') # Step 2: Check if any of these test files handle errors from MigrateTssFunds. for file in $test_files; do echo "Checking file: $file" rg 'MigrateTssFunds' -A 5 "$file" | rg 'err' doneLength of output: 3982
x/crosschain/keeper/msg_server_whitelist_erc20.go (1)
100-100: Lack of test coverage for error cases.The error cases for the
GetChainParamsByChainIDandWhitelistERC20CmdCCTXcalls are not covered by tests. Ensure to add test cases to verify these scenarios.Also applies to: 128-128
x/authority/types/authorization_list.go (1)
26-26: LGTM!The addition of
"/zetachain.zetacore.crosschain.MsgMigrateERC20CustodyFunds"to theAdminPolicyMessageslist is correct and follows the existing pattern.x/crosschain/types/message_migrate_erc20_custody_funds_test.go (5)
16-89: Comprehensive test coverage forValidateBasicmethod.The test cases for
TestNewMsgMigrateERC20CustodyFunds_ValidateBasiccover various scenarios and are well-structured.
91-134: Comprehensive test coverage forGetSignersmethod.The test cases for
TestNewMsgMigrateERC20CustodyFunds_GetSignerscover valid and invalid signers and are well-structured.
136-145: Correct test forTypemethod.The test case for
TestNewMsgMigrateERC20CustodyFunds_Typecorrectly verifies the message type.
147-156: Correct test forRoutemethod.The test case for
TestNewMsgMigrateERC20CustodyFunds_Routecorrectly verifies the message route.
158-169: Correct test forGetSignBytesmethod.The test case for
TestNewMsgMigrateERC20CustodyFunds_GetSignBytescorrectly verifies that the method does not panic.proto/zetachain/zetacore/crosschain/tx.proto (3)
37-38: LGTM!The addition of the
MigrateERC20CustodyFundsRPC method to theMsgservice is correct and follows the existing pattern.
195-204: LGTM!The definition of the
MsgMigrateERC20CustodyFundsmessage is correct and follows the existing pattern.
206-206: LGTM!The definition of the
MsgMigrateERC20CustodyFundsResponsemessage is correct and follows the existing pattern.x/crosschain/types/cmd_cctxs.go (6)
56-63: LGTM!The function
GetERC20CustodyMigrationCCTXIndexStringis straightforward and correct.
193-202: LGTM!The function
GetTssMigrationCCTXIndexStringis straightforward and correct.
204-244: LGTM!The function
newCmdCCTXis well-structured and correct.
79-81: Verify the correctness of the hash generation.Ensure that the generated hash is correct and unique for each transaction.
Verification successful
Hash Generation in
WhitelistERC20CmdCCTXFunctionThe hash generation in the
WhitelistERC20CmdCCTXfunction appears to be correctly implemented using thecrypto.Keccak256Hashfunction withzrc20Address.Bytes()and[]byte("WhitelistERC20"). This ensures the hash is unique for eachzrc20Addressand the specific context of "WhitelistERC20".Implementation Details:
- The function generates a hash using the
Keccak256Hashfunction.- The hash is based on the bytes of the
zrc20Addressand a static string "WhitelistERC20".- This hash is then used to create a new
CrossChainTxobject.Verification Steps:
- The uniqueness of the hash is ensured by the unique
zrc20Addressand the static suffix.- The correctness of the hash generation can be verified through unit tests, as seen in
cmd_cctxs_test.go.Recommendation:
- Ensure comprehensive unit tests cover various scenarios to validate the uniqueness and correctness of the generated hash.
Files to Review:
x/crosschain/types/cmd_cctxs.gox/crosschain/types/cmd_cctxs_test.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the uniqueness of the hash generation. # Test: Search for the function usage and verify the generated hash. rg --type go -A 5 $'WhitelistERC20CmdCCTX'Length of output: 2143
Script:
#!/bin/bash # Extract the implementation of the WhitelistERC20CmdCCTX function ast-grep --lang go --pattern $'func WhitelistERC20CmdCCTX($_, $_, $_, $_, $_) { $$$ }'Length of output: 89
Script:
#!/bin/bash # Search for the WhitelistERC20CmdCCTX function definition and include context lines to capture the full implementation. rg --type go -A 20 'func WhitelistERC20CmdCCTX('Length of output: 155
Script:
#!/bin/bash # Search for the WhitelistERC20CmdCCTX function definition and include context lines to capture the full implementation. rg --type go -A 50 'func WhitelistERC20CmdCCTX\('Length of output: 2957
97-191: Ensure proper error handling and gas fee calculations.The function
MigrateFundCmdCCTXhandles multiple cases and calculates gas fees. Ensure that the gas fee calculations are correct and that errors are properly handled.Verification successful
Ensure proper error handling and gas fee calculations.
The function
MigrateFundCmdCCTXhas been verified to handle gas fee calculations and error handling correctly through comprehensive test cases for both EVM and Bitcoin chains. The tests validate the function's behavior by checking for errors and verifying the expected values of various fields.
- The test cases cover different scenarios and ensure that the gas fee calculations are accurate.
- Error handling is properly validated through assertions in the test cases.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of gas fee calculations and error handling. # Test: Search for the function usage and verify the gas fee calculations and error handling. rg --type go -A 10 $'MigrateFundCmdCCTX'Length of output: 7485
Script:
#!/bin/bash # Extract the relevant test cases and their assertions to verify the gas fee calculations and error handling. # Search for the test cases related to `MigrateFundCmdCCTX` and extract their details. rg --type go -A 20 'func TestMigrateFundCmdCCTX' x/crosschain/types/cmd_cctxs_test.goLength of output: 679
Script:
#!/bin/bash # Extract the remaining parts of the test case to see the assertions and verify the correctness of the gas fee calculations and error handling. # Search for the test cases related to `MigrateFundCmdCCTX` and extract their details. rg --type go -A 50 'func TestMigrateFundCmdCCTX' x/crosschain/types/cmd_cctxs_test.goLength of output: 1851
Script:
#!/bin/bash # Extract the remaining parts of the test case for Bitcoin to verify the correctness of the gas fee calculations and error handling. # Search for the test cases related to `MigrateFundCmdCCTX` and extract their details. rg --type go -A 50 'returns a new CCTX for migrating funds on Bitcoin' x/crosschain/types/cmd_cctxs_test.goLength of output: 1889
Tools
GitHub Check: codecov/patch
[warning] 134-134: x/crosschain/types/cmd_cctxs.go#L134
Added line #L134 was not covered by tests
[warning] 155-155: x/crosschain/types/cmd_cctxs.go#L155
Added line #L155 was not covered by tests
31-33: Verify the correctness of the index string and hash generation.Ensure that the generated index string and hash are correct and unique for each transaction.
zetaclient/chains/evm/signer/admin_cmd_test.go (3)
81-127: Ensure comprehensive coverage of edge cases.The function
TestSigner_SignWhitelistERC20Cmdappears to be comprehensive, but ensure that all edge cases are covered and that the test assertions are correct.
14-78: Ensure comprehensive coverage of edge cases.The function
TestSigner_SignCommandTxappears to be comprehensive, but ensure that all edge cases are covered and that the test assertions are correct.
130-193: Ensure comprehensive coverage of edge cases.The function
TestSigner_SignMigrateERC20CustodyFundsCmdappears to be comprehensive, but ensure that all edge cases are covered and that the test assertions are correct.Verification successful
Ensure comprehensive coverage of edge cases.
The test functions in
admin_cmd_test.gocover various scenarios for theSignercommands, including successful signing, invalid parameters, and key signing failures. However, to ensure comprehensive edge case coverage, consider adding tests for:
- Handling of edge cases related to parameter validation, such as empty strings, null values, and incorrect data types.
- Behavior under different network conditions, such as timeouts and connection failures.
- Additional failure modes, such as unexpected errors or exceptions.
Suggested locations for additional tests:
TestSigner_SignCommandTxTestSigner_SignWhitelistERC20CmdTestSigner_SignMigrateERC20CustodyFundsCmdTestSigner_SignMigrateTssFundsCmdScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the coverage of edge cases in the test function. # Test: Search for the test function and verify the coverage of edge cases. rg --type go -A 20 $'TestSigner_SignMigrateERC20CustodyFundsCmd'Length of output: 1657
Script:
#!/bin/bash # Description: Retrieve the complete implementation of the TestSigner_SignMigrateERC20CustodyFundsCmd function. # Search for the test function and include more context lines to capture the full implementation. rg --type go -A 50 $'func TestSigner_SignMigrateERC20CustodyFundsCmd' zetaclient/chains/evm/signer/admin_cmd_test.goLength of output: 1660
Script:
#!/bin/bash # Description: Retrieve the complete implementation of the TestSigner_SignMigrateERC20CustodyFundsCmd function. # Search for the test function and include more context lines to capture the full implementation. rg --type go -A 100 $'func TestSigner_SignMigrateERC20CustodyFundsCmd' zetaclient/chains/evm/signer/admin_cmd_test.goLength of output: 3068
Script:
#!/bin/bash # Description: Verify if additional edge cases are covered in the test suite by searching for related test functions. # Search for other test functions in the same file to ensure comprehensive coverage of edge cases. rg --type go '^func TestSigner_' zetaclient/chains/evm/signer/admin_cmd_test.goLength of output: 300
Script:
#!/bin/bash # Description: Retrieve the complete implementations of the other test functions in the file to verify their coverage. # Search for the implementations of the other test functions and include sufficient context lines to capture the full implementations. rg --type go -A 50 $'func TestSigner_SignCommandTx' zetaclient/chains/evm/signer/admin_cmd_test.go rg --type go -A 50 $'func TestSigner_SignWhitelistERC20Cmd' zetaclient/chains/evm/signer/admin_cmd_test.go rg --type go -A 50 $'func TestSigner_SignMigrateTssFundsCmd' zetaclient/chains/evm/signer/admin_cmd_test.goLength of output: 4771
docs/spec/crosschain/messages.md (1)
275-287: Verify the correctness and consistency of the field types and descriptions.The new message type
MsgMigrateERC20CustodyFundsappears to be well-defined, but ensure that the field types and descriptions are correct and consistent with other message types.Verification successful
Verification successful: The field types and descriptions in
MsgMigrateERC20CustodyFundsare correct and consistent with other message types.
- The field types (
string,int64) are consistent with those used in other message types.- The descriptions follow the same pattern and provide clear information.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness and consistency of the field types and descriptions. # Test: Search for the message type and verify the field types and descriptions. rg --type md -A 10 $'MsgMigrateERC20CustodyFunds'Length of output: 1548
Script:
#!/bin/bash # Description: List all message types in the docs/spec/crosschain/messages.md file and their fields for comparison. # Extract all message types and their fields from the specified file. rg --type md -A 10 $'message ' docs/spec/crosschain/messages.mdLength of output: 4213
Tools
Markdownlint
281-281: Column: 1
Hard tabs(MD010, no-hard-tabs)
282-282: Column: 1
Hard tabs(MD010, no-hard-tabs)
283-283: Column: 1
Hard tabs(MD010, no-hard-tabs)
284-284: Column: 1
Hard tabs(MD010, no-hard-tabs)
285-285: Column: 1
Hard tabs(MD010, no-hard-tabs)
x/crosschain/keeper/msg_server_migrate_erc20_custody_funds_test.go (8)
17-59: Ensure comprehensive test coverage.The test case "can create CCTX to migrate ERC20 custody funds" is well-structured and covers the positive scenario. However, consider adding additional assertions to verify the state changes in the observer module and the correct handling of the gas price.
61-86: LGTM!The sub-test "should fail if not authorized" correctly verifies the failure scenario for unauthorized access.
88-124: LGTM!The sub-test "should fail if can't find chain nonces" correctly verifies the failure scenario when chain nonces are not found.
126-162: LGTM!The sub-test "should fail if can't find current TSS" correctly verifies the failure scenario when the current TSS is not found.
164-198: LGTM!The sub-test "should fail if can't find chain params" correctly verifies the failure scenario when chain params are not found.
200-236: LGTM!The sub-test "should fail if can't find gas price" correctly verifies the failure scenario when the gas price is not found.
238-282: LGTM!The sub-test "should fail if priority fees higher than gas price" correctly verifies the failure scenario when priority fees are higher than the gas price.
284-322: LGTM!The sub-test "should fail if can't set outbound info" correctly verifies the failure scenario when outbound info cannot be set.
zetaclient/chains/evm/signer/signer_test.go (15)
Line range hint
83-93: LGTM!The test case
TestSigner_SetGetConnectorAddresscorrectly verifies the getter and setter functionality for the connector address.
Line range hint
95-105: LGTM!The test case
TestSigner_SetGetERC20CustodyAddresscorrectly verifies the getter and setter functionality for the ERC20 custody address.
Line range hint
107-124: LGTM!The test case
TestSigner_TryProcessOutboundcorrectly verifies the outbound processing functionality.
Line range hint
126-153: LGTM!The test case
TestSigner_SignOutboundcorrectly verifies the signing of outbound transactions.
Line range hint
155-180: LGTM!The test case
TestSigner_SignRevertTxcorrectly verifies the signing of revert transactions.
Line range hint
182-207: LGTM!The test case
TestSigner_SignCancelTxcorrectly verifies the signing of cancel transactions.
Line range hint
209-235: LGTM!The test case
TestSigner_SignWithdrawTxcorrectly verifies the signing of withdraw transactions.
Line range hint
237-267: LGTM!The test case
TestSigner_SignERC20WithdrawTxcorrectly verifies the signing of ERC20 withdraw transactions.
Line range hint
269-295: LGTM!The test case
TestSigner_BroadcastOutboundcorrectly verifies the broadcasting of outbound transactions.
Line range hint
297-303: LGTM!The test case
TestSigner_getEVMRPCcorrectly verifies the EVM RPC functionality.
Line range hint
305-309: LGTM!The test case
TestSigner_SignerErrorMsgcorrectly verifies the error message generation for the signer.
Line range hint
311-326: LGTM!The helper function
makeCtxcorrectly creates the context for testing.
Line range hint
71-81: LGTM!The helper function
verifyTxSignaturecorrectly verifies the signature of a transaction.
Line range hint
83-93: LGTM!The helper function
verifyTxBodyBasicscorrectly verifies the basic components of a transaction body.
Line range hint
17-37: LGTM!The helper function
getNewEvmSignercorrectly creates a new EVM chain signer for testing.x/authority/types/authorization_list_test.go (5)
Line range hint
17-61: LGTM!The test case
TestAuthorizationList_SetAuthorizationscorrectly verifies the setting of authorizations in the authorization list.
Line range hint
63-112: LGTM!The test case
TestAuthorizationList_GetAuthorizationscorrectly verifies the retrieval of authorizations from the authorization list.
Line range hint
114-179: LGTM!The test case
TestAuthorizationList_Validatecorrectly verifies the validation of the authorization list.
Line range hint
181-247: LGTM!The test case
TestAuthorizationList_RemoveAuthorizationscorrectly verifies the removal of authorizations from the authorization list.
Line range hint
249-417: LGTM!The test case
TestDefaultAuthorizationsListcorrectly verifies the default authorizations list.cmd/zetae2e/local/local.go (1)
302-302: New test case added:TestMigrateERC20CustodyFundsName.The new test case
TestMigrateERC20CustodyFundsNamehas been added to theadminTestRoutine. Ensure that this test case is correctly implemented and integrated into the test suite.e2e/e2etests/e2etests.go (2)
111-111: New constant added:TestMigrateERC20CustodyFundsName.The new constant
TestMigrateERC20CustodyFundsNameis defined with the string value"migrate_erc20_custody_funds". This addition is consistent with the naming conventions of existing constants.
576-581: New test case added:TestMigrateERC20CustodyFunds.The new test case
TestMigrateERC20CustodyFundshas been added to theAllE2ETestsslice. Ensure that this test case is correctly implemented and integrated into the test suite.x/crosschain/types/cmd_cctxs_test.go (5)
16-105: Comprehensive test forMigrateERC20CustodyFundsCmdCCTX.The test function
TestMigrateERC20CustodyFundsCmdCCTXcovers multiple scenarios to ensure the correctness of theMigrateERC20CustodyFundsCmdCCTXfunction. The test cases are well-structured and thorough.
108-142: Comprehensive test forGetERC20CustodyMigrationCCTXIndexString.The test function
TestGetERC20CustodyMigrationCCTXIndexStringcovers multiple scenarios to ensure the correctness of theGetERC20CustodyMigrationCCTXIndexStringfunction. The test cases are well-structured and thorough.
145-199: Comprehensive test forWhitelistERC20CmdCCTX.The test function
TestWhitelistERC20CmdCCTXcovers multiple scenarios to ensure the correctness of theWhitelistERC20CmdCCTXfunction. The test cases are well-structured and thorough.
201-476: Comprehensive test forMigrateFundCmdCCTX.The test function
TestMigrateFundCmdCCTXcovers multiple scenarios to ensure the correctness of theMigrateFundCmdCCTXfunction. The test cases are well-structured and thorough.
479-539: Comprehensive test forGetTssMigrationCCTXIndexString.The test function
TestGetTssMigrationCCTXIndexStringcovers multiple scenarios to ensure the correctness of theGetTssMigrationCCTXIndexStringfunction. The test cases are well-structured and thorough.x/crosschain/keeper/msg_server_migrate_tss_funds_test.go (2)
113-118: Verify the integration of the new index string logic.The variable
indexStringis now derived from the new functioncrosschaintypes.GetTssMigrationCCTXIndexString. Ensure that this change is correctly integrated and does not break existing functionality.Verification successful
Verified the integration of the new index string logic.
The function
crosschaintypes.GetTssMigrationCCTXIndexStringis correctly integrated and used consistently across the codebase. The dedicated test functionTestGetTssMigrationCCTXIndexStringensures its correctness and functionality.
x/crosschain/types/cmd_cctxs.gox/crosschain/types/cmd_cctxs_test.gox/crosschain/keeper/msg_server_migrate_tss_funds_test.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of the new index string logic. # Test: Search for the function usage. Expect: Only occurrences of the new function. rg --type go -A 5 $'GetTssMigrationCCTXIndexString'Length of output: 4128
Line range hint
77-83:
Verify the correctness and impact of the index string generation change.The function
crosschaintypes.GetTssMigrationCCTXIndexStringis now used to generate the index string. Ensure this change aligns with the intended functionality and that all dependent code segments are updated accordingly.Verification successful
Verify the correctness and impact of the index string generation change.
The function
crosschaintypes.GetTssMigrationCCTXIndexStringis used in multiple places and is well-integrated into the codebase. Ensure that the old functionkeeper.GetIndexStringForTssMigrationis no longer in use and that the new function is correctly implemented and tested.
Verification complete: The index string generation change is correctly implemented and tested.
- The old function
keeper.GetIndexStringForTssMigrationis no longer in use.- The new function
GetTssMigrationCCTXIndexStringis well-integrated into the codebase and thoroughly tested.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness and impact of the index string generation change. # Test: Search for the function usage. Expect: Only occurrences of the new function. rg --type go -A 5 $'GetTssMigrationCCTXIndexString'Length of output: 4128
Script:
#!/bin/bash # Description: Verify that the old function `keeper.GetIndexStringForTssMigration` is no longer in use. # Test: Search for any remaining usage of the old function. rg --type go -A 5 $'GetIndexStringForTssMigration'Length of output: 49
Script:
#!/bin/bash # Description: Verify the implementation and testing of the new function `GetTssMigrationCCTXIndexString`. # Test: Search for the implementation of the new function. ast-grep --lang go --pattern $'func GetTssMigrationCCTXIndexString($_, $_, $_, $_, $_) {\n $$$\n}' # Test: Search for the tests of the new function. rg --type go -A 5 $'TestGetTssMigrationCCTXIndexString'Length of output: 643
changelog.md (1)
7-7: Stylistic change approved.The change in capitalization from "Add" to "add" is acceptable.
Co-authored-by: skosito <[email protected]>
lumtis
added
UPGRADE_TESTS
Description
MsgWhitelistERC20andMsgMigrateTSSMsgMigrateERC20CustodyFundsadds a new admin cmd cctx that tells the TSS to withdraw funds from custody to a new contractCloses: #2280
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation