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

Decimal256 #1067

Merged
merged 13 commits into from
Sep 7, 2021
Merged

Decimal256 #1067

merged 13 commits into from
Sep 7, 2021

Conversation

uint
Copy link
Contributor

@uint uint commented Sep 1, 2021

Whew. We've got our own Decimal256.

Addresses #1052. I'm not sure if there's any more work there except for getting this PR in. I'll review tomorrow.

Closes #1052

@tac0turtle
Copy link

Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

Nice stuff. I'm not so sure about the change from 18 decimal places to 36. Let's see what community says.

packages/std/src/math/decimal256.rs Outdated Show resolved Hide resolved
packages/std/src/math/decimal256.rs Outdated Show resolved Hide resolved
packages/std/src/math/decimal256.rs Outdated Show resolved Hide resolved
packages/std/src/math/uint256.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

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

lgtm.

packages/std/src/math/decimal256.rs Outdated Show resolved Hide resolved
packages/std/src/math/decimal256.rs Outdated Show resolved Hide resolved
@webmaster128
Copy link
Member

webmaster128 commented Sep 6, 2021

Alright, the two open changes:

  • rebase to current main and utilize const constructors
  • change number of decimals to 18

Then 🚀

@uint
Copy link
Contributor Author

uint commented Sep 6, 2021

Rebased.

@uint uint requested a review from webmaster128 September 6, 2021 15:53
CHANGELOG.md Outdated
@@ -21,6 +21,7 @@ and this project adheres to
`Into<Uint128>` rather than `Into<u128>`.
- cosmwasm-crypto: Update dependency `k256` to ^0.9.6.
- cosmwasm-std: Add enum cases `Shl` to `OverflowOperation` (breaking; [#1071]).
- cosmwasm-std: `Decimal256` now has 18 decimal places.
Copy link
Member

Choose a reason for hiding this comment

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

I think we can drop this as Decimal256 is newly created in this PR

CHANGELOG.md Outdated Show resolved Hide resolved
packages/std/src/math/decimal256.rs Outdated Show resolved Hide resolved
packages/std/src/math/decimal256.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated
@@ -40,6 +41,7 @@ and this project adheres to
`Uint64`, `Uint128`, `Uint256`, and `Uint512`.
- cosmwasm-std: Exposed `Uint{64, 128, 256}::full_mul` for full multiplication
that cannot overflow.
- cosmwasm-std: Added the `Decimal256` type with 18 decimal places.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I guess this ended up in the wrong section after the rebase and got me confused. Let's fix this...

@uint uint requested a review from webmaster128 September 7, 2021 10:28
@uint uint merged commit 8907d4a into main Sep 7, 2021
@uint uint deleted the decimal256 branch September 7, 2021 10:29
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.

Cosmwasm-std feature requests: Uint256, Decimal256, checked-ops, sqrt
4 participants