refactor(votes)!: drop support for header proofs #3125
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 📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request encompasses a series of changes across various files, primarily focused on simplifying and enhancing the functionality of the outbound tracker within the ZetaChain node. Key modifications include the removal of block header proof support, updates to the OpenAPI specification, and enhancements to the changelog. Additionally, several message types related to cross-chain transactions have been streamlined by removing unnecessary fields. The changes also involve renaming methods for clarity and consistency, particularly in the context of outbound tracker operations. Changes
Assessment against linked issues
Possibly related PRs
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
|
swift1337
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3125 +/- ##
===========================================
- Coverage 63.29% 63.23% -0.06%
===========================================
Files 422 422
Lines 29959 29865 -94
===========================================
- Hits 18962 18886 -76
+ Misses 10157 10144 -13
+ Partials 840 835 -5
|
swift1337
requested review from
brewmaster012,
kingpinXD,
lumtis,
ws4charlie,
skosito and
fbac
as code owners
swift1337
added
UPGRADE_TESTS
![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
🧹 Outside diff range and nitpick comments (28)
proto/zetachain/zetacore/crosschain/outbound_tracker.proto (1)
9-11: LGTM! Consider enhancing the documentation.The use of
reservedfield is correct and maintains backward compatibility. The comment provides good historical context.Consider expanding the comment to document the rationale:
- // used to be `bool proven` (for block header verification) + // Field 3 was previously `bool proven` used for block header verification. + // Removed as part of dropping support for header proofs (see PR #3125).x/crosschain/client/cli/tx_add_outbound_tracker.go (1)
35-36: LGTM! Consider enhancing documentation.The simplified message construction aligns well with the removal of header proof support. The code is clean and follows Cosmos SDK patterns correctly.
Consider adding these documentation improvements:
func CmdAddOutboundTracker() *cobra.Command { cmd := &cobra.Command{ Use: "add-outbound-tracker [chain] [nonce] [tx-hash]", + Long: `Add an outbound tracker for cross-chain transaction monitoring. + +Note: This command uses the simplified message format that doesn't require proof verification. +Example: + zetacored tx crosschain add-outbound-tracker 1 100 0x1234... --from mykey`, Short: "Add an outbound tracker", Args: cobra.ExactArgs(3),x/crosschain/keeper/msg_server_add_inbound_tracker.go (2)
25-29: LGTM! Consider enhancing error context.The authorization check implementation is clean and well-structured. The var block improves readability by clearly separating the two authorization conditions.
Consider capturing the specific reason for authorization failure to provide more detailed error messages:
var ( - isAuthorizedPolicy = k.GetAuthorityKeeper().CheckAuthorization(ctx, msg) == nil + authorityErr = k.GetAuthorityKeeper().CheckAuthorization(ctx, msg) + isAuthorizedPolicy = authorityErr == nil isObserver = k.GetObserverKeeper().IsNonTombstonedObserver(ctx, msg.Creator) )
Proof-related code still present in the codebase
The verification reveals remaining proof-related code that needs to be addressed:
x/crosschain/types/tx.pb.go: Contains proofs package importx/crosschain/types/legacy_msgs.pb.go: Contains legacy message types with proof fieldsx/crosschain/types/expected_keepers.go: Contains proof verification interfaceThese remnants should be removed to complete the header proof support removal:
- Remove the proofs package import from generated protobuf files
- Clean up legacy message types containing proof fields
- Remove proof verification interface from expected keepers
🔗 Analysis chain
Line range hint
1-43: Verify complete removal of proof-related code.The changes align with the PR objective of removing header proof support. Let's verify that all proof-related code has been completely removed from related files.
The initial search revealed some proof-related code in legacy message types and imports. We need to verify if these are actually removed in the current proto definitions and check for any remaining proof verification logic in the keeper implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any remaining proof-related code in the crosschain module echo "Searching for proof-related code in crosschain module..." rg -i "proof|header" --type go x/crosschain/ echo "Searching for proof-related fields in protobuf definitions..." rg -i "proof|header" --type proto x/crosschain/Length of output: 13200
Script:
#!/bin/bash # Let's check for any remaining proof-related code in proto files and legacy message types # Check proto files specifically fd -e proto . x/crosschain/ -x cat {} # Check for any remaining proof verification logic in keeper rg -i "verifyproof|proof.*verification" x/crosschain/keeper/ # Check for any imports of the proofs package rg -l "\"github.com/zeta-chain/node/pkg/proofs\"" x/crosschain/Length of output: 272
x/crosschain/types/message_add_outbound_tracker.go (1)
13-19: Consider improving parameter naming and adding documentation.The constructor could benefit from the following improvements:
- Rename the
chainparameter tochainIdfor consistency with the struct field- Add function documentation explaining the parameters and their constraints
-func NewMsgAddOutboundTracker(creator string, chain int64, nonce uint64, txHash string) *MsgAddOutboundTracker { +// NewMsgAddOutboundTracker creates a new MsgAddOutboundTracker instance +// Parameters: +// creator: The address of the message creator +// chainId: The target chain identifier +// nonce: The transaction nonce +// txHash: The transaction hash +func NewMsgAddOutboundTracker(creator string, chainId int64, nonce uint64, txHash string) *MsgAddOutboundTracker { return &MsgAddOutboundTracker{ Creator: creator, - ChainId: chain, + ChainId: chainId, Nonce: nonce, TxHash: txHash, }x/crosschain/types/message_add_outbound_tracker_test.go (3)
27-28: Consider adding more chain ID edge casesWhile the current test case validates negative chain IDs, consider adding test cases for other invalid scenarios such as zero or maximum uint64 values to ensure comprehensive validation.
{ name: "invalid chain id - zero", msg: types.NewMsgAddOutboundTracker(sample.AccAddress(), 0, 1, ""), err: sdkerrors.ErrInvalidChainID, }, { name: "invalid chain id - max uint64", msg: types.NewMsgAddOutboundTracker(sample.AccAddress(), math.MaxUint64, 1, ""), err: sdkerrors.ErrInvalidChainID, },
32-32: Enhance test data qualityThe valid test case uses an empty string for txHash. Consider using a realistic hash value to make the test more meaningful.
-msg: types.NewMsgAddOutboundTracker(sample.AccAddress(), 1, 1, ""), +msg: types.NewMsgAddOutboundTracker(sample.AccAddress(), 1, 1, "0x1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef"),
Line range hint
15-44: Add test coverage for nonce validationThe test suite lacks validation for the nonce parameter. Consider adding test cases to validate:
- Zero nonce
- Negative nonce
- Maximum uint64 nonce
{ name: "invalid nonce - zero", msg: types.NewMsgAddOutboundTracker(sample.AccAddress(), 1, 0, "hash"), err: sdkerrors.ErrInvalidSequence, }, { name: "invalid nonce - negative", msg: types.NewMsgAddOutboundTracker(sample.AccAddress(), 1, -1, "hash"), err: sdkerrors.ErrInvalidSequence, }, { name: "invalid nonce - max uint64", msg: types.NewMsgAddOutboundTracker(sample.AccAddress(), 1, math.MaxUint64, "hash"), err: sdkerrors.ErrInvalidSequence, },zetaclient/chains/ton/signer/signer_tracker.go (2)
66-67: Improve documentation clarity.The comment "Note that this method has a check for noop" is vague. Consider expanding it to explain what the noop check entails and why it's important in this context.
- // Note that this method has a check for noop + // PostOutboundTracker includes idempotency checks to prevent duplicate trackers
Line range hint
19-24: Consider implementing exponential backoff for polling.The current implementation uses a fixed 1-second polling interval. Consider implementing exponential backoff to reduce load on the TON node during high traffic periods while potentially detecting successful transactions faster on the first few attempts.
const ( timeout = 60 * time.Second initialTick = 100 * time.Millisecond maxTick = 5 * time.Second )zetaclient/chains/evm/signer/outbound_tracker_reporter.go (1)
Line range hint
39-72: Consider implementing exponential backoff for monitoring retries.The monitoring loop currently uses a fixed 10-second delay between retries. For better resource utilization and network interaction, consider implementing an exponential backoff strategy.
Here's a suggested implementation:
+ backoff := time.Second for { - time.Sleep(10 * time.Second) + time.Sleep(backoff) + if backoff < 30*time.Second { // Cap at 30 seconds + backoff *= 2 + }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 73-73: zetaclient/chains/evm/signer/outbound_tracker_reporter.go#L73
Added line #L73 was not covered by testszetaclient/zetacore/tx.go (1)
Line range hint
86-99: Consider enhancing robustness and documentation.While the implementation is correct, consider these improvements:
- Add a more descriptive comment explaining the function's purpose and return values
- Consider adding context timeout for the database operations
-// PostOutboundTracker adds an outbound tracker +// PostOutboundTracker records a new outbound transaction hash for the given chain and nonce. +// Returns the Zeta transaction hash if successful, or an empty string if the hash already exists. +// Returns an error if the broadcast fails. func (c *Client) PostOutboundTracker(ctx context.Context, chainID int64, nonce uint64, txHash string) (string, error) { + // Add timeout to prevent hanging operations + ctx, cancel := context.WithTimeout(ctx, 30*time.Second) + defer cancel()zetaclient/zetacore/broadcast_test.go (2)
85-88: Consider using named constants for magic numbers.While the message construction is correct, the test would be more maintainable with named constants for the gas price values (10000, 1000) to clarify their purpose and make updates easier.
+const ( + testGasPrice = 10000 + testBaseFee = 1000 + testBlockNum = 1 +) -msg := crosschaintypes.NewMsgVoteGasPrice(address.String(), chains.Ethereum.ChainId, 10000, 1000, 1) +msg := crosschaintypes.NewMsgVoteGasPrice(address.String(), chains.Ethereum.ChainId, testGasPrice, testBaseFee, testBlockNum)
101-104: Consider extracting message creation to a helper function.The message creation logic is duplicated between test cases. Consider extracting it to a helper function to improve maintainability and reduce duplication.
+func createTestVoteGasPriceMsg(address string) (*crosschaintypes.MsgVoteGasPrice, error) { + return crosschaintypes.NewMsgVoteGasPrice( + address, + chains.Ethereum.ChainId, + testGasPrice, + testBaseFee, + testBlockNum, + ), nil +} // In test cases: -msg := crosschaintypes.NewMsgVoteGasPrice(address.String(), chains.Ethereum.ChainId, 10000, 1000, 1) +msg, err := createTestVoteGasPriceMsg(address.String()) +require.NoError(t, err)x/crosschain/keeper/msg_server_add_outbound_tracker.go (1)
57-62: Consider more idiomatic boolean variable names.The boolean variable names could be more idiomatic in Go. Consider using
hasAuthPolicyandisActiveObserverfor better clarity.-isAuthorizedPolicy = k.GetAuthorityKeeper().CheckAuthorization(ctx, msg) == nil -isObserver = k.GetObserverKeeper().IsNonTombstonedObserver(ctx, msg.Creator) +hasAuthPolicy = k.GetAuthorityKeeper().CheckAuthorization(ctx, msg) == nil +isActiveObserver = k.GetObserverKeeper().IsNonTombstonedObserver(ctx, msg.Creator)x/crosschain/keeper/msg_server_add_inbound_tracker_test.go (2)
Line range hint
20-47: Consider table-driven tests for better organization.The test cases follow a similar pattern and could benefit from a table-driven approach to reduce duplication and improve maintainability.
Here's a suggested refactor:
func TestMsgServer_AddInboundTracker(t *testing.T) { tests := []struct { name string setup func(k *keeper.Keeper, ctx sdk.Context) (*types.MsgAddInboundTracker, *mock.Mock, *mock.Mock) expectedErr error shouldExist bool }{ { name: "fail normal user submit", setup: func(k *keeper.Keeper, ctx sdk.Context) (*types.MsgAddInboundTracker, *mock.Mock, *mock.Mock) { // Setup code here }, expectedErr: authoritytypes.ErrUnauthorized, shouldExist: false, }, // Add other test cases } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { k, ctx, _, _ := keepertest.CrosschainKeeperWithMocks(t, keepertest.CrosschainMockOptions{ UseAuthorityMock: true, UseObserverMock: true, }) msg, authorityMock, observerMock := tt.setup(k, ctx) msgServer := keeper.NewMsgServerImpl(*k) _, err := msgServer.AddInboundTracker(ctx, msg) if tt.expectedErr != nil { require.ErrorIs(t, err, tt.expectedErr) } else { require.NoError(t, err) } _, found := k.GetInboundTracker(ctx, msg.ChainId, msg.TxHash) require.Equal(t, tt.shouldExist, found) }) } }
Line range hint
93-102: Enhance assertions in successful test cases.The successful test case only verifies existence but not the actual content of the stored tracker.
Add assertions to verify the stored tracker's fields:
_, err := msgServer.AddInboundTracker(ctx, &msg) require.NoError(t, err) -_, found := k.GetInboundTracker(ctx, chainID, txHash) +tracker, found := k.GetInboundTracker(ctx, chainID, txHash) require.True(t, found) +require.Equal(t, msg.ChainId, tracker.ChainId) +require.Equal(t, msg.TxHash, tracker.TxHash) +require.Equal(t, msg.CoinType, tracker.CoinType)zetaclient/chains/ton/observer/observer_test.go (1)
Line range hint
254-259: LGTM! Consider enhancing test readability.The rename from
AddOutboundTrackertoPostOutboundTrackeraligns with the PR objectives to simplify the codebase. The mock setup is correctly implemented and maintains test coverage.Consider enhancing test readability by using named constants for the mock arguments:
ts.zetacore.On( - "PostOutboundTracker", - mock.Anything, - mock.Anything, - mock.Anything, - mock.Anything, + "PostOutboundTracker", + mock.Anything, // context + mock.Anything, // chainID + mock.Anything, // nonce + mock.Anything, // txHash ).Maybe().Run(catcher).Return("", nil)zetaclient/chains/ton/signer/signer_test.go (1)
Line range hint
249-254: Consider tightening mock argument matchers.The change from
AddOutboundTrackertoPostOutboundTrackeris correct and aligns with the PR objectives. However, the mock setup could be more specific about the expected arguments.Consider replacing generic
mock.Anythingmatchers with type-specific ones for better test reliability:ts.zetacore.On( "PostOutboundTracker", - mock.Anything, - mock.Anything, - mock.Anything, - mock.Anything, + mock.AnythingOfType("context.Context"), + mock.AnythingOfType("int64"), + mock.AnythingOfType("uint64"), + mock.AnythingOfType("string"), ).Maybe().Run(catcher).Return("", nil)zetaclient/chains/ton/observer/outbound.go (3)
Line range hint
205-213: Consider enhancing error context.The error returned from
PostOutboundTrackercould benefit from additional context about the operation being performed.- _, err = ob.ZetacoreClient().PostOutboundTracker(ctx, chainID, nonce, hash) - - return err + if _, err = ob.ZetacoreClient().PostOutboundTracker(ctx, chainID, nonce, hash); err != nil { + return errors.Wrap(err, "failed to post outbound tracker") + } + return nil
Line range hint
196-204: Consider adding structured logging.The warning log for non-TSS signer could benefit from structured fields for better observability.
- ob.Logger().Inbound.Warn(). - Fields(txLogFields(tx)). - Str("transaction.ton.signer", evmSigner.String()). - Msg("observeGateway: addOutboundTracker: withdrawal signer is not TSS. Skipping") + ob.Logger().Inbound.Warn(). + Fields(txLogFields(tx)). + Str("transaction.ton.signer", evmSigner.String()). + Str("transaction.ton.expected_signer", ob.TSS().EVMAddress().String()). + Msg("observeGateway: addOutboundTracker: withdrawal signer is not TSS. Skipping")
Line range hint
183-193: Consider adding unit tests for extractWithdrawal.The
extractWithdrawalhelper function performs critical validation but appears to lack test coverage.Would you like me to help generate comprehensive unit tests for this function?
x/crosschain/keeper/msg_server_add_outbound_tracker_test.go (2)
Line range hint
1-290: Enhance test organization and readability.While the test cases are comprehensive, consider these improvements for better maintainability:
- Group related test cases using subtests (already done well)
- Consider extracting common setup code into helper functions to reduce duplication
- Add test cases for concurrent access scenarios if applicable
Consider extracting the common setup code into a helper function:
func setupOutboundTrackerTest(t *testing.T) (*keeper.Keeper, sdk.Context, *mock.Mock, *mock.Mock, string) { k, ctx, _, _ := keepertest.CrosschainKeeperWithMocks(t, keepertest.CrosschainMockOptions{ UseAuthorityMock: true, UseObserverMock: true, }) authorityMock := keepertest.GetCrosschainAuthorityMock(t, k) observerMock := keepertest.GetCrosschainObserverMock(t, k) admin := sample.AccAddress() return k, ctx, authorityMock, observerMock, admin }
246-247: Remove unnecessary empty line.There's an unnecessary empty line between
TxHashandNoncefields.TxHash: newHash, - Nonce: 42,zetaclient/chains/bitcoin/signer/signer.go (1)
Line range hint
472-486: Consider decomposing TryProcessOutbound into smaller, focused functionsThe method handles multiple responsibilities including transaction signing, broadcasting, and outbound tracking. Consider breaking it down into smaller functions for better maintainability and testability:
func (signer *Signer) TryProcessOutbound(...) { defer signer.handleOutboundPanic(outboundID, outboundProcessor) if err := signer.validateOutboundParams(cctx, params); err != nil { logger.Error().Err(err).Msg("invalid outbound parameters") return } tx, err := signer.prepareAndSignTransaction(ctx, params, btcObserver, height) if err != nil { logger.Error().Err(err).Msg("failed to prepare and sign transaction") return } if err := signer.broadcastAndTrack(ctx, tx, params, zetacoreClient, btcObserver); err != nil { logger.Error().Err(err).Msg("failed to broadcast and track transaction") return } }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 472-472: zetaclient/chains/bitcoin/signer/signer.go#L472
Added line #L472 was not covered by testszetaclient/zetacore/tx_test.go (1)
172-174: Enhance error test case with specific assertions.The failure test case could be more descriptive and include specific error assertions:
- t.Run("add tx hash fail", func(t *testing.T) { + t.Run("should handle broadcast error when posting outbound tracker", func(t *testing.T) { tendermintMock.SetError(errors.New("broadcast error")) hash, err := client.PostOutboundTracker(ctx, chainID, nonce, "") - assert.Error(t, err) + assert.ErrorContains(t, err, "broadcast error") assert.Empty(t, hash) })zetaclient/testutils/mocks/zetacore_client.go (1)
Line range hint
655-682: Consider improving parameter naming for better readability.The parameter
_a1is not descriptive. Consider renaming it toblameDataorblameto better reflect its purpose.-func (_m *ZetacoreClient) PostVoteBlameData(ctx context.Context, _a1 *blame.Blame, chainID int64, index string) (string, error) { - ret := _m.Called(ctx, _a1, chainID, index) +func (_m *ZetacoreClient) PostVoteBlameData(ctx context.Context, blameData *blame.Blame, chainID int64, index string) (string, error) { + ret := _m.Called(ctx, blameData, chainID, index)changelog.md (1)
Line range hint
1-24: Remove duplicate empty Tests section.There is an unnecessary empty "Tests" section between the "Refactor" and "Fixes" sections under "Unreleased". This breaks the logical organization of the changelog.
Remove the duplicate empty section to maintain a clean structure:
### Refactor * [3118](https://github.com/zeta-chain/node/pull/3118) - zetaclient: remove hsm signer * [3122](https://github.com/zeta-chain/node/pull/3122) - improve & refactor zetaclientd cli * [3125](https://github.com/zeta-chain/node/pull/3125) - drop support for header proofs -### Tests ### Fixes * [3041](https://github.com/zeta-chain/node/pull/3041) - replace libp2p public DHT with private gossip peer discovery and connection gater for inbound connections🧰 Tools
🪛 Markdownlint
17-17: null
Multiple headings with the same content(MD024, no-duplicate-heading)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (4)
typescript/zetachain/zetacore/crosschain/outbound_tracker_pb.d.tsis excluded by!**/*_pb.d.tstypescript/zetachain/zetacore/crosschain/tx_pb.d.tsis excluded by!**/*_pb.d.tsx/crosschain/types/outbound_tracker.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/crosschain/types/tx.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
📒 Files selected for processing (25)
changelog.md(1 hunks)docs/openapi/openapi.swagger.yaml(0 hunks)docs/spec/crosschain/messages.md(0 hunks)proto/zetachain/zetacore/crosschain/outbound_tracker.proto(1 hunks)proto/zetachain/zetacore/crosschain/tx.proto(2 hunks)x/crosschain/client/cli/tx_add_outbound_tracker.go(1 hunks)x/crosschain/keeper/msg_server_add_inbound_tracker.go(1 hunks)x/crosschain/keeper/msg_server_add_inbound_tracker_test.go(5 hunks)x/crosschain/keeper/msg_server_add_outbound_tracker.go(2 hunks)x/crosschain/keeper/msg_server_add_outbound_tracker_test.go(8 hunks)x/crosschain/types/message_add_outbound_tracker.go(1 hunks)x/crosschain/types/message_add_outbound_tracker_test.go(1 hunks)zetaclient/chains/bitcoin/signer/signer.go(1 hunks)zetaclient/chains/evm/signer/outbound_tracker_reporter.go(1 hunks)zetaclient/chains/interfaces/interfaces.go(1 hunks)zetaclient/chains/solana/signer/outbound_tracker_reporter.go(1 hunks)zetaclient/chains/ton/observer/observer_test.go(1 hunks)zetaclient/chains/ton/observer/outbound.go(1 hunks)zetaclient/chains/ton/signer/signer_test.go(1 hunks)zetaclient/chains/ton/signer/signer_tracker.go(1 hunks)zetaclient/testutils/mocks/zetacore_client.go(1 hunks)zetaclient/zetacore/broadcast_test.go(2 hunks)zetaclient/zetacore/client_vote.go(0 hunks)zetaclient/zetacore/tx.go(2 hunks)zetaclient/zetacore/tx_test.go(1 hunks)
💤 Files with no reviewable changes (3)
- docs/openapi/openapi.swagger.yaml
- docs/spec/crosschain/messages.md
- zetaclient/zetacore/client_vote.go
🧰 Additional context used
📓 Path-based instructions (21)
proto/zetachain/zetacore/crosschain/outbound_tracker.proto (1)
Pattern **/*.proto: Review the Protobuf definitions, point out issues relative to compatibility, and expressiveness.
proto/zetachain/zetacore/crosschain/tx.proto (1)
Pattern **/*.proto: Review the Protobuf definitions, point out issues relative to compatibility, and expressiveness.
x/crosschain/client/cli/tx_add_outbound_tracker.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_add_inbound_tracker.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_add_inbound_tracker_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_add_outbound_tracker.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_add_outbound_tracker_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/types/message_add_outbound_tracker.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/types/message_add_outbound_tracker_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/signer/signer.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/signer/outbound_tracker_reporter.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/interfaces/interfaces.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/solana/signer/outbound_tracker_reporter.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/ton/observer/observer_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/ton/observer/outbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/ton/signer/signer_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/ton/signer/signer_tracker.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/testutils/mocks/zetacore_client.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/zetacore/broadcast_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/zetacore/tx.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/zetacore/tx_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_add_outbound_tracker.go
[warning] 65-65: x/crosschain/keeper/msg_server_add_outbound_tracker.go#L65
Added line #L65 was not covered by tests
zetaclient/chains/bitcoin/signer/signer.go
[warning] 472-472: zetaclient/chains/bitcoin/signer/signer.go#L472
Added line #L472 was not covered by tests
zetaclient/chains/evm/signer/outbound_tracker_reporter.go
[warning] 73-73: zetaclient/chains/evm/signer/outbound_tracker_reporter.go#L73
Added line #L73 was not covered by tests
zetaclient/chains/solana/signer/outbound_tracker_reporter.go
[warning] 84-84: zetaclient/chains/solana/signer/outbound_tracker_reporter.go#L84
Added line #L84 was not covered by tests
🔇 Additional comments (22)
x/crosschain/keeper/msg_server_add_inbound_tracker.go (1)
32-32: LGTM! Error handling follows best practices.
The error handling implementation properly wraps the unauthorized error with relevant context using errorsmod.Wrapf.
x/crosschain/types/message_add_outbound_tracker.go (1)
13-19: Verify gRPC field reservation for backwards compatibility.
The PR objectives mention reserving old gRPC fields for backwards compatibility. Let's verify this in the protocol buffer definitions.
✅ Verification successful
gRPC field reservation is properly implemented
The proto definition in proto/zetachain/zetacore/crosschain/tx.proto correctly reserves fields 5, 6, and 7 that were previously used for block header proofs properties, maintaining backwards compatibility as required.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if removed fields are properly reserved in proto files
# Expected: Find proto definitions with reserved fields for proof, blockHash, and txIndex
# Search for MsgAddOutboundTracker in proto files
rg -A 10 "message MsgAddOutboundTracker" --type proto
Length of output: 85
Script:
#!/bin/bash
# Let's try finding proto files first using fd
fd -e proto
# Then let's try searching for the message definition with case insensitive search
fd -e proto -x rg -i "message MsgAddOutboundTracker" -A 10 {}
Length of output: 3580
x/crosschain/types/message_add_outbound_tracker_test.go (1)
22-23: LGTM: Test case properly validates invalid address scenario
The simplified constructor call maintains the essential test coverage while aligning with the removal of header proof support.
zetaclient/chains/ton/signer/signer_tracker.go (1)
67-67: LGTM: API simplification aligns with PR objectives.
The change from AddOutboundTracker to PostOutboundTracker with simplified parameters aligns well with the PR objective of removing header proofs support.
zetaclient/chains/evm/signer/outbound_tracker_reporter.go (2)
Line range hint 73-82: LGTM: Method change aligns with header proof removal.
The replacement of AddOutboundTracker with PostOutboundTracker successfully implements the PR objective of removing header proof support while maintaining proper error handling and response processing.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 73-73: zetaclient/chains/evm/signer/outbound_tracker_reporter.go#L73
Added line #L73 was not covered by tests
73-73: Add test coverage for the new PostOutboundTracker implementation.
The static analysis indicates that the modified outbound tracking logic lacks test coverage. Given this is a critical path for transaction reporting, we should ensure it's properly tested.
Would you like me to help create a test suite for this functionality? The test should cover:
- Successful outbound tracking
- Error handling scenarios
- Duplicate reporting prevention
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 73-73: zetaclient/chains/evm/signer/outbound_tracker_reporter.go#L73
Added line #L73 was not covered by tests
zetaclient/zetacore/tx.go (2)
86-87: LGTM! Clean refactoring aligned with PR objectives.
The function rename and parameter simplification effectively support the removal of header proofs while maintaining clear intent and essential functionality.
Line range hint 86-99: Verify complete removal of header proof references.
Let's ensure all callers have been updated and no header proof references remain.
✅ Verification successful
No header proof references found in the modified code.
The search results confirm that:
- The function name
PostOutboundTrackeris correctly used and there are no stale references to old names. - The header proof references found in the codebase are unrelated to this code change as they belong to different modules (
lightclient,observer) and are used for different purposes (block header verification).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify complete removal of header proofs and function rename
# Test 1: Check for any remaining references to AddOutboundTracker
echo "Checking for old function name references..."
rg "AddOutboundTracker" --type go
# Test 2: Check for any remaining header proof references in related files
echo "Checking for remaining header proof references..."
rg -i "header.*proof" --type go
Length of output: 21929
zetaclient/chains/solana/signer/outbound_tracker_reporter.go (2)
Line range hint 19-94: LGTM: Well-structured implementation with robust error handling.
The function demonstrates good practices including proper resource cleanup, comprehensive logging, and appropriate transaction confirmation handling with timeout mechanisms.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 84-84: zetaclient/chains/solana/signer/outbound_tracker_reporter.go#L84
Added line #L84 was not covered by tests
84-84: Verify consistent API migration across the codebase.
The change from AddOutboundTracker to PostOutboundTracker aligns with the PR objectives. Let's verify the consistency of this API change across the codebase.
✅ Verification successful
Let me gather more information about the PostOutboundTracker implementation to verify the API migration.
API migration is consistent and properly implemented
The verification shows a clean migration from AddOutboundTracker to PostOutboundTracker:
- The old
AddOutboundTrackerreferences are only present in legacy code, tests, and protobuf definitions - The new
PostOutboundTrackeris consistently implemented across all chain-specific modules:zetaclient/chains/solana/signer/outbound_tracker_reporter.gozetaclient/chains/evm/signer/outbound_tracker_reporter.gozetaclient/chains/bitcoin/signer/signer.gozetaclient/chains/ton/signer/signer_tracker.go
- The interface is properly defined in
zetaclient/chains/interfaces/interfaces.go - The core implementation is in
zetaclient/zetacore/tx.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the migration from AddOutboundTracker to PostOutboundTracker
# Check for any remaining instances of AddOutboundTracker
echo "Checking for remaining AddOutboundTracker references:"
rg "AddOutboundTracker"
# Verify PostOutboundTracker implementation consistency
echo -e "\nVerifying PostOutboundTracker implementations:"
ast-grep --pattern 'PostOutboundTracker($$$)'
Length of output: 19581
Script:
#!/bin/bash
# Search for PostOutboundTracker implementations and references
echo "Searching for PostOutboundTracker references:"
rg "PostOutboundTracker" -A 2
# Search for interface definitions and method signatures
echo -e "\nSearching for interface definitions:"
ast-grep --pattern 'interface $_ {
$$$
PostOutboundTracker($$$)
$$$
}'
Length of output: 3887
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 84-84: zetaclient/chains/solana/signer/outbound_tracker_reporter.go#L84
Added line #L84 was not covered by tests
x/crosschain/keeper/msg_server_add_outbound_tracker.go (3)
71-71: LGTM: Proof field removal aligns with PR objectives.
The simplified TxHash structure without proof-related fields aligns well with the PR objective to drop support for header proofs.
65-65: Enhance error message and add test coverage.
The error message could be more descriptive, and this line needs test coverage.
Consider enhancing the error message:
-return nil, cosmoserrors.Wrapf(authoritytypes.ErrUnauthorized, "Creator %s", msg.Creator)
+return nil, cosmoserrors.Wrapf(authoritytypes.ErrUnauthorized, "Creator %s is neither an authorized policy holder nor an active observer", msg.Creator)🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 65-65: x/crosschain/keeper/msg_server_add_outbound_tracker.go#L65
Added line #L65 was not covered by tests
83-85: Verify case-insensitive hash comparison requirement.
The code uses case-insensitive comparison for transaction hashes. Please confirm if this is intentional and required for all supported chains.
✅ Verification successful
Let me gather more context about the hash comparisons and chain-specific implementations.
Let me check the hash validation implementation and chain-specific requirements.
Based on the analysis of the codebase, particularly the ValidateHashForChain function in x/crosschain/types/validate.go, I can now provide a definitive response about the case-insensitive hash comparison.
Case-insensitive hash comparison is appropriate for this implementation
The case-insensitive comparison is correct because:
- Ethereum hashes are validated using
hexutil.Decodewhich accepts both upper and lower case hex strings - Bitcoin hashes are validated using a case-insensitive regex pattern
^[a-fA-F0-9]{64}$ - The validation logic explicitly supports both cases for all supported chains
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if case-sensitive comparison is used elsewhere
rg -A 2 "EqualFold.*[Hh]ash"
Length of output: 828
Script:
#!/bin/bash
# Search for hash comparison patterns and hash handling in the codebase
ast-grep --pattern 'func $_ ($$$) {
$$$
strings.EqualFold($hash, $_)
$$$
}'
# Look for hash validation or formatting patterns
rg -A 3 "func.*(ValidateHash|FormatHash|NormalizeHash)"
# Check if there are any chain-specific hash handling
rg -A 2 "Hash.*[Ff]ormat.*chain"
Length of output: 764
Script:
#!/bin/bash
# Check the hash validation implementation
cat x/crosschain/types/validate.go
# Look for chain-specific hash formats or requirements
rg -A 5 "type.*Chain.*struct"
# Check for any hash format constants or requirements
rg "Hash.*Format|HashFormat|HASH_FORMAT"
Length of output: 69880
proto/zetachain/zetacore/crosschain/tx.proto (3)
68-70: Well-structured removal of block header proof fields.
The use of reserved statements for field numbers 5, 6, 7 correctly maintains wire format compatibility while removing the block header proof functionality. The comment effectively documents the historical context of these fields.
95-97: Consistent handling of outbound tracker fields.
The changes mirror those made to MsgAddInboundTracker, maintaining a consistent approach to removing block header proof support while preserving backwards compatibility.
Line range hint 1-7: Consider removing unused proofs import.
Since block header proof support has been removed, the import of zetachain/zetacore/pkg/proofs/proofs.proto might no longer be necessary. Consider removing this import if it's not used elsewhere in the file.
Let's verify if the proofs import is still needed:
✅ Verification successful
The previous result only confirmed the presence of the string "proofs" but we need to see the actual usage context to determine if it's still being used in message definitions or just an unused import.
Let's check if there are any actual usages of types from proofs.proto in message definitions or field types. The regex pattern looks for either:
- Message definitions containing proofs.* types
- Direct usage of proofs.* types in field definitions
Remove unused proofs.proto import
The import of zetachain/zetacore/pkg/proofs/proofs.proto is indeed unused in this file. No message definitions or field types from the proofs package are being utilized, making this import redundant.
- Remove line:
import "zetachain/zetacore/pkg/proofs/proofs.proto";
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if proofs.proto types are still used in this file
rg -l "proofs\." proto/zetachain/zetacore/crosschain/tx.proto
Length of output: 108
Script:
#!/bin/bash
# Check for actual usage of proofs types in the file content
rg "proofs\." proto/zetachain/zetacore/crosschain/tx.proto -A 2 -B 2
Length of output: 351
Script:
#!/bin/bash
# Search for any proofs related message types or fields in the file
rg -U "(?s)message [^}]+proofs\.[^}]+}|proofs\.[A-Za-z]+" proto/zetachain/zetacore/crosschain/tx.proto
Length of output: 157
zetaclient/chains/ton/observer/outbound.go (1)
211-211: Verify the API change across the codebase.
The change from AddOutboundTracker to PostOutboundTracker aligns with the PR objectives. Let's verify this change is consistent across the codebase.
✅ Verification successful
API change from AddOutboundTracker to PostOutboundTracker is consistent
The change aligns with the codebase's evolution. The old AddOutboundTracker references are in protobuf definitions and core types, while all client implementations correctly use PostOutboundTracker. The change is part of a broader API modernization effort mentioned in the release notes (v19 migration).
Key findings:
PostOutboundTrackeris properly defined in theZetacoreClientinterface- All chain-specific implementations (EVM, Solana, TON, Bitcoin) consistently use the new method
- The change is documented in release notes as part of v19 breaking changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the API change from AddOutboundTracker to PostOutboundTracker
# Test 1: Check for any remaining references to AddOutboundTracker
echo "Checking for remaining AddOutboundTracker references..."
rg "AddOutboundTracker"
# Test 2: Verify PostOutboundTracker implementation exists in ZetacoreClient interface
ast-grep --pattern 'interface ZetacoreClient {
$$$
PostOutboundTracker($$$) $$$
$$$
}'
Length of output: 19523
Script:
#!/bin/bash
# Let's check for PostOutboundTracker implementation and usage
# Check for PostOutboundTracker references
echo "Checking for PostOutboundTracker references..."
rg "PostOutboundTracker"
# Check for ZetacoreClient interface definition
echo -e "\nChecking for ZetacoreClient interface..."
rg -A 10 "interface ZetacoreClient"
Length of output: 2024
zetaclient/chains/interfaces/interfaces.go (2)
135-135: LGTM! Method signature simplification.
The simplified PostOutboundTracker method signature aligns with the PR objective to remove header proofs. The rename from Add to Post better reflects HTTP semantics.
Let's verify all callers have been updated to the new signature:
✅ Verification successful
Let me search for any potential callers that might still be using the old method name to ensure complete migration.
Interface change successfully migrated
The verification shows that all occurrences of AddOutboundTracker are either in protobuf definitions, test files, or backend message handlers. The client implementation in zetaclient/zetacore/tx.go correctly uses the new PostOutboundTracker signature, and there are no remaining calls to the old method name in the client code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to ensure they're using the new signature
# Expect: No calls with old signature parameters (proof, blockHash, txIndex)
# Search for any remaining calls with old signature
rg "AddOutboundTracker.*proof.*blockHash.*txIndex"
# Search for calls to new method to verify correct usage
rg "PostOutboundTracker\((ctx|context).*context\.Context,.*int64,.*uint64,.*string\)"
Length of output: 606
Script:
#!/bin/bash
# Search for any remaining calls with old method name
rg "AddOutboundTracker"
# Also check for any potential interface implementations
ast-grep --pattern 'type $_ struct {
$$$
}
func ($_ $_) AddOutboundTracker($$$) {
$$$
}'
Length of output: 19403
Line range hint 67-84: LGTM! Verify interface implementations.
The removal of PostVoteBlockHeader from the ZetacoreVoter interface aligns with the PR objective to drop header proofs support.
Let's verify all implementations of this interface have been updated:
✅ Verification successful
Interface changes are consistent with implementation
The removal of PostVoteBlockHeader from the ZetacoreVoter interface has been properly implemented. The actual implementation in zetaclient/zetacore/client_vote.go and its mock in zetaclient/testutils/mocks/zetacore_client.go are both aligned with the interface changes, containing only the required methods (PostVoteGasPrice, PostVoteInbound, PostVoteOutbound, PostVoteBlameData).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all implementations of ZetacoreVoter interface to ensure they're updated
# Expect: No implementations still containing PostVoteBlockHeader method
# Search for struct types implementing ZetacoreVoter
ast-grep --pattern 'type $_ struct {
$$$
}
func ($_ $_) PostVoteGasPrice($$$) ($$$) {
$$$
}
func ($_ $_) PostVoteInbound($$$) ($$$) {
$$$
}
func ($_ $_) PostVoteOutbound($$$) ($$$) {
$$$
}
func ($_ $_) PostVoteBlameData($$$) ($$$) {
$$$
}'
# Search for any remaining PostVoteBlockHeader implementations
rg "func.*PostVoteBlockHeader.*\("
Length of output: 292
Script:
#!/bin/bash
# Let's try a different approach to find implementations
# First, find all Go files that might contain ZetacoreVoter implementations
fd -e go | xargs rg -l "ZetacoreVoter"
# Then search for any remaining PostVoteBlockHeader implementations
# with more context to understand the containing type/struct
rg -A 5 -B 5 "PostVoteBlockHeader"
# Also search for struct types that might implement the interface
rg "type \w+Client struct" -A 5
Length of output: 9982
Script:
#!/bin/bash
# Let's search for the actual implementation of ZetacoreVoter
# First, find files that might contain the implementation
fd -e go | xargs rg -l "PostVoteGasPrice|PostVoteInbound|PostVoteOutbound|PostVoteBlameData"
# Then get the content of these files to analyze the implementation
fd -e go | xargs rg -B 10 -A 10 "func.*\(.*\).*PostVoteGasPrice"
Length of output: 4733
x/crosschain/keeper/msg_server_add_outbound_tracker_test.go (1)
42-45: Verify test coverage after removing proof-related fields.
The message structure has been correctly simplified by removing proof-related fields (Proof, BlockHash, TxIndex). However, we should ensure that:
- All edge cases previously covered by proof-related tests are still handled appropriately
- The test suite maintains adequate coverage of the simplified message handling logic
Also applies to: 74-77, 117-120, 142-145, 178-181, 203-206, 243-247, 284-287
✅ Verification successful
Let me gather more information about the test coverage by examining the test file more closely.
Let me try a different approach to examine the test coverage.
Test coverage is comprehensive after proof-related field removal
After analyzing the test file, I can confirm that the test coverage remains robust and complete after removing the proof-related fields. The test suite includes:
-
Authorization tests:
- Admin can add tracker
- Observer can add tracker
-
Core functionality tests:
- Adding hash to existing tracker
- Early return for non-pending CCTX
- Handling unsupported chains
- Missing CCTX validation
- Maximum tracker hashes limit
- Duplicate hash prevention
The removal of proof-related fields (Proof, BlockHash, TxIndex) has not impacted the test coverage as these were authentication-related fields that are now handled through the observer/admin authorization flow, which is thoroughly tested.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify test coverage for AddOutboundTracker message handler
# Look for any remaining proof-related code that might need cleanup
# Check for any remaining proof-related code
rg -i "proof|blockHash|txIndex" --type go
# Look for test coverage patterns
ast-grep --pattern 'func Test.*AddOutboundTracker.*\(.*\) {
$$$
}'
Length of output: 157493
Script:
#!/bin/bash
# Check test coverage for AddOutboundTracker message handler
ast-grep --pattern 'func Test.*AddOutboundTracker.*\(.*\) {
$$$
}'
# Look for test cases and assertions
rg -A 10 "func Test.*AddOutboundTracker.*" x/crosschain/keeper/msg_server_add_outbound_tracker_test.go
# Check for any remaining proof-related test cases
rg -i "proof|blockHash|txIndex" x/crosschain/keeper/msg_server_add_outbound_tracker_test.go
Length of output: 269
Script:
#!/bin/bash
# Check the entire test file content
cat x/crosschain/keeper/msg_server_add_outbound_tracker_test.go
# Look for test function declarations with context
rg -B 2 -A 20 "^func Test.*AddOutboundTracker" x/crosschain/keeper/msg_server_add_outbound_tracker_test.go
Length of output: 10069
zetaclient/chains/bitcoin/signer/signer.go (1)
472-476: LGTM! Method rename improves clarity
The renaming from AddOutboundTracker to PostOutboundTracker and parameter simplification aligns well with the PR's objective of simplifying the codebase. The new name better reflects the HTTP-like nature of the operation.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 472-472: zetaclient/chains/bitcoin/signer/signer.go#L472
Added line #L472 was not covered by tests
zetaclient/testutils/mocks/zetacore_client.go (1)
627-653: LGTM! Clean implementation of PostOutboundTracker.
The implementation follows the standard mock pattern and properly handles all edge cases for return values.
|
No need to run V2_MIGRATION_TESTS this was to test gateway migration but will no longer work on current PR since gateway is already enabled in the localnet setup. |
If the module is well isolated, and doesn't touch any external part this is fine, more overhead removing it. We will still need to have a conversation if the entire idea should be dropped. |
zetaclient.Note that we can't simply remove the lightclient module because it will break genesis export/import. To fully drop all of this code, we need to come up with a cleanup strategy.
Closes #3110
Summary by CodeRabbit
Release Notes
New Features
Refactor
Bug Fixes
Tests
Documentation