Skip to content

Range limit Amount#3692

Closed
yancyribbens wants to merge 4 commits intorust-bitcoin:masterfrom
yancyribbens:range-limit-amount
Closed

Range limit Amount#3692
yancyribbens wants to merge 4 commits intorust-bitcoin:masterfrom
yancyribbens:range-limit-amount

Conversation

@yancyribbens
Copy link
Copy Markdown
Contributor

@yancyribbens yancyribbens commented Dec 3, 2024

Draft PR

The thrust of this PR is to limit Amount constructors to MAX_MONEY and return NONE if exceeded. In so doing, currently limit from_sats() as a prototype.

TODOs

  • Fix remaining tests that use values greater than MAX_MONEY
  • Enforce other constructs in the same way: from_btc(), from_int_btc() from_int_btc_const from_str_in from_str_with_denomination
  • Enforce the invariant in all addition and multiplication functions

Next PR TODO:

  • Address SignedAmount in the same way

closes #3691

@yancyribbens yancyribbens force-pushed the range-limit-amount branch 2 times, most recently from ff9e8c2 to 3be7af9 Compare December 4, 2024 01:57
@github-actions github-actions bot added C-bitcoin PRs modifying the bitcoin crate C-units PRs modifying the units crate test C-primitives labels Dec 4, 2024
@coveralls
Copy link
Copy Markdown

coveralls commented Dec 4, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 12182293262

Details

  • 145 of 151 (96.03%) changed or added relevant lines in 11 files are covered.
  • 7 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.05%) to 82.824%
Changes Missing Coverage Covered Lines Changed/Added Lines %
units/src/amount/signed.rs 3 4 75.0%
units/src/amount/unsigned.rs 25 27 92.59%
units/src/fee_rate.rs 9 12 75.0%
Files with Coverage Reduction New Missed Lines %
units/src/amount/signed.rs 2 69.05%
units/src/amount/unsigned.rs 5 82.35%
Totals Coverage Status
Change from base Build 12121100676: -0.05%
Covered Lines: 20161
Relevant Lines: 24342
💛 - Coveralls

@yancyribbens
Copy link
Copy Markdown
Contributor Author

yancyribbens commented Dec 4, 2024

Hmm that's right, unwrap() isn't available in const context 🤦 . Might need to rethink this approach.

@tcharding
Copy link
Copy Markdown
Member

tcharding commented Dec 4, 2024

The unwraps are ugly, perhaps we should add a from_sat_unchecked function that doesn't enforce the invariant?

@jamillambert jamillambert mentioned this pull request Dec 4, 2024
Merged
@jamillambert
Change Amount::MAX to equal MAX_MONEY
6950c0a 
To prevent rounding errors converting to and from f64 change
`Amount::MAX` to `MAX_MONEY` which is below the limit in f64 that has
issues.

Add checks to `from_str_in`, `checked_add` and `checked_mul` that the
result is below MAX, where previously a u64 overflow was relied on.

Change tests to account for new lower MAX that is within the range of
SignedAmount and does not overflow so easily

Remove overflow tests

`Amount::MAX` is now below `u64::MAX` and within the range of values for
`SignedAmount`.   These tests therefore do not overflow.
In effective_value there is no error with `Amount::MAX` and the correct
value is returned.
In psbt the removed test is effectively the same as the previous test.

Modify `Amount` tests to work with new `MAX`

Tests need to be changed that checked values above the new `MAX` or
`Amount::MAX` was out of range for `SignedAmount` which it isn't anymore
@yancyribbens yancyribbens force-pushed the range-limit-amount branch 5 times, most recently from 88edcc7 to 39a834e Compare December 4, 2024 20:39
@github-actions
Copy link
Copy Markdown

github-actions bot commented Dec 4, 2024

🚨 API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.

@github-actions github-actions bot added the API break This PR requires a version bump for the next release label Dec 4, 2024
apoelstra added a commit that referenced this pull request Dec 9, 2024
@apoelstra
Merge #3693: Change Amount::MAX from u64::MAX to Amount::MAX_MONEY
b579e12 
6950c0a Change `Amount::MAX` to equal `MAX_MONEY` (Jamil Lambert, PhD)

Pull request description:

  As discussed in #3688 and #3691 using `u64::MAX` causes errors when converting to `f64` so `MAX_MONEY` is should be used as the maximum `Amount`.

   - `Amount::MAX` changed to equal `MAX_MONEY`
   - Add a check to add, multiply and from_str functions
   - Change tests to account for new lower maximum

  Different approach to the existing PR #3692.  I have only done Amount and not SignedAmount until there is feedback on which approach to use.

ACKs for top commit:
  tcharding:
    ACK 6950c0a
  apoelstra:
    ACK 6950c0a; successfully ran local tests

Tree-SHA512: 03ebf39c47b19ba88d95235538039f28bfa29f4499618fab25c9b627684c348ed41caef682e8f0e01ca62cf9ced8a1183fe3ed861bffeb9609b09440ddfb1c92
@yancyribbens yancyribbens mentioned this pull request Dec 10, 2024
Closed
@yancyribbens
Copy link
Copy Markdown
Contributor Author

yancyribbens commented Dec 20, 2024

closing. see: #3794

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

API break This PR requires a version bump for the next release C-bitcoin PRs modifying the bitcoin crate C-primitives C-units PRs modifying the units crate test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Remove Amount::MAX and replace it with the value in MAX_MONEY

4 participants

@yancyribbens @coveralls @tcharding @jamillambert