feat(math): Upstream GDA based decimal type #20085
Conversation
|
Important Auto Review SkippedReview was skipped due to path filters Files ignored due to path filters (2)
You can disable this status message by setting the WalkthroughThe updates primarily focus on transitioning from the Changes
Assessment against linked issues
Possibly related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
julienrbrt
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // get the precision multiplier, do not mutate result | ||
| func precisionMultiplier(prec int64) *big.Int { | ||
| if prec < 0 { | ||
| panic(fmt.Sprintf("negative precision %v", prec)) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| } | ||
|
|
||
| if prec > LegacyPrecision { | ||
| panic(fmt.Sprintf("too much precision, maximum %v, provided %v", LegacyPrecision, prec)) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| func (d LegacyDec) RoundInt64() int64 { | ||
| chopped := chopPrecisionAndRoundNonMutative(d.i) | ||
| if !chopped.IsInt64() { | ||
| panic("Int64() out of bound") |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
github-actions
bot
added
C:x/distribution
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Actionable comments outside the diff hunks (1)
x/distribution/README.md (1)
Line range hint
84-86: Adjust the indentation of the unordered list to improve the markdown formatting.- * [Concepts](#concepts) - * [State](#state) - * [Validator Distribution](#validator-distribution) + * [Concepts](#concepts) + * [State](#state) + * [Validator Distribution](#validator-distribution)
|
Not sure if all of the changes from LegacyDec to Dec should be in this PR though as it should probably be separated out into separate PR's |
Yes, so we should revert the documentation changes then and only update them when we actually migrate a module imho |
This reverts commit 115ecc4.
github-actions
bot
removed
C:x/distribution
|
sorry i didnt mean update the docs in the way that was done here. I meant add/modify docs that show users to use legacydec. can we add docs in how to build a module on what math type should be used. additional a note saying the sdk uses the legacy dec but this is the replacement. Then how could users migrate, or can they even. A module that uses legacy dec, how can it interpret a new dec value? The docs may not exist at all so we should add them |
|
Should these docs changes be in a separate PR ? Because from what I'm seeing, it will have to not only be added into the tutorial (which needs to be created) but I think should also have an additional section probably in building modules and throughout the docs as math isnt really referenced anywhere in the docs as it currently stands. |
|
This pr is quite slim don't think it needs a separate pr. |
| ``` | ||
| BenchmarkCompareLegacyDecAndNewDec/LegacyDec-10 8621032 143.8 ns/op 144 B/op 3 allocs/op | ||
| BenchmarkCompareLegacyDecAndNewDec/NewDec-10 5206173 238.7 ns/op 176 B/op 7 allocs/op | ||
| BenchmarkCompareLegacyDecAndNewDecQuoInteger/LegacyDec-10 5767692 205.1 ns/op 232 B/op 6 allocs/op | ||
| BenchmarkCompareLegacyDecAndNewDecQuoInteger/NewDec-10 23172602 51.75 ns/op 16 B/op 2 allocs/op | ||
| BenchmarkCompareLegacyAddAndDecAdd/LegacyDec-10 21157941 56.33 ns/op 80 B/op 2 allocs/op | ||
| BenchmarkCompareLegacyAddAndDecAdd/NewDec-10 24133659 48.92 ns/op 48 B/op 1 allocs/op | ||
| BenchmarkCompareLegacySubAndDecMul/LegacyDec-10 14256832 87.47 ns/op 80 B/op 2 allocs/op | ||
| BenchmarkCompareLegacySubAndDecMul/NewDec-10 18273994 65.68 ns/op 48 B/op 1 allocs/op | ||
| BenchmarkCompareLegacySubAndDecSub/LegacyDec-10 19988325 64.46 ns/op 80 B/op 2 allocs/op | ||
| BenchmarkCompareLegacySubAndDecSub/NewDec-10 27430347 42.45 ns/op 8 B/op 1 allocs/op | ||
| ``` |
There was a problem hiding this comment.
Replace hard tabs with spaces in the benchmarking code block for consistency with Markdown formatting standards.
|
|
||
| * **Enhanced Precision**: `Dec` uses the [apd](https://github.com/cockroachdb/apd) library for arbitrary precision decimals, suitable for accurate financial calculations. | ||
| * **Immutable Operations**: `Dec` operations are safer for concurrent use as they do not mutate the original values. | ||
| * **Better Performance**: `Dec` operations are faster and more efficient than `LegacyDec`. |
There was a problem hiding this comment.
Very good to explain the motivation!
| suite.Suite | ||
| } | ||
|
|
||
| func TestDecimalTestSuite(t *testing.T) { |
There was a problem hiding this comment.
personal preference: there is no need for a test suite as there is no global state or a system under test setup. I find vanilla Go tests much more readable.
| func NewNonNegativeDecFromString(s string) (Dec, error) { | ||
| d, err := NewDecFromString(s) | ||
| if err != nil { | ||
| return Dec{}, ErrInvalidDecString.Wrap(err.Error()) |
There was a problem hiding this comment.
nit: here and others: the error returned from NewDecFromString is namespaced already. No need wrap them again. This would give an odd error string
| new(big.Int).Mul(i.BigIntMut(), precisionMultiplier(prec)), | ||
| func NewDecFromString(s string) (Dec, error) { | ||
| if s == "" { | ||
| s = "0" |
There was a problem hiding this comment.
🤔 I would expect an error for an empty string. Can you elaborate on the use cases?
There was a problem hiding this comment.
I noticed that apd also supports some strings like NaN, sNaN, Infinity, inf,...
You prevent infinite but would it make sense to limit this type to pure numeric strings as in LegacyDec? If people assume same behaviour, this may open up some fraud scenarios.
There was a problem hiding this comment.
I think we should limit this to pure numeric strings and prohibit NaN, etc. I'm thinking that likely the handling of empty string as zero was considered convenience
| // Format format decimal state | ||
| func (d LegacyDec) Format(s fmt.State, verb rune) { | ||
| _, err := s.Write([]byte(d.String())) | ||
| func NewPositiveDecFromString(s string) (Dec, error) { |
There was a problem hiding this comment.
What is the difference between this and NewNonNegativeDecFromString ? If possible, let's get rid of one constructor. Personally, I prefer this method name over the double negative.
There was a problem hiding this comment.
Positive can't be zero where as NonNegative can be zero. I think that's the difference, but we should optimize this API generally for what's going to be the best UX
|
|
||
| bzInt, err := d.i.MarshalText() | ||
| func NewPositiveFixedDecFromString(s string, max uint32) (Dec, error) { |
There was a problem hiding this comment.
I have noticed the variety of constructor methods that include different types and a checks. This does not scale well. If you can refactor the checks to some more generic approach, this may not go wild in the future:
type SetupConstraint func(Dec) error
func NotNegative() SetupConstraint {
return func(d Dec) error {
if d.IsNegative() {return ErrInvalidDecString.Wrap("is negative") }
return nil
}
}
func MaxDecimals(max uint32) SetupConstraint {
return func(d Dec) error {
if d.NumDecimalPlaces() > max {return ErrInvalidDecString.Wrapf("exceeds maximum decimal places: %d", max) }
return nil
}
}
func NewDecFromString(s string, c ...SetupConstraint) (Dec, error) {
...
// apply setup constraints
for _, v := range c {
if err := v(d1); err != nil {
return Dec{}, err
}
}
return d1, nil
}There was a problem hiding this comment.
We probably shouldn't consider the existing constructors to be the ideal API, let's review all this and upgrade.
| return value | ||
| } | ||
| // NewDecFinite returns a decimal with a value of coeff * 10^exp. | ||
| func NewDecFinite(coeff int64, exp int32) Dec { |
There was a problem hiding this comment.
I was confused by the name but I am not a native speaker. Is this the same functionality as LegacyNewDecWithPrec was? Then this name could be used to make it easier to find or
| func NewDecFinite(coeff int64, exp int32) Dec { | |
| func NewDecimalFromCoefficientExponent(coeff int64, exp int32) Dec { |
| ) | ||
|
|
||
| type decimalTestSuite struct { | ||
| suite.Suite | ||
| func TestDec(t *testing.T) { |
There was a problem hiding this comment.
I have not used rapid before but I would feel more comfortable to see some sanity checks on the methods to ensure basic assumptions are correct. This will help with regression as well.
There was a problem hiding this comment.
Having both property based testing (i.e. rapid) plus table tests with specific cases would be ideal. Property based testing can look for edge cases and rapid also works with go fuzz. Table tests will make sure we cover a well defined set of known cases. Doing both is ideal IMHO
There was a problem hiding this comment.
Actionable comments posted: 2
Out of diff range and nitpick comments (2)
testutil/dec.go (1)
12-37: Consider adding documentation for theDiffDecimalsMigrationfunction.It would be beneficial to include a comment describing the purpose and usage of this function, especially since it involves data migration and integrity checks which are critical operations.
testutil/math_utils_test.go (1)
43-45: Clarify the use of0xFFinprefixRange.Adding a comment to explain why
0xFFis appended to the prefix inprefixRangewould improve code readability and maintainability, especially for new developers or external contributors.
testutil/math_utils_test.go
Outdated
| func TestDiffDecimalsMigration(t *testing.T) { | ||
| key := storetypes.NewKVStoreKey("test") | ||
| ctx := testutil.DefaultContext(key, storetypes.NewTransientStoreKey("transient")) | ||
| for i := int64(0); i < 5; i++ { | ||
| legacyDec := math.LegacyNewDec(i) | ||
| dec := math.NewDecFromInt64(i) | ||
| ctx.KVStore(key).Set([]byte(fmt.Sprintf("legacy_%d", i)), []byte(legacyDec.String())) | ||
| ctx.KVStore(key).Set([]byte(fmt.Sprintf("new_%d", i)), []byte(dec.String())) | ||
| } | ||
|
|
||
| hashLegacy := computeHash(ctx, key, "legacy_") | ||
| hashNew := computeHash(ctx, key, "new_") | ||
| require.Equal(t, hashLegacy, hashNew, "Hashes do not match") | ||
| } |
There was a problem hiding this comment.
Enhance the test coverage for DiffDecimalsMigration.
The current test only checks for hash equality but does not verify the correctness of the migration logic itself. Would you like me to help by adding more comprehensive tests or opening a GitHub issue to track this enhancement?
testutil/math_utils_test.go
Outdated
| func computeHash(ctx sdk.Context, key storetypes.StoreKey, prefix string) string { | ||
| h := sha256.New() | ||
| start, end := prefixRange(prefix) | ||
| it := ctx.KVStore(key).Iterator(start, end) | ||
| defer it.Close() | ||
| for ; it.Valid(); it.Next() { | ||
| h.Write(it.Key()) | ||
| h.Write(it.Value()) | ||
| } | ||
| return hex.EncodeToString(h.Sum(nil)) | ||
| } |
There was a problem hiding this comment.
Optimize the computeHash function.
Consider using a more efficient way to handle byte slices in computeHash. Currently, the function recalculates the hash for each prefix, which might be optimized.
This reverts commit 9f3fc57.
| new(big.Int).Mul(i.BigIntMut(), precisionMultiplier(prec)), | ||
| func NewDecFromString(s string) (Dec, error) { | ||
| if s == "" { | ||
| s = "0" |
There was a problem hiding this comment.
I noticed that apd also supports some strings like NaN, sNaN, Infinity, inf,...
You prevent infinite but would it make sense to limit this type to pure numeric strings as in LegacyDec? If people assume same behaviour, this may open up some fraud scenarios.
| return chopPrecisionAndRound(tmp) | ||
| // Quo returns a new Dec with value `x/y` (formatted as decimal128, 34 digit precision) without mutating any | ||
| // argument and error if there is an overflow. | ||
| func (x Dec) Quo(y Dec) (Dec, error) { |
There was a problem hiding this comment.
I did some tests with Quo and the precision is much higher than 34 digits. Do you need to do rounding here? Working with NaN caused some for me unexpected behaviour. Examples:
https://gist.github.com/alpe/5a27babec8005fa347fdd0f0094b1a75
There was a problem hiding this comment.
Maybe, we should make sure we agree on the expected behavior and then have tests to check it.
| if amount.IsNegative() { | ||
| panic(fmt.Sprintf("negative decimal coin amount: %v\n", amount)) |
There was a problem hiding this comment.
Consider improving the panic message for clarity.
- panic(fmt.Sprintf("negative decimal coin amount: %v\n", amount))
+ panic(fmt.Sprintf("attempted to create DecCoin with negative amount: %v", amount))This change makes the error message more specific about the error condition and removes the unnecessary newline character.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if amount.IsNegative() { | |
| panic(fmt.Sprintf("negative decimal coin amount: %v\n", amount)) | |
| if amount.IsNegative() { | |
| panic(fmt.Sprintf("attempted to create DecCoin with negative amount: %v", amount)) |
|
Would be good to have some reference to the original provenance of this code coming from https://github.com/regen-network/regen-ledger/tree/main/types/math |
There was a problem hiding this comment.
Actionable comments posted: 5
Out of diff range and nitpick comments (5)
docs/build/building-modules/18-decimal-handling.md (5)
8-8: Consider adding a comma before "but" to improve readability.
14-14: Add a comma after "use" for grammatical correctness.
20-20: Add a comma after "example" to improve the flow of the sentence.
111-111: Add a comma after "migration" to improve readability.
Line range hint
125-125: Remove the extra blank line to maintain consistency in document formatting.-
| * **Immutable Operations**: `Dec` operations are safer for concurrent use as they do not mutate the original values. | ||
| * **Better Performance**: `Dec` operations are faster and more efficient than `LegacyDec`.` | ||
|
|
||
| ## Using `Dec` in Modules that havent used `LegacyDec` |
There was a problem hiding this comment.
Correct the spelling of "haven't" in the heading.
- ## Using `Dec` in Modules that havent used `LegacyDec`
+ ## Using `Dec` in Modules that haven't used `LegacyDec`Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| ## Using `Dec` in Modules that havent used `LegacyDec` | |
| ## Using `Dec` in Modules that haven't used `LegacyDec` |
|
|
||
| If you are creating a new module or updating an existing module that has not used `LegacyDec`, you can directly use `Dec` without any changes. | ||
|
|
||
| As an example we will use `DecCoin` which is a common type used in the Cosmos SDK. |
There was a problem hiding this comment.
Consider adding a comma after "As an example" for better readability.
- As an example we will use `DecCoin` which is a common type used in the Cosmos SDK.
+ As an example, we will use `DecCoin` which is a common type used in the Cosmos SDK.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| As an example we will use `DecCoin` which is a common type used in the Cosmos SDK. | |
| As an example, we will use `DecCoin` which is a common type used in the Cosmos SDK. |
|
|
||
| ```go | ||
| import ( | ||
| "cosmossdk.io/math" |
There was a problem hiding this comment.
Use spaces instead of tabs for indentation to maintain consistency in Markdown formatting.
- "cosmossdk.io/math"
+ "cosmossdk.io/math"Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| "cosmossdk.io/math" | |
| "cosmossdk.io/math" |
+1, we should imho add a |
Co-Authored-By: Aaron Craelius <aaronc@users.noreply.github.com>
| b.Run("LegacyDec", func(b *testing.B) { | ||
| for i := 0; i < b.N; i++ { | ||
| _ = legacyB1.Quo(LegacyNewDec(1)) | ||
| } | ||
| }) | ||
|
|
||
| b.Run("NewDec", func(b *testing.B) { | ||
| for i := 0; i < b.N; i++ { | ||
| _, _ = newB1.QuoInteger(NewDecFromInt64(1)) | ||
| } | ||
| }) |
There was a problem hiding this comment.
These benchmarks are wrong, they benchmark the dec allocation along with Quo by one!
So we should allocate the one dec outside the loop!
But to do a useful benchmark, we need numbers at far bigger word sizes, not single word!
| @@ -78,7 +78,8 @@ require ( | |||
| github.com/beorn7/perks v1.0.1 // indirect | |||
| github.com/btcsuite/btcd/btcec/v2 v2.3.3 // indirect | |||
| github.com/cespare/xxhash v1.1.0 // indirect | |||
| github.com/cespare/xxhash/v2 v2.3.0 // indirect | |||
| github.com/cespare/xxhash/v2 v2.2.0 // indirect | |||
| github.com/cockroachdb/apd/v2 v2.0.2 // indirect | |||
There was a problem hiding this comment.
Curious why we landed on apd v2 instead of v3? Didn't see a discussion in either of #7773 (comment) or #11783 , just a brief mention. In some testing we did it showed 20-30% better performance improvements in overall runtime.
There was a problem hiding this comment.
This just looks like an oversight. The reference code was written a while ago (before v3) and this code should be updated to use the latest version
Description
Closes: #11783
Upstreams the GDA decimal type within math and separated the dec with the legacy dec. In addition added benchmarks for the new type in comparison to legacy.
Please see the benchmarking results below:
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
New Features
Dec) for enhanced safety and precision in arithmetic operations across various modules.Decstructure.Documentation
Tests
Bug Fixes