fix(speed-up): resolve BTC RBF address paths and refine action UX#12381
Merged
Conversation
…osition The BTC speed-up modal was deriving each input's BIP44 path by treating an address's position in unchained's combined receive+change list as its BIP44 addressIndex. The two have no defined relation, so signing failed with "Can not sign for this input with the key X" whenever the metadata captured at original-send time wasn't available. Resolve each vin's addressNList from `account.chainSpecific.addresses[].path` (adds chain-adapters `Address.path`; requires the matching unchained `path` passthrough), current UTXO paths, and stored metadata. Throw a clear error if none match instead of guessing with brute-force candidates. Also restructures the modal: useQuery replaces the manual fetch effect + refs + withRetry, pure helpers (summarize/classify/reconstruct) live in speedUpUtils.ts, the bitcoin base asset constant replaces the BTC_ASSET_ID/store-fetched btcAsset pair, and the misleading `assetId` prop is dropped since the modal is BTC-only by design. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…action UX Now that unchained returns a BIP44 `path` on every xpub-derived address, the speed-up modal can: - Classify each vout deterministically via `addressNList[3] === 1`, dropping the `intendedSendSats` amount-match heuristic and its known self-send / same-value edge cases. - Resolve input addressNLists from the account's owned-address map alone, removing the redundant `getUtxos` round trip and collapsing `buildOwnedAddressNListMap` to a single source. Also tightens the RBF notification UX: - New translation for `transactionHistory.replaced` so the replaced action reads "You sped up your send of X BTC" instead of the bare word "Replaced", which doubled up with the status pill. - Replaced status pill switches from yellow to gray so it's visually distinct from Pending. - Dispatches the Replaced upsert before the new Pending insert so the reducer's auto-stamp orders the new tx above the replaced one. Adds unit coverage for `classifyOriginalOutputs` and `reconstructReplacementInputs` (chain-index branches, metadata priority, multi-vin indexing, empty-array guard, throw paths). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
More reviews will be available in 18 minutes and 19 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughRefactors the Bitcoin RBF speed-up flow: adds an optional derivation ChangesBitcoin RBF Speed-Up Refactoring
Sequence DiagramsequenceDiagram
participant User
participant SpeedUpModal
participant speedUpQuery
participant speedUpMutation
participant Blockchain
participant ActionService
User->>SpeedUpModal: open modal (txHash)
SpeedUpModal->>speedUpQuery: fetch original tx & reconstruct inputs/outputs
speedUpQuery-->>SpeedUpModal: OriginalTxSummary + ReconstructedInputs/Outputs
User->>SpeedUpModal: set fee & confirm
SpeedUpModal->>speedUpMutation: build & sign replacement BTCSignTx
speedUpMutation->>Blockchain: broadcast replacement tx
Blockchain-->>speedUpMutation: replacement txHash
speedUpMutation->>ActionService: mark original as Replaced
speedUpMutation->>ActionService: upsert new Send action (Pending)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/components/Layout/Header/ActionCenter/components/SpeedUpModal.tsx`:
- Around line 308-330: The current code filters out payment outputs without an
address (using .filter(Boolean(o.address))) which drops OP_RETURN/data outputs
and changes tx semantics; instead, preserve all outputs from
classifyOriginalOutputs by removing that filter and map each output to a
BTCSignTxOutputSpend entry: when o.address exists set addressType to
BTCOutputAddressType.Spend and include address/amount as before, and when
o.address is missing preserve the output as a data/return output (include any
script/data fields present on the original output) or set an appropriate enum
value (e.g., BTCOutputAddressType.Return/Data) if available; update the mapping
used to produce spendOutputs (and the analogous mapping at lines ~342-344) so
addressless outputs are passed through to signing rather than dropped.
- Around line 119-131: The useEffect that assigns selectedFeeRate from
speedUpQuery.data?.summary.feeRate should be guarded so it doesn't overwrite a
user-selected bump—only set selectedFeeRate when it is currently undefined/null
(or when a new tx is loaded), e.g. protect the assignment inside the effect with
a check for selectedFeeRate state or a separate "userHasSelected" flag;
reference selectedFeeRate, useEffect, and speedUpQuery. For addressless outputs
make payment/value math and output construction consistent: either reconstruct
and include addressless outputs in outputs (so paymentOutputs and spendOutputs
match) or remove their value from totalPaymentValue/newChangeSats and omit them
from outputs (or detect and block speed-up when addressless outputs exist);
reference paymentOutputs, spendOutputs, outputs, and newChangeSats so the
change/fee computation and final outputs are aligned.
In `@src/components/Layout/Header/ActionCenter/components/speedUpUtils.ts`:
- Around line 261-280: The code currently only uses vin.addresses?.[0]
(vinAddress) when resolving the BIP44 path and throws if addressNList is
missing; change the lookup to fall back to the spent output's address from
prevTx.vout[voutIndex] (e.g. inspect
prevTx.vout[voutIndex].scriptPubKey?.addresses?.[0] or value equivalent) before
failing: keep using metadata?.inputs[index]?.addressNList first, then
vinAddress, then the prev output address, and finally ownedAddressMap.get(...)
to produce addressNList so that path resolution succeeds when the provider
omitted vin addresses; update references around resolveVinVoutIndex, voutIndex,
vinAddress, prevTx.vout[voutIndex], metadata?.inputs[index]?.addressNList, and
ownedAddressMap accordingly.
🪄 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: CHILL
Plan: Pro
Run ID: 5131bfb4-c6f2-463c-ad17-5a7427a58cba
📒 Files selected for processing (7)
packages/chain-adapters/src/utxo/types.tssrc/assets/translations/en/main.jsonsrc/components/Layout/Header/ActionCenter/ActionCenter.tsxsrc/components/Layout/Header/ActionCenter/components/ActionStatusTag.tsxsrc/components/Layout/Header/ActionCenter/components/SpeedUpModal.tsxsrc/components/Layout/Header/ActionCenter/components/speedUpUtils.test.tssrc/components/Layout/Header/ActionCenter/components/speedUpUtils.ts
💤 Files with no reviewable changes (1)
- src/components/Layout/Header/ActionCenter/ActionCenter.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/Layout/Header/ActionCenter/components/speedUpUtils.ts (1)
351-356: 💤 Low valueType assertion is validated but could be more defensive.
The
as stringassertion on line 353 is safe because line 324 already returns an error for non-OP_RETURN outputs without addresses. However, for extra safety and self-documentation, consider an explicit guard:if (!output.address) continue // unreachable after validation, but defensiveThis would make the invariant explicit and avoid future refactoring risks.
🤖 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 `@src/components/Layout/Header/ActionCenter/components/speedUpUtils.ts` around lines 351 - 356, Add a defensive guard before constructing the BTCSignTxOutputSpend: explicitly check output.address and skip/continue if falsy (e.g. if (!output.address) continue) so the creation of spend (BTCSignTxOutputSpend with addressType BTCOutputAddressType.Spend and pushing into signOutputs) never relies solely on the prior validation invariant; keep the rest of the logic (creating spend and signOutputs.push(spend)) unchanged.
🤖 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.
Nitpick comments:
In `@src/components/Layout/Header/ActionCenter/components/speedUpUtils.ts`:
- Around line 351-356: Add a defensive guard before constructing the
BTCSignTxOutputSpend: explicitly check output.address and skip/continue if falsy
(e.g. if (!output.address) continue) so the creation of spend
(BTCSignTxOutputSpend with addressType BTCOutputAddressType.Spend and pushing
into signOutputs) never relies solely on the prior validation invariant; keep
the rest of the logic (creating spend and signOutputs.push(spend)) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 69f9bc72-04b9-40c2-82b1-1a07725fb4c9
📒 Files selected for processing (5)
packages/chain-adapters/src/utxo/types.tssrc/assets/translations/en/main.jsonsrc/components/Layout/Header/ActionCenter/components/SpeedUpModal.tsxsrc/components/Layout/Header/ActionCenter/components/speedUpUtils.test.tssrc/components/Layout/Header/ActionCenter/components/speedUpUtils.ts
💤 Files with no reviewable changes (1)
- packages/chain-adapters/src/utxo/types.ts
Refactor the replacement-tx output construction to a pure, testable helper
and fix three correctness issues found during review.
- Preserve OP_RETURN data (Thorchain/Relay/ButterSwap memos, send-with-memo)
by lifting it to `txToSign.opReturnData`. The previous code silently
dropped addressless outputs while still counting their value, which
shifted sats into change and broke the swap memo on the replacement.
- Refuse speed-up on multiple change outputs. The previous loop would
have emitted each change position at the full `newChangeSats` value,
producing outputs > inputs and an invalid tx.
- Refuse speed-up on non-OP_RETURN addressless outputs (bare multisig,
non-standard scripts).
- Display the actual fee the user will pay (`actualNewFeeSats`). When new
change would land below dust, those sats get absorbed into the fee by
the wallet — the previous UI showed only the targeted fee, so the user
agreed to one amount and silently paid slightly more.
Output building moves to `buildReplacementOutputs` in speedUpUtils, which
returns either the built `{ outputs, opReturnData }` or `{ error }` with a
translation key the caller passes straight to `translate()`. 12 new unit
tests cover: single change, change-first ordering, OP_RETURN lift,
below-dust drop, exact-dust boundary, negative change, multi-change error,
multi-OP_RETURN error, addressless-non-OP_RETURN error, no-change tx,
amount preservation.
OP_RETURN exact-position preservation would require BTCSignTxOutputMemo
support in hdwallet-native and hdwallet-ledger; both currently only
handle the legacy top-level `opReturnData` field which appends at the
end. For our memo consumers (Thorchain Bifrost, Relay, ButterSwap)
position doesn't matter.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kaladinlight
force-pushed
the
fix/btc-speed-up-address-resolution
branch
from
902079c to
420106f
Compare
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
1 participant
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.
Description
Fixes BTC RBF (speed-up) signing failures and tightens the RBF action UX.
Path resolution
The speed-up modal was deriving each input's BIP44 path by treating an address's position in unchained's combined receive+change list as its BIP44
addressIndex. The two have no defined relation, so signing failed withCan not sign for this input with the key Xwhenever the metadata captured at original-send time wasn't available.Each vin's
addressNListis now resolved viaaccount.chainSpecific.addresses[].path(adds chain-adaptersAddress.path; requires the matching unchainedpathpassthrough), current UTXO paths, and stored metadata, with a clear error thrown if none match — no more brute-force candidate guessing.Output classification
With unchained now returning a BIP44
pathon every xpub-derived address, each vout is classified deterministically viaaddressNList[3] === 1, dropping theintendedSendSatsamount-match heuristic and its known self-send / same-value edge cases. InputaddressNLists are resolved from the account's owned-address map alone, removing the redundantgetUtxosround trip.Modal refactor
useQueryreplaces the manual fetch effect + refs +withRetrysummarizeOriginalTx,classifyOriginalOutputs,reconstructReplacementInputs) live inspeedUpUtils.tsBTC_ASSET_ID/store-fetchedbtcAssetpairassetIdprop dropped since the modal is BTC-onlyRBF action UX
transactionHistory.replacedtranslation so the replaced action reads "You sped up your send of X BTC" instead of the bare "Replaced", which doubled up with the status pillAdds unit coverage for
classifyOriginalOutputsandreconstructReplacementInputs(chain-index branches, metadata priority, multi-vin indexing, empty-array guard, throw paths).Issue (if applicable)
closes #
Risk
Medium. Touches on-chain transaction construction (RBF replacement signing) for BTC. The fix is narrowly scoped to address-path resolution and output classification, but anyone speeding up a BTC tx exercises this code.
Testing
Engineering
addressNList[3] === 1)pnpm jest src/components/Layout/Header/ActionCenter/components/speedUpUtils.test.tsOperations
Not flagged. QA should regression-test the BTC RBF flow end-to-end on a preview environment with each supported wallet (Native, Ledger, KeepKey).
Screenshots (if applicable)
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Style
Tests