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

Fix DecCoins Bugs #4058

Merged
merged 11 commits into from Apr 5, 2019
Merged

Conversation

alexanderbez
Copy link
Contributor

@alexanderbez alexanderbez commented Apr 5, 2019

  • Panic on zero decimal input
  • Don't include zero decimal coins in results
  • Add TOC to CHANGELOG to make it easier to navigate and add entries when working against release branches.

closes: #4050


  • Targeted PR against correct branch (see CONTRIBUTING.md)

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

  • Wrote tests

  • Updated relevant documentation (docs/)

  • Added a relevant changelog entry: sdkch add [section] [stanza] [message]

  • rereviewed Files changed in the github PR explorer


For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@alexanderbez alexanderbez mentioned this pull request Apr 5, 2019
10 tasks
@codecov
Copy link

codecov bot commented Apr 5, 2019

Codecov Report

Merging #4058 into release/v0.34.0 will increase coverage by <.01%.
The diff coverage is 27.27%.

@@                 Coverage Diff                 @@
##           release/v0.34.0    #4058      +/-   ##
===================================================
+ Coverage               60%   60.01%   +<.01%     
===================================================
  Files                  212      212              
  Lines                15111    15121      +10     
===================================================
+ Hits                  9068     9075       +7     
- Misses                5420     5423       +3     
  Partials               623      623

Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

These changes look fine, but I don't think they cover all the cases:

  • Couldn't Mul on line 321 also return 0 if the multiplied-by decimal is small?
  • DecCoin bugs #4050 mentions that ParseDecCoins needs to be changed, but I don't think it does, since it calls IsValid() - maybe we could add a testcase though

@alexanderbez
Copy link
Contributor Author

@cwgoes

Couldn't Mul on line 321 also return 0 if the multiplied-by decimal is small?

Yes, fixed.

#4050 mentions that ParseDecCoins needs to be changed, but I don't think it does, since it calls IsValid() - maybe we could add a testcase though

Yes, I mentioned that in the issue. I don't see how it's broken.

@alexanderbez alexanderbez merged commit 576eb51 into release/v0.34.0 Apr 5, 2019
@alexanderbez alexanderbez deleted the bez/4050-fix-more-dec-coins-bugs branch April 5, 2019 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants