fix(planner): stop transfer-merge spiral in recordsFee fee estimate#16
Merged
Conversation
Non-max transfer branch of recordsFee never assigned outputAmount; it stayed at the 0n initializer. estimateRecords then treated every UTXO as "not yet sufficient" (outputAmount >= expectedOutput is always false), pushing all records into payRecords and re-simulating with the 3-input cascade merge. feeSummary.feeCount ballooned (e.g. 32×0.1 transferring 0.01 reported feeCount=16, not 1). Planner.plan uses selectTransferInputs (greedy by amount, bounded by 3) for the actual input selection, so on-chain submissions were not affected on the SDK side. The bug corrupted only the fee summary displayed via okWithMerge / feeSummary. The frontend's own copy of this function at client/app/src/hook/useCommitmentEstimated.ts had the same bug and there did drive actual tx submission — see OCash PR #385 for that fix. Fix: set outputAmount = expectedOutput (or expectedOutput - fee when expectedIsWithFee) in the specified-amount branch, mirroring the max branch; also zero outputAmount when the insufficient-balance guard clears cost. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request addresses a bug in the recordsFee function where outputAmount was not assigned in the non-max branch, leading to incorrect fee calculations and unnecessary UTXO merging. A regression test was added to verify the fix. Feedback suggests resetting cost to 0n when outputAmount is negative to ensure consistency and correct flag calculation.
| } else { | ||
| cost = expectedIsWithFee ? expectedOutput : expectedOutput + fee; | ||
| outputAmount = expectedIsWithFee ? expectedOutput - fee : expectedOutput; | ||
| if (outputAmount < 0n) outputAmount = 0n; |
There was a problem hiding this comment.
For consistency with the maxUint256 branch (lines 139-142) and to ensure the okWithMerge flag is correctly calculated as false when fees exceed the requested amount, cost should also be reset to 0n when outputAmount is negative.
Suggested change
| if (outputAmount < 0n) outputAmount = 0n; | |
| if (outputAmount < 0n) { | |
| outputAmount = 0n; | |
| cost = 0n; | |
| } |
es-kt
approved these changes
Apr 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Mirror fix to OCash PR #385, which fixed the same bug in the frontend's copy of this function.
Bug
estimateRecords(used to producefeeSummary/maxSummary/okWithMerge) inflatesfeeCountdramatically:feeCount = 16,mergeCount = 15(cascade-merge full balance)feeCount = 1(a single 0.1 UTXO covers required amount + fee)Root cause
src/planner/planner.ts:143-145— inrecordsFee, theaction === 'transfer'non-max branch setcostbut forgotoutputAmount:estimateRecordsat line 249 then evaluates:For
expectedIsWithFee = false(default), the check is0n >= expectedOutput, always false → forEach keeps pushing records → all UTXOs accumulate →recordsFeere-simulates with cascade merge →feeCount= (N_utxos - 2)/2 + 1.Scope of impact
On the SDK side, only fee summary display is affected.
Planner.planusesselectTransferInputs(greedy by amount, capped at 3) for actual input selection, so on-chain transactions were correct. But downstream consumers readingfeeSummary.feeCount/mergeCount(UI tx-count displays,okWithMergegating) got wrong values.The frontend copy at
client/app/src/hook/useCommitmentEstimated.tshad the same bug and there DID drive submission —utxoTransfer.tsreadsfeeEstimated.payInfo.recordsandfeeCountdirectly and submits that many on-chain txs. A user on BSC Testnet burned 0.0064 BNB (16 × relayer fee) on a single 0.01 transfer. That's fixed in OCash PR #385.Fix
Tests
Added regression test in
tests/planner.test.tscovering the bug scenario (32 × 0.1 UTXOs, transfer 0.01). Pre-fix output:feeCount=16. Post-fix:feeCount=1.All 5 tests in the file pass with the fix.
Test plan
pnpm test -- tests/planner.test.tspassesfeeSummary.feeCountcontinue to render correctly🤖 Generated with Claude Code