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

Coin type changes #235

Merged

Conversation

jstuczyn
Copy link
Contributor

This pull request comes as a result of actually using the library and finding couple of things rather cumbersome to use, in particular the Coin type. There is no easy way to work with its amount due to the lack of any getters on the Decimal and the type itself only providing addition-related capabilities. I've decided to take a possibly controversial approach of changing the amount field to an u128. I've done it for a couple of reasons:

  • even cosmos-sdk's Coin is represented with a big.Int. (Dec is used for DecCoin). Now, the counter argument is that big.Int is capable of representing values up to 2^256 - 1 which is a significantly more than what u128 provides. Plus it can also represent negative values. However, do negative coins make any sense and would be useful on their own? Also, u128 still has higher range than the current Decimal with internally used u64.
  • I've taken some inspiration from CosmWasm's coin type. In their case, however, they have an additional thin Uint128 wrapper around the u128 primitive mostly to help with the serialization for their clients, which is not applicable in this case.
  • the crate is still in the pre 1.0 stage thus, even though it's a breaking change, I'd argue it's not as disruptive as if it would have been done later.

What's your opinion on it? I would be more than happy to discuss it and introduce any additional changes. Perhaps rather than going with primitive u128 a different wrapper with properties equivalent to Go's big.Int would be more appropriate?

@tony-iqlusion
Copy link
Member

I agree the current Decimal newtype is unhelpful in its current state.

The only usage of Decimal is amount, so if you're migrating that field away from it you can delete the Decimal newtype entirely.

We can consider this for the next round of breaking changes.

@jstuczyn
Copy link
Contributor Author

That's a very fair suggestion - I've gone ahead and removed it.

@tony-iqlusion tony-iqlusion merged commit 1a7a1b0 into cosmos:main Jul 23, 2022
@tony-iqlusion
Copy link
Member

Thanks! This will go out in the next breaking release (v0.8)

@tony-iqlusion tony-iqlusion mentioned this pull request Jul 26, 2022
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.

2 participants