DRAFT: Fees #216
DRAFT: Fees #216
Conversation
WalkthroughAdds a FeesProvider context with fee-calculation APIs, a new InfoLine UI component, a fee-aware collaborative exit function, and updates send/receive screens to compute/display fees and use fee-aware exit flows. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI Components
participant Fees as FeesProvider
participant AspCtx as AspContext
participant Lib as src/lib/asp.ts
participant Wallet as Wallet/Txn handler
UI->>Fees: useContext(FeesContext)
Fees->>AspCtx: read aspInfo.fees.intentFee
AspCtx-->>Fees: fee values (or 0)
Fees-->>UI: calc*Fee(...) results
UI->>UI: render InfoLine / compute lightningFeeText
alt Off-chain / submarine swap
UI->>Wallet: sendOffChain(details.total)
Wallet-->>UI: txid/result
else On-chain exit with fees
UI->>Lib: collaborativeExitWithFees(wallet, inputAmount, outputAmount, address)
Lib->>Lib: collect & sort UTXOs, select inputs to cover inputAmount
Lib->>Lib: create outputs (address: outputAmount, change: offchainAddr)
Lib-->>UI: settlement result (txid)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/components/Info.tsx(2 hunks)src/index.tsx(2 hunks)src/lib/asp.ts(1 hunks)src/providers/fees.tsx(1 hunks)src/screens/Wallet/Receive/Amount.tsx(4 hunks)src/screens/Wallet/Receive/QrCode.tsx(2 hunks)src/screens/Wallet/Send/Details.tsx(4 hunks)src/screens/Wallet/Send/Form.tsx(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-07T14:41:35.991Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 106
File: src/lib/bolt11.ts:153-155
Timestamp: 2025-07-07T14:41:35.991Z
Learning: In src/lib/bolt11.ts, the code is copied from the original bitcoinjs/bolt11 library (https://github.com/bitcoinjs/bolt11/blob/master/payreq.js) and should be kept unchanged to maintain compatibility with the upstream source, even if there are potential improvements or syntax issues.
Applied to files:
src/screens/Wallet/Receive/Amount.tsx
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Applied to files:
src/screens/Wallet/Receive/Amount.tsxsrc/lib/asp.tssrc/screens/Wallet/Send/Details.tsx
🧬 Code graph analysis (7)
src/components/Info.tsx (4)
src/components/FlexCol.tsx (1)
FlexCol(15-40)src/components/FlexRow.tsx (1)
FlexRow(16-45)src/icons/Info.tsx (1)
InfoIcon(1-14)src/components/Text.tsx (1)
TextSecondary(91-97)
src/index.tsx (3)
src/providers/fees.tsx (1)
FeesProvider(19-66)src/providers/options.tsx (1)
OptionsProvider(127-162)src/providers/nudge.tsx (1)
NudgeProvider(18-50)
src/providers/fees.tsx (2)
src/lib/types.ts (1)
Vtxo(94-94)src/providers/asp.tsx (1)
AspContext(15-20)
src/screens/Wallet/Receive/Amount.tsx (2)
src/lib/format.ts (2)
prettyNumber(85-88)prettyAmount(27-34)src/components/Info.tsx (1)
InfoLine(32-41)
src/screens/Wallet/Send/Details.tsx (3)
src/providers/fees.tsx (1)
FeesContext(12-17)src/lib/constants.ts (1)
defaultFee(2-2)src/lib/asp.ts (2)
sendOffChain(190-192)collaborativeExitWithFees(68-99)
src/screens/Wallet/Receive/QrCode.tsx (1)
src/components/Info.tsx (1)
InfoLine(32-41)
src/screens/Wallet/Send/Form.tsx (2)
src/providers/fees.tsx (1)
FeesContext(12-17)src/components/Info.tsx (1)
InfoLine(32-41)
⏰ 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: test
Deploying wallet-signet with
|
| Latest commit: |
6eb71c5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://00aeba4f.wallet-23u.pages.dev |
| Branch Preview URL: | https://fees.wallet-23u.pages.dev |
Deploying wallet-mutinynet with
|
| Latest commit: |
646dcb2
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0200d344.arkade-wallet.pages.dev |
| Branch Preview URL: | https://fees.arkade-wallet.pages.dev |
Deploying wallet-bitcoin with
|
| Latest commit: |
6109946
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0dc09691.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://fees.wallet-bitcoin.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/screens/Wallet/Send/Form.tsx(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Applied to files:
src/screens/Wallet/Send/Form.tsx
🧬 Code graph analysis (1)
src/screens/Wallet/Send/Form.tsx (2)
src/providers/fees.tsx (1)
FeesContext(12-17)src/components/Info.tsx (1)
InfoLine(32-41)
⏰ 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: test
🔇 Additional comments (5)
src/screens/Wallet/Send/Form.tsx (5)
42-43: LGTM!The new imports for
FeesContextandInfoLineare properly integrated and used throughout the component.
48-48: LGTM!The
calcOnchainOutputFeefunction is correctly extracted fromFeesContextand used appropriately for fee calculations.
60-60: LGTM!The
deductFromAmountstate variable is properly initialized and integrated with the fee deduction flow.
291-298: LGTM! Previous issue addressed.The fee deduction logic correctly guards against negative amounts and properly clamps the satoshis value. This implementation addresses the concern raised in the previous review about potential negative values when fees exceed available balance.
395-395: LGTM!The
InfoLinecomponent provides clear user feedback when fees will be deducted from the send amount. The orange color and placement are appropriate for this warning.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Andrew Camilleri <evilkukka@gmail.com>
| calcOnchainOutputFee: () => 0, | ||
| }) | ||
|
|
||
| export const FeesProvider = ({ children }: { children: ReactNode }) => { |
There was a problem hiding this comment.
We should probably create a helper for computing fees inside ts-sdk (later)
Signed-off-by: Andrew Camilleri <evilkukka@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/screens/Wallet/Send/Form.tsx (2)
92-96: Consider addingsvcWalletto the available-balance effect dependencies.The effect currently reruns only when
balancechanges. If thesvcWalletinstance can change whilebalanceremains the same,availableBalancewould stay tied to the old wallet. AddingsvcWalletto the dependency array would make this more robust, and you might also optionally handle errors fromgetBalance()to avoid silent failures.- useEffect(() => { - if (!svcWallet) return - svcWallet.getBalance().then((bal) => setAvailableBalance(bal.available)) - }, [balance]) + useEffect(() => { + if (!svcWallet) return + svcWallet + .getBalance() + .then((bal) => setAvailableBalance(bal.available)) + .catch(() => setAvailableBalance(0)) + }, [balance, svcWallet])
270-276: On-chain fee deduction is now safe; consider resetting for LNURL to keep UI consistent.The new logic looks good overall:
- The
useEffectat lines 270-276 ensuresdeductFromAmountis only applied for pure on-chain sends (addressset, noarkAddress/invoice), fixing the earlier risk of applying on-chain fee deduction to ark/invoice sends.- In
handleContinue, thedeductFromAmountbranch now computesfee, derivesspendable = availableBalance - fee, bails out whenspendable <= 0, and clamps the amount withMath.min(satoshis, spendable), which prevents negative or over-spend satoshis and addresses the previous critical issue.- The
InfoLineat line 411 clearly surfaces when fees will be deducted from the amount.One small edge case: when switching from an on-chain address to an LNURL,
parseRecipientsetslnUrlbut doesn’t clearsendInfo.address, so the guardif (!sendInfo.address || sendInfo.arkAddress || sendInfo.invoice)stays false.deductFromAmountcan remaintrue, so the InfoLine still shows even though the LNURL branch inhandleContinuewill run instead of the fee-deduction branch. Behavior is correct, but the UI hint can become misleading.You can treat LNURL as “non-onchain” in the effect to keep
deductFromAmountand the InfoLine aligned with the actual send path:- useEffect(() => { - if (!sendInfo.address || sendInfo.arkAddress || sendInfo.invoice) { + useEffect(() => { + if (!sendInfo.address || sendInfo.arkAddress || sendInfo.invoice || sendInfo.lnUrl) { setDeductFromAmount(false) return } setDeductFromAmount(satoshis + calcOnchainOutputFee() > availableBalance) - }, [availableBalance, satoshis, sendInfo.address, sendInfo.arkAddress, sendInfo.invoice]) + }, [availableBalance, satoshis, sendInfo.address, sendInfo.arkAddress, sendInfo.invoice, sendInfo.lnUrl])This keeps the fee-deduction indicator strictly tied to actual on-chain sends.
Also applies to: 308-315, 411-411
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/index.tsx(2 hunks)src/lib/asp.ts(1 hunks)src/screens/Wallet/Receive/Amount.tsx(4 hunks)src/screens/Wallet/Send/Form.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/screens/Wallet/Receive/Amount.tsx
- src/index.tsx
- src/lib/asp.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Applied to files:
src/screens/Wallet/Send/Form.tsx
🧬 Code graph analysis (1)
src/screens/Wallet/Send/Form.tsx (2)
src/providers/fees.tsx (1)
FeesContext(12-17)src/components/Info.tsx (1)
InfoLine(32-41)
⏰ 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: test
- GitHub Check: Cloudflare Pages: wallet-bitcoin
🔇 Additional comments (1)
src/screens/Wallet/Send/Form.tsx (1)
42-43: FeesContext anddeductFromAmountwiring looks solid.Pulling
calcOnchainOutputFeefromFeesContextand introducing the localdeductFromAmountflag integrates cleanly with the existing state, and the defaultfalseinitialization is a safe starting point. No issues here.Also applies to: 48-60
Implements fees on onchain outputs.
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.