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

Consolidate fee_amount and amount_needed #662

Conversation

csralvall
Copy link
Contributor

Description

Before this commit fee_amount and amount_needed were passed as independent
parameters. From the perspective of coin selection algorithms, they are always
used jointly for the same purpose, to create a coin selection with a total
effective value greater than it's summed values.

This commit removes the abstraction that the use of the two parameter
introduced by consolidating both into a single parameter, target_amount, who
carries their values added up.

Resolves: #641

Notes to the reviewers

I just updated old tests and didn't create new ones because almost all changes
are renames and "logic changes" (like the addition of the selection fee) are
tested in the modified tests.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature
  • I've updated CHANGELOG.md

Bugfixes:

  • This pull request breaks the existing API
  • I'm linking the issue being fixed by this PR

Copy link
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

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

Concept ACK, a small comment :)

src/wallet/coin_selection.rs Outdated Show resolved Hide resolved
@evanlinjin
Copy link
Member

Concept ACK for me.

This PR is rather straight forward, and for the context on my work with multi-descriptor support (#486), it would be good to merge this PR soon (since I will also be modifying the wallet/{mod|coin_selection}.rs files).

Other than the concerns mentioned in #662 (comment), I think this PR is good to merge.

@csralvall csralvall force-pushed the 641_consolidate_amount_needed_and_fee_amount branch from 641718b to 84317d4 Compare July 18, 2022 19:49
@danielabrozzoni
Copy link
Member

Please rebase to fix the conflicts, then I'll ACK :)

@csralvall csralvall force-pushed the 641_consolidate_amount_needed_and_fee_amount branch 4 times, most recently from 40126f5 to 6493bbf Compare July 26, 2022 18:08
@danielabrozzoni
Copy link
Member

Code review ACK 6493bbf

@csralvall csralvall force-pushed the 641_consolidate_amount_needed_and_fee_amount branch 2 times, most recently from 01d195a to 0f83d47 Compare August 1, 2022 00:51
@csralvall
Copy link
Contributor Author

Rebased with master.

  • I've added an extra line to the CHANGELOG.md about the new meaning of
    fee_amount in CoinSelectionResult which wasn't explicitly described before.
  • I've removed the Resolves: ... part of the commit as I've realized it creates
    a lot of automatic logs in related PRs and Issues, each time we add new changes,
    duplicating info and making issue navigation cumbersome.
    At the end, it's the maintainer who will put that line in the merge commit.
    Maybe we can use some format to avoid the trigger and at the same time suggest
    the fixed issues to the maintainer.
    If you think it's not a serious issue, I can continue writing commits like
    before.

@csralvall csralvall force-pushed the 641_consolidate_amount_needed_and_fee_amount branch 2 times, most recently from e9b51ef to 6eb299b Compare August 2, 2022 18:26
Copy link
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

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

tACK 6eb299b

Before this commit `fee_amount` and `amount_needed` were passed as independent
parameters. From the perspective of coin selection algorithms, they are always
used jointly for the same purpose, to create a coin selection with a total
effective value greater than it's summed values.

This commit removes the abstraction that the use of the two parameter
introduced by consolidating both into a single parameter, `target_amount`, who
carries their values added up.
@danielabrozzoni danielabrozzoni force-pushed the 641_consolidate_amount_needed_and_fee_amount branch from 6eb299b to e8df3d2 Compare August 3, 2022 10:21
@danielabrozzoni
Copy link
Member

I rebased by myself, now it's ready for merge

re-ACK e8df3d2 - I tested with the fuzzer, run it for 13,000,000 iterations, couldn't find any crash :)

@afilini afilini merged commit 9c0a769 into bitcoindevkit:master Aug 3, 2022
@csralvall csralvall deleted the 641_consolidate_amount_needed_and_fee_amount branch August 18, 2022 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Consolidate amount_needed and fee_amount in one parameter
5 participants