refactor: use protocol contracts V2 with Bitcoin deposits #3426
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 comprehensive updates to support Bitcoin transactions with Protocol Contract Version 2 architecture. The changes span multiple components of the ZetaChain ecosystem, focusing on enhancing cross-chain transaction handling, introducing new status categories, and updating contract interactions. Key modifications include updating inbound event processing, adding new protocol contract version support, and refactoring various contract-related utilities. Changes
Assessment against linked issues
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
|
lumtis
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3426 +/- ##
===========================================
+ Coverage 63.28% 64.42% +1.13%
===========================================
Files 437 436 -1
Lines 30746 30376 -370
===========================================
+ Hits 19459 19571 +112
+ Misses 10473 9963 -510
- Partials 814 842 +28
|
![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
🧹 Nitpick comments (8)
testutil/contracts/Example.sol (1)
46-51: Consider restrictingonCallvisibility.The
onCallfunction is marked aspublic, allowing direct external calls. Consider usinginternalvisibility since this appears to be an implementation detail used only byonCrossChainCall.e2e/e2etests/test_bitcoin_std_deposit_and_call_revert_other_address.go (1)
45-47: LGTM: Improved transaction verification flow.The changes properly verify transaction mining before checking the revert status, with added logging for better debugging. Consider adding a timeout check to ensure the test fails gracefully if mining takes too long.
+const maxMiningWaitTime = 5 * time.Minute + cctx := utils.WaitCctxMinedByInboundHash(r.Ctx, txHash.String(), r.CctxClient, r.Logger, r.CctxTimeout) +require.NotNil(r, cctx, "transaction not mined within %s", maxMiningWaitTime) r.Logger.CCTX(*cctx, "bitcoin_std_memo_deposit")e2e/e2etests/test_bitcoin_deposit_and_withdraw_with_dust.go (1)
49-50: Enhance logging with context.While adding logging is good, consider enriching it with additional context about the test scenario.
-r.Logger.CCTX(*cctx, "deposit") +r.Logger.CCTX(*cctx, "deposit with dust amount test")zetaclient/types/event.go (1)
113-113: Consider adding explicit category validation.The default return of
InboundCategoryProcessablemight benefit from explicit validation to ensure all conditions are met.+ // Validate basic requirements for processable events + if event.Amount == 0 || len(event.Memo) == 0 { + return InboundCategoryUnknown + } return InboundCategoryProcessablezetaclient/chains/bitcoin/observer/event.go (1)
194-204: Consider improving cross-chain call detection logic.The current implementation determines cross-chain calls based on memo bytes length. This might lead to false positives if the memo contains data that's not intended for cross-chain calls.
Consider using a more explicit method to determine cross-chain calls, such as:
-crosschaintypes.WithCrossChainCall(len(event.MemoBytes) > 0), +crosschaintypes.WithCrossChainCall(event.MemoBytes != nil && len(event.MemoBytes) > 0 && !bytes.Equal(event.MemoBytes, []byte(constant.DonationMessage))),zetaclient/chains/bitcoin/observer/event_test.go (2)
Line range hint
360-384: Consider adding test cases for new fields.While the changes correctly implement V2 protocol version support, consider adding test cases that specifically validate the behavior of
RevertOptionsandIsCrossChainCallfields.t.Run("should handle revert options correctly", func(t *testing.T) { event := createTestBtcEvent(t, &chaincfg.MainNetParams, []byte("dummy memo"), nil) amountSats := big.NewInt(1000) vote := ob.NewInboundVoteFromLegacyMemo(&event, amountSats) require.True(t, vote.RevertOptions.IsEmpty()) }) t.Run("should set cross chain call flag correctly", func(t *testing.T) { event := createTestBtcEvent(t, &chaincfg.MainNetParams, []byte("dummy memo"), nil) amountSats := big.NewInt(1000) vote := ob.NewInboundVoteFromLegacyMemo(&event, amountSats) require.True(t, vote.IsCrossChainCall) })
Line range hint
420-441: Consider validating hex address format.The change to use
MemoStd.Receiver.Hex()is correct, but consider adding validation to ensure the hex address format is valid.t.Run("should validate receiver hex format", func(t *testing.T) { receiver := sample.EthAddress() event := createTestBtcEvent(t, &chaincfg.MainNetParams, []byte("dummy"), &memo.InboundMemo{ FieldsV0: memo.FieldsV0{ Receiver: receiver, }, }) vote := ob.NewInboundVoteFromStdMemo(&event, big.NewInt(1000)) require.True(t, strings.HasPrefix(vote.Receiver, "0x")) require.Equal(t, 42, len(vote.Receiver)) })docs/openapi/openapi.swagger.yaml (1)
59047-59052: Enhance the documentation with protocol V2 context.The addition of
INVALID_RECEIVER_ADDRESSstatus improves error handling for Bitcoin deposits. However, consider expanding the description to clarify its relationship with protocol V2 contracts.- INSUFFICIENT_DEPOSITOR_FEE: this field is specifically for Bitcoin when the deposit amount is less than depositor fee - - INVALID_RECEIVER_ADDRESS: the receiver address parsed from the inbound is invalid + - INVALID_RECEIVER_ADDRESS: the receiver address parsed from the inbound is invalid (introduced with protocol + contracts V2 for enhanced Bitcoin deposit validation)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
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!**/*.bintypescript/zetachain/zetacore/crosschain/cross_chain_tx_pb.d.tsis excluded by!**/*_pb.d.tsx/crosschain/types/cross_chain_tx.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
📒 Files selected for processing (27)
changelog.md(1 hunks)docs/openapi/openapi.swagger.yaml(1 hunks)e2e/e2etests/test_bitcoin_deposit.go(1 hunks)e2e/e2etests/test_bitcoin_deposit_and_withdraw_with_dust.go(1 hunks)e2e/e2etests/test_bitcoin_std_deposit_and_call_revert_other_address.go(1 hunks)e2e/utils/contracts.go(2 hunks)proto/zetachain/zetacore/crosschain/cross_chain_tx.proto(1 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)x/crosschain/types/cctx.go(2 hunks)zetaclient/chains/bitcoin/observer/event.go(5 hunks)zetaclient/chains/bitcoin/observer/event_test.go(5 hunks)zetaclient/chains/solana/observer/inbound.go(1 hunks)zetaclient/types/event.go(2 hunks)zetaclient/types/event_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- testutil/contracts/Dapp.json
- testutil/contracts/DappReverter.json
- testutil/contracts/Depositor.json
🧰 Additional context used
📓 Path-based instructions (17)
proto/zetachain/zetacore/crosschain/cross_chain_tx.proto (1)
Pattern **/*.proto: Review the Protobuf definitions, point out issues relative to compatibility, and expressiveness.
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.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_bitcoin_std_deposit_and_call_revert_other_address.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.
testutil/contracts/Depositor.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/types/cctx.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/types/event_test.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/Dapp.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/utils/contracts.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.
e2e/e2etests/test_bitcoin_deposit.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.
e2e/e2etests/test_bitcoin_deposit_and_withdraw_with_dust.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/observer/event_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/observer/event.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
📓 Learnings (1)
zetaclient/chains/bitcoin/observer/event.go (1)
Learnt from: ws4charlie
PR: zeta-chain/node#3025
File: zetaclient/chains/bitcoin/observer/event.go:110-115
Timestamp: 2024-11-12T13:20:12.658Z
Learning: In the `DecodeEventMemoBytes` function, a non-nil `memoStd` with a non-nil `err` indicates that the memo bytes are well-formatted as a memo but contain improper data.
🪛 GitHub Check: codecov/patch
x/crosschain/types/cctx.go
[warning] 139-141: x/crosschain/types/cctx.go#L139-L141
Added lines #L139 - L141 were not covered by tests
🔇 Additional comments (32)
e2e/e2etests/test_bitcoin_deposit.go (1)
22-22: LGTM: Protocol version verification added.The assertion correctly verifies that Bitcoin deposits use protocol contract V2, ensuring compatibility with the new protocol version.
testutil/contracts/Example.sol (1)
43-44: LGTM: Clean refactor of common logic.The modification to
onCrossChainCallproperly delegates to the newonCallfunction, improving code reusability.e2e/utils/contracts.go (1)
27-28: Consider potential overflow risks with uint64 conversion.The direct conversion to uint64 might truncate large values. Consider adding overflow checks or using SafeMath for values that could exceed uint64 range:
-amount.Uint64(), -bar.Uint64(), +require.True(t, amount.IsUint64(), "amount exceeds uint64 range") +require.True(t, bar.IsUint64(), "bar exceeds uint64 range") +amount.Uint64(), +bar.Uint64(),Also applies to: 43-44
e2e/e2etests/test_bitcoin_deposit_and_withdraw_with_dust.go (1)
39-43: Consider adding validation for payload size.The appended payload might affect the Bitcoin transaction size. Consider adding validation to ensure the combined size of withdrawer address and payload doesn't exceed Bitcoin's limitations.
zetaclient/types/event.go (1)
24-25: LGTM! Clear and descriptive naming.The rename from
InboundCategoryGoodtoInboundCategoryProcessableimproves clarity by better describing the category's purpose.testutil/contracts/Example.abi (1)
66-110: LGTM! Well-structured function definition.The new
onCallfunction is well-defined with appropriate parameter types and state mutability. The structure follows Solidity best practices.zetaclient/types/event_test.go (1)
75-81: LGTM! Test cases updated consistently.The test case updates correctly reflect the category rename while maintaining comprehensive coverage.
zetaclient/chains/bitcoin/observer/event.go (3)
78-78: LGTM: Improved semantic clarity in category naming.The change from
InboundCategoryGoodtoInboundCategoryProcessablebetter reflects the actual state of the inbound event.
221-243: LGTM: Robust cross-chain call detection.The implementation correctly identifies cross-chain calls by checking both
OpCodeCallandOpCodeDepositAndCalloperations, providing a more reliable detection mechanism.
119-126: LGTM: Enhanced legacy memo handling.The update improves the handling of legacy memo decoding and ensures consistent state by updating the MemoBytes field with the decoded payload.
Please ensure test coverage for this change. Run the following to check coverage:
✅ Verification successful
Test coverage verified: Comprehensive test suite present
The code changes are thoroughly tested in
event_test.gowith theTest_DecodeEventMemoBytestest suite, which covers:
- Standard and legacy memo decoding
- MemoBytes field updates
- Error handling
- Chain-specific behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check test coverage for the DecodeMemoBytes method go test -coverprofile=coverage.out ./zetaclient/chains/bitcoin/observer/ go tool cover -func=coverage.out | grep -A 1 "DecodeMemoBytes"Length of output: 16555
Script:
#!/bin/bash # Search for test files containing DecodeMemoBytes ast-grep --pattern 'func Test$_($_, $_) { $$$ DecodeMemoBytes $$$ }' # Search for test files in the observer directory fd ".*_test\.go$" | grep -i "observer"Length of output: 4149
Script:
#!/bin/bash # Check content of event_test.go for related test cases cat zetaclient/chains/bitcoin/observer/event_test.go # Search for any test cases that might be using MemoBytes ast-grep --pattern 'func Test$_($_, $_) { $$$ MemoBytes $$$ }'Length of output: 13683
x/crosschain/types/cctx.go (2)
120-124: LGTM: Well-documented V1 compatibility logic.The comments clearly explain why V1 logic is retained during the transition period, with references to relevant issues.
138-146: Add test coverage for Bitcoin revert address handling.The new Bitcoin-specific revert address logic lacks test coverage.
Run the following to verify test coverage:
✅ Verification successful
Bitcoin revert address handling has sufficient test coverage
The codebase includes comprehensive test coverage for Bitcoin revert address handling in both
revert_options_test.goandcctx_test.go, covering address validation, network compatibility, and error cases.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check test coverage for Bitcoin revert address handling go test -coverprofile=coverage.out ./x/crosschain/types/ go tool cover -func=coverage.out | grep -A 1 "AddRevertOutbound"Length of output: 430
Script:
#!/bin/bash # Search for test files and Bitcoin-related test cases rg -l "TestCctx" ./x/crosschain/types/ rg -A 10 -B 10 "IsBitcoinChain|RevertAddress" ./x/crosschain/types/*_test.goLength of output: 25288
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 139-141: x/crosschain/types/cctx.go#L139-L141
Added lines #L139 - L141 were not covered by testszetaclient/chains/bitcoin/observer/event_test.go (1)
58-63: LGTM! Improved naming clarity.The rename from
InboundCategoryGoodtoInboundCategoryProcessablebetter describes the purpose and state of the inbound event.testutil/contracts/Withdrawer.go (1)
29-29: LGTM! Improved contract binding implementation.The changes follow best practices by:
- Using
abi.ConvertTypefor type conversions- Updating contract bytecode
- Using
GetAbi()for consistent ABI handlingAlso applies to: 35-35, 160-164
testutil/contracts/Depositor.go (1)
29-29: LGTM! Consistent contract binding implementation.The changes maintain consistency with other contract implementations by following the same pattern of improvements.
Also applies to: 35-35, 160-164
zetaclient/chains/solana/observer/inbound.go (1)
431-431: LGTM! Consistent category naming.The change to use
InboundCategoryProcessablemaintains consistency with the updated category naming convention.testutil/contracts/Example.go (2)
41-42: LGTM: Contract metadata updates are consistent.The updated ABI and bytecode correctly reflect the contract's interface changes, including the new
onCallmethod.
358-377: LGTM: OnCall method implementations are well-structured.The new method implementations follow the standard go-ethereum binding patterns with proper error handling and type safety.
testutil/contracts/Dapp.go (2)
29-29: LGTM: Import addition aligns with go-ethereum practices.The addition of
abi.ConvertTypeimport supports proper type handling in the contract bindings.
179-183: LGTM: Improved ABI handling in bindDapp.The updated implementation uses the more efficient
GetAbi()method and maintains consistent error handling patterns across contract bindings.testutil/contracts/Reverter.json (1)
Line range hint
1-54: LGTM! Test contract looks well-structured.The contract ABI is properly defined with clear parameter types and the bytecode reflects the compiled implementation.
testutil/contracts/Withdrawer.json (1)
Line range hint
1-32: LGTM! Test contract looks well-structured.The contract ABI is properly defined with clear parameter types and the bytecode reflects the compiled implementation.
proto/zetachain/zetacore/crosschain/cross_chain_tx.proto (1)
37-38: LGTM! New status enum value is well-documented.The addition of
INVALID_RECEIVER_ADDRESSstatus enhances error handling for inbound transactions with invalid receiver addresses. This change is linked to issue #3435 as noted in previous reviews.✅ Verification successful
Verified: New status enum value is correctly implemented as per issue #3435
The addition of
INVALID_RECEIVER_ADDRESSstatus directly implements the requirement from issue #3435 to handle invalid receiver addresses in cross-chain transactions, allowing the protocol to document and revert such transactions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the linked issue exists and is related to this change. gh issue view 3435Length of output: 109
Script:
#!/bin/bash # Verify issue #3435 and search for related references gh issue view 3435 echo "---" # Search for issue references in commit messages git log --grep="#3435" --oneline echo "---" # Search for any code references to the issue number rg -i "3435" --type-add 'proto:*.proto'Length of output: 87320
testutil/contracts/Example.json (2)
67-111: LGTM! New function is well-defined.The new
onCallfunction follows consistent patterns with existing cross-chain call functions, with proper parameter types and structure.
158-158: LGTM! Bytecode update reflects the new function.The bytecode has been properly updated to include the implementation of the new
onCallfunction.changelog.md (1)
Line range hint
1-24: LGTM! Changelog is well-maintained.Changes are properly categorized and documented with clear references to pull requests. The entries align with the code changes observed in the reviewed files.
testutil/contracts/DappReverter.go (3)
35-35: LGTM: Contract bytecode updated.The contract bytecode has been updated to reflect the latest implementation.
160-164: LGTM: Improved ABI parsing approach.The migration from direct ABI parsing to using
GetAbi()enhances maintainability and consistency.
29-29: Verify the necessity of the unused import.The
abi.ConvertTypeimport is currently unused. If it's intended for future use, consider adding a comment explaining its purpose.✅ Verification successful
The unused import is an expected pattern in contract bindings.
The
abi.ConvertTypeimport is part of the standard boilerplate for auto-generated Go contract bindings. While currently unused, it should be retained as it may be required in future regenerations of the contract bindings.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any usage of ConvertType in the codebase rg "ConvertType" -A 3 -B 3Length of output: 86779
Script:
#!/bin/bash # Search for ConvertType usage specifically in DappReverter.go rg "ConvertType" testutil/contracts/DappReverter.goLength of output: 72
testutil/contracts/Reverter.go (3)
29-29: Duplicate observation: Verify the necessity of the unused import.Similar to DappReverter.go, the
abi.ConvertTypeimport is currently unused.
42-42: LGTM: Contract bytecode updated.The contract bytecode has been updated to reflect the latest implementation.
167-171: LGTM: Consistent ABI parsing approach.The migration to
GetAbi()maintains consistency with other contract bindings.
Co-authored-by: Charlie Chen <[email protected]>
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Description
Closes #2711
Summary by CodeRabbit
Release Notes
New Features
Refactoring
Protocol Updates