feat: add new a new msg to remove inbound trackers #3469
Conversation
📝 WalkthroughWalkthroughA new feature has been implemented to remove inbound trackers through a dedicated Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Command
participant MsgServer as Keeper (MsgServer)
participant Auth as AuthorityKeeper
CLI->>MsgServer: Send MsgRemoveInboundTracker message
MsgServer->>Auth: CheckAuthorization(msg.creator)
Auth-->>MsgServer: Return authorization status
MsgServer->>MsgServer: Call RemoveInboundTrackerIfExists(chain_id, tx_hash)
MsgServer-->>CLI: Return MsgRemoveInboundTrackerResponse
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3469 +/- ##
===========================================
+ Coverage 65.28% 65.32% +0.03%
===========================================
Files 441 443 +2
Lines 30429 30463 +34
===========================================
+ Hits 19865 19899 +34
Misses 9708 9708
Partials 856 856
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
x/crosschain/client/cli/tx_inbound_tracker.go (1)
59-63: Add input validation for tx-hash.Consider validating the transaction hash format before proceeding with the command.
Add validation:
argChain, err := strconv.ParseInt(args[0], 10, 64) if err != nil { return err } argTxHash := args[1] +if len(argTxHash) == 0 { + return fmt.Errorf("transaction hash cannot be empty") +}docs/openapi/openapi.swagger.yaml (1)
59098-59099: New Object Definition Added:crosschainMsgRemoveInboundTrackerResponseThe new response object is correctly added with its type set to
object. Please ensure that, if applicable, extended metadata (e.g., descriptions, examples, or property definitions) is documented either here or in the accompanying API documentation to facilitate client integration and clarity.docs/spec/crosschain/messages.md (1)
37-48: Documentation Update for MsgRemoveInboundTracker:
The new documentation section forMsgRemoveInboundTrackeris concise and clear. The protobuf code snippet correctly mirrors the proto definition (with thecreator,chain_id, andtx_hashfields) and the associated description succinctly explains its purpose. Additionally, if markdown lint warnings regarding hard tabs arise, consider replacing any tab characters with spaces.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
43-43: Hard tabs
Column: 1(MD010, no-hard-tabs)
44-44: Hard tabs
Column: 1(MD010, no-hard-tabs)
45-45: Hard tabs
Column: 1(MD010, no-hard-tabs)
docs/cli/zetacored/cli.md (1)
10604-10659: Detailed Command Documentation Enhancement
The new documentation block for theremove-inbound-trackercommand is comprehensive and clear. However, there are a few stylistic improvements to consider:
- Fenced Code Block Language Specification: Several fenced code blocks (e.g., the command usage snippet starting at line 10611 and others around line 10617 and 10648) lack a language identifier. Specifying a language (such as
bashorconsole) will enhance readability and ensure compliance with markdownlint (MD040).- Hard Tabs: There’s an indication of hard tabs (e.g., around line 10658). Replace these with spaces to align with the style guidelines.
Proposed diff for fenced code block language identifier improvement:
-``` +```bash zetacored tx crosschain remove-inbound-tracker [chain-id] [tx-hash] [flags] -``` +```These minor adjustments will improve the clarity and maintainability of the CLI documentation.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
10605-10605: Hard tabs
Column: 54(MD010, no-hard-tabs)
10611-10611: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
10617-10617: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
10648-10648: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
10658-10658: Hard tabs
Column: 54(MD010, no-hard-tabs)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
typescript/zetachain/zetacore/crosschain/tx_pb.d.tsis excluded by!**/*_pb.d.tsx/crosschain/types/tx.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
📒 Files selected for processing (14)
changelog.md(1 hunks)docs/cli/zetacored/cli.md(2 hunks)docs/openapi/openapi.swagger.yaml(1 hunks)docs/spec/crosschain/messages.md(1 hunks)proto/zetachain/zetacore/crosschain/tx.proto(3 hunks)x/authority/types/authorization_list.go(1 hunks)x/authority/types/authorization_list_test.go(1 hunks)x/crosschain/client/cli/tx.go(1 hunks)x/crosschain/client/cli/tx_inbound_tracker.go(2 hunks)x/crosschain/keeper/msg_remove_inbound_tracker.go(1 hunks)x/crosschain/keeper/msg_remove_inbound_tracker_test.go(1 hunks)x/crosschain/types/codec.go(2 hunks)x/crosschain/types/message_remove_inbound_tracker.go(1 hunks)x/crosschain/types/message_remove_inbound_tracker_test.go(1 hunks)
🧰 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.
x/crosschain/client/cli/tx.gox/authority/types/authorization_list.gox/authority/types/authorization_list_test.gox/crosschain/client/cli/tx_inbound_tracker.gox/crosschain/types/codec.gox/crosschain/types/message_remove_inbound_tracker_test.gox/crosschain/keeper/msg_remove_inbound_tracker.gox/crosschain/keeper/msg_remove_inbound_tracker_test.gox/crosschain/types/message_remove_inbound_tracker.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/crosschain/tx.proto
🪛 markdownlint-cli2 (0.17.2)
docs/cli/zetacored/cli.md
10333-10333: Hard tabs
Column: 100
(MD010, no-hard-tabs)
10611-10611: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
10617-10617: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
10648-10648: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
10658-10658: Hard tabs
Column: 54
(MD010, no-hard-tabs)
docs/spec/crosschain/messages.md
43-43: Hard tabs
Column: 1
(MD010, no-hard-tabs)
44-44: Hard tabs
Column: 1
(MD010, no-hard-tabs)
45-45: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🔇 Additional comments (11)
x/crosschain/keeper/msg_remove_inbound_tracker.go (1)
26-26: Consider error handling for RemoveInboundTrackerIfExists.The
RemoveInboundTrackerIfExistsoperation might fail, but its errors are not being captured or propagated.Consider handling potential errors:
-k.RemoveInboundTrackerIfExists(ctx, msg.ChainId, msg.TxHash) -return &types.MsgRemoveInboundTrackerResponse{}, nil +if err := k.RemoveInboundTrackerIfExists(ctx, msg.ChainId, msg.TxHash); err != nil { + return nil, errorsmod.Wrapf(err, "failed to remove inbound tracker for chain %d and tx %s", msg.ChainId, msg.TxHash) +} +return &types.MsgRemoveInboundTrackerResponse{}, nilx/crosschain/client/cli/tx.go (1)
34-34: LGTM!The command registration follows the established pattern and is properly integrated.
x/crosschain/types/codec.go (1)
22-22: LGTM: Message registration is properly implemented.The new message type
MsgRemoveInboundTrackeris correctly registered in bothRegisterCodecandRegisterInterfacesfunctions, following the established pattern.Also applies to: 46-46
x/crosschain/keeper/msg_remove_inbound_tracker_test.go (1)
16-92: LGTM: Well-structured test cases.The test cases are well-organized using the Arrange-Act-Assert pattern and cover essential scenarios:
- Authorization failure
- Successful removal
- Non-existent tracker handling
x/crosschain/types/message_remove_inbound_tracker_test.go (2)
14-67: LGTM: Comprehensive ValidateBasic test cases.The test cases for ValidateBasic cover all essential scenarios including invalid address, valid cases, and edge cases with unsupported chain IDs and transaction hashes.
69-107: LGTM: Well-structured GetSigners test cases.The test cases properly validate both successful and panic scenarios for GetSigners.
x/authority/types/authorization_list.go (1)
51-51: LGTM: Emergency policy message properly registered.The new message URL
/zetachain.zetacore.crosschain.MsgRemoveInboundTrackeris correctly added to the EmergencyPolicyMessages slice, maintaining consistency with other emergency operations.x/authority/types/authorization_list_test.go (1)
408-408: LGTM! Well-placed authorization for the new message type.The addition of
MsgRemoveInboundTrackerto the emergency policy list is appropriate, as it's grouped with similar critical operations and properly tested.proto/zetachain/zetacore/crosschain/tx.proto (2)
21-22: New RPC Method Addition:
The addition of theRemoveInboundTrackerRPC method in theMsgservice follows the established pattern for similar methods and appears well integrated. Please ensure that this service method is properly wired up with the corresponding keeper logic and codec registration later in the codebase.
85-96: New Message Definitions for Inbound Tracker Removal:
The new messageMsgRemoveInboundTracker(with fieldscreator,chain_id, andtx_hash) is clearly defined and documented. Its corresponding response message,MsgRemoveInboundTrackerResponse, is correctly declared (even if empty) to serve its purpose. This design is consistent with similar existing messages.changelog.md (1)
5-8: Changelog Update for New Feature:
The changelog entry under the "Features" section is clear and appropriately documents the addition ofMsgRemoveInboundTracker, including the PR reference and a brief description of its purpose. This concise update helps maintain transparency in feature changes.
| func (msg *MsgRemoveInboundTracker) GetSigners() []sdk.AccAddress { | ||
| creator, err := sdk.AccAddressFromBech32(msg.Creator) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| return []sdk.AccAddress{creator} | ||
| } |
There was a problem hiding this comment.
Avoid panic in GetSigners.
The GetSigners method should handle errors gracefully instead of panicking. Consider returning an empty slice on error.
Apply this change:
func (msg *MsgRemoveInboundTracker) GetSigners() []sdk.AccAddress {
creator, err := sdk.AccAddressFromBech32(msg.Creator)
if err != nil {
- panic(err)
+ return []sdk.AccAddress{}
}
return []sdk.AccAddress{creator}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (msg *MsgRemoveInboundTracker) GetSigners() []sdk.AccAddress { | |
| creator, err := sdk.AccAddressFromBech32(msg.Creator) | |
| if err != nil { | |
| panic(err) | |
| } | |
| return []sdk.AccAddress{creator} | |
| } | |
| func (msg *MsgRemoveInboundTracker) GetSigners() []sdk.AccAddress { | |
| creator, err := sdk.AccAddressFromBech32(msg.Creator) | |
| if err != nil { | |
| return []sdk.AccAddress{} | |
| } | |
| return []sdk.AccAddress{creator} | |
| } |
| func (msg *MsgRemoveInboundTracker) ValidateBasic() error { | ||
| _, err := sdk.AccAddressFromBech32(msg.Creator) | ||
| if err != nil { | ||
| return errorsmod.Wrapf(sdkerrors.ErrInvalidAddress, "invalid creator address (%s)", err) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance basic validation.
The ValidateBasic method only validates the creator address. Consider adding validation for chain ID and transaction hash.
Apply this enhancement:
func (msg *MsgRemoveInboundTracker) ValidateBasic() error {
_, err := sdk.AccAddressFromBech32(msg.Creator)
if err != nil {
return errorsmod.Wrapf(sdkerrors.ErrInvalidAddress, "invalid creator address (%s)", err)
}
+ if msg.ChainId <= 0 {
+ return errorsmod.Wrap(sdkerrors.ErrInvalidRequest, "chain ID must be positive")
+ }
+ if len(msg.TxHash) == 0 {
+ return errorsmod.Wrap(sdkerrors.ErrInvalidRequest, "transaction hash cannot be empty")
+ }
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (msg *MsgRemoveInboundTracker) ValidateBasic() error { | |
| _, err := sdk.AccAddressFromBech32(msg.Creator) | |
| if err != nil { | |
| return errorsmod.Wrapf(sdkerrors.ErrInvalidAddress, "invalid creator address (%s)", err) | |
| } | |
| return nil | |
| } | |
| func (msg *MsgRemoveInboundTracker) ValidateBasic() error { | |
| _, err := sdk.AccAddressFromBech32(msg.Creator) | |
| if err != nil { | |
| return errorsmod.Wrapf(sdkerrors.ErrInvalidAddress, "invalid creator address (%s)", err) | |
| } | |
| if msg.ChainId <= 0 { | |
| return errorsmod.Wrap(sdkerrors.ErrInvalidRequest, "chain ID must be positive") | |
| } | |
| if len(msg.TxHash) == 0 { | |
| return errorsmod.Wrap(sdkerrors.ErrInvalidRequest, "transaction hash cannot be empty") | |
| } | |
| return nil | |
| } |
| "github.com/zeta-chain/node/x/crosschain/types" | ||
| ) | ||
|
|
||
| func TestMsgServer_AddInboundTracker(t *testing.T) { |
There was a problem hiding this comment.
Fix incorrect test function name.
The test function name TestMsgServer_AddInboundTracker doesn't match its purpose. It's testing RemoveInboundTracker functionality.
Apply this diff to fix the function name:
-func TestMsgServer_AddInboundTracker(t *testing.T) {
+func TestMsgServer_RemoveInboundTracker(t *testing.T) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestMsgServer_AddInboundTracker(t *testing.T) { | |
| func TestMsgServer_RemoveInboundTracker(t *testing.T) { |
| func TestMsgRemoveInboundTracker_GetSignBytes(t *testing.T) { | ||
| msg := types.NewMsgRemoveInboundTracker( | ||
| sample.AccAddress(), | ||
| chains.Goerli.ChainId, | ||
| sample.ZetaIndex(t), | ||
| ) | ||
| require.Equal(t, msg.GetSignBytes(), msg.GetSignBytes()) | ||
| } |
There was a problem hiding this comment.
Fix ineffective GetSignBytes test.
The current test is comparing the same method call with itself, which doesn't validate the actual sign bytes content.
Apply this diff to improve the test:
- require.Equal(t, msg.GetSignBytes(), msg.GetSignBytes())
+ require.NotEmpty(t, msg.GetSignBytes())
+ require.Equal(t, string(msg.GetSignBytes()), string(msg.GetSignBytes()), "sign bytes should be deterministic")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestMsgRemoveInboundTracker_GetSignBytes(t *testing.T) { | |
| msg := types.NewMsgRemoveInboundTracker( | |
| sample.AccAddress(), | |
| chains.Goerli.ChainId, | |
| sample.ZetaIndex(t), | |
| ) | |
| require.Equal(t, msg.GetSignBytes(), msg.GetSignBytes()) | |
| } | |
| func TestMsgRemoveInboundTracker_GetSignBytes(t *testing.T) { | |
| msg := types.NewMsgRemoveInboundTracker( | |
| sample.AccAddress(), | |
| chains.Goerli.ChainId, | |
| sample.ZetaIndex(t), | |
| ) | |
| require.NotEmpty(t, msg.GetSignBytes()) | |
| require.Equal(t, string(msg.GetSignBytes()), string(msg.GetSignBytes()), "sign bytes should be deterministic") | |
| } |
| * [zetacored tx crosschain add-outbound-tracker](#zetacored-tx-crosschain-add-outbound-tracker) - Add an outbound tracker | ||
| * [zetacored tx crosschain migrate-tss-funds](#zetacored-tx-crosschain-migrate-tss-funds) - Migrate TSS funds to the latest TSS address | ||
| * [zetacored tx crosschain refund-aborted](#zetacored-tx-crosschain-refund-aborted) - Refund an aborted tx , the refund address is optional, if not provided, the refund will be sent to the sender/tx origin of the cctx. | ||
| * [zetacored tx crosschain remove-inbound-tracker](#zetacored-tx-crosschain-remove-inbound-tracker) - Remove an inbound tracker | ||
| * [zetacored tx crosschain remove-outbound-tracker](#zetacored-tx-crosschain-remove-outbound-tracker) - Remove an outbound tracker | ||
| * [zetacored tx crosschain update-tss-address](#zetacored-tx-crosschain-update-tss-address) - Create a new TSSVoter | ||
| * [zetacored tx crosschain vote-gas-price](#zetacored-tx-crosschain-vote-gas-price) - Broadcast message to vote gas price |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
New Command Entry and Markdown Style Consistency
The new command entry for zetacored tx crosschain remove-inbound-tracker is well integrated into the command list. Please verify that the anchor link (#zetacored-tx-crosschain-remove-inbound-tracker) correctly points to the corresponding section. Additionally, the static analysis hint indicates that a hard tab is detected on this line; consider replacing any hard tabs with spaces to adhere to markdownlint guidelines.
Proposed diff for hard tab replacement (if applicable):
- * [zetacored tx crosschain remove-inbound-tracker](#zetacored-tx-crosschain-remove-inbound-tracker) - Remove an inbound tracker
+ * [zetacored tx crosschain remove-inbound-tracker](#zetacored-tx-crosschain-remove-inbound-tracker) - Remove an inbound trackerError: No shell scripts or web queries found
🏁 Script executed:
#!/bin/bash
# Verify that the anchor link for "remove-inbound-tracker" in the command list correctly points to an existing section header
# and check if there are any hard tabs in the vicinity (lines 10320 to 10340) in the markdown file.
echo "Searching for markdown headers containing 'remove-inbound-tracker':"
rg -n "^#{1,6}\s+.*remove-inbound-tracker" docs/cli/zetacored/cli.md
echo ""
echo "Displaying lines 10320 to 10340 to inspect potential hard tabs (visible as '^I'):"
sed -n '10320,10340p' docs/cli/zetacored/cli.md | cat -ALength of output: 2364
Anchor Link Validity and Hard Tab Removal
- The anchor link
#zetacored-tx-crosschain-remove-inbound-trackercorrectly points to the corresponding header found at line 10607 in the markdown file. - Hard tabs are present in the command list entries (as shown by the
^Icharacters). It is recommended to replace these with spaces to comply with markdownlint guidelines.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
10330-10330: Hard tabs
Column: 96
(MD010, no-hard-tabs)
10331-10331: Hard tabs
Column: 90
(MD010, no-hard-tabs)
10332-10332: Hard tabs
Column: 84
(MD010, no-hard-tabs)
10333-10333: Hard tabs
Column: 100
(MD010, no-hard-tabs)
10334-10334: Hard tabs
Column: 102
(MD010, no-hard-tabs)
10335-10335: Hard tabs
Column: 92
(MD010, no-hard-tabs)
10336-10336: Hard tabs
Column: 84
(MD010, no-hard-tabs)
Description
Closes #1345
How Has This Been Tested?
Summary by CodeRabbit