Change coin_selection and DescriptorExt::dust_value to use Amount type#1763
Merged
notmandatory merged 3 commits intobitcoindevkit:masterfrom Dec 11, 2024
Merged
Conversation
3504440 to
5a0e19d
Compare
Member
Author
|
Thanks to @thesimplekid for bringing to my attention that native rust integer types can silently roll-over. Even though this is very unlikely to happen, using The rest of the code base is already updated to use |
rustaceanrob
reviewed
Dec 9, 2024
ValuedMammal
reviewed
Dec 9, 2024
Member
Author
|
Just as an FYI, I did a little test to confirm that rust integer underflow does wrap and isn't caught as a static check if the values aren't known at compile time. This only applies when built in release mode, in debug mode it panics. #[test]
#[cfg(not(debug_assertions))]
fn test_integer_underflow() {
use rand::Rng;
let num = rand::thread_rng().gen_range(11..21);
let ten: u64 = 10;
let underflow = ten - num;
// println!("10 - {} => underflow {}", num, underflow);
assert!(underflow > 0)
} |
5a0e19d to
d2f49a7
Compare
3 tasks
d2f49a7 to
7f1e4d2
Compare
7f1e4d2 to
2198e85
Compare
…ternally Using named types make the API and internal code easier to read and reason about since it makes it clear that the values are bitcoin amounts. Also to create these types the units (ie .from_sat()) must be specified. Using Amount and SignedAmount also makes internal code safer against overflow errors. In particular because these types will panic if an amount overflow occurs. Using u64/i64 on the otherhand can silently rollover. See: https://doc.rust-lang.org/book/ch03-02-data-types.html#integer-overflow
f1ed237 to
eeb5372
Compare
eeb5372 to
2a9eeed
Compare
notmandatory
added a commit
that referenced
this pull request
Dec 23, 2024
…te_tx a2f7a8b refactor(wallet): cleanup and remove unused code in create_tx (Steve Myers) Pull request description: ### Description Cleanup and remove unused code in `Wallet::create_tx`, this was noticed during review of #1763. See: #1763 (comment) fixes #1710 ### Notes to the reviewers In addition to removing the unneeded assignments to `fee_amount` and `received` I also refactored creation of the change output to be an `if let` instead of `match` statement since it only needs to do something if there is `Excess::Change`. I should have done this cleanup as part of #1048. ### Changelog notice None, only internal cleanup. ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing ACKs for top commit: ValuedMammal: reACK a2f7a8b Tree-SHA512: 64e5895ff3dc11f71c48b6c436d5c812504d0a24e92f1fdf451936f372d95ccdd8d89e5ac634a041bdee0a4836182f05127864ed744d560c9f8ec560e092c559
This file contains hidden or 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
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.
Description
refactor(coin_selection)!: use Amount and SignedAmount for API and internally
refactor(chain)!: use Amount for DescriptorExt::dust_value()
Using named types make the API and internal code easier to read and reason about since it makes it clear that the values are bitcoin amounts. Also to create these types the units (ie .from_sat()) must be specified.
Notes to the reviewers
For coin_selection using Amount and SignedAmount makes internal code safer against overflow errors. In particular because these types will panic if an amount overflow occurs. Using u64/i64 on the other hand can silently rollover. See: https://doc.rust-lang.org/book/ch03-02-data-types.html#integer-overflow
This is a continuation of the work done in #1595. Since this is an API breaking change I would like to include it in the 1.0.0-beta milestone if possible.
Changelog notice
Checklists
All Submissions:
cargo fmtandcargo clippybefore committingBugfixes: