fix: use the ZEVM address pulled from memo as Receiver in MsgVoteInbound #3242
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📝 WalkthroughWalkthroughThis pull request introduces several significant updates across multiple components of the codebase. A new telemetry endpoint Changes
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
|
ws4charlie
requested review from
brewmaster012,
kingpinXD,
lumtis,
skosito and
swift1337
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
e2e/e2etests/test_solana_deposit_call.go (1)
32-32: LGTM with a minor enhancement suggestionThe receiver address verification is correctly implemented. Consider enhancing the assertion with a custom error message for better debugging:
-require.Equal(r, cctx.GetCurrentOutboundParam().Receiver, contractAddr.Hex()) +require.Equal(r, cctx.GetCurrentOutboundParam().Receiver, contractAddr.Hex(), "receiver address mismatch in outbound params")e2e/e2etests/test_solana_deposit.go (1)
32-32: Maintain consistency with error messagesThe receiver address verification is correct. For consistency with other tests, enhance the assertion with a descriptive error message:
-require.Equal(r, cctx.GetCurrentOutboundParam().Receiver, r.EVMAddress().Hex()) +require.Equal(r, cctx.GetCurrentOutboundParam().Receiver, r.EVMAddress().Hex(), "receiver address mismatch in outbound params")e2e/e2etests/test_spl_deposit.go (2)
45-45: Maintain consistency with error messagesThe receiver address verification is correct. For consistency with other tests, enhance the assertion with a descriptive error message:
-require.Equal(r, cctx.GetCurrentOutboundParam().Receiver, r.EVMAddress().Hex()) +require.Equal(r, cctx.GetCurrentOutboundParam().Receiver, r.EVMAddress().Hex(), "receiver address mismatch in outbound params")
Line range hint
1-67: Consider refactoring common test patternsThe receiver address verification pattern is now repeated across multiple test files. Consider extracting this common verification logic into a helper function in the
utilspackage:// utils/cctx.go func RequireCCTXReceiver(t require.TestingT, cctx *crosschaintypes.CrossChainTx, expectedReceiver string, msgAndArgs ...interface{}) { require.Equal(t, cctx.GetCurrentOutboundParam().Receiver, expectedReceiver, msgAndArgs...) }This would improve maintainability and ensure consistent verification across all tests.
zetaclient/chains/solana/observer/inbound.go (1)
Line range hint
276-282: Consider adding error handling for memo decodingWhile the current implementation works, it might be beneficial to add specific error handling for different memo decoding scenarios.
Consider adding structured error handling:
err := event.DecodeMemo() if err != nil { + if errors.Is(err, types.ErrInvalidMemoFormat) { + ob.Logger().Inbound.Warn().Fields(lf).Msg("invalid memo format") + } else if errors.Is(err, types.ErrMemoTooShort) { + ob.Logger().Inbound.Warn().Fields(lf).Msg("memo too short") + } ob.Logger().Inbound.Info().Fields(lf).Msgf("invalid memo bytes: %s", hex.EncodeToString(event.Memo)) return nil }changelog.md (1)
22-22: Consider enhancing the changelog entry with more context.The current entry could be more informative by explaining:
- The impact of the incorrect receiver assignment
- The benefits of using the decoded ZEVM address from memo
- The affected components (indexers/explorers)
Suggested enhancement:
-* [3242](https://github.com/zeta-chain/node/pull/3242) - set the `Receiver` of `MsgVoteInbound` to the address pulled from solana memo +* [3242](https://github.com/zeta-chain/node/pull/3242) - set the `Receiver` of `MsgVoteInbound` to the ZEVM address decoded from Solana memo instead of the original Solana sender address. While this fix does not affect CCTX processing in the zetacore, it improves clarity for indexers and explorers by using the correct decoded receiver address.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
changelog.md(1 hunks)e2e/e2etests/test_solana_deposit.go(1 hunks)e2e/e2etests/test_solana_deposit_call.go(1 hunks)e2e/e2etests/test_spl_deposit.go(1 hunks)e2e/e2etests/test_spl_deposit_and_call.go(1 hunks)zetaclient/chains/solana/observer/inbound.go(3 hunks)zetaclient/chains/solana/observer/inbound_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
e2e/e2etests/test_solana_deposit_call.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_spl_deposit.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_spl_deposit_and_call.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_solana_deposit.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/solana/observer/inbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/solana/observer/inbound_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (4)
e2e/e2etests/test_spl_deposit_and_call.go (1)
52-52: LGTM: Good addition of receiver address verification
The assertion correctly verifies that the cross-chain transaction's receiver matches the deployed contract address, enhancing the test coverage.
zetaclient/chains/solana/observer/inbound_test.go (1)
126-127: LGTM: Enhanced test coverage for receiver address handling
The test modifications properly verify that the receiver address from the event is correctly propagated to the vote message.
Also applies to: 131-132
zetaclient/chains/solana/observer/inbound.go (2)
215-215: LGTM: Proper initialization of receiver field
The receiver field is correctly initialized as empty, with a clear comment indicating it will be populated from the memo later. This change aligns with the PR objective and improves code clarity.
Also applies to: 243-243
291-291: LGTM: Fixed receiver address assignment in vote message
The vote message now correctly uses the decoded receiver address from the event instead of the sender address, fixing the core issue described in the PR objectives.
…-solana-MsgVoteInbound-Receiver-address
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3242 +/- ##
========================================
Coverage 61.78% 61.78%
========================================
Files 431 431
Lines 30786 30786
========================================
Hits 19021 19021
Misses 10909 10909
Partials 856 856
|
…und (#3242) * use the ZEVM address decoded from memo as Receiver in MsgVoteInbound * add Receiver check in unit test * add changelog entry * fix unit test
Description
We've been setting
MsgVoteInbound.Receiverto original Solana sender address. This is not correct even though it doesn't bother cctx processing in the zetacore (where the decoded receiver is taken). This PR will be setting the decodedReceiverdirectly toMsgVoteInbound.Receiverto make it clearer to indexer/explorer.How Has This Been Tested?
Summary by CodeRabbit
Release Notes
New Features
/systemtimefor enhanced monitoring.Bug Fixes
MsgVoteInboundby setting the receiver from the Solana memo.Tests