Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix inaccurate fee estimations #5303

Merged
merged 2 commits into from
Apr 30, 2024
Merged

Fix inaccurate fee estimations #5303

merged 2 commits into from
Apr 30, 2024

Conversation

kyranjamie
Copy link
Collaborator

@kyranjamie kyranjamie commented Apr 24, 2024

Try out Leather build aab07aeExtension build, Test report, Storybook, Chromatic

This PR fixes the issues relating to fee estimations describe in Notion.

The previously calculation lead to a situation whereby some estimations would be counting one fewer input than actually used, thus vastly decreasing the fee rate.

Open to feedback how we can extend the tests of the coin selection.

@fbwoolf can you please QA this to double check

2024-04-25-000182.mp4

@kyranjamie kyranjamie force-pushed the fix/fee-estimations branch 4 times, most recently from 75746d8 to 2015df6 Compare April 24, 2024 14:52
@kyranjamie kyranjamie force-pushed the fix/fee-estimations branch from 2015df6 to aab07ae Compare April 25, 2024 08:14
@markmhendrickson markmhendrickson linked an issue Apr 25, 2024 that may be closed by this pull request
@kyranjamie kyranjamie marked this pull request as ready for review April 25, 2024 13:37
@fbwoolf
Copy link
Contributor

fbwoolf commented Apr 25, 2024

cc @314159265359879 to help QA here?

Copy link
Contributor

@fbwoolf fbwoolf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good here, and I'm not running into any bugs testing it. 👍

@alter-eggo
Copy link
Contributor

alter-eggo commented Apr 26, 2024

@kyranjamie I believe we need to fix calc in selectTaprootInscriptionTransferCoins as well?

@alter-eggo
Copy link
Contributor

alter-eggo commented Apr 26, 2024

checked btc send with 3ns inputs and 2ns outputs and it worked for me 👍

@kyranjamie
Copy link
Collaborator Author

@kyranjamie I believe we need to fix calc in selectTaprootInscriptionTransferCoins as well?

Eurgh, this needs refactoring. Though I think it doesn't have same problem, as the other with the for loop.

// From the address of the recipient, we infer the output type
[outputAddressTypeWithFallback + '_output_count']: outputLength,
[outputAddressTypeWithFallback + '_output_count']: outputCount,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as I understand we can skip adding _outputs_counts for different output types? 🤔
like it is now in getSizeInfoMultipleRecipients

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not entirely sure what you mean by this @alter-eggo ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You do this as well in the multiple recipients code acc[type + '_output_count'] = count;

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open for changes if needed here, but merging to not hold up fixes

@kyranjamie kyranjamie added this pull request to the merge queue Apr 30, 2024
Merged via the queue into dev with commit d12bb82 Apr 30, 2024
28 checks passed
@kyranjamie kyranjamie deleted the fix/fee-estimations branch April 30, 2024 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fee set in Leather is not what it is transmitted with
3 participants