feat: cancel invalid withdrawAndCall if execution fails with unknown error#3837
feat: cancel invalid withdrawAndCall if execution fails with unknown error#3837ws4charlie merged 5 commits intodevelopfrom
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 update introduces a mechanism to deliberately trigger and handle transaction reverts in Sui's Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ZetaClient
participant SuiContract
participant MoveOnCall
User->>ZetaClient: Initiate withdrawAndCall with data
ZetaClient->>SuiContract: Broadcast transaction
SuiContract->>MoveOnCall: Execute on_call(data)
alt data == "revert"
MoveOnCall-->>SuiContract: Abort with ENonceMismatch
SuiContract-->>ZetaClient: Execution error with command index
ZetaClient->>ZetaClient: Parse error, determine non-retryable
ZetaClient-->>User: Transaction cancelled, funds safe
else data valid
MoveOnCall-->>SuiContract: Success
SuiContract-->>ZetaClient: Success response
ZetaClient-->>User: Transaction completed
end
Assessment against linked issues
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
|
!!!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 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3837 +/- ##
========================================
Coverage 64.40% 64.40%
========================================
Files 466 466
Lines 33761 33761
========================================
Hits 21743 21743
Misses 11011 11011
Partials 1007 1007
🚀 New features to boost your workflow:
|
ws4charlie
added
zetaclient
![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 (1)
pkg/contracts/sui/errors.go (1)
116-132: Add explicit range check for command indexThe current implementation assumes the command index is always within uint16 range, but this might not always be the case.
Consider adding an explicit range check before the conversion:
func extractCommandIndex(errorMsg string) (uint16, error) { matches := commandIndexRegex.FindStringSubmatch(errorMsg) if len(matches) != 2 { return 0, errors.New("no command index found") } cmdIndex, err := strconv.ParseUint(matches[1], 10, 16) if err != nil { return 0, errors.Wrap(err, "unable to convert command index to uint16") } + // Validate the command index range explicitly + if cmdIndex > 65535 { + return 0, errors.Errorf("command index %d exceeds uint16 range", cmdIndex) + } // #nosec G103 always in range return uint16(cmdIndex), nil }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
changelog.md(1 hunks)e2e/contracts/sui/example/sources/example.move(2 hunks)e2e/e2etests/test_sui_token_withdraw_and_call_revert_with_call.go(2 hunks)e2e/e2etests/test_sui_withdraw_and_call_revert_with_call.go(2 hunks)e2e/runner/sui.go(2 hunks)pkg/contracts/sui/errors.go(2 hunks)pkg/contracts/sui/errors_test.go(2 hunks)zetaclient/chains/sui/signer/signer_tx.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/sui/signer/signer_tx.goe2e/e2etests/test_sui_withdraw_and_call_revert_with_call.goe2e/e2etests/test_sui_token_withdraw_and_call_revert_with_call.gopkg/contracts/sui/errors_test.goe2e/runner/sui.gopkg/contracts/sui/errors.go
🧬 Code Graph Analysis (1)
e2e/runner/sui.go (3)
e2e/runner/runner.go (1)
E2ERunner(80-207)pkg/contracts/sui/payload.go (2)
CallPayload(26-35)NewCallPayload(37-43)e2e/config/config.go (1)
SuiExample(148-154)
🪛 GitHub Check: codecov/patch
zetaclient/chains/sui/signer/signer_tx.go
[warning] 291-291: zetaclient/chains/sui/signer/signer_tx.go#L291
Added line #L291 was not covered by tests
🔇 Additional comments (16)
e2e/contracts/sui/example/sources/example.move (2)
6-8: Good addition of explanatory comments for the error code.The comments clearly explain the purpose of
ENonceMismatchconstant and its relationship to the zetaclient's error handling, which enhances code readability and maintainability.
62-65: Well-implemented error condition for testing withdrawAndCall reverts.This addition implements a specific error path to test the cancellation of invalid withdrawAndCall transactions. The use of a special "revert" message ensures it's easy to trigger this condition in tests.
e2e/runner/sui.go (2)
20-23: Good practice defining the revert trigger message as a constant.Centralizing the message value as a constant improves maintainability by ensuring consistency between the contract and test code.
266-281: Well-structured implementation for the revert payload creator.The
SuiCreateExampleWACPayloadForRevertmethod mirrors the existing payload creation approach while providing a specialized version for testing revert scenarios. The comment clearly explains its purpose and relationship to the on_call function.changelog.md (1)
26-26: Clear and concise changelog entry.The entry effectively communicates the feature's purpose and benefits - cancelling Sui withdrawAndCall transactions when they cannot proceed due to invalid data in the on_call operation.
zetaclient/chains/sui/signer/signer_tx.go (1)
291-291:Details
❓ Verification inconclusive
Improved error message for execution status checking failures.
The error message now more accurately refers to the execution status "error" rather than "error code", which better aligns with the enhanced error parsing implemented in the Sui contracts error handling.
I noticed the static analysis indicates this line lacks test coverage. Consider adding a test case that triggers this error path to ensure complete coverage:
🏁 Script executed:
#!/bin/bash # Search for existing tests of the broadcastWithdrawalWithFallback function grep -r "Test.*broadcastWithdrawalWithFallback" --include="*.go" ./Length of output: 69
🏁 Script executed:
#!/bin/bash # Broader search for any references to broadcastWithdrawalWithFallback in test files grep -R "broadcastWithdrawalWithFallback" --include="*_test.go" -n .Length of output: 68
Verify test coverage for
broadcastWithdrawalWithFallbackerror pathI wasn’t able to find any existing unit tests that exercise the error return on line 291 of
zetaclient/chains/sui/signer/signer_tx.go. Please manually confirm whether your test suite covers this failure path—or, if not, add a dedicated test case that triggers the error inbroadcastWithdrawalWithFallback.• Location to cover:
zetaclient/chains/sui/signer/signer_tx.goat line 291 (errors.Wrapf(err, "unable to check tx execution status error: %s", ...))🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 291-291: zetaclient/chains/sui/signer/signer_tx.go#L291
Added line #L291 was not covered by testse2e/e2etests/test_sui_token_withdraw_and_call_revert_with_call.go (2)
26-28: Improved test robustness with dedicated revert payload generatorThe change from a manually constructed invalid payload to a specialized revert payload generator improves test reliability and maintainability by creating a standardized way to trigger the revert scenario.
48-48: Consistent application of revert payloadThe updated parameter correctly utilizes the newly generated revert payload, ensuring that the test properly exercises the transaction cancellation path.
e2e/e2etests/test_sui_withdraw_and_call_revert_with_call.go (2)
26-28: Improved test robustness with dedicated revert payload generatorThe change from a manually constructed invalid payload to a specialized revert payload generator improves test reliability and maintainability by creating a standardized way to trigger the revert scenario.
47-47: Consistent application of revert payloadThe updated parameter correctly utilizes the newly generated revert payload, ensuring that the test properly exercises the transaction cancellation path.
pkg/contracts/sui/errors_test.go (4)
9-9: Function name aligned with implementationRenaming the test function to match the function under test enhances code clarity and maintainability.
61-65: Enhanced test structure for error messagesThe test struct has been improved with a dedicated
errMsgfield for validating error messages and a more descriptiveerrorMsgExecfield name, making the tests more maintainable and explicit.
67-96: Comprehensive test coverage for non-retryable scenariosThe expanded test cases provide thorough coverage of various non-retryable error scenarios, including different MoveAbort codes, function contexts, and command index values. This ensures the error handling logic correctly identifies errors that should trigger transaction cancellation.
101-106: Improved error assertion logicThe updated test logic now properly checks for expected error messages, enhancing the verification of error handling behavior.
pkg/contracts/sui/errors.go (2)
44-46: Well-defined regex for command index extractionThe new regex pattern correctly extracts command indices from Sui execution error messages, enabling more precise error handling.
80-114: Enhanced retryability determination based on command indexThe refactored function correctly implements a more granular approach to determining error retryability based on the command index, allowing for precise handling of different error scenarios in the Sui command sequence:
- Command index 0: withdraw_impl errors (potentially retryable)
- Command index 1: gas budget transfer errors (non-retryable)
- Command index 2: on_call errors (non-retryable)
This implementation aligns with the PR objective to cancel outbound transactions when on_call operations fail due to invalid data.
… few comments around function and variable
…-cancel-invalid-withdrawAndCall-tx
4 participants
Description
If
withdrawAndCallcan't go through due to unknown errors,ZetaClientshould check execution error in detail and cancel the outbound properly. This is a safety check that prevents Sui outbound blockageCloses: #3799
Closes: #3778
How Has This Been Tested?
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores