fix: rpc send transfer recipient choose fee #5238
Merged
Conversation
This file contains 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
alter-eggo
force-pushed
the
fix/send-transfer-recipient
branch
from
0c65366
to
a9ac595
Compare
|
Thanks @alter-eggo, think we should include in this PR:
|
alter-eggo
force-pushed
the
fix/send-transfer-recipient
branch
from
04e11d2
to
e387351
Compare
kyranjamie
reviewed
Apr 16, 2024
| (value, context) => { | ||
| const contextOptions = context.options as any; | ||
| const network = contextOptions.from[1].value.network as BitcoinNetworkModes; | ||
| return btcAddressNetworkValidator(network).isValidSync(value); |
There was a problem hiding this comment.
magic number, what's 1?
There was a problem hiding this comment.
seems, there is no other ways to get network here
kyranjamie
reviewed
Apr 16, 2024
| {} as Record<AddressType, number> | ||
| ); | ||
|
|
||
| const outputsData = (Object.keys(outputTypesLengthMap) as AddressType[]).map(v => { |
There was a problem hiding this comment.
what is v? please name fully
There was a problem hiding this comment.
renamed to outputAddressType
kyranjamie
reviewed
Apr 16, 2024
| (acc: Record<AddressType, number>, outputType) => { | ||
| // we add 1 output for change address if not sending max | ||
| if (!acc['p2wpkh'] && !isSendMax) { | ||
| acc['p2wpkh'] = 1; |
There was a problem hiding this comment.
default output length value
kyranjamie
reviewed
Apr 16, 2024
| // Fee has already been deducted from the amount with send all | ||
| const outputs = recipients.map(({ address, amount }) => ({ value: BigInt(amount), address })); | ||
|
|
||
| const fee = Math.ceil(sizeInfo.txVBytes * feeRate); |
There was a problem hiding this comment.
use BigNumber for math
There was a problem hiding this comment.
most of this fn is copy-pasted, I can refactor math there later?
alter-eggo
force-pushed
the
fix/send-transfer-recipient
branch
from
e387351
to
84f2919
Compare
kyranjamie
approved these changes
Apr 16, 2024
There was a problem hiding this comment.
Thanks @alter-eggo. Let's follow up with tests and refactor as soon as this is shipped!
alter-eggo
force-pushed
the
fix/send-transfer-recipient
branch
from
84f2919
to
a739014
Compare
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.
This pr fixes bug with rpc send transfer fees selection. Here I just duplicated components in order not to affect other flows. In next pr I will refactor it