Skip to content

Add allow_dust method to TxBuilder#689

Merged
danielabrozzoni merged 1 commit intobitcoindevkit:masterfrom
terror:master
Aug 31, 2022
Merged

Add allow_dust method to TxBuilder#689
danielabrozzoni merged 1 commit intobitcoindevkit:masterfrom
terror:master

Conversation

@terror
Copy link
Copy Markdown
Contributor

@terror terror commented Jul 29, 2022

We needed this for testing our wallet with dust, does this look like a reasonable feature? If so, I'll go ahead and add a test, update the changelog, etc.

@danielabrozzoni
Copy link
Copy Markdown
Member

Concept ACK! Transactions with below-dust ouputs are non-standard, but still valid, so I think BDK should be able to create them. It could also be useful in some very rare cases, like for creating a notification output to an address.

Comment thread src/wallet/tx_builder.rs
@terror terror marked this pull request as ready for review August 4, 2022 17:09
@terror terror requested a review from danielabrozzoni August 4, 2022 22:53
@danielabrozzoni
Copy link
Copy Markdown
Member

The code looks good to me, can you please squash all the commits into the first one?

@danielabrozzoni
Copy link
Copy Markdown
Member

Mh... There are still two commits, and the latest one's message is a bit weird. I'd use interactive git rebases to fix (https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History) - I would reword the first commit to change the message, and drop entirely the second commit

@terror
Copy link
Copy Markdown
Contributor Author

terror commented Aug 12, 2022

Mh... There are still two commits, and the latest one's message is a bit weird. I'd use interactive git rebases to fix (https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History) - I would reword the first commit to change the message, and drop entirely the second commit

@danielabrozzoni Should be good to go now. PTAL

Copy link
Copy Markdown
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.

utACK 61ad455

Copy link
Copy Markdown
Contributor

@vladimirfomene vladimirfomene left a comment

Choose a reason for hiding this comment

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

Tested ACK

Copy link
Copy Markdown
Contributor

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

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

Review ACK.. Code looks good to me.. A small test at this stage would be really nice..

@danielabrozzoni
Copy link
Copy Markdown
Member

@rajarshimaitra do you have in mind any other additional test? There's already a test_allow_dust_limit, I can't think of any other that could be useful

Copy link
Copy Markdown
Contributor

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

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

tACK 61ad455

Huh.. I have no idea how I missed the test in last pass.. Thanks @danielabrozzoni yup thats all, we don't need anything else..

This looks good to go..

Comment thread CHANGELOG.md Outdated
- Change the meaning of the `fee_amount` field inside `CoinSelectionResult`: from now on the `fee_amount` will represent only the fees asociated with the utxos in the `selected` field of `CoinSelectionResult`.
- New `RpcBlockchain` implementation with various fixes.
- Return balance in separate categories, namely `confirmed`, `trusted_pending`, `untrusted_pending` & `immature`.
- Add `allow_dust` method on `TxBuilder`.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You should rebase and move this under the new unreleased section

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hey @terror, sorry for the ping, but the feature freeze is tomorrow, which means that if you fix this comment by tomorrow we can merge the PR and release allow_dust as a part of bdk 0.22! :)

Copy link
Copy Markdown
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.

ACK 257a687

@danielabrozzoni
Copy link
Copy Markdown
Member

Oh no, I just noticed... @terror can you sign your commit? Otherwise I can't merge. If you prefer, I can sign it with my key.

Add TxBuilder::allow_dust() that skips checking the dust limit
@danielabrozzoni
Copy link
Copy Markdown
Member

tACK bfd7b2f

@danielabrozzoni danielabrozzoni merged commit 4fbd852 into bitcoindevkit:master Aug 31, 2022
@afilini afilini mentioned this pull request Sep 8, 2022
21 tasks
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.

5 participants