-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Fix #7640: tally calculation precision error #7641
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7641 +/- ##
==========================================
- Coverage 54.34% 54.34% -0.01%
==========================================
Files 610 610
Lines 38629 38627 -2
==========================================
- Hits 20992 20990 -2
Misses 15493 15493
Partials 2144 2144 |
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.
I've never looked at Dec's code, I thought Dec would handle arithmetic like this correctly. Anyways, this lgtm too, but I would prefer having someone with more experience on this code have a look too.
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.
Thanks! can you add a test case that matches your example from the issue?
We should always be using this order of operations, I wonder if we can lint for it somehow. |
@amaurymartiny Dec has limited precision, hardcode defaulted to 18 decimal points |
I don't know how to setup related chain environment in unit tests, or simulation tests, not clear about how the simulation framework works either, I need to read more about that. |
@yihuang It could just be a Dec unit test on |
just added a simple test case. |
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
I think this PR requires a Bug Fix changelog entry |
Solution: - change `(a / b) * c` to `a * b / c`
thanks, done. |
Solution:
(a / b) * c
toa * c / b
Description
closes: #7640
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerCodecov Report
in the comment section below once CI passes