Skip to content
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

Enforce displaying Amount with trailing zeros #2604

Merged
merged 1 commit into from
Mar 17, 2024

Conversation

448-OG
Copy link
Contributor

@448-OG 448-OG commented Mar 16, 2024

It is common to display bitcoins using trailing zeros upto 8 decimals. This commit enforces:

  • Displaying Amount in BTC with trailing zeroes by eight decimal places if a precision on the Amount is not specified.
  • Displaying Amount in BTC upto the precision specified truncating the insignificant zeros.
  • Displaying amount in BTC without any decimals if the remainder of the amount divided by the satoshis in 1 BTC is equal to zero using formula satoshis.rem_euclid(Amount::ONE_BTC.to_sat()) != 0

These are not breaking changes and all previous tests pass.

A testcase is added to for changes introduced.

Resolves: #2136

@github-actions github-actions bot added the C-units PRs modifying the units crate label Mar 16, 2024
@coveralls
Copy link

coveralls commented Mar 16, 2024

Pull Request Test Coverage Report for Build 8316149718

Details

  • 37 of 37 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 83.927%

Totals Coverage Status
Change from base Build 8307227520: 0.02%
Covered Lines: 19732
Relevant Lines: 23511

💛 - Coveralls

@apoelstra
Copy link
Member

The CI failure is real -- you're introducing a dependence on alloc in the fmt method that didn't used to be there.

In principle we shouldn't need one.

Interestingly my local CI passes -- so we've found a blind spot in it! Let me investigate..

@apoelstra
Copy link
Member

Oh, lol, the blind spot is the entire units crate. Oops. This may also explain another "how did this get past CI?" situation last week..

@sanket1729
Copy link
Member

I forgot what our consensus was about respecting FromStr and Display invariant? Do we make an effort to maintain it?

@448-OG
Copy link
Contributor Author

448-OG commented Mar 17, 2024

The CI failure is real -- you're introducing a dependence on alloc in the fmt method that didn't used to be there.

In principle we shouldn't need one.

Interestingly my local CI passes -- so we've found a blind spot in it! Let me investigate..

Yeah! Realized it was an issue with allocation but I was not able to make changes yesterday. Should be fixed now.

My tests also passed locally. Not sure why

@448-OG
Copy link
Contributor Author

448-OG commented Mar 17, 2024

What is the current policy on running rustfmt, my editor does auto format but when I do a diff I see so many changes related to formatting

@448-OG
Copy link
Contributor Author

448-OG commented Mar 17, 2024

So I also need to fix it for tests, somehow they succeed locally

@apoelstra
Copy link
Member

What is the current policy on running rustfmt, my editor does auto format but when I do a diff I see so many changes related to formatting

Please do not commit unrelated changes. Usually there should not be very many because we have a weekly bot which does all the formatting changes, but lately we have been skipping the bot because one mantainer claims undue difficulty in rebasing.

So you will need to override your editor or be careful about what changes you commit, for now.

@apoelstra
Copy link
Member

I forgot what our consensus was about respecting FromStr and Display invariant? Do we make an effort to maintain it?

Yes, we "make an effort". But there are some exceptional cases, most notably Script, and I don't think we've worked out an explicit policy.

@sanket1729
Copy link
Member

@448-OG, you can use just format that would internally use the nightly formatter. In your editor is vscode, you can configure project specific settings for rust-bitcoin and set the formatter as just format

It is common to display bitcoins using trailing zeros upto 8 decimals.
This commit enforces:
- Displaying Amount in BTC with trailing zeroes by eight decimal places
  if a precision on the Amount is not specified.
- Displaying Amount in BTC upto the precision specified truncating the insignificant zeros.
- Displaying amount in BTC without any decimals if the remainder of the amount
  divided by the satoshis in 1 BTC is equal to zero using formula `satoshis.rem_euclid(Amount::ONE_BTC.to_sat()) != 0`

These are not breaking changes and all previous tests pass.

A testcase is added to for changes introduced.

Resolves: rust-bitcoin#2136
Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

ACK d887423

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK d887423

@apoelstra apoelstra merged commit 93ca42c into rust-bitcoin:master Mar 17, 2024
187 checks passed
@448-OG 448-OG deleted the amount_8decimal_precision branch March 17, 2024 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-units PRs modifying the units crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shouldn't Amount handle decimals formatting with {:.8} ?
4 participants