feat: implement MsgUpdateERC20CustodyPauseStatus to pause or unpause ERC20 Custody
#2681
Conversation
Co-authored-by: skosito <skostic9242@gmail.com>
|
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 changes implement a command to pause or unpause the ERC20 Custody contract as part of the migration to version 2. This feature enhances governance over custody operations, allowing for administrative control to prevent deposits into the outdated version during the migration process. The new functionality is integrated into various components, including the API, messages, and testing frameworks, ensuring comprehensive control and validation of the custody status. Changes
Sequence Diagram(s)sequenceDiagram
participant Admin
participant API
participant Keeper
participant Signer
participant Blockchain
Admin->>API: Send Pause/Unpause Command
API->>Keeper: Validate Command and Update Status
Keeper->>Signer: Prepare Transaction for Status Update
Signer->>Blockchain: Send Transaction
Blockchain->>Keeper: Confirm Status Update
Keeper-->>API: Acknowledge Status Change
API-->>Admin: Respond with Success/Failure
Assessment against linked issues
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 #2681 +/- ##
===========================================
+ Coverage 70.85% 71.02% +0.16%
===========================================
Files 344 346 +2
Lines 18677 18807 +130
===========================================
+ Hits 13234 13358 +124
- Misses 4855 4858 +3
- Partials 588 591 +3
|
Co-authored-by: skosito <skostic9242@gmail.com>
Co-authored-by: skosito <skostic9242@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Outside diff range, codebase verification and nitpick comments (4)
proto/zetachain/zetacore/crosschain/tx.proto (2)
211-218: Clarify the purpose of fields inMsgUpdateERC20CustodyPauseStatus.The message structure is clear, but consider adding comments to describe the purpose of each field, especially
creatorandchain_id, to enhance readability and maintainability.message MsgUpdateERC20CustodyPauseStatus { string creator = 1; // The address of the entity initiating the command int64 chain_id = 2; // The ID of the blockchain where the custody is located // pause or unpause // true = pause, false = unpause bool pause = 3; }
220-220: Consider future extensibility inMsgUpdateERC20CustodyPauseStatusResponse.Currently,
MsgUpdateERC20CustodyPauseStatusResponseonly includescctx_index. Consider if additional fields might be needed in the future for more comprehensive responses.x/crosschain/types/cmd_cctxs.go (2)
33-34: Ensure consistent naming for gas multipliers.The naming of
ERC20CustodyPausingGasMultiplierEVMshould be consistent with other similar constants. Ensure that the naming convention aligns with the rest of the codebase.
120-126: Enhance theGetERC20CustodyPausingCmdCCTXIndexStringfunction for consistency.Ensure that the index string format is consistent with other similar functions to maintain uniformity across the codebase.
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 (20)
- 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_pause_erc20_custody.go (1 hunks)
- pkg/constant/constant.go (2 hunks)
- proto/zetachain/zetacore/crosschain/events.proto (1 hunks)
- proto/zetachain/zetacore/crosschain/tx.proto (2 hunks)
- x/authority/types/authorization_list.go (1 hunks)
- x/authority/types/authorization_list_test.go (1 hunks)
- x/crosschain/keeper/msg_server_migrate_erc20_custody_funds_test.go (2 hunks)
- x/crosschain/keeper/msg_server_update_erc20_custody_pause_status.go (1 hunks)
- x/crosschain/keeper/msg_server_update_erc20_custody_pause_status_test.go (1 hunks)
- x/crosschain/types/cmd_cctxs.go (3 hunks)
- x/crosschain/types/cmd_cctxs_test.go (1 hunks)
- x/crosschain/types/message_update_erc20_custody_pause_status.go (1 hunks)
- x/crosschain/types/message_update_erc20_custody_pause_status_test.go (1 hunks)
- zetaclient/chains/evm/signer/signer_admin.go (3 hunks)
- zetaclient/chains/evm/signer/signer_admin_test.go (6 hunks)
Additional context used
Path-based instructions (17)
x/crosschain/types/message_update_erc20_custody_pause_status.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.pkg/constant/constant.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_pause_erc20_custody.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_update_erc20_custody_pause_status.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/crosschain/types/message_update_erc20_custody_pause_status_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_admin.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.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/signer_admin_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.x/crosschain/keeper/msg_server_update_erc20_custody_pause_status_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.
GitHub Check: codecov/patch
x/crosschain/keeper/msg_server_update_erc20_custody_pause_status.go
[warning] 94-94: x/crosschain/keeper/msg_server_update_erc20_custody_pause_status.go#L94
Added line #L94 was not covered by testszetaclient/chains/evm/signer/signer_admin.go
[warning] 119-119: zetaclient/chains/evm/signer/signer_admin.go#L119
Added line #L119 was not covered by tests
[warning] 137-137: zetaclient/chains/evm/signer/signer_admin.go#L137
Added line #L137 was not covered by tests
LanguageTool
docs/spec/crosschain/messages.md
[misspelling] ~291-~291: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ... UpdateERC20CustodyPauseStatus creates a admin cmd cctx to update the pause stat...(EN_A_VS_AN)
Markdownlint
docs/spec/crosschain/messages.md
295-295: Column: 1
Hard tabs(MD010, no-hard-tabs)
296-296: Column: 1
Hard tabs(MD010, no-hard-tabs)
297-297: Column: 1
Hard tabs(MD010, no-hard-tabs)
Additional comments not posted (38)
docs/openapi/openapi.swagger.yaml (1)
Line range hint
1-1:
Confirm the addition of the new response object.The new object
crosschainMsgUpdateERC20CustodyPauseStatusResponsewith thecctx_indexproperty has been added. Ensure that this addition is consistent with the existing API design and that thecctx_indexproperty is adequately documented and utilized within the API.x/crosschain/types/message_update_erc20_custody_pause_status.go (4)
9-9: Define constants with appropriate naming conventions.The constant
TypeUpdateERC20CustodyPauseStatusis well-defined. Ensure that this naming convention is consistently used across the codebase for message types.
13-23: Constructor function for message struct is well-implemented.The
NewMsgUpdateERC20CustodyPauseStatusfunction correctly initializes the struct with the provided parameters. Ensure that input validation is handled elsewhere if necessary.
25-31: Implement routing and type methods correctly.The
RouteandTypemethods are implemented correctly for the message. Ensure that theRouterKeyis defined and used consistently in the module.
46-52: Validate input thoroughly inValidateBasic.The
ValidateBasicmethod correctly checks the creator address. Ensure additional validation is performed if required by business logic.proto/zetachain/zetacore/crosschain/events.proto (1)
79-83: New Protobuf messageEventERC20CustodyPausingis well-defined.The message fields are appropriately chosen for the event's purpose. Ensure that these fields are documented and used consistently across the system, and consider adding comments for clarity.
pkg/constant/constant.go (1)
20-21: Constants for ERC20 custody pause status management are well-defined.The addition of
CmdUpdateERC20CustodyPauseStatus,OptionPause, andOptionUnpauseconstants is clear and enhances the command interface for managing the ERC20 custody contract's operational state.Also applies to: 35-39
e2e/e2etests/test_pause_erc20_custody.go (1)
13-80: End-to-end test for ERC20 custody pause/unpause is comprehensive and well-structured.The test effectively verifies the pausing and unpausing functionality of the ERC20 custody contract, ensuring that the operations are correctly executed and reflected in the contract's state.
x/crosschain/keeper/msg_server_update_erc20_custody_pause_status.go (1)
13-99: FunctionUpdateERC20CustodyPauseStatusis well-implemented with comprehensive error handling.The function effectively manages the creation of a CCTX for pausing/unpausing the ERC20 custody contract, with appropriate checks and event emission. However, line 94 is not covered by tests.
Please ensure that the error handling path at line 94 is covered by tests to improve code coverage and reliability.
Tools
GitHub Check: codecov/patch
[warning] 94-94: x/crosschain/keeper/msg_server_update_erc20_custody_pause_status.go#L94
Added line #L94 was not covered by testsx/crosschain/types/message_update_erc20_custody_pause_status_test.go (5)
15-59: Comprehensive test coverage forValidateBasic.The test cases effectively cover both valid and invalid scenarios for the
ValidateBasicmethod ofMsgUpdateERC20CustodyPauseStatus.
61-99: Robust testing forGetSigners.The test cases effectively cover both valid and invalid signer scenarios for the
GetSignersmethod ofMsgUpdateERC20CustodyPauseStatus.
102-109: Correct verification of message type.The test accurately verifies the
Typemethod ofMsgUpdateERC20CustodyPauseStatus.
111-118: Correct verification of router key.The test accurately verifies the
Routemethod ofMsgUpdateERC20CustodyPauseStatus.
120-129: Ensures robustness ofGetSignBytes.The test correctly ensures that the
GetSignBytesmethod ofMsgUpdateERC20CustodyPauseStatusdoes not panic.zetaclient/chains/evm/signer/signer_admin.go (1)
Line range hint
16-29: Consistent integration of new command.The addition of
CmdUpdateERC20CustodyPauseStatusin theSignAdminTxfunction is consistent with existing patterns and correctly routes the command to its handler.x/authority/types/authorization_list.go (1)
26-26: Appropriate addition toAdminPolicyMessages.The addition of
"/zetachain.zetacore.crosschain.MsgUpdateERC20CustodyPauseStatus"toAdminPolicyMessagesis appropriate and aligns with the intent of restricting execution to the admin policy address.proto/zetachain/zetacore/crosschain/tx.proto (1)
40-41: Ensure compatibility with existing services.The new
UpdateERC20CustodyPauseStatusRPC method should be checked for compatibility with existing services and clients that interact with this proto file. Ensure that all necessary updates are made to accommodate this new method.zetaclient/chains/evm/signer/signer_admin_test.go (2)
67-81: Ensure comprehensive test coverage forCmdUpdateERC20CustodyPauseStatus.The test case for
CmdUpdateERC20CustodyPauseStatusis well-structured and covers the signing process. Ensure that edge cases, such as invalid command parameters, are also tested.
213-289: New Test Function:TestSigner_SignUpdateERC20CustodyPauseStatusCmdis comprehensive.The test function covers various scenarios such as successful signing for pause and unpause, invalid parameters, and TSS pause scenarios. This provides robust coverage for the new functionality.
x/crosschain/keeper/msg_server_migrate_erc20_custody_funds_test.go (1)
8-8: New Assertion: VerifyRelayedMessageincludesCmdMigrateERC20CustodyFunds.The addition of this assertion strengthens the test by ensuring that the correct command is included in the relayed message. This is a valuable check for validating the migration logic.
Also applies to: 62-62
x/crosschain/keeper/msg_server_update_erc20_custody_pause_status_test.go (8)
18-72: Comprehensive Test:TestKeeper_UpdateERC20CustodyPauseStatuscovers pause functionality.The test case effectively verifies the creation of a CCTX for pausing the ERC20 custody status. It includes checks for command inclusion and gas price calculations, ensuring robust validation of the pause functionality.
74-97: Test for Unauthorized Access: Properly handles unauthorized scenarios.The test ensures that unauthorized attempts to update the ERC20 custody pause status are correctly handled, which is crucial for maintaining security.
99-133: Test for Missing Chain Nonces: Validates error handling.The test correctly checks for the absence of chain nonces, ensuring that the system responds appropriately to this missing data.
135-169: Test for Missing TSS: Validates error handling.This test confirms that the system handles the absence of TSS keys correctly, which is essential for maintaining the integrity of the custody update process.
171-203: Test for Missing Chain Params: Ensures proper error handling.The test case verifies that the absence of chain parameters is handled correctly, which is important for the robustness of the custody update process.
205-239: Test for Missing Gas Price: Validates error handling.This test ensures that the system correctly handles scenarios where the gas price is unavailable, maintaining the reliability of the custody update process.
241-283: Test for Invalid Gas Amount: Correctly checks priority fees.The test case effectively validates that priority fees higher than the gas price are handled as errors, ensuring proper transaction cost management.
285-321: Test for Unsupported Chain Params: Validates error handling.This test confirms that the system handles unsupported chain parameters correctly, which is crucial for ensuring compatibility and stability.
x/authority/types/authorization_list_test.go (1)
417-417: Addition ofMsgUpdateERC20CustodyPauseStatusto AdminPolicyMessageList is appropriate.The inclusion of this message type URL aligns with the PR objectives to manage ERC20 Custody pause status, enhancing test coverage for admin policy authorizations.
cmd/zetae2e/local/local.go (1)
305-305: Addition ofTestPauseERC20CustodyNameto admin tests is appropriate.This test case enhances the test suite by covering the new functionality for pausing ERC20 custody, aligning with the PR objectives.
e2e/e2etests/e2etests.go (2)
111-111: Definition ofTestPauseERC20CustodyNameis clear and consistent.The test name is descriptive and aligns with naming conventions, facilitating easy identification within the test suite.
576-581: Addition ofTestPauseERC20CustodyNametoAllE2ETestsis well-integrated.The test case provides necessary coverage for the new pause functionality, with a clear description and appropriate argument setup.
x/crosschain/types/cmd_cctxs_test.go (5)
113-154: Approved: Comprehensive index uniqueness tests.The test cases effectively cover variations in
tssPubKey,nonce,chainID, anderc20Address, ensuring the uniqueness of the index string. The addition ofchainIDenhances the robustness of the test.
157-234: Approved: Thorough testing of command context creation.The test accurately verifies the creation of a command context for pausing ERC20 custody. It ensures uniqueness across different
nonce,tssPubKey, andchainIDvalues. The integration ofchainIDis well-executed.
237-270: Approved: Effective uniqueness testing for index strings.The test cases comprehensively cover different scenarios by varying
tssPubKey,nonce, andchainID. The assertions effectively verify the uniqueness of the index strings.
Line range hint
10-109: Approved: Comprehensive test coverage for custody fund migration.The test cases effectively validate the creation of command contexts for migrating ERC20 custody funds, ensuring uniqueness across different parameters. The integration of various scenarios is well-handled.
Line range hint
272-306: Approved: Effective testing of ERC20 whitelisting command contexts.The test cases thoroughly cover the creation of command contexts for whitelisting ERC20 tokens, ensuring uniqueness based on the ZRC20 address. The assertions are correctly implemented.
changelog.md (1)
13-13: Approved: Accurate and clear changelog entry.The changelog entry clearly and concisely describes the implementation of
MsgUpdateERC20CustodyPauseStatus, accurately reflecting the changes made in the PR.
Description
Closes: #2677
Similar to #2630 the message sends an admin cmd cctx.
It updates the ERC20Custody to paused or unpaused.
In practice, it will be used in V2 migration #2551 to pause the custody v1 to migrate to the new custody and prevent deposits in the meantime. Unpausing allow to revert in case of some frictions in the process
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation