feat: make Bitcoin deposit with invalid memo reverting #3615
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 a changelog entry and several code modifications for handling Bitcoin deposits with invalid memos. It adds new enum values (INVALID_MEMO) in both the OpenAPI specification and protobuf definitions, expands test coverage with new end-to-end and unit tests, refactors outbound processing via a switch-case in the deposit workflow, updates parameter naming in the validation function, and revises inbound vote message logic by removing legacy methods and introducing updated ones. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as InitiateOutbound Caller
participant Gateway as CCTXGatewayZEVM
participant Validator as ValidateOutboundZEVM
note over Gateway: Process deposit based\non inbound observation status
Caller->>Gateway: Call InitiateOutbound(deposit, status)
alt Status = INSUFFICIENT_DEPOSITOR_FEE
Gateway->>Caller: Abort deposit with specific error
else Status = INVALID_MEMO
Gateway->>Validator: Call ValidateOutboundZEVM(invalid memo error, shouldRevert=true)
Validator-->>Gateway: Return with reverted status
else Status = SUCCESS
Gateway->>Gateway: Process deposit normally
else Unknown Status
Gateway->>Caller: Abort with unknown status error
end
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 #3615 +/- ##
===========================================
+ Coverage 64.53% 64.56% +0.02%
===========================================
Files 469 469
Lines 32694 32772 +78
===========================================
+ Hits 21099 21159 +60
- Misses 10635 10648 +13
- Partials 960 965 +5
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
zetaclient/chains/bitcoin/observer/inbound.go (1)
114-126: 💡 Verification agent🧩 Analysis chain
Ensure test coverage for the updated confirmation mode check.
The modification from
types.ConfirmationMode_FASTtocrosschaintypes.ConfirmationMode_FASTis correctly implemented, but static analysis indicates this line is not covered by tests. Consider extending test coverage to verify the behavior when this confirmation mode is checked.
🏁 Script executed:
#!/bin/bash # Description: Check for test cases that verify the ConfirmationMode_FAST behavior # Look for tests that verify inbound messages with FAST confirmation mode # Search for test files that might test this functionality rg -A 3 -B 3 "ConfirmationMode_FAST" --type goLength of output: 8751
Enhance Test Coverage for FAST Confirmation Handling
The shift to using
crosschaintypes.ConfirmationMode_FASTin the inbound observer is confirmed by several end-to-end tests (e.g., ine2e/e2etests/test_bitcoin_deposit_fast_confirmation.go). However, while these tests validate that the FAST mode is correctly set, it is not clear that both the error branch (whenIsInboundEligibleForFastConfirmationfails) and the non-eligible scenario are explicitly exercised. Please add targeted unit tests to cover the following cases:
- Simulate a scenario where
IsInboundEligibleForFastConfirmationreturns an error, ensuring that the error is wrapped and the fallback value (blockNumber - 1) is returned.- Test the branch where the FAST confirmation eligibility check returns false, verifying that the loop correctly skips processing.
These additions will guarantee robust coverage of all decision branches in the updated confirmation mode logic.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 114-114: zetaclient/chains/bitcoin/observer/inbound.go#L114
Added line #L114 was not covered by tests
🧹 Nitpick comments (2)
docs/openapi/openapi.swagger.yaml (1)
57691-57694: Refine the Enum Description for ClarityThe updated description for
INVALID_MEMO("parse memo is invalid") could be improved for grammatical correctness and clarity. Consider revising it to "parsed memo is invalid" or "the provided Bitcoin memo is invalid." For instance:- - INVALID_MEMO: parse memo is invalid + - INVALID_MEMO: parsed memo is invalidThis change will enhance readability and maintain a professional tone in the API documentation.
e2e/e2etests/test_bticoin_deposit_invalid_memo_revert.go (1)
35-36: Missing error message verification for invalid memo case.The test validates the transaction status but lacks assertions for error message content in the invalid memo case, unlike the empty memo case above (lines 24-25).
Consider adding the same error message assertions for consistency:
utils.RequireCCTXStatus(r, cctx, crosschaintypes.CctxStatus_Reverted) require.EqualValues(r, crosschaintypes.InboundStatus_INVALID_MEMO, cctx.InboundParams.Status) +require.Contains(r, cctx.CctxStatus.StatusMessage, "inbound observation failed") +require.Contains(r, cctx.CctxStatus.ErrorMessage, "insufficient depositor fee")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
typescript/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 (15)
changelog.md(1 hunks)cmd/zetae2e/local/bitcoin.go(1 hunks)docs/openapi/openapi.swagger.yaml(1 hunks)e2e/e2etests/e2etests.go(2 hunks)e2e/e2etests/test_bitcoin_deposit_abort_with_low_deposit_fee.go(2 hunks)e2e/e2etests/test_bticoin_deposit_invalid_memo_revert.go(1 hunks)proto/zetachain/zetacore/crosschain/cross_chain_tx.proto(1 hunks)testutil/sample/crosschain.go(1 hunks)x/crosschain/keeper/cctx_gateway_zevm.go(2 hunks)x/crosschain/keeper/cctx_gateway_zevm_test.go(2 hunks)x/crosschain/keeper/cctx_orchestrator_validate_outbound.go(1 hunks)zetaclient/chains/bitcoin/observer/event.go(0 hunks)zetaclient/chains/bitcoin/observer/event_test.go(0 hunks)zetaclient/chains/bitcoin/observer/inbound.go(4 hunks)zetaclient/chains/bitcoin/observer/inbound_test.go(7 hunks)
💤 Files with no reviewable changes (2)
- zetaclient/chains/bitcoin/observer/event.go
- zetaclient/chains/bitcoin/observer/event_test.go
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.proto`: Review the Protobuf definitions, point out iss...
**/*.proto: Review the Protobuf definitions, point out issues relative to compatibility, and expressiveness.
proto/zetachain/zetacore/crosschain/cross_chain_tx.proto
`**/*.go`: Review the Go code, point out issues relative to ...
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetae2e/local/bitcoin.gox/crosschain/keeper/cctx_gateway_zevm_test.gotestutil/sample/crosschain.goe2e/e2etests/test_bticoin_deposit_invalid_memo_revert.goe2e/e2etests/test_bitcoin_deposit_abort_with_low_deposit_fee.goe2e/e2etests/e2etests.gox/crosschain/keeper/cctx_orchestrator_validate_outbound.gozetaclient/chains/bitcoin/observer/inbound.gox/crosschain/keeper/cctx_gateway_zevm.gozetaclient/chains/bitcoin/observer/inbound_test.go
🧠 Learnings (2)
zetaclient/chains/bitcoin/observer/inbound.go (1)
Learnt from: ws4charlie
PR: zeta-chain/node#2899
File: zetaclient/chains/bitcoin/observer/inbound.go:37-38
Timestamp: 2024-11-12T13:20:12.658Z
Learning: In `BTCInboundEvent`, it's acceptable to use `float64` for monetary values (`Value` and `DepositorFee`) because precision is ensured through conversion to integer when building the vote message.
zetaclient/chains/bitcoin/observer/inbound_test.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: lint
e2e/e2etests/test_bitcoin_deposit_abort_with_low_deposit_fee.go
[failure] 6-6:
File is not properly formatted (gci)
🪛 GitHub Actions: Linters and SAST
e2e/e2etests/test_bitcoin_deposit_abort_with_low_deposit_fee.go
[error] 1-1: File is not properly formatted (gci)
🪛 GitHub Check: codecov/patch
zetaclient/chains/bitcoin/observer/inbound.go
[warning] 114-114: zetaclient/chains/bitcoin/observer/inbound.go#L114
Added line #L114 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: start-e2e-test / e2e
🔇 Additional comments (32)
zetaclient/chains/bitcoin/observer/inbound.go (5)
9-19: Import reorganization enhances code readability.The import statements have been appropriately updated to include the necessary packages for memo handling and to use the more specific
crosschaintypesinstead of the generaltypespackage, which aligns with current best practices for module dependencies.
189-195: Robust error handling for invalid memo processing.This change properly improves error handling by setting an invalid memo status rather than simply returning nil. This approach gives downstream processes more context about the failure reason and enables appropriate actions like reverting the transaction.
209-217: Well-designed branching logic for memo type handling.The conditional logic effectively differentiates between legacy and standard memos, directing the flow to appropriate handlers. This design pattern enhances maintainability by isolating type-specific processing.
264-296: Clean implementation of legacy memo handling.The
NewInboundVoteFromLegacyMemomethod is well-structured with:
- Clear confirmation mode determination
- Appropriate parameter passing
- Proper use of cosmosmath for big integer handling
The function correctly leverages the crosschain options parameter for indicating cross-chain calls based on memo content.
298-341: Comprehensive implementation for standard memo handling.The
NewInboundVoteFromStdMemofunction appropriately:
- Extracts revert options from the memo
- Determines if it's a cross-chain call based on OpCode
- Sets the confirmation mode based on block confirmation status
- Uses the correct receiver address from the memo
This implementation properly handles all the nuances of standard memos including revert options and cross-chain calls.
cmd/zetae2e/local/bitcoin.go (1)
41-46: Test coverage appropriately extended for invalid memo handling.The addition of
e2etests.TestBitcoinDepositInvalidMemoRevertNameto the advanced Bitcoin deposit tests ensures that the new invalid memo handling feature is properly tested in end-to-end scenarios. This follows best practices of ensuring that functional changes are accompanied by appropriate tests.proto/zetachain/zetacore/crosschain/cross_chain_tx.proto (1)
45-47: Appropriate Protobuf enum extension for invalid memo status.The addition of the
INVALID_MEMO = 3enum value toInboundStatusis well-documented with a clear comment. This extension properly complements the implementation in the observer code and maintains the sequential numbering convention of the enum values.Verify that all consumers of the InboundStatus enum properly handle this new value:
#!/bin/bash # Description: Find all switch statements on InboundStatus to ensure they handle the new INVALID_MEMO value # Look for switch statements on InboundStatus rg "switch.*InboundStatus" --type go -A 10 -B 2docs/openapi/openapi.swagger.yaml (1)
57685-57689: Validate the Addition ofINVALID_MEMOto the EnumThe inclusion of the new
INVALID_MEMOvalue within the enum is well positioned and meets the objective of reverting Bitcoin deposits with invalid memos. Ensure that downstream logic and documentation correctly reference this status.changelog.md (1)
30-30: Feature addition looks good.This changelog entry correctly documents the new behavior for Bitcoin deposits with invalid memos. The link to the PR is properly formatted and the description is clear and concise.
e2e/e2etests/e2etests.go (2)
122-122: Constant naming follows established pattern.The new test constant follows the established naming convention for Bitcoin test cases in this file.
912-918: Well-structured test definition.The test definition correctly follows the project's E2E test pattern with appropriate minimum version requirement. The test description is clear and informative.
x/crosschain/keeper/cctx_gateway_zevm_test.go (3)
4-6: Necessary imports added.These imports are necessary for the new test cases. The naming conventions follow Go best practices with appropriate aliases.
88-111: Comprehensive test for unknown inbound status.This test ensures that when an unrecognized inbound status is encountered, the system properly aborts the transaction rather than proceeding with potentially unsafe processing.
113-165: Thorough test for invalid memo handling.The test properly validates that transactions with invalid memo status are correctly moved to a PendingRevert state. The mock setup is comprehensive, covering all necessary dependencies for the revert outbound creation.
testutil/sample/crosschain.go (2)
175-175: Changes the randomized CoinType to a fixed value for more deterministic tests.The change from a random coin type to a fixed
coin.CoinType_Gasvalue improves test stability and predictability. This modification ensures that tests using this sample generator will have consistent behavior.
182-182: Replaces random inbound status with consistent SUCCESS status.Changing from
InboundStatusFromRand(r)to a fixedtypes.InboundStatus_SUCCESSensures that tests using this function will produce consistent results. This aligns with the PR objective of making Bitcoin deposit behavior more deterministic regarding invalid memos.x/crosschain/keeper/cctx_orchestrator_validate_outbound.go (2)
39-39: Improved parameter naming for better semantic clarity.Renaming from
isContractRevertedtoshouldRevertbetter conveys the intent of this parameter. The parameter represents a decision about whether to perform a revert action rather than a statement about the current state of the contract.
41-41: Updated condition to match the renamed parameter.This change uses the updated parameter name in the conditional check, maintaining the original logic but with improved semantic clarity.
e2e/e2etests/test_bticoin_deposit_invalid_memo_revert.go (2)
1-38: Well-structured test for Bitcoin deposits with invalid memos.This test effectively validates two key scenarios:
- Deposits with empty memos
- Deposits with invalid memo content
The test ensures that in both cases the cross-chain transactions are properly reverted and marked with the correct
INVALID_MEMOstatus. The structure follows a consistent pattern of arrangement, action, and assertion that aids in readability and maintenance.
23-25: Complete assertion coverage for empty memo case.The assertions thoroughly verify:
- The overall CCTX status (Reverted)
- The specific inbound status (INVALID_MEMO)
- The presence of appropriate error messages
This provides robust validation of the error handling path.
e2e/e2etests/test_bitcoin_deposit_abort_with_low_deposit_fee.go (1)
33-35: Enhanced error validation with more specific checks.The updated assertions provide more comprehensive validation:
- Checking the status message for "inbound observation failed"
- Verifying the error message contains "insufficient depositor fee"
- Explicitly validating the inbound status against the expected enumeration value
This improves test reliability and aligns with the PR's focus on proper error handling for Bitcoin deposits.
x/crosschain/keeper/cctx_gateway_zevm.go (5)
30-39: Well-structured control flow for handling observation statuses.The switch-case structure provides a clean and maintainable approach for handling different inbound observation statuses, improving code readability compared to a sequence of if-else statements.
39-43: Good implementation of invalid memo handling.This code correctly implements the feature to revert transactions with invalid memos, which was the primary objective of this PR. The implementation is clear and concise, using the existing
ValidateOutboundZEVMmethod with an appropriate error message.
43-56: Clear error handling for deposit failures.The error handling logic is well-structured, with appropriate comments explaining the exceptional case when an error occurs but the contract is not reverted. The use of
ProcessAbortwith detailed error messages will help with debugging.
58-62: Proper context management and commit logic.The code correctly validates the outbound status and only commits the cached context when necessary (when the status is
OutboundMinedorPendingRevert), which is an efficient use of the caching mechanism.
64-71: Comprehensive error handling with default case.The default case properly handles any unknown observation statuses, providing a robust safety mechanism. The error message includes the specific invalid status value, which will be helpful for debugging.
zetaclient/chains/bitcoin/observer/inbound_test.go (6)
3-31: Updated imports align with new functionality requirements.The addition of imports for
cosmosmath,coin, andmemopackages correctly supports the new test functions and memo handling logic.
169-173: Enhanced test struct with explicit observation status.The addition of the
observationStatusfield to the test struct allows for more precise testing of the inbound vote message generation logic, particularly for validating the status handling.
185-186: Clear test cases for different memo scenarios.Test cases now explicitly specify the expected observation status (
SUCCESSorINVALID_MEMO), making the test assertions more precise and the test intentions clearer.Also applies to: 193-194, 201-202
226-228: Explicit verification of observation status.The test now properly verifies that the generated vote message has the expected observation status, which is essential for validating the core functionality of this PR.
687-731: Comprehensive test for legacy memo handling.This new test function thoroughly validates the creation of inbound vote messages from legacy memos, covering all relevant fields and ensuring that the protocol contract version and revert options are correctly set.
733-791: Thorough test for standard memo handling.The test function for standard memos is well-implemented, with proper setup of test data, including revert options and receiver address. It verifies that all fields of the generated vote message are correctly populated.
e2e/e2etests/test_bitcoin_deposit_abort_with_low_deposit_fee.go
Outdated
Show resolved
Hide resolved
|
E2E was passing locally, will investigate but not a blocker to already review the change |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
lumtis
added
UPGRADE_TESTS
|
!!!WARNING!!! Be very careful about using Only suppress a single rule (or a specific set of rules) within a section of code, while continuing to scan for other problems. To do this, you can list the rule(s) to be suppressed within the #nosec annotation, e.g: /* #nosec G401 */ or //#nosec G201 G202 G203 Pay extra attention to the way |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Part of #3617
Make the CCTX reverts if the Bitcoin memo has an invalid format or is empty.
Summary by CodeRabbit
Bug Fixes
Documentation
Tests
Refactor