Improve dust txs when sending max amount (#4979) #5472
Conversation
WalkthroughThe changes improve the logic for local coin selection in Bitcoin transactions to avoid leaving dust UTXOs when sending all available funds. Updates were made across multiple files to refine the logic for handling transactions, fees, and UTXO selection. Additional tests were also introduced to ensure the correctness of these modifications. Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
| }, | ||
| ]; | ||
| }[] = | ||
| changeAmount <= BTC_P2WPKH_DUST_AMOUNT |
There was a problem hiding this comment.
Is it ok to leave the dust amount in the wallet? @fbwoolf
There was a problem hiding this comment.
@alter-eggo, thoughts on these changes? It looks like you've made the most recent changes to this code?
|
|
||
| const determineUtxosArgs = { | ||
| amount: satAmount, | ||
| const determineUtxosArgs: DetermineUtxosForSpendArgs = { |
There was a problem hiding this comment.
satAmount is not used in determineUtxosForSpendArgs. Is the amount provided for other reasons? Is it ok to remove it? Is this the source of this issue?
There was a problem hiding this comment.
yeah, it's ok to remove it since we provide amounts in recipient model
|
@alter-eggo it might make sense for you to review these changes? I think this touches code you've worked on recently adding multiple recipients? |
|
@fbwoolf yeah will take a look, @kyranjamie and me refactored it not so long ago |
|
Thanks a lot @friedger, great improvement! Taking a look at implementation now, may tidy commits |
|
Rewrote logic a little to make more readable, but otherwise think this is good. @friedger to verify changed logic. |
![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
Outside diff range and nitpick comments (6)
src/app/components/bitcoin-custom-fee/hooks/use-bitcoin-custom-fee.tsx (1)
Line range hint
23-47: Refactor to improve readability and maintainability.The
useBitcoinCustomFeefunction could be refactored for better readability by separating the logic into smaller, more manageable functions. This would make the code easier to understand and maintain, especially as it grows in complexity.export function useBitcoinCustomFee({ isSendingMax, recipients }: UseBitcoinCustomFeeArgs) { const { data: utxos = [] } = useCurrentNativeSegwitUtxos(); const btcMarketData = useCryptoCurrencyMarketDataMeanAverage('BTC'); const calculateFee = useCallback((feeRate: number) => { if (!feeRate || !utxos.length) return { fee: 0, fiatFeeValue: '' }; const determineUtxosArgs: DetermineUtxosForSpendArgs = { recipients, utxos, feeRate, }; const { fee } = isSendingMax ? determineUtxosForSpendAll(determineUtxosArgs) : determineUtxosForSpend(determineUtxosArgs); return { fee, fiatFeeValue: `~ ${i18nFormatCurrency( baseCurrencyAmountInQuote(createMoney(Math.ceil(fee), 'BTC'), btcMarketData) )}`, }; }, [utxos, isSendingMax, recipients, btcMarketData]); return calculateFee; }src/app/components/bitcoin-custom-fee/bitcoin-custom-fee-input.tsx (1)
Line range hint
3-3: Remove unused import to clean up the code.- import { createMoney, satToBtc } from '@leather-wallet/utils'; + import { satToBtc } from '@leather-wallet/utils';src/app/common/transactions/bitcoin/coinselect/local-coin-selection.ts (1)
Line range hint
37-40: ReplaceforEachwithfor...offor better performance and readability.- recipients.forEach(recipient => { + for (const recipient of recipients) { if (!validate(recipient.address)) throw new Error('Cannot calculate spend of invalid address type'); });Also applies to: 66-69
src/app/components/bitcoin-custom-fee/bitcoin-custom-fee.tsx (1)
Line range hint
1-1: Optimize imports by removing unused ones.- import { Dispatch, SetStateAction, useCallback, useRef } from 'react'; + import { Dispatch, SetStateAction, useCallback, useRef } from 'react'; // Assuming these are used elsewhere in the file - import { createMoney } from '@leather-wallet/utils'; - import { SendCryptoAssetSelectors } from '@tests/selectors/send.selectors'; - import { Form, Formik } from 'formik'; - import { Stack, styled } from 'leather-styles/jsx'; - import * as yup from 'yup';Also applies to: 8-9, 15-16
src/app/features/dialogs/increase-fee-dialog/increase-btc-fee-dialog.tsx (1)
Line range hint
7-8: Remove unused imports to clean up the code.- import { btcToSat, createMoney, formatMoney } from '@leather-wallet/utils'; + import { btcToSat, formatMoney } from '@leather-wallet/utils'; // Assuming createMoney is not used elsewhere in the filesrc/app/common/transactions/bitcoin/coinselect/local-coin-selection.spec.ts (1)
Line range hint
26-26: Specify a more appropriate type instead ofany.In several places in the test file,
anyis used where a more specific type could be applied. This would improve type safety and code clarity.- const demoUtxos = [{ value: 8200 }, { value: 8490 }, ...] as any; + const demoUtxos: UtxoResponseItem[] = [{ value: 8200 }, { value: 8490 }, ...];Also applies to: 36-36, 52-52, 79-79, 155-155, 183-183, 191-191, 224-224
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- src/app/common/transactions/bitcoin/coinselect/local-coin-selection.spec.ts (2 hunks)
- src/app/common/transactions/bitcoin/coinselect/local-coin-selection.ts (3 hunks)
- src/app/components/bitcoin-custom-fee/bitcoin-custom-fee-input.tsx (3 hunks)
- src/app/components/bitcoin-custom-fee/bitcoin-custom-fee.tsx (3 hunks)
- src/app/components/bitcoin-custom-fee/hooks/use-bitcoin-custom-fee.tsx (2 hunks)
- src/app/features/dialogs/increase-fee-dialog/increase-btc-fee-dialog.tsx (1 hunks)
Additional context used
GitHub Check: typecheck
src/app/components/bitcoin-custom-fee/bitcoin-custom-fee-input.tsx
[failure] 3-3:
'createMoney' is declared but its value is never read.src/app/components/bitcoin-custom-fee/bitcoin-custom-fee.tsx
[failure] 3-3:
'createMoney' is declared but its value is never read.
Biome
src/app/common/transactions/bitcoin/coinselect/local-coin-selection.ts
[error] 37-40: Prefer for...of instead of forEach.
[error] 66-69: Prefer for...of instead of forEach.
src/app/components/bitcoin-custom-fee/bitcoin-custom-fee.tsx
[error] 1-1: Some named imports are only used as types.
[error] 8-9: All these imports are only used as types.
[error] 15-16: All these imports are only used as types.
src/app/features/dialogs/increase-fee-dialog/increase-btc-fee-dialog.tsx
[error] 7-8: All these imports are only used as types.
src/app/common/transactions/bitcoin/coinselect/local-coin-selection.spec.ts
[error] 26-26: Unexpected any. Specify a different type.
[error] 36-36: Unexpected any. Specify a different type.
[error] 52-52: Unexpected any. Specify a different type.
[error] 79-79: Unexpected any. Specify a different type.
[error] 155-155: Unexpected any. Specify a different type.
[error] 183-183: Unexpected any. Specify a different type.
[error] 191-191: Unexpected any. Specify a different type.
[error] 224-224: Unexpected any. Specify a different type.
|
@alter-eggo want to review this PR before we merge in? @kyranjamie has already approved |
|
@markmhendrickson from the code side - lgtm |
|
@alter-eggo to fix conflicts and merge |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
src/app/features/dialogs/increase-fee-dialog/increase-btc-fee-dialog.tsx (2)
Line range hint
1-3: Remove unused imports.The
btcToSatandcreateMoneyimports are no longer used in the file.-import { btcToSat, createMoney, formatMoney } from '@leather.io/utils'; +import { formatMoney } from '@leather.io/utils';
[!TIP]
Codebase VerificationEnsure
txis defined.The
txvariable can beundefinedasuseLocationStateWithCachedoes not guarantee a defined value without a default. Ensuretxis checked forundefinedbefore use.
src/app/features/dialogs/increase-fee-dialog/increase-btc-fee-dialog.tsx: Line 93Analysis chain
Line range hint
14-15: Ensuretxis defined.The
txvariable is used before the null check. Ensure thatuseLocationStateWithCacheguaranteestxis defined.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `useLocationStateWithCache` guarantees `tx` is defined before use. # Test: Search for the `useLocationStateWithCache` implementation and check its return type. rg --type tsx 'useLocationStateWithCache'Length of output: 69
Script:
#!/bin/bash # Description: Verify that `useLocationStateWithCache` guarantees `tx` is defined before use. # Test: Search for the `useLocationStateWithCache` implementation and check its return type. rg --type ts 'useLocationStateWithCache'Length of output: 3489
Script:
#!/bin/bash # Description: Examine the implementation of `useLocationStateWithCache` and its usage in `increase-btc-fee-dialog.tsx`. # Display the implementation of `useLocationStateWithCache`. rg --type ts 'export function useLocationStateWithCache' -A 10 # Display the relevant lines around the usage of `useLocationStateWithCache` in `increase-btc-fee-dialog.tsx`. rg 'useLocationStateWithCache' -A 10 src/app/features/dialogs/increase-fee-dialog/increase-btc-fee-dialog.tsxLength of output: 2695
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- src/app/common/transactions/bitcoin/coinselect/local-coin-selection.spec.ts (2 hunks)
- src/app/common/transactions/bitcoin/coinselect/local-coin-selection.ts (3 hunks)
- src/app/components/bitcoin-custom-fee/bitcoin-custom-fee-input.tsx (4 hunks)
- src/app/components/bitcoin-custom-fee/bitcoin-custom-fee.tsx (4 hunks)
- src/app/components/bitcoin-custom-fee/hooks/use-bitcoin-custom-fee.tsx (2 hunks)
- src/app/features/dialogs/increase-fee-dialog/increase-btc-fee-dialog.tsx (1 hunks)
Additional context used
Learnings (1)
src/app/features/dialogs/increase-fee-dialog/increase-btc-fee-dialog.tsx (1)
Learnt from: pete-watters PR: leather-io/extension#5536 File: src/app/features/dialogs/transaction-action-dialog/stx-transaction-action-dialog.tsx:40-40 Timestamp: 2024-07-02T12:35:48.552Z Learning: The `FeeComponent` prop in `stx-transaction-action-dialog.tsx` should be typed as `React.ComponentType<{ currentFee: number }>` to ensure type safety, based on the `IncreaseFeeField` component's expected props.
Additional comments not posted (13)
src/app/components/bitcoin-custom-fee/hooks/use-bitcoin-custom-fee.tsx (2)
9-9: LGTM!The import of
DetermineUtxosForSpendArgsand related functions is consistent with the removal of theamountparameter fromUseBitcoinCustomFeeArgs.
Line range hint
22-46:
LGTM!The function
useBitcoinCustomFeehas been updated to remove theamountparameter fromUseBitcoinCustomFeeArgs. The changes are consistent and maintain the intended functionality.src/app/components/bitcoin-custom-fee/bitcoin-custom-fee-input.tsx (1)
7-7: LGTM!The import of
satToBtcfrom@leather.io/utilsis consistent with the removal of theamountparameter from thePropsinterface.src/app/common/transactions/bitcoin/coinselect/local-coin-selection.ts (5)
4-4: LGTM!The import of
BTC_P2WPKH_DUST_AMOUNTfrom@leather.io/constantsis necessary for handling dust amounts in transactions.
18-21: LGTM!The addition of the
Outputinterface is necessary for defining the structure of transaction outputs.
22-22: LGTM!The addition of the
DetermineUtxosForSpendArgsinterface is necessary for defining the arguments for thedetermineUtxosForSpendfunction.
110-121: LGTM!The logic for handling change amounts and outputs ensures that change amounts are handled correctly, preventing dust amounts from remaining.
122-127: LGTM!The logic for creating the
outputsarray ensures that the array includes both recipient outputs and change outputs.src/app/components/bitcoin-custom-fee/bitcoin-custom-fee.tsx (1)
Line range hint
1-1:
LGTM!The import of necessary modules and components is consistent with the removal of the
amountparameter from theBitcoinCustomFeePropsinterface.src/app/features/dialogs/increase-fee-dialog/increase-btc-fee-dialog.tsx (1)
Line range hint
41-41: Ensurerecipientsis correctly initialized.The
recipientsarray usescreateMoneywhich is imported but not used elsewhere. Verify thatcreateMoneyis necessary and correctly used.Verification successful
Verification Successful:
createMoneyis necessary and correctly used.
- The
createMoneyfunction is extensively used throughout the codebase, including the file under review (increase-btc-fee-dialog.tsx).- The initialization of
recipientsusingcreateMoneyis appropriate and necessary.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `createMoney` is necessary and correctly used in `recipients`. # Test: Search for `createMoney` usage in the codebase to ensure it is necessary. rg --type tsx 'createMoney'Length of output: 55
Script:
#!/bin/bash # Description: Verify that `createMoney` is necessary and correctly used in `recipients`. # Test: Search for `createMoney` usage in TypeScript and TSX files to ensure it is necessary. rg --type ts,tsx 'createMoney'Length of output: 61
Script:
#!/bin/bash # Description: Verify that `createMoney` is necessary and correctly used in `recipients`. # Test: Search for `createMoney` usage in TypeScript and TSX files to ensure it is necessary. rg --type-add 'ts:*.{ts,tsx}' --type ts 'createMoney'Length of output: 17130
src/app/common/transactions/bitcoin/coinselect/local-coin-selection.spec.ts (3)
4-5: Verify new imports usage.Ensure that
filterUneconomicalUtxosandgetSizeInfoare necessary and correctly used in the tests.Verification successful
Verified new imports usage.
The functions
filterUneconomicalUtxosandgetSizeInfoare necessary and correctly used in thelocal-coin-selection.spec.tsfile.
filterUneconomicalUtxosis used on line 14.getSizeInfois used on line 17.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `filterUneconomicalUtxos` and `getSizeInfo` are necessary and correctly used in the tests. # Test: Search for `filterUneconomicalUtxos` and `getSizeInfo` usage in the codebase. rg --type ts 'filterUneconomicalUtxos' rg --type ts 'getSizeInfo'Length of output: 2635
172-202: Ensure test coverage for dust UTXOs.The test
that spending all economical spendable utxos does not result in dust utxosensures that no dust UTXOs are created. Verify the logic and edge cases.
204-231: Ensure test coverage for sendMax scenario.The test
that spending all utxos with sendMax does not result in dust utxosensures that no dust UTXOs are created when sending the maximum amount. Verify the logic and edge cases.Verification successful
Ensure test coverage for sendMax scenario.
The test in
local-coin-selection.spec.tsensures that no dust UTXOs are created when sending the maximum amount. The current test covers thesendMaxscenario adequately by verifying the absence of dust UTXOs and checking the fee calculation. No additional edge cases were found in other test files.
src/app/common/transactions/bitcoin/coinselect/local-coin-selection.spec.ts: Test forsendMaxscenario is comprehensive.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the test for sendMax scenario comprehensively covers the scenarios and edge cases. # Test: Search for sendMax handling in the codebase to ensure comprehensive test coverage. rg --type ts 'sendMax'Length of output: 4697
Script:
#!/bin/bash # Extract and analyze test cases related to sendMax from the identified test files. # Search for test cases in all spec files that include 'sendMax'. rg --type ts --glob '*spec.ts' 'sendMax' -A 10Length of output: 2093
This PR
Summary by CodeRabbit
New Features
Refactor
Bug Fixes