fix: use multiple SUI coins for PTB gas payment#3877
Conversation
… of withdrawAndCall tx
|
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 📝 WalkthroughWalkthroughThe changes introduce a new parameter to methods responsible for selecting SUI coin objects, ensuring that only coins with a sufficient balance to cover the gas fee are chosen. All relevant interface, implementation, and test signatures have been updated to accommodate this parameter. Error handling and deterministic selection logic are improved. Changes
Sequence Diagram(s)sequenceDiagram
participant Signer
participant SuiClient
participant SuiChain
Signer->>SuiClient: getWithdrawAndCallObjectRefs(ctx, withdrawCapID, onCallObjectIDs, gasBudget)
SuiClient->>SuiChain: GetSuiCoinObjectRef(ctx, owner, wantBalance=gasBudget)
SuiChain-->>SuiClient: SUI Coin ObjectRef (with sufficient balance)
SuiClient-->>Signer: Object references for transaction
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
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3877 +/- ##
===========================================
- Coverage 64.73% 64.69% -0.04%
===========================================
Files 469 469
Lines 34333 34350 +17
===========================================
- Hits 22224 22222 -2
- Misses 11081 11101 +20
+ Partials 1028 1027 -1
🚀 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: 1
🔭 Outside diff range comments (1)
zetaclient/chains/sui/client/client.go (1)
255-335: 💡 Verification agent🧩 Analysis chain
Add unit tests for enhanced logic
The static analysis indicates that the new logic lacks test coverage. Consider adding unit tests that verify:
- Coin sorting works correctly
- Latest version detection functions properly
- Balance-based selection operates as expected
- All error paths are covered
🏁 Script executed:
#!/bin/bash # Check for any existing tests of the GetSuiCoinObjectRef function echo "Searching for existing tests of GetSuiCoinObjectRef function..." rg -A 3 "Test.*GetSuiCoinObjectRef" --type goLength of output: 184
To ensure there truly aren’t any existing tests covering
GetSuiCoinObjectRef, let’s search all Go files and explicitly within test files:
🏁 Script executed:
#!/bin/bash echo "1) Scanning all Go files for ‘GetSuiCoinObjectRef’..." rg -n "GetSuiCoinObjectRef" --type go echo "2) Scanning only *_test.go files for ‘GetSuiCoinObjectRef’..." rg -n "GetSuiCoinObjectRef" --glob "*_test.go"Length of output: 1837
Add unit tests for Client.GetSuiCoinObjectRef
There are currently no direct tests covering the logic in zetaclient/chains/sui/client/client.go. Please add a new test file (e.g. chains/sui/client/client_test.go) that exercises:
- Deterministic sorting of coins.Data by CoinObjectId
- Selection of the highest-version coin
- Picking a coin whose balance ≥ wantBalance
- All error paths (no coins returned, invalid coin type, parse‐uint failures, no qualified coin)
Target locations:
- chains/sui/client/client.go → add tests in chains/sui/client/client_test.go
- Use testutils/mocks to stub SuiXGetCoinsRequest responses
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 256-256: zetaclient/chains/sui/client/client.go#L256
Added line #L256 was not covered by tests
[warning] 261-263: zetaclient/chains/sui/client/client.go#L261-L263
Added lines #L261 - L263 were not covered by tests
[warning] 265-266: zetaclient/chains/sui/client/client.go#L265-L266
Added lines #L265 - L266 were not covered by tests
[warning] 270-272: zetaclient/chains/sui/client/client.go#L270-L272
Added lines #L270 - L272 were not covered by tests
[warning] 275-277: zetaclient/chains/sui/client/client.go#L275-L277
Added lines #L275 - L277 were not covered by tests
[warning] 280-280: zetaclient/chains/sui/client/client.go#L280
Added line #L280 was not covered by tests
[warning] 283-283: zetaclient/chains/sui/client/client.go#L283
Added line #L283 was not covered by tests
[warning] 288-288: zetaclient/chains/sui/client/client.go#L288
Added line #L288 was not covered by tests
[warning] 291-291: zetaclient/chains/sui/client/client.go#L291
Added line #L291 was not covered by tests
[warning] 293-294: zetaclient/chains/sui/client/client.go#L293-L294
Added lines #L293 - L294 were not covered by tests
[warning] 298-300: zetaclient/chains/sui/client/client.go#L298-L300
Added lines #L298 - L300 were not covered by tests
[warning] 303-306: zetaclient/chains/sui/client/client.go#L303-L306
Added lines #L303 - L306 were not covered by tests
[warning] 308-310: zetaclient/chains/sui/client/client.go#L308-L310
Added lines #L308 - L310 were not covered by tests
[warning] 316-316: zetaclient/chains/sui/client/client.go#L316
Added line #L316 was not covered by tests
🧹 Nitpick comments (2)
zetaclient/chains/sui/signer/withdraw_and_call_test.go (1)
322-326: Consider adding specific test cases for balance selectionWhile the test has been updated to include the new parameter, it doesn't explicitly test the core functionality - selecting a coin with sufficient balance. Consider adding test cases that verify:
- A case where multiple coins exist but only one has sufficient balance
- A case where no coin has sufficient balance
- A case where multiple coins have sufficient balance but only the one with latest version is selected
// Example test case structure: { name: "select coin with sufficient balance", mockCoins: models.CoinPage{ Data: []models.CoinData{ {CoinObjectId: "id1", Version: "1", Balance: "50", Digest: "digest1"}, {CoinObjectId: "id2", Version: "2", Balance: "150", Digest: "digest2"}, }, }, wantBalance: 100, expectedObjectId: "id2", },changelog.md (1)
49-49: Improve description clarity for consistency
The Fixes section favors concise phrasing. Consider rewording to align with existing entries:
- Use “with sufficient balance” instead of “that contains enough balance to pay”
- Use “cover” instead of “pay” for gas fees
Apply this diff:
-* [3877](https://github.com/zeta-chain/node/pull/3877) - choose latest SUI coin object that contains enough balance to pay PTB transaction gas fee +* [3877](https://github.com/zeta-chain/node/pull/3877) - choose latest SUI coin object with sufficient balance to cover PTB transaction gas fee
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
changelog.md(1 hunks)zetaclient/chains/sui/client/client.go(2 hunks)zetaclient/chains/sui/signer/signer.go(1 hunks)zetaclient/chains/sui/signer/signer_tx.go(1 hunks)zetaclient/chains/sui/signer/withdraw_and_call.go(2 hunks)zetaclient/chains/sui/signer/withdraw_and_call_test.go(1 hunks)zetaclient/testutils/mocks/sui_client.go(1 hunks)zetaclient/testutils/mocks/sui_gen.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.gozetaclient/chains/sui/signer/signer_tx.gozetaclient/testutils/mocks/sui_gen.gozetaclient/chains/sui/signer/withdraw_and_call_test.gozetaclient/chains/sui/signer/withdraw_and_call.gozetaclient/testutils/mocks/sui_client.gozetaclient/chains/sui/client/client.go
🪛 GitHub Check: codecov/patch
zetaclient/chains/sui/signer/signer_tx.go
[warning] 154-154: zetaclient/chains/sui/signer/signer_tx.go#L154
Added line #L154 was not covered by tests
zetaclient/chains/sui/client/client.go
[warning] 256-256: zetaclient/chains/sui/client/client.go#L256
Added line #L256 was not covered by tests
[warning] 261-263: zetaclient/chains/sui/client/client.go#L261-L263
Added lines #L261 - L263 were not covered by tests
[warning] 265-266: zetaclient/chains/sui/client/client.go#L265-L266
Added lines #L265 - L266 were not covered by tests
[warning] 270-272: zetaclient/chains/sui/client/client.go#L270-L272
Added lines #L270 - L272 were not covered by tests
[warning] 275-277: zetaclient/chains/sui/client/client.go#L275-L277
Added lines #L275 - L277 were not covered by tests
[warning] 280-280: zetaclient/chains/sui/client/client.go#L280
Added line #L280 was not covered by tests
[warning] 283-283: zetaclient/chains/sui/client/client.go#L283
Added line #L283 was not covered by tests
[warning] 288-288: zetaclient/chains/sui/client/client.go#L288
Added line #L288 was not covered by tests
[warning] 291-291: zetaclient/chains/sui/client/client.go#L291
Added line #L291 was not covered by tests
[warning] 293-294: zetaclient/chains/sui/client/client.go#L293-L294
Added lines #L293 - L294 were not covered by tests
[warning] 298-300: zetaclient/chains/sui/client/client.go#L298-L300
Added lines #L298 - L300 were not covered by tests
[warning] 303-306: zetaclient/chains/sui/client/client.go#L303-L306
Added lines #L303 - L306 were not covered by tests
[warning] 308-310: zetaclient/chains/sui/client/client.go#L308-L310
Added lines #L308 - L310 were not covered by tests
[warning] 316-316: zetaclient/chains/sui/client/client.go#L316
Added line #L316 was not covered by tests
🔇 Additional comments (15)
zetaclient/testutils/mocks/sui_gen.go (1)
24-24: Interface method signature updated to include wantBalance parameter.The GetSuiCoinObjectRef interface method has been correctly updated to include the new wantBalance parameter, ensuring that generated mocks will match the actual implementation signature. This maintains type compatibility across the codebase.
zetaclient/chains/sui/signer/signer.go (1)
37-37: Updated RPC interface to include wantBalance parameter.The GetSuiCoinObjectRef method signature in the RPC interface has been updated to include the wantBalance parameter of type uint64. This change correctly propagates the interface contract to implementers, ensuring they select coin objects with sufficient balance for transactions.
zetaclient/chains/sui/client/client.go (6)
254-256: Improved function documentationThe updated function signature and documentation clearly explain the purpose of the added
wantBalanceparameter. This change aligns with the PR objective to select coins with sufficient balance for gas fees.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 256-256: zetaclient/chains/sui/client/client.go#L256
Added line #L256 was not covered by tests
261-267: Improved error handling with more specific error messagesThe switch statement enhances error handling by providing distinct error paths for different failure scenarios, making debugging easier.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 261-263: zetaclient/chains/sui/client/client.go#L261-L263
Added lines #L261 - L263 were not covered by tests
[warning] 265-266: zetaclient/chains/sui/client/client.go#L265-L266
Added lines #L265 - L266 were not covered by tests
269-273: Good deterministic sorting implementationSorting coins by object ID ensures deterministic selection across observers, which is critical for consensus systems.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 270-272: zetaclient/chains/sui/client/client.go#L270-L272
Added lines #L270 - L272 were not covered by tests
274-295: Clear separation of coin selection logicThe first loop correctly identifies the latest coin version across all coins, which is a good first step in the selection process.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 275-277: zetaclient/chains/sui/client/client.go#L275-L277
Added lines #L275 - L277 were not covered by tests
[warning] 280-280: zetaclient/chains/sui/client/client.go#L280
Added line #L280 was not covered by tests
[warning] 283-283: zetaclient/chains/sui/client/client.go#L283
Added line #L283 was not covered by tests
[warning] 288-288: zetaclient/chains/sui/client/client.go#L288
Added line #L288 was not covered by tests
[warning] 291-291: zetaclient/chains/sui/client/client.go#L291
Added line #L291 was not covered by tests
[warning] 293-294: zetaclient/chains/sui/client/client.go#L293-L294
Added lines #L293 - L294 were not covered by tests
297-312: Effective implementation of balance-based selectionThe second loop properly selects the first coin with the latest version that meets the balance requirement, which fulfills the PR objective.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 298-300: zetaclient/chains/sui/client/client.go#L298-L300
Added lines #L298 - L300 were not covered by tests
[warning] 303-306: zetaclient/chains/sui/client/client.go#L303-L306
Added lines #L303 - L306 were not covered by tests
[warning] 308-310: zetaclient/chains/sui/client/client.go#L308-L310
Added lines #L308 - L310 were not covered by tests
314-317: Clear error messaging for edge caseThe error message explains the exact reason for failure when no qualified coin is found, which aids in troubleshooting.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 316-316: zetaclient/chains/sui/client/client.go#L316
Added line #L316 was not covered by testszetaclient/chains/sui/signer/withdraw_and_call.go (2)
137-142: Interface update to include gas budget parameterThe function signature has been properly updated to include the
gasBudgetparameter, which is required for the enhanced coin selection logic.
207-208: Correctly passing gasBudget to GetSuiCoinObjectRefThe call to
GetSuiCoinObjectRefnow includes thegasBudgetparameter, ensuring that a coin with sufficient balance is selected for gas payment.zetaclient/testutils/mocks/sui_client.go (3)
108-110: Correctly updated mock method signatureThe mock function signature has been properly updated to match the interface changes.
118-123: Updated type assertions for the new parameterAll function type assertions have been correctly updated to handle the additional
wantBalanceparameter.
127-129: Properly updated error function type assertionsThe error handling function type assertions have been correctly updated for the new parameter.
zetaclient/chains/sui/signer/withdraw_and_call_test.go (2)
322-323: Updated mock expectations with the new parameterThe mock expectation now correctly includes a matcher for the new
wantBalanceparameter.
325-326: Test call updated to provide gas budget parameterThe test now passes a value of 100 for the gas budget parameter, ensuring the function is tested with the new parameter.
ws4charlie
changed the title
3 participants
Description
One single SUI coin object may not cover PTB transaction gas fee. Using multiple SUI coin objects for gas payment is a reliable way to avoid potential insufficient funds.
Closes: #3874
How Has This Been Tested?
Summary by CodeRabbit
Bug Fixes
Documentation