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

Respect precision while formatting Amount #2682

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

448-OG
Copy link
Contributor

@448-OG 448-OG commented Apr 13, 2024

All characters beyond the needed precision are truncated

In the case of a SignedAmount the default precision is now 8 just like the Amount

Just like the existing code, no width is added if there exists fractional values just like the existing code works.

Tests have been improved to handle precision

Resolves: #2136

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

coveralls commented Apr 13, 2024

Pull Request Test Coverage Report for Build 8757209561

Details

  • 74 of 74 (100.0%) changed or added relevant lines in 2 files are covered.
  • 13 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.06%) to 83.194%

Files with Coverage Reduction New Missed Lines %
io/src/lib.rs 13 71.03%
Totals Coverage Status
Change from base Build 8745295649: 0.06%
Covered Lines: 19256
Relevant Lines: 23146

💛 - Coveralls

units/src/amount.rs Outdated Show resolved Hide resolved
units/src/amount.rs Outdated Show resolved Hide resolved
units/src/amount.rs Outdated Show resolved Hide resolved
units/src/amount.rs Outdated Show resolved Hide resolved
@448-OG 448-OG force-pushed the master branch 2 times, most recently from bdb2e5b to 9881b91 Compare April 14, 2024 23:30
@github-actions github-actions bot added the C-io PRs modifying the io crate label Apr 14, 2024
@@ -211,8 +211,7 @@ impl<T: AsRef<[u8]>> Read for Cursor<T> {
let start_pos = self.pos.try_into().unwrap_or(inner.len());
let read = core::cmp::min(inner.len().saturating_sub(start_pos), buf.len());
buf[..read].copy_from_slice(&inner[start_pos..start_pos + read]);
self.pos =
self.pos.saturating_add(read.try_into().unwrap_or(u64::MAX /* unreachable */));
self.pos = self.pos.saturating_add(read.try_into().unwrap_or(u64::MAX /* unreachable */));
Copy link
Member

Choose a reason for hiding this comment

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

In 9881b91:

This appears to be format-only and unrelated to this PR.

units/src/amount.rs Outdated Show resolved Hide resolved
@apoelstra
Copy link
Member

Can you add a test which parses 543.53524 BTC and then tries to format it with .5?

@448-OG
Copy link
Contributor Author

448-OG commented Apr 15, 2024

543.53524 BTC

Done

@apoelstra
Copy link
Member

Hmmmm, something strange is going on.

Your new test passes, but if you replace Amount::from_btc(543.53524) with Amount::from_str("543535239ubtc") then it fails.

Can you add the from_str variant as a test vector, and fix whatever is causing it to fail (it 0s out all the precise values).

@448-OG
Copy link
Contributor Author

448-OG commented Apr 15, 2024

Hmmmm, something strange is going on.

Your new test passes, but if you replace Amount::from_btc(543.53524) with Amount::from_str("543535239ubtc") then it fails.

Can you add the from_str variant as a test vector, and fix whatever is causing it to fail (it 0s out all the precise values).

I see a bug where values are ignored instead it adds zeros and rounding of. Working on a fix

All characters beyond the needed precision are truncated

In the case of a SignedAmount the default precision is now 8
just like the Amount

Replaces self.fmt_value_in() with fmt_satoshi_in() so that
FomartOptions is not made public which would be the requirements
if self.fmt_value_in() is used.

Tests have been improved to handle precision

Resolves: rust-bitcoin#2136
@448-OG
Copy link
Contributor Author

448-OG commented Apr 20, 2024

I need help understanding how to fix the fuzz tests

@apoelstra
Copy link
Member

@448-OG possibly you will be better off just trying to write the formatting functions "properly" from scratch rather than tweaking the existing ones, which are pretty complicated. Though I'm doubtful because much of the complexity is inherent to the suffixing system.

To try to zero in on fuzztest failures sometimes it's easiest to try minimizing the test cases. Just add a if data.len() > 10 { return; } to the top of the fuzztest, and if it finds a problem, reduce 10 to 9 and so on, until it always passes. After iterating on this a bit you'll find the smallest string that causes the test to fail, which may be simpler to diagnose than a long one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-io PRs modifying the io crate C-units PRs modifying the units crate test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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