feat(fungible): implement MsgUpdateZRC20Name
#3541
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 📝 WalkthroughWalkthroughThis pull request introduces a new message type, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ZetaTxServer
participant MsgServer
participant AuthorityKeeper
participant Keeper
Client->>ZetaTxServer: Submit MsgUpdateZRC20Name\n(creator, token address, new name, symbol)
ZetaTxServer->>MsgServer: Route MsgUpdateZRC20Name request
MsgServer->>AuthorityKeeper: CheckAuthorization(creator)
AuthorityKeeper-->>MsgServer: Authorization validated (or error)
MsgServer->>Keeper: Call UpdateZRC20Name
Keeper->>Keeper: Validate ZRC20 address and existence
Keeper->>Keeper: Execute ZRC20SetName (if name provided)
Keeper->>Keeper: Execute ZRC20SetSymbol (if symbol provided)
Keeper-->>MsgServer: Return MsgUpdateZRC20NameResponse
MsgServer-->>ZetaTxServer: Relay response
ZetaTxServer-->>Client: Return transaction result (tx hash)
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
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3541 +/- ##
===========================================
+ Coverage 64.68% 64.69% +0.01%
===========================================
Files 453 455 +2
Lines 31310 31495 +185
===========================================
+ Hits 20252 20377 +125
- Misses 10189 10229 +40
- Partials 869 889 +20
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
e2e/e2etests/test_update_zrc20_name.go (1)
12-54: Enhance test coverage with negative test cases.While the happy path is well tested, consider adding tests for:
- Unauthorized access attempts
- Invalid ZRC20 addresses
- Empty name and symbol
- Special characters in name/symbol
x/fungible/types/message_update_zrc20_name_test.go (1)
14-89: LGTM with a minor suggestion for test readability.The test suite provides comprehensive coverage of the message type's functionality.
Consider making the test case descriptions more descriptive. For example:
- name: "valid message", + name: "should accept valid message with both name and symbol", - name: "valid message with empty name", + name: "should accept valid message with only symbol update",x/fungible/keeper/zrc20_methods_test.go (2)
15-41: Consider adding error test cases for ZRC20SetName.While the happy path is tested, consider adding test cases for:
- Invalid ZRC20 address
- Empty name
- Error scenarios from EVM calls
29-41: Consider adding error test cases for ZRC20SetSymbol.Similar to the name tests, consider adding test cases for:
- Invalid ZRC20 address
- Empty symbol
- Error scenarios from EVM calls
e2e/e2etests/e2etests.go (1)
133-136: Consider adding a comment to document the test's purpose.The admin tests section has a comment block explaining its purpose. Consider adding documentation for this new test to maintain consistency.
/* Admin tests Test admin functionalities */ +// TestUpdateZRC20Name tests the ability to update the name and symbol of ZRC20 tokens +// through admin policy group governance. runner.NewE2ETest( TestUpdateZRC20NameName, "update ZRC20 name and symbol",Also applies to: 975-981
docs/openapi/openapi.swagger.yaml (1)
59565-59566: Review the New OpenAPI Definition forfungibleMsgUpdateZRC20NameResponseThe new response object is defined cleanly and follows the expected structure by simply specifying the type as an object. Please verify that this minimal definition aligns with the API documentation and your intended behavior. If future enhancements (such as including status codes, error messages, or additional metadata) are expected, consider incorporating those fields or adding a descriptive comment for clarity.
docs/spec/fungible/messages.md (1)
155-166: Documentation Consistency Suggestion
The newMsgUpdateZRC20Namemessage is clearly documented and its proto block is well formatted. For consistency with other message descriptions, consider adding an explicit note regarding the required admin authorization (e.g. “Authorized: admin policy group 2”) similar to the other messages in this file.proto/zetachain/zetacore/fungible/tx.proto (1)
142-149: Proto Message Definition Assessment
The newly introducedMsgUpdateZRC20Namemessage is structured correctly with all required fields (creator, zrc20_address, name, symbol) and the appropriate field numbering. Consider, as an optional enhancement, adding inline comments or field options if future validations or custom types become necessary.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
typescript/zetachain/zetacore/fungible/tx_pb.d.tsis excluded by!**/*_pb.d.tsx/fungible/types/tx.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
📒 Files selected for processing (18)
changelog.md(1 hunks)cmd/zetae2e/local/local.go(1 hunks)docs/openapi/openapi.swagger.yaml(1 hunks)docs/spec/fungible/messages.md(1 hunks)e2e/e2etests/e2etests.go(2 hunks)e2e/e2etests/test_update_zrc20_name.go(1 hunks)proto/zetachain/zetacore/fungible/tx.proto(2 hunks)x/authority/migrations/v3/migrate.go(2 hunks)x/authority/migrations/v3/migrate_test.go(1 hunks)x/authority/types/authorization_list.go(1 hunks)x/authority/types/authorization_list_test.go(1 hunks)x/fungible/keeper/msg_server_update_zrc20_name.go(1 hunks)x/fungible/keeper/msg_server_update_zrc20_name_test.go(1 hunks)x/fungible/keeper/zrc20_methods.go(1 hunks)x/fungible/keeper/zrc20_methods_test.go(5 hunks)x/fungible/types/codec.go(2 hunks)x/fungible/types/message_update_zrc20_name.go(1 hunks)x/fungible/types/message_update_zrc20_name_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- x/authority/types/authorization_list_test.go
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.go`: Review the Go code, point out issues relative to ...
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetae2e/local/local.gox/fungible/types/codec.gox/authority/types/authorization_list.goe2e/e2etests/test_update_zrc20_name.gox/authority/migrations/v3/migrate_test.gox/authority/migrations/v3/migrate.gox/fungible/keeper/msg_server_update_zrc20_name_test.gox/fungible/keeper/msg_server_update_zrc20_name.gox/fungible/keeper/zrc20_methods_test.goe2e/e2etests/e2etests.gox/fungible/keeper/zrc20_methods.gox/fungible/types/message_update_zrc20_name_test.gox/fungible/types/message_update_zrc20_name.go
`**/*.proto`: Review the Protobuf definitions, point out iss...
**/*.proto: Review the Protobuf definitions, point out issues relative to compatibility, and expressiveness.
proto/zetachain/zetacore/fungible/tx.proto
🪛 GitHub Check: codecov/patch
x/fungible/keeper/msg_server_update_zrc20_name.go
[warning] 51-52: x/fungible/keeper/msg_server_update_zrc20_name.go#L51-L52
Added lines #L51 - L52 were not covered by tests
[warning] 57-58: x/fungible/keeper/msg_server_update_zrc20_name.go#L57-L58
Added lines #L57 - L58 were not covered by tests
x/fungible/keeper/zrc20_methods.go
[warning] 36-37: x/fungible/keeper/zrc20_methods.go#L36-L37
Added lines #L36 - L37 were not covered by tests
[warning] 53-54: x/fungible/keeper/zrc20_methods.go#L53-L54
Added lines #L53 - L54 were not covered by tests
[warning] 56-57: x/fungible/keeper/zrc20_methods.go#L56-L57
Added lines #L56 - L57 were not covered by tests
[warning] 70-71: x/fungible/keeper/zrc20_methods.go#L70-L71
Added lines #L70 - L71 were not covered by tests
[warning] 87-88: x/fungible/keeper/zrc20_methods.go#L87-L88
Added lines #L87 - L88 were not covered by tests
[warning] 90-91: x/fungible/keeper/zrc20_methods.go#L90-L91
Added lines #L90 - L91 were not covered by tests
[warning] 103-104: x/fungible/keeper/zrc20_methods.go#L103-L104
Added lines #L103 - L104 were not covered by tests
[warning] 119-120: x/fungible/keeper/zrc20_methods.go#L119-L120
Added lines #L119 - L120 were not covered by tests
[warning] 123-124: x/fungible/keeper/zrc20_methods.go#L123-L124
Added lines #L123 - L124 were not covered by tests
[warning] 128-129: x/fungible/keeper/zrc20_methods.go#L128-L129
Added lines #L128 - L129 were not covered by tests
[warning] 132-133: x/fungible/keeper/zrc20_methods.go#L132-L133
Added lines #L132 - L133 were not covered by tests
[warning] 137-138: x/fungible/keeper/zrc20_methods.go#L137-L138
Added lines #L137 - L138 were not covered by tests
[warning] 150-151: x/fungible/keeper/zrc20_methods.go#L150-L151
Added lines #L150 - L151 were not covered by tests
[warning] 166-167: x/fungible/keeper/zrc20_methods.go#L166-L167
Added lines #L166 - L167 were not covered by tests
[warning] 170-171: x/fungible/keeper/zrc20_methods.go#L170-L171
Added lines #L170 - L171 were not covered by tests
[warning] 175-176: x/fungible/keeper/zrc20_methods.go#L175-L176
Added lines #L175 - L176 were not covered by tests
[warning] 179-180: x/fungible/keeper/zrc20_methods.go#L179-L180
Added lines #L179 - L180 were not covered by tests
[warning] 184-185: x/fungible/keeper/zrc20_methods.go#L184-L185
Added lines #L184 - L185 were not covered by tests
🔇 Additional comments (12)
x/authority/migrations/v3/migrate.go (1)
21-25: LGTM! Authorization setup is correct and secure.The authorization for
MsgUpdateZRC20Nameis properly configured with the appropriate admin policy group, maintaining security controls for token updates.x/fungible/types/message_update_zrc20_name.go (1)
44-58: LGTM! Validation is thorough and complete.The ValidateBasic implementation correctly checks:
- Creator address validity
- ZRC20 address format
- Presence of at least one update field
x/fungible/types/codec.go (1)
21-21: LGTM! Codec registration is complete and consistent.The message type is properly registered in both the Amino codec and interface registry, following the established pattern.
Also applies to: 36-36
x/authority/migrations/v3/migrate_test.go (1)
19-19: LGTM!The test correctly removes the authorization for
MsgUpdateZRC20Namebefore migration, ensuring proper testing of the migration process.x/fungible/keeper/msg_server_update_zrc20_name.go (1)
15-62: LGTM with suggestions for test coverage.The implementation is well-structured with proper authorization checks, input validation, and error handling.
The error paths in contract method calls lack test coverage. Consider adding test cases for:
- Line 51: Failure in
ZRC20SetName- Line 57: Failure in
ZRC20SetSymbol🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 51-52: x/fungible/keeper/msg_server_update_zrc20_name.go#L51-L52
Added lines #L51 - L52 were not covered by tests
[warning] 57-58: x/fungible/keeper/msg_server_update_zrc20_name.go#L57-L58
Added lines #L57 - L58 were not covered by testsx/authority/types/authorization_list.go (1)
37-37: LGTM!The message is correctly added to
AdminPolicyMessages, ensuring that only admin policy addresses can update ZRC20 names and symbols.x/fungible/keeper/msg_server_update_zrc20_name_test.go (1)
16-195: LGTM! Comprehensive test coverage for the ZRC20 name/symbol update functionality.The test suite thoroughly covers essential scenarios including authorization, validation, error cases, and successful updates. The test structure follows best practices with clear arrange-act-assert patterns.
cmd/zetae2e/local/local.go (1)
333-333: LGTM! Added ZRC20 name update test to admin test suite.The test case is appropriately placed in the admin test routine since it involves admin-only functionality.
e2e/e2etests/e2etests.go (2)
149-149: LGTM!The constant follows the established naming convention for test names.
975-981: LGTM!The test registration follows the established pattern for admin tests, including:
- Appropriate placement in the admin tests section
- Consistent use of
WithMinimumVersion- Clear and concise test description
proto/zetachain/zetacore/fungible/tx.proto (1)
31-31: RPC Method Addition Verification
The addition of the RPC method
rpc UpdateZRC20Name(MsgUpdateZRC20Name) returns (MsgUpdateZRC20NameResponse);
is consistent with the other service definitions. Please verify that the backend implementation properly enforces the necessary authorization checks and integrates with the existing codec registrations.changelog.md (1)
14-15: Changelog Entry Clarity
The changelog entry clearly documents the implementation ofMsgUpdateZRC20Namealong with the PR reference. This meets the documentation standards for change logs. Ensure that any related follow‐up changes (such as updates in tests or additional documentation) are consistently mentioned in future entries.
Description
Closes #3540
Allow to update then name and symbol of a ZRC20 with admin policy group
Summary by CodeRabbit