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

Implement initial simple denom convert utils #3747

Merged
merged 19 commits into from
Mar 16, 2019
Merged

Conversation

alexanderbez
Copy link
Contributor

@alexanderbez alexanderbez commented Feb 26, 2019

Implement denom registration and a coin conversion utility.

This assumes we do not touch ParseCoin/s. IMHO, that should only take in the base denom -- uatom.

/cc @cwgoes @rigelrozanski

ref: #3510


  • 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 entries in PENDING.md with issue #

  • 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)

@codecov
Copy link

codecov bot commented Feb 26, 2019

Codecov Report

Merging #3747 into develop will increase coverage by 0.03%.
The diff coverage is 77.77%.

@@             Coverage Diff             @@
##           develop    #3747      +/-   ##
===========================================
+ Coverage    60.95%   60.98%   +0.03%     
===========================================
  Files          192      193       +1     
  Lines        14360    14386      +26     
===========================================
+ Hits          8753     8774      +21     
- Misses        5033     5035       +2     
- Partials       574      577       +3

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.

What about just one function, "ConvertDenom", which takes an amount, a source denom, and a desired denom, and converts one to the other (if the denoms are recognized)? Then we can easily add new denoms without changing the function calls.

@alexanderbez alexanderbez marked this pull request as ready for review March 6, 2019 15:58
@jackzampolin
Copy link
Member

types/denom.go Show resolved Hide resolved
@alexanderbez
Copy link
Contributor Author

@cwgoes @rigelrozanski bump <3

@jleni
Copy link
Member

jleni commented Mar 8, 2019

Would it be possible to use SI prefixes correctly?

@cwgoes
Copy link
Contributor

cwgoes commented Mar 11, 2019

Would it be possible to use SI prefixes correctly?

Absolutely let's do - what should we change?

@alexanderbez
Copy link
Contributor Author

alexanderbez commented Mar 11, 2019

We should at the very least use u|uatom|10^-6 and m|matom|10^-3. Thoughts @jleni?

Copy link
Contributor

@rigelrozanski rigelrozanski left a comment

Choose a reason for hiding this comment

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

I don't think we should be using anything except uatom and atom in this tool - everything else just adds confusion AFIAC. Also I think the unit's are off based on what I'm seeing in the code (Bez's comment seems correct though, confusingly) https://www.google.com/search?q=si+prefixes&source=lnms&tbm=isch&sa=X&ved=0ahUKEwjCxMfutvvgAhULpYMKHQFUAD4Q_AUIDigB&biw=1269&bih=696#imgrc=F26w4WQUVJaWDM:

types/denom_test.go Outdated Show resolved Hide resolved
@alexanderbez
Copy link
Contributor Author

@rigelrozanski Those are standard de-facto SI prefixes. We should've caught this sooner tbh. Also, wrt to only atom and uatom. This functionality makes no assumptions on denoms -- it can be used by any developer/application. For Gaia, it'll only have atom and uatom.

@cwgoes
Copy link
Contributor

cwgoes commented Mar 12, 2019

I don't see why we should avoid other prefixes - uatom can stay the actual denomination, and the tooling can default to show (and maybe accept values) in atom.

@alexanderbez
Copy link
Contributor Author

@cwgoes @rigelrozanski @jleni updated

Copy link
Contributor

@rigelrozanski rigelrozanski left a comment

Choose a reason for hiding this comment

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

Couple minor comments to make things a bit more clear - but looks good. Also you're right, I forgot this is the util not the gaia implementation, so nbd about using a bunch of SI examples

types/denom_test.go Outdated Show resolved Hide resolved
types/denom_test.go Outdated Show resolved Hide resolved
types/denom_test.go Outdated Show resolved Hide resolved
@alexanderbez
Copy link
Contributor Author

Updated. Should be good to merge.

@alexanderbez
Copy link
Contributor Author

Bump @cwgoes

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.

Needs a changelog entry, otherwise the code LGTM.

I wonder if we should warn the user somehow when truncation results in loss of precision.

@alexanderbez
Copy link
Contributor Author

Added a pending log entry @cwgoes

@alexanderbez
Copy link
Contributor Author

Fixed CI and added pending log entry @cwgoes

@cwgoes cwgoes merged commit 25408e7 into develop Mar 16, 2019
@cwgoes cwgoes deleted the bez/atom-denom-conv branch March 16, 2019 13:18
@zmanian zmanian mentioned this pull request Feb 27, 2020
4 tasks
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.

6 participants