-
Notifications
You must be signed in to change notification settings - Fork 329
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
Change coin_selection and DescriptorExt::dust_value to use Amount type #1763
Change coin_selection and DescriptorExt::dust_value to use Amount type #1763
Conversation
3504440
to
5a0e19d
Compare
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 |
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK d2f49a7
d2f49a7
to
7f1e4d2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 7f1e4d2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 7f1e4d2
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 2a9eeed
…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
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 fmt
andcargo clippy
before committingBugfixes: