[WIP] swap: enable UTXO selection.#4140
Conversation
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR implements UTXO (Unspent Transaction Output) selection functionality across the application. The backend adds methods to compute selected UTXO amounts, track selected inputs in active transaction proposals, and expose this information through a new ✨ 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/coins/btc/transaction_test.go`:
- Around line 412-421: Add a test case that exercises the SelectedUTXOsAmount
guard for non-spendable selections: create a test (e.g.,
TestSelectedUTXOsAmount_NonSpendableOutpoint) using testAccount() and build a
selected outpoint (via wire.NewOutPoint) that is not spendable by the account,
call account.SelectedUTXOsAmount with that outpoint in the map, and assert the
call returns the expected failure (an error) rather than a valid amount (do not
assert coin.NewAmountFromInt64); reuse the existing test setup patterns from
TestSelectedUTXOsAmount to locate the account and outpoint creation.
In `@frontends/web/src/api/account.ts`:
- Around line 352-355: TSelectedUTXO currently requires address but backend can
omit it; update the type definition for TSelectedUTXO so address is optional
(e.g., change to address?: string) and then update any call sites or
destructuring that assume address is always present to handle undefined (look
for usages of TSelectedUTXO, selectedUtxos, and any code that reads .address to
add fallbacks or null checks).
In `@frontends/web/src/routes/account/send/components/confirm/selected-utxos.tsx`:
- Around line 11-20: The grouping function groupUTXOsByAddress should normalize
stringified null/undefined addresses so they don't become literal keys; inside
the loop that iterates over Object.entries(selectedUTXOs) (variables outpoint
and address), coerce address to a normalized key (e.g. treat address === 'null'
|| address === 'undefined' || address == null as ''), then use that normalized
key when creating/looking up utxosByAddress and when pushing outpoint; this
ensures the existing downstream `||` checks for empty strings still work and
users see the translated "unknown" instead of the literal "null"/"undefined".
In `@frontends/web/src/routes/market/swap/swap.tsx`:
- Around line 306-319: The lookup failure currently clears selected UTXO state
via setSelectedSwapUTXOsAmount('') and setSellSelectedUTXOs(false) but does not
restore the user's manual sell amount (sellAmount), which can unexpectedly blank
the quote; before initiating the UTXO amount lookup capture the current
sellAmount into a ref (e.g., prevSellAmountRef) or temp variable, and in both
the !response.success branch and the .catch branch restore that saved value by
calling setSellAmount(prevSellAmountRef.current) (or equivalent) while still
clearing selected UTXO state with setSelectedSwapUTXOsAmount('') and
setSellSelectedUTXOs(false).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bf161d53-2e9c-41cd-a819-eb74932864b5
📒 Files selected for processing (18)
backend/coins/btc/account.gobackend/coins/btc/handlers/handlers.gobackend/coins/btc/transaction.gobackend/coins/btc/transaction_test.gobackend/handlers/handlers.gobackend/swap.gobackend/swap_test.gofrontends/web/src/api/account.tsfrontends/web/src/api/swap.tsfrontends/web/src/locales/en/app.jsonfrontends/web/src/routes/account/send/components/confirm/confirm.module.cssfrontends/web/src/routes/account/send/components/confirm/confirm.tsxfrontends/web/src/routes/account/send/components/confirm/selected-utxos.module.cssfrontends/web/src/routes/account/send/components/confirm/selected-utxos.tsxfrontends/web/src/routes/market/swap/components/swap-confirm.tsxfrontends/web/src/routes/market/swap/swap.module.cssfrontends/web/src/routes/market/swap/swap.test.tsxfrontends/web/src/routes/market/swap/swap.tsx
💤 Files with no reviewable changes (1)
- frontends/web/src/routes/account/send/components/confirm/confirm.module.css
| func TestSelectedUTXOsAmount(t *testing.T) { | ||
| account := testAccount(t) | ||
| selectedOutPoint := *wire.NewOutPoint(&chainhash.Hash{}, 1) | ||
|
|
||
| amount, err := account.SelectedUTXOsAmount(map[wire.OutPoint]struct{}{ | ||
| selectedOutPoint: {}, | ||
| }) | ||
| require.NoError(t, err) | ||
| require.Equal(t, coin.NewAmountFromInt64(1000000), amount) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add a non-spendable outpoint test case for SelectedUTXOsAmount.
This test currently validates only the success path; the method’s key guard (selected outpoint not spendable) is untested.
Suggested test addition
func TestSelectedUTXOsAmount(t *testing.T) {
account := testAccount(t)
selectedOutPoint := *wire.NewOutPoint(&chainhash.Hash{}, 1)
amount, err := account.SelectedUTXOsAmount(map[wire.OutPoint]struct{}{
selectedOutPoint: {},
})
require.NoError(t, err)
require.Equal(t, coin.NewAmountFromInt64(1000000), amount)
+
+ _, err = account.SelectedUTXOsAmount(map[wire.OutPoint]struct{}{
+ *wire.NewOutPoint(&chainhash.Hash{}, 999): {},
+ })
+ require.ErrorContains(t, err, "is not spendable")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestSelectedUTXOsAmount(t *testing.T) { | |
| account := testAccount(t) | |
| selectedOutPoint := *wire.NewOutPoint(&chainhash.Hash{}, 1) | |
| amount, err := account.SelectedUTXOsAmount(map[wire.OutPoint]struct{}{ | |
| selectedOutPoint: {}, | |
| }) | |
| require.NoError(t, err) | |
| require.Equal(t, coin.NewAmountFromInt64(1000000), amount) | |
| } | |
| func TestSelectedUTXOsAmount(t *testing.T) { | |
| account := testAccount(t) | |
| selectedOutPoint := *wire.NewOutPoint(&chainhash.Hash{}, 1) | |
| amount, err := account.SelectedUTXOsAmount(map[wire.OutPoint]struct{}{ | |
| selectedOutPoint: {}, | |
| }) | |
| require.NoError(t, err) | |
| require.Equal(t, coin.NewAmountFromInt64(1000000), amount) | |
| _, err = account.SelectedUTXOsAmount(map[wire.OutPoint]struct{}{ | |
| *wire.NewOutPoint(&chainhash.Hash{}, 999): {}, | |
| }) | |
| require.ErrorContains(t, err, "is not spendable") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/coins/btc/transaction_test.go` around lines 412 - 421, Add a test
case that exercises the SelectedUTXOsAmount guard for non-spendable selections:
create a test (e.g., TestSelectedUTXOsAmount_NonSpendableOutpoint) using
testAccount() and build a selected outpoint (via wire.NewOutPoint) that is not
spendable by the account, call account.SelectedUTXOsAmount with that outpoint in
the map, and assert the call returns the expected failure (an error) rather than
a valid amount (do not assert coin.NewAmountFromInt64); reuse the existing test
setup patterns from TestSelectedUTXOsAmount to locate the account and outpoint
creation.
| export type TSelectedUTXO = { | ||
| outPoint: string; | ||
| address: string; | ||
| }; |
There was a problem hiding this comment.
Make address optional in TSelectedUTXO to match backend payload.
Line 354 requires address, but selected UTXO address can be missing for some inputs. Keeping it required hides a real undefined case and weakens downstream fallback handling.
💡 Suggested fix
export type TSelectedUTXO = {
outPoint: string;
- address: string;
+ address?: string;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export type TSelectedUTXO = { | |
| outPoint: string; | |
| address: string; | |
| }; | |
| export type TSelectedUTXO = { | |
| outPoint: string; | |
| address?: string; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontends/web/src/api/account.ts` around lines 352 - 355, TSelectedUTXO
currently requires address but backend can omit it; update the type definition
for TSelectedUTXO so address is optional (e.g., change to address?: string) and
then update any call sites or destructuring that assume address is always
present to handle undefined (look for usages of TSelectedUTXO, selectedUtxos,
and any code that reads .address to add fallbacks or null checks).
| const groupUTXOsByAddress = (selectedUTXOs: TSelectedUTXOs): TUTXOsByAddress => { | ||
| const utxosByAddress: TUTXOsByAddress = {}; | ||
| for (const [outpoint, address] of Object.entries(selectedUTXOs)) { | ||
| if (!utxosByAddress[address]) { | ||
| utxosByAddress[address] = []; | ||
| } | ||
| utxosByAddress[address].push(outpoint); | ||
| } | ||
| return utxosByAddress; | ||
| }; |
There was a problem hiding this comment.
Consider handling stringified null/undefined addresses explicitly.
When an address value is null or undefined, Object.entries() coerces them to the string keys "null" or "undefined". Later, these strings are truthy and won't trigger the fallback logic at lines 45 and 47, so users would see the literal text "null" or "undefined" instead of the translated "unknown".
🛡️ Suggested fix to handle stringified nullish values
const groupUTXOsByAddress = (selectedUTXOs: TSelectedUTXOs): TUTXOsByAddress => {
const utxosByAddress: TUTXOsByAddress = {};
for (const [outpoint, address] of Object.entries(selectedUTXOs)) {
- if (!utxosByAddress[address]) {
+ const key = (address && address !== 'undefined' && address !== 'null') ? address : '';
+ if (!utxosByAddress[key]) {
- utxosByAddress[address] = [];
+ utxosByAddress[key] = [];
}
- utxosByAddress[address].push(outpoint);
+ utxosByAddress[key].push(outpoint);
}
return utxosByAddress;
};This normalizes all nullish or stringified-nullish addresses to an empty string, which the existing || checks will handle correctly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const groupUTXOsByAddress = (selectedUTXOs: TSelectedUTXOs): TUTXOsByAddress => { | |
| const utxosByAddress: TUTXOsByAddress = {}; | |
| for (const [outpoint, address] of Object.entries(selectedUTXOs)) { | |
| if (!utxosByAddress[address]) { | |
| utxosByAddress[address] = []; | |
| } | |
| utxosByAddress[address].push(outpoint); | |
| } | |
| return utxosByAddress; | |
| }; | |
| const groupUTXOsByAddress = (selectedUTXOs: TSelectedUTXOs): TUTXOsByAddress => { | |
| const utxosByAddress: TUTXOsByAddress = {}; | |
| for (const [outpoint, address] of Object.entries(selectedUTXOs)) { | |
| const key = (address && address !== 'undefined' && address !== 'null') ? address : ''; | |
| if (!utxosByAddress[key]) { | |
| utxosByAddress[key] = []; | |
| } | |
| utxosByAddress[key].push(outpoint); | |
| } | |
| return utxosByAddress; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontends/web/src/routes/account/send/components/confirm/selected-utxos.tsx`
around lines 11 - 20, The grouping function groupUTXOsByAddress should normalize
stringified null/undefined addresses so they don't become literal keys; inside
the loop that iterates over Object.entries(selectedUTXOs) (variables outpoint
and address), coerce address to a normalized key (e.g. treat address === 'null'
|| address === 'undefined' || address == null as ''), then use that normalized
key when creating/looking up utxosByAddress and when pushing outpoint; this
ensures the existing downstream `||` checks for empty strings still work and
users see the translated "unknown" instead of the literal "null"/"undefined".
| if (!response.success) { | ||
| setSelectedSwapUTXOsAmount(''); | ||
| setSellSelectedUTXOs(false); | ||
| return; | ||
| } | ||
| setSelectedSwapUTXOsAmount(response.amount.amount); | ||
| }) | ||
| .catch(() => { | ||
| if (canceled) { | ||
| return; | ||
| } | ||
| setSelectedSwapUTXOsAmount(''); | ||
| setSellSelectedUTXOs(false); | ||
| }); |
There was a problem hiding this comment.
Restore manual sell amount when UTXO amount lookup fails.
Line 308 and Line 318 disable sellSelectedUTXOs, but the manual amount is not restored. This can leave sellAmount blank/stale and unexpectedly clear the quote flow.
💡 Suggested fix
if (!response.success) {
setSelectedSwapUTXOsAmount('');
+ setSellAmount(manualSellAmountRef.current);
setSellSelectedUTXOs(false);
return;
}
...
.catch(() => {
if (canceled) {
return;
}
setSelectedSwapUTXOsAmount('');
+ setSellAmount(manualSellAmountRef.current);
setSellSelectedUTXOs(false);
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontends/web/src/routes/market/swap/swap.tsx` around lines 306 - 319, The
lookup failure currently clears selected UTXO state via
setSelectedSwapUTXOsAmount('') and setSellSelectedUTXOs(false) but does not
restore the user's manual sell amount (sellAmount), which can unexpectedly blank
the quote; before initiating the UTXO amount lookup capture the current
sellAmount into a ref (e.g., prevSellAmountRef) or temp variable, and in both
the !response.success branch and the .catch branch restore that saved value by
calling setSellAmount(prevSellAmountRef.current) (or equivalent) while still
clearing selected UTXO state with setSelectedSwapUTXOsAmount('') and
setSellSelectedUTXOs(false).
1 participant
Note, this is an entirely vibe-coded feature just because I had extra usage available and wanted to see how much work it would take to implement the feature. It is NOT ready for review, I haven't even looked at the code.
Before asking for reviews, here is a check list of the most common things you might need to consider: