refactor: use protocol contracts V2 with Solana deposits#3432
refactor: use protocol contracts V2 with Solana deposits#3432
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 significant enhancements to the Solana deposit handling mechanism by implementing protocol contracts V2. The changes span multiple files, focusing on updating the Changes
Assessment against linked issues
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 #3432 +/- ##
===========================================
- Coverage 64.42% 64.42% -0.01%
===========================================
Files 436 436
Lines 30376 30368 -8
===========================================
- Hits 19571 19565 -6
+ Misses 9963 9962 -1
+ Partials 842 841 -1
|
![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 comments (1)
changelog.md (1)
Line range hint
1-24: Remove duplicate entries in the Unreleased section.The entry "3379 - add Avalanche, Arbitrum and World Chain in chain info" appears both in the Unreleased section and v26.0.0 section.
Keep only one instance of this entry in the most relevant section (v26.0.0 in this case) and remove it from the Unreleased section.
🧹 Nitpick comments (9)
pkg/contracts/solana/inbound.go (5)
16-24: Consider leveraging typed address fields.
Storing the receiver address as a string is functional, but using a typed address (e.g.,ethcommon.Address) can improve type safety and reduce conversion overhead.
70-74: Wrap error returns with contextual information.
Returningerrdirectly is correct but consider wrapping the error or adding context (e.g., "parseReceiver failed") to simplify debugging.
107-111: Avoid repeated parse logic.
parseReceiveris repeatedly called across multiple methods. Extracting a common handling function or unifying logic may simplify maintenance.
171-175: Consolidate repeated parsing code.
As with other deposit parsing, moving shared functionality to a helper function may help avoid duplication.
208-212: Reduce code duplication when parsing receivers.
This block is similar to previous patterns; consider extracting a shared utility method for parseReceiver calls and validations.zetaclient/chains/solana/observer/inbound_test.go (1)
78-89: Add test cases for cross-chain call scenarios.The test case only covers basic deposit scenarios. Consider adding test cases for:
- Cross-chain calls with IsCrossChainCall=true
- Various receiver address formats
changelog.md (3)
22-22: Standardize formatting with other entries.The entry should end with a period to maintain consistency with other entries in the changelog.
-* [3432](https://github.com/zeta-chain/node/pull/3432) - use protocol contracts V2 with Solana deposits +* [3432](https://github.com/zeta-chain/node/pull/3432) - use protocol contracts V2 with Solana deposits.
Line range hint
3-24: Enhance clarity of change descriptions.Some entries would benefit from more detailed descriptions to better convey their impact and purpose. For example:
- "cosmos-sdk v.50.x upgrade" should include major changes or breaking changes
- "implement orchestrator V2" should explain the benefits or improvements
Consider expanding these entries to provide more context about the changes and their impact on the system.
Line range hint
1-1: Add version and date information.The changelog should include version numbers and dates for better tracking.
Add a header with version information:
# CHANGELOG +# Current Version: v26.0.0 +# Last Updated: January 2025
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
testutil/contracts/Dapp.binis excluded by!**/*.bintestutil/contracts/DappReverter.binis excluded by!**/*.bintestutil/contracts/Depositor.binis excluded by!**/*.bintestutil/contracts/Example.binis excluded by!**/*.bintestutil/contracts/Reverter.binis excluded by!**/*.bintestutil/contracts/Withdrawer.binis excluded by!**/*.bin
📒 Files selected for processing (20)
changelog.md(1 hunks)pkg/contracts/solana/inbound.go(7 hunks)pkg/contracts/solana/inbound_test.go(4 hunks)testutil/contracts/Dapp.go(3 hunks)testutil/contracts/Dapp.json(1 hunks)testutil/contracts/DappReverter.go(2 hunks)testutil/contracts/DappReverter.json(1 hunks)testutil/contracts/Depositor.go(2 hunks)testutil/contracts/Depositor.json(1 hunks)testutil/contracts/Example.abi(1 hunks)testutil/contracts/Example.go(2 hunks)testutil/contracts/Example.json(2 hunks)testutil/contracts/Example.sol(1 hunks)testutil/contracts/Reverter.go(3 hunks)testutil/contracts/Reverter.json(1 hunks)testutil/contracts/Withdrawer.go(2 hunks)testutil/contracts/Withdrawer.json(1 hunks)zetaclient/chains/solana/observer/inbound.go(3 hunks)zetaclient/chains/solana/observer/inbound_test.go(3 hunks)zetaclient/types/event.go(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- testutil/contracts/Dapp.json
- testutil/contracts/Withdrawer.json
- testutil/contracts/Reverter.json
- testutil/contracts/Depositor.json
🧰 Additional context used
📓 Path-based instructions (11)
zetaclient/types/event.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.
testutil/contracts/Reverter.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
pkg/contracts/solana/inbound_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
pkg/contracts/solana/inbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
testutil/contracts/Withdrawer.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
testutil/contracts/Example.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
testutil/contracts/Depositor.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.
testutil/contracts/DappReverter.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
testutil/contracts/Dapp.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🪛 GitHub Check: codecov/patch
zetaclient/chains/solana/observer/inbound.go
[warning] 256-267: zetaclient/chains/solana/observer/inbound.go#L256-L267
Added lines #L256 - L267 were not covered by tests
🔇 Additional comments (48)
pkg/contracts/solana/inbound.go (6)
6-6: Import looks correct for Ethereum address conversion.
No issues with leveraging "github.com/ethereum/go-ethereum/common" for address parsing.
76-82: Validate the lack of memo.
The fieldMemohere is set to an empty byte slice. Confirm that deposit instructions truly lack relevant memo data before discarding it.
118-124: Usage of instruction data for memo looks appropriate.
The deposit-and-call scenario properly populatesMemofrominstDepositAndCall.
177-183: Confirm SPL deposit memo usage.
Similar to standard deposit logic, verify that dropping memo data is intentional for SPL deposits.
219-225: Cross-chain call handling is consistent.
PopulatingIsCrossChainCallaligns well with deposit-and-call flows.
281-288: Rejecting zero addresses is appropriate.
Disallowing empty addresses prevents erroneous transactions. The parse function looks correct.testutil/contracts/Example.sol (2)
43-44: Refactored call flow is readable.
Delegating logic toonCallenhances modularity by removing duplicate code inonCrossChainCall.
46-51: ExposingonCallincreases flexibility.
MarkingonCallas public is reasonable for external invocation or testing.zetaclient/types/event.go (1)
70-72: Field addition aligns with cross-chain logic.
IncludingIsCrossChainCallinInboundEventis consistent with deposit struct changes; ensure corresponding unit tests are updated to cover this field.testutil/contracts/Example.abi (1)
66-110: LGTM: ABI definition for new onCall function.The new function signature follows the standard ABI format and maintains consistency with the existing
onCrossChainCallfunction.zetaclient/chains/solana/observer/inbound_test.go (1)
124-131: Enhance message testing coverage.Add test cases to verify different message formats and edge cases.
zetaclient/chains/solana/observer/inbound.go (2)
302-305: LGTM: Protocol version update to V2.The update to protocol version V2 and addition of cross-chain call support is implemented correctly.
229-240: Add test coverage for inbound event creation.The event creation logic needs test coverage, particularly for:
- Cross-chain call scenarios
- Different receiver address formats
- Various memo and message combinations
✅ Verification successful
Test coverage for inbound event creation is sufficient.
The codebase already has comprehensive test coverage for inbound event creation, including:
- Cross-chain call scenarios via
Test_FilterInboundEvents- Different receiver address formats (both Solana and ETH) in
Test_BuildInboundVoteMsgFromEvent- Various memo combinations in
Test_IsEventProcessable🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check test coverage for event creation ast-grep --pattern 'func Test_.*InboundEvent.*\('Length of output: 51
Script:
#!/bin/bash # Find test files in the solana observer directory fd ".*_test.go$" zetaclient/chains/solana/observer/ # Search for test functions and inbound-related tests using ripgrep rg -A 5 "^func Test" zetaclient/chains/solana/observer/ --type go # Look for inbound-related code in test files rg -i "inbound|deposit" zetaclient/chains/solana/observer/ --type go -C 5Length of output: 49959
testutil/contracts/DappReverter.go (1)
29-29: LGTM: Updated contract bindings.The changes to the generated bindings are correct:
- Added abi.ConvertType import
- Updated to use MetaData.GetAbi() for ABI parsing
Also applies to: 160-164
testutil/contracts/Reverter.go (3)
29-29: LGTM: Import addition aligns with go-ethereum updates.The addition of
abi.ConvertTypeimport reference follows the standard pattern for go-ethereum bindings.
167-171: LGTM: Improved ABI retrieval method.The switch to
GetAbi()is a more maintainable approach than direct string parsing, aligning with go-ethereum's recommended patterns.
42-42: Verify the new contract bytecode.The contract bytecode has been updated. While this is a generated file, it's crucial to ensure the new bytecode matches the compiled Solidity contract.
✅ Verification successful
Bytecode verification successful
The contract bytecode in the Go file is verified to be correct. It matches exactly with both the JSON compilation artifacts and the binary file, and correctly implements the Reverter contract's functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the contract bytecode matches the Solidity source # Look for the Solidity source file and its compilation artifacts fd -e sol -e json "Reverter" | grep -v "test"Length of output: 43
Script:
#!/bin/bash # Check the contents and surrounding files echo "=== Checking the known Go file ===" cat testutil/contracts/Reverter.go echo -e "\n=== Looking for Solidity files in surrounding directories ===" find . -type f -name "*.sol" | grep -i "revert" echo -e "\n=== Checking parent directories of the contract ===" ls -R testutil/contracts/Length of output: 13447
Script:
#!/bin/bash # Check the Solidity source and compare bytecodes echo "=== Solidity Source ===" cat testutil/contracts/Reverter.sol echo -e "\n=== Comparing Bytecodes ===" echo "Checking if bytecodes match between .json and .go files..." jq -r '.bytecode' testutil/contracts/Reverter.jsonLength of output: 766
Script:
#!/bin/bash # Examine the JSON file structure echo "=== JSON Structure ===" cat testutil/contracts/Reverter.json echo -e "\n=== Checking .bin file ===" cat testutil/contracts/Reverter.binLength of output: 4051
pkg/contracts/solana/inbound_test.go (4)
59-65: LGTM: Well-structured test data initialization.The test data initialization is comprehensive, including all required fields for the V2 protocol contracts.
164-170: LGTM: Cross-chain call test data properly configured.The test case correctly sets up cross-chain call data with appropriate values and message payload.
268-274: LGTM: SPL token deposit test data properly configured.The test data correctly includes SPL token-specific fields and addresses.
373-379: LGTM: SPL token cross-chain call test data properly configured.The test data correctly combines SPL token handling with cross-chain messaging.
testutil/contracts/Withdrawer.go (3)
29-29: LGTM: Import addition aligns with go-ethereum updates.The addition of
abi.ConvertTypeimport reference follows the standard pattern for go-ethereum bindings.
160-164: LGTM: Improved ABI retrieval method.The switch to
GetAbi()is a more maintainable approach than direct string parsing, aligning with go-ethereum's recommended patterns.
35-35: Verify the new contract bytecode.The contract bytecode has been updated. While this is a generated file, it's crucial to ensure the new bytecode matches the compiled Solidity contract.
✅ Verification successful
Contract bytecode difference is expected and correct
The bytecode in
testutil/contracts/Withdrawer.godiffers from the source because the Solidity contract has been updated to include bothonCrossChainCallandonCallfunctions. The new bytecode reflects these functional improvements and is compiled with Solidity 0.8.26.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the contract bytecode matches the Solidity source # Look for the Solidity source file and its compilation artifacts fd -e sol -e json "Withdrawer" | grep -v "test"Length of output: 126
Script:
#!/bin/bash # Read both files to compare bytecodes and verify contract echo "=== Solidity Source ===" cat e2e/contracts/withdrawer/Withdrawer.sol echo -e "\n=== JSON Artifact ===" cat e2e/contracts/withdrawer/Withdrawer.jsonLength of output: 8242
testutil/contracts/Depositor.go (3)
29-29: LGTM: Import addition aligns with go-ethereum updates.The addition of
abi.ConvertTypeimport reference follows the standard pattern for go-ethereum bindings.
160-164: LGTM: Improved ABI retrieval method.The switch to
GetAbi()is a more maintainable approach than direct string parsing, aligning with go-ethereum's recommended patterns.
35-35: Verify the new contract bytecode.The contract bytecode has been updated. While this is a generated file, it's crucial to ensure the new bytecode matches the compiled Solidity contract.
✅ Verification successful
Bytecode verification successful
The contract bytecode in Depositor.go matches exactly with the compiled bytecode in Depositor.bin, confirming that the Go bindings are properly synchronized with the Solidity contract compilation artifacts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the contract bytecode matches the Solidity source # Look for the Solidity source file and its compilation artifacts fd -e sol -e json "Depositor" | grep -v "test"Length of output: 44
Script:
#!/bin/bash # Try multiple approaches to find the Solidity contract # 1. Look for Solidity files in the contracts directory find . -type f -name "*.sol" 2>/dev/null # 2. Search for contract definition rg "contract Depositor" -t sol # 3. Look for any files containing "Depositor" in contracts directory find . -type f -path "*/contracts/*" -name "*Depositor*" # 4. Check if there's a hardhat or truffle config find . -type f -name "hardhat.config.*" -o -name "truffle-config.*"Length of output: 1501
Script:
#!/bin/bash # Extract bytecode from Go file and compare with .bin file echo "=== Bytecode from Depositor.go ===" rg -A 1 'Bin: "0x' ./testutil/contracts/Depositor.go | tail -n 1 echo -e "\n=== Bytecode from Depositor.bin ===" cat ./testutil/contracts/Depositor.binLength of output: 3980
testutil/contracts/Example.go (7)
41-42: LGTM: ABI and bytecode updates for protocol contracts V2.The ABI and bytecode updates correctly reflect the addition of the
onCallfunction while maintaining existing functionality.
358-377: LGTM: Clean implementation of OnCall methods.The implementation follows established patterns, includes proper documentation, and maintains consistency with other contract methods.
41-42: LGTM! ABI and bytecode updates for protocol contracts V2.The metadata updates correctly reflect the addition of the new
onCallfunction while maintaining compatibility with existing functions.
358-377: LGTM! Well-structured OnCall implementations.The new OnCall functions follow the established binding patterns with proper error handling and type safety.
Line range hint
29-29: LGTM! Required import for type conversions.The addition of
abi.ConvertTypeimport is necessary for proper type handling in the contract bindings.
41-42: LGTM! ABI and bytecode updates for new onCall function.The ABI has been correctly updated to include the new onCall function with proper parameter types and the bytecode reflects the implementation.
358-377: LGTM! Well-structured OnCall method implementations.The implementations follow the established pattern with proper error handling and type safety. The methods are well-documented and consistent with other contract bindings.
testutil/contracts/Dapp.go (5)
29-29: LGTM: Required import for type conversions.The addition of
abi.ConvertTypeimport is necessary for proper type handling in contract bindings.
179-183: LGTM: Improved ABI handling using metadata.The change to use
DappMetaData.GetAbi()improves reliability by ensuring ABI consistency across the codebase.
179-183: LGTM! Improved ABI handling approach.The switch to using
DappMetaData.GetAbi()improves maintainability and follows go-ethereum's recommended practices for contract bindings.
29-29: LGTM! Added required import for type conversions.The addition of
abi.ConvertTypeimport is necessary for proper type handling in contract bindings.
179-183: LGTM! Improved ABI handling in bindDapp.The change to use
DappMetaData.GetAbi()improves reliability and consistency in ABI handling, reducing the risk of parsing errors.testutil/contracts/DappReverter.json (3)
18-18: LGTM: Updated contract bytecode.The bytecode update aligns with the protocol contracts V2 implementation.
18-18: LGTM! Updated contract bytecode.The binary update aligns with the contract changes while maintaining the same interface.
18-18: LGTM! Updated contract binary.The binary has been updated to reflect the latest contract implementation while maintaining the same interface.
testutil/contracts/Example.json (6)
67-111: LGTM: Well-structured onCall ABI definition.The ABI definition for
onCallis complete and consistent with the implementation.
158-158: LGTM: Updated contract bytecode.The bytecode update correctly reflects the addition of the
onCallfunction.
67-111: LGTM! Well-structured onCall ABI definition.The new onCall function ABI is properly structured with correct parameter types and naming conventions.
158-158: LGTM! Updated contract bytecode.The binary update correctly reflects the ABI changes and maintains proper compiler settings.
67-111: LGTM! Well-defined onCall ABI addition.The new onCall function is properly defined with correct parameter types and structure, matching the onCrossChainCall signature.
158-158: LGTM! Updated contract binary with new function implementation.The binary has been properly updated to include the new onCall function implementation while maintaining compatibility.
skosito
left a comment
skosito
left a comment
There was a problem hiding this comment.
will modifications related to withdrawals be in diff PR?
I think there are no change needed for withdrawals |
|
Note: realize Solana is not part of the default tests, rerunning now |
3 participants
Description
Closes #2712
Summary by CodeRabbit
Based on the comprehensive summary of changes, here are the release notes:
New Features
onCallfunction in Example contract for cross-chain interactionsImprovements
Bug Fixes
Refactor