Conversation
WalkthroughThe Unroll method on ArkClient was changed to accept an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
client.go (2)
502-506: Fix build: remove unused totalVtxosAmount.totalVtxosAmount is computed but never used; Go will not compile with an unused local.
Apply:
- totalVtxosAmount := uint64(0) - for _, vtxo := range vtxos { - totalVtxosAmount += vtxo.Amount - }
1560-1590: Correct coin selection math and typos; stop selecting once sufficient.
- amountToSelect should decrement by current utxo amount, not cumulative selectedAmount.
- Break the outer address loop once amountToSelect <= 0 to avoid over-selection.
- Typo: uncomfirmedBalance → unconfirmedBalance (also in error string).
- selectedCoins := make([]explorer.Utxo, 0) - selectedAmount := uint64(0) - amountToSelect := int64(fees) - txutils.ANCHOR_VALUE - uncomfirmedBalance := uint64(0) - for _, addr := range addresses { + selectedCoins := make([]explorer.Utxo, 0) + selectedAmount := uint64(0) + amountToSelect := int64(fees) - txutils.ANCHOR_VALUE + unconfirmedBalance := uint64(0) + for _, addr := range addresses { + if amountToSelect <= 0 { + break + } utxos, err := a.explorer.GetUtxos(addr) if err != nil { return "", err } for _, utxo := range utxos { - if !utxo.Status.Confirmed { - uncomfirmedBalance += utxo.Amount + if !utxo.Status.Confirmed { + unconfirmedBalance += utxo.Amount continue } selectedCoins = append(selectedCoins, utxo) selectedAmount += utxo.Amount - amountToSelect -= int64(selectedAmount) + amountToSelect -= int64(utxo.Amount) if amountToSelect <= 0 { break } } } if amountToSelect > 0 { - return "", fmt.Errorf( - "not enough confirmed funds to select %d (uncomfirmed balance: %d)", - amountToSelect, uncomfirmedBalance, - ) + return "", fmt.Errorf( + "not enough confirmed funds to select %d (unconfirmed balance: %d)", + amountToSelect, unconfirmedBalance, + ) }
🧹 Nitpick comments (2)
client.go (2)
540-549: Preserve sentinel while improving message.Wrap the detailed message with ErrWaitingForConfirmation so callers can use errors.Is(err, ErrWaitingForConfirmation).
- return nil, fmt.Errorf( - "exit is running, waiting for confirmation(s): %s", - strings.Join(txids, ", "), - ) + return nil, fmt.Errorf( + "%w: %s", + ErrWaitingForConfirmation, + strings.Join(txids, ", "), + )
555-579: Avoid UTXO selection races: lock around bumpAnchorTx/broadcast.bumpAnchorTx selects/spends on-chain UTXOs; consider guarding this section with coinSelectionLock like other send/settle paths.
- packageResponses := make([]string, 0, len(transactions)) - - for _, parent := range transactions { + packageResponses := make([]string, 0, len(transactions)) + + a.coinSelectionLock.Lock() + defer a.coinSelectionLock.Unlock() + for _, parent := range transactions { ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ark_sdk.go(1 hunks)client.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ark_sdk.go (1)
types/types.go (1)
Outpoint(71-74)
client.go (4)
types/types.go (1)
Outpoint(71-74)api-spec/protobuf/gen/ark/v1/types.pb.go (3)
Outpoint(24-30)Outpoint(43-43)Outpoint(58-60)types.go (1)
CoinSelectOptions(133-140)redemption/redeem.go (1)
ErrPendingConfirmation(191-193)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sdk unit tests
🔇 Additional comments (2)
client.go (1)
491-494: Good: outpointsFilter is correctly threaded into getVtxos.Passing OutpointsFilter via CoinSelectOptions here is the right place; aligns with the new signature.
ark_sdk.go (1)
41-41: Breaking API change: ArkClient.Unroll signature changed; document in release notes and consider version bump.The signature change from
Unroll(ctx context.Context) errortoUnroll(ctx context.Context, outpointsFilter []types.Outpoint) ([]string, error)is confirmed. No internal call sites were found in the codebase. Since this is a public SDK interface, external consumers will need to adapt. Ensure the change is documented in release notes or CHANGELOG and consider a major version bump per semver.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
client.go (1)
552-625: Unroll breaks test contract: missing "no vtxos to unroll" error and ErrWaitingForConfirmation wrappingThe implementation violates two existing test expectations in
test/e2e/exit_test.go:
Empty vtxos case returns
(nil, nil)instead of error (line 570)
The test loop expectsUnrollto eventually return an error containing"no vtxos to unroll"to break. Withnilerror, the loop conditionif err == nil { continue }will spin indefinitely since there are no vtxos to process.Pending-confirmation error loses ErrWaitingForConfirmation wrapping (lines 618–621)
When vtxos are waiting for confirmation, the code builds a new error viafmt.Errorf("exit is running, waiting for confirmation(s): %s", ...)without wrappingErrWaitingForConfirmation. Tests checkif errors.Is(err, arksdk.ErrWaitingForConfirmation)to retry; this will always fail with the current implementation, breaking test flow.Fix by:
- Returning
fmt.Errorf("no vtxos to unroll")on line 570 instead ofnil- Wrapping with
%won line 618:fmt.Errorf("%w: exit is running, waiting for confirmation(s): %s", ErrWaitingForConfirmation, ...)- Returning
fmt.Errorf("no vtxos to unroll")on line 623 for consistency (though currently unreachable)test/e2e/exit_test.go (1)
206-221: Tests expect Unroll to return wrapped ErrWaitingForConfirmation and a "no vtxos to unroll" error, but implementation returns different valuesThe test logic is sound, but the current
Unrollimplementation doesn't match its expectations:
- When
len(vtxos) == 0,Unrollreturns(nil, nil)(line 571), so the test assertionrequire.ErrorContains(t, err, "no vtxos to unroll")at lines 220 and 294 never triggers.- When confirmations are pending,
Unrollreturns a plainfmt.Errorf(...)without wrappingErrWaitingForConfirmation(lines 608–614), soerrors.Is(err, arksdk.ErrWaitingForConfirmation)at lines 215 and 289 will never match.To align the implementation with test expectations, update
Unrollto:
- Return
fmt.Errorf("no vtxos to unroll")whenlen(vtxos) == 0.- Return
fmt.Errorf("%w: %s", arksdk.ErrWaitingForConfirmation, strings.Join(txids, ", "))in the pending-confirmation branch.Once these changes are made, the test loops will exercise the intended control flow without modification.
🧹 Nitpick comments (2)
client.go (2)
1801-1827: Minor typo and message clarity in bumpAnchorTx error for unconfirmed fundsThe new unconfirmed-balance tracking is good, but there are a couple of small nits:
- Variable and message typo:
uncomfirmedBalanceand"uncomfirmed balance"(Lines 1801, 1825–1827) should beunconfirmedBalance/"unconfirmed balance"for readability.- The message
"not enough confirmed funds to select %d (uncomfirmed balance: %d)"usesamountToSelect(the remaining shortfall) as the first%d. Consider wording it explicitly as “shortfall” to avoid confusion with “total required funds”.Example tweak:
- uncomfirmedBalance := uint64(0) + unconfirmedBalance := uint64(0) @@ - if !utxo.Status.Confirmed { - uncomfirmedBalance += utxo.Amount - continue - } + if !utxo.Status.Confirmed { + unconfirmedBalance += utxo.Amount + continue + } @@ - if amountToSelect > 0 { - return "", fmt.Errorf( - "not enough confirmed funds to select %d (uncomfirmed balance: %d)", - amountToSelect, uncomfirmedBalance, - ) - } + if amountToSelect > 0 { + return "", fmt.Errorf( + "not enough confirmed funds: shortfall %d (unconfirmed balance: %d)", + amountToSelect, unconfirmedBalance, + ) + }Purely cosmetic, but it makes logs and error messages clearer when diagnosing fee-bump funding issues.
2709-2735: OutpointsFilter behavior looks correct; consider deduping filter logicThe new OutpointsFilter handling in:
getClaimableBoardingUtxos(Lines 2709–2725),getExpiredBoardingUtxos(Lines 2767–2783), andgetVtxos(Lines 2802–2804 viafilterByOutpoints),is consistent and does what it says: restricts to the provided outpoints before applying the rest of the logic.
Given you now filter vtxos via
filterByOutpointsand inline essentially the same loop twice for boarding utxos, you might want to centralize that logic to avoid divergence over time (e.g., shared helperfilterUtxosByOutpoints(utxos []explorer.Utxo, filter []types.Outpoint)that returns the sliced list), but this is non-blocking.Also applies to: 2767-2789, 2802-2804
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ark_sdk.go(1 hunks)client.go(8 hunks)test/e2e/exit_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ark_sdk.go
🧰 Additional context used
🧬 Code graph analysis (1)
client.go (4)
types/types.go (1)
Outpoint(71-74)api-spec/protobuf/gen/ark/v1/types.pb.go (3)
Outpoint(24-30)Outpoint(43-43)Outpoint(58-60)types.go (1)
CoinSelectOptions(134-145)redemption/redeem.go (1)
ErrPendingConfirmation(191-193)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sdk unit tests
- GitHub Check: Sdk integration tests
2 participants
This PR contains several improvements to Unroll method:
submitpackagecalls@altafan please review
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.