Skip to content

Remove TxBuilder allow_shrinking() and unneeded context param#1386

Merged
notmandatory merged 1 commit intobitcoindevkit:masterfrom
notmandatory:fix/remove_allow_shrinking
May 23, 2024
Merged

Remove TxBuilder allow_shrinking() and unneeded context param#1386
notmandatory merged 1 commit intobitcoindevkit:masterfrom
notmandatory:fix/remove_allow_shrinking

Conversation

@notmandatory
Copy link
Member

Description

Remove wallet::TxBuilder::allow_shrinking() and unneeded TxBuilder context param.

Fixes #1374

Notes to the reviewers

The allow_shrinking function was the only one using the TxBuilder FeeBump context and it's useful to have CreateTx context functions available for building FeeBump transactions, see updated tests.

Changelog notice

Changed

  • Removed TxBuilder::allow_shrinking() function.

Checklists

All Submissions:

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

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@notmandatory notmandatory self-assigned this Mar 25, 2024
@notmandatory notmandatory added bug Something isn't working module-wallet api A breaking API change labels Mar 25, 2024
@notmandatory notmandatory added this to the 1.0.0-alpha milestone Mar 25, 2024
@notmandatory notmandatory force-pushed the fix/remove_allow_shrinking branch from 7abec98 to b51fa22 Compare March 25, 2024 01:57
@notmandatory
Copy link
Member Author

notmandatory commented Mar 25, 2024

@LLFourn I did a little more than just remove TxBuilder::allow_shrinking in this PR. If you think it's too much to remove the TxBuilder context I can put it back in, but then I'll need to either remove the two tests I updated or add the functions I needed to work for either context.

@notmandatory notmandatory force-pushed the fix/remove_allow_shrinking branch 2 times, most recently from 25a81fa to 4073c43 Compare March 25, 2024 02:38
@ValuedMammal
Copy link
Collaborator

LGTM

Conceptually, I think allow_shrinking is still a valuable method to have in the api for TxBuilder, but for the moment it's worth reconsidering how this will work in relation to RBF, coin selection, etc.

@notmandatory notmandatory force-pushed the fix/remove_allow_shrinking branch 3 times, most recently from e35839a to 1ab9204 Compare April 12, 2024 00:34
@ValuedMammal
Copy link
Collaborator

ACK 1ab9204

Copy link
Collaborator

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

ACK 1ab9204

@notmandatory notmandatory force-pushed the fix/remove_allow_shrinking branch 2 times, most recently from f3dcb60 to 97e2508 Compare May 17, 2024 03:04
Copy link
Collaborator

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

ACK 97e2508

@ValuedMammal
Copy link
Collaborator

ACK 97e2508

@notmandatory notmandatory force-pushed the fix/remove_allow_shrinking branch from 97e2508 to 096b8ef Compare May 23, 2024 14:46
@notmandatory notmandatory merged commit 2a055de into bitcoindevkit:master May 23, 2024
@notmandatory notmandatory mentioned this pull request May 23, 2024
33 tasks
@notmandatory notmandatory deleted the fix/remove_allow_shrinking branch May 26, 2025 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api A breaking API change bug Something isn't working module-wallet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

RBF API is awkward (remove allow_shrinking)

3 participants