feat(math): Upstream GDA based decimal type v2 #20536
Conversation
samricotta
changed the title
WalkthroughWalkthroughThe update introduces significant enhancements to decimal handling in the Cosmos SDK, transitioning from the Changes
Sequence Diagram(s)sequenceDiagram
participant Developer as Dev
participant Documentation as Docs
participant LegacyDec as Legacy
participant NewDec as Dec
participant MathOperations as MathOps
Dev->>Docs: Read migration guide
Docs-->>Dev: Provides steps and examples
Dev->>Legacy: Uses existing LegacyDec
Legacy-->>Dev: Performs operations
Dev->>NewDec: Switch to Dec
NewDec-->>Dev: Enhanced precision and safety
Dev->>MathOps: Implement new utilities
MathOps-->>Dev: Safe and precise calculations
Assessment against linked 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 Documentation and Community
|
![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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Outside diff range, codebase verification and nitpick comments (9)
math/math.go (2)
1-1: Improve package documentationThe package documentation should be more detailed to provide better context on the purpose and usage of the package.
-// Package math provides helper functions for doing mathematical calculations and parsing for the ecocredit module. +// Package math provides helper functions for performing mathematical calculations and parsing, specifically tailored for the ecocredit module in the Cosmos SDK.
25-38: Function SubNonNegative: Improve error messageThe error message in
SubNonNegativecould be more descriptive.- return z, fmt.Errorf("result negative during non-negative subtraction") + return z, fmt.Errorf("result is negative during non-negative subtraction: %s - %s", x, y)docs/build/building-modules/18-decimal-handling.md (3)
8-8: Fix grammatical error.Add a comma after "Regen Network's
regen-ledger".- The implementation of `Dec` is adapted from Regen Network's `regen-ledger`, specifically from [this module](https://github.com/regen-network/regen-ledger/tree/main/types/math). + The implementation of `Dec` is adapted from Regen Network's `regen-ledger`, specifically from [this module](https://github.com/regen-network/regen-ledger/tree/main/types/math).
18-18: Add missing comma.Add a comma after "for concurrent use".
- * **Immutable Operations**: `Dec` operations are safer for concurrent use as they do not mutate the original values. + * **Immutable Operations**: `Dec` operations are safer for concurrent use, as they do not mutate the original values.Tools
LanguageTool
[uncategorized] ~18-~18: Possible missing comma found.
Context: ...ec` operations are safer for concurrent use as they do not mutate the original valu...(AI_HYDRA_LEO_MISSING_COMMA)
25-25: Add missing comma.Add a comma after "As an example".
- 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.Tools
LanguageTool
[typographical] ~25-~25: Consider adding a comma after this introductory phrase.
Context: ...y useDecwithout any changes. As an example we will useDecCoinwhich is a common...(AS_A_NN_COMMA)
x/group/go.mod (1)
Several local directories specified in the replace directives do not exist.
The following directories are missing, which may affect the functionality if they are required:
../../api../../collections../../core../../depinject../accounts../auth../authz../bank../consensus../distribution../gov../mint../protocolpool../slashing../stakingPlease ensure these directories are correctly referenced or created as needed.
Analysis chain
Line range hint
191-196: Verify replace directives.Ensure that the local directories specified in the replace directives are correct and the changes do not affect the functionality.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the replace directives. # Test: Ensure that the project builds successfully with the local replace directives. go mod tidy go build ./...Length of output: 170
Script:
#!/bin/bash # Description: Verify the existence of local directories specified in replace directives in go.mod # Extract replace directives from go.mod replace_directives=$(grep -E 'replace .* => \./' x/group/go.mod) # Check if the directories exist echo "$replace_directives" | while read -r line; do dir=$(echo "$line" | awk -F'=> ' '{print $2}') if [ -d "$dir" ]; then echo "Directory exists: $dir" else echo "Directory does not exist: $dir" fi doneLength of output: 232
Script:
#!/bin/bash # Description: Display the content of x/group/go.mod to verify the presence of replace directives # Display the content of go.mod cat x/group/go.modLength of output: 8904
Script:
#!/bin/bash # Description: Verify the existence of local directories specified in replace directives in go.mod # Directories to check directories=( "../../" "../../api" "../../collections" "../../core" "../../depinject" "../accounts" "../auth" "../authz" "../bank" "../consensus" "../distribution" "../gov" "../mint" "../protocolpool" "../slashing" "../staking" ) # Check if the directories exist for dir in "${directories[@]}"; do if [ -d "$dir" ]; then echo "Directory exists: $dir" else echo "Directory does not exist: $dir" fi doneLength of output: 2471
math/dec_legacy.go (1)
612-620: Remove unnecessary ASCII art.The ASCII art is unnecessary and can be removed to improve readability.
- // ____ - // __| |__ "chop 'em - // ` \ round!" - // ___|| ~ _ -bankers - // | | __ - // | | | __|__|__ - // |_____: / | $$$ | - // |________|math/dec_test.go (2)
118-150: Comprehensive test cases forNewDecFromInt64.The test cases cover a wide range of scenarios, including zero, positive, negative, and extreme values. This ensures robustness in the
NewDecFromInt64function.Use
math.MinInt64for consistency.For the "min value" test case, using the
math.MinInt64constant improves readability and consistency.- src: -9223372036854775808, + src: math.MinInt64,
401-593: Comprehensive test cases forQuo.The test cases cover a wide range of scenarios, including zero, positive, negative, and extreme values. This ensures robustness in the
Quofunction.Remove debug print statement.
The debug print statement in the test case "1.234 / -123 = 1.111" should be removed to keep the test output clean.
- if name == "1.234 / -123 = 1.111" { - fmt.Println("got", got) - }
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Commits
Files that changed from the base of the PR and between 6ea50ef and 0470327c41dab958ad2fcc96601a215c42d0f7f2.
Files ignored due to path filters (3)
go.sumis excluded by!**/*.summath/go.sumis excluded by!**/*.sumx/group/go.sumis excluded by!**/*.sum
Files selected for processing (12)
- docs/build/building-modules/18-decimal-handling.md (1 hunks)
- go.mod (2 hunks)
- math/dec.go (1 hunks)
- math/dec_bench_test.go (1 hunks)
- math/dec_legacy.go (1 hunks)
- math/dec_legacy_test.go (1 hunks)
- math/dec_rapid_test.go (1 hunks)
- math/dec_test.go (1 hunks)
- math/go.mod (1 hunks)
- math/math.go (1 hunks)
- x/group/go.mod (8 hunks)
- x/group/internal/math/dec.go (1 hunks)
Additional context used
Path-based instructions (9)
math/math.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/group/internal/math/dec.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.docs/build/building-modules/18-decimal-handling.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"math/dec_bench_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"math/dec.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.math/dec_rapid_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"math/dec_legacy.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.math/dec_legacy_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"math/dec_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
LanguageTool
docs/build/building-modules/18-decimal-handling.md
[uncategorized] ~18-~18: Possible missing comma found.
Context: ...ec` operations are safer for concurrent use as they do not mutate the original valu...(AI_HYDRA_LEO_MISSING_COMMA)
[typographical] ~25-~25: Consider adding a comma after this introductory phrase.
Context: ...y useDecwithout any changes. As an example we will useDecCoinwhich is a common...(AS_A_NN_COMMA)
Additional comments not posted (112)
math/go.mod (5)
3-3: Upgrade to Go 1.21The Go version has been updated to 1.21. Ensure that the codebase is compatible with this version and take advantage of any new features or performance improvements.
6-6: New dependency: github.com/cockroachdb/apd/v3The
github.com/cockroachdb/apd/v3dependency has been introduced. This library is crucial for decimal arithmetic operations. Ensure its usage is well-documented and tested.
8-8: Updated dependency: golang.org/x/expThe
golang.org/x/expdependency has been updated. Verify that the changes in the new version do not introduce any breaking changes or regressions.
13-30: Indirect dependencies addedSeveral indirect dependencies have been added. Ensure these dependencies are necessary and do not introduce any security vulnerabilities or bloat.
33-38: Direct dependencies updatedDirect dependencies such as
cosmossdk.io/errors,github.com/cosmos/cosmos-sdk, and others have been updated. Verify that these updates are compatible with the existing codebase.math/math.go (4)
13-18: Ensure precision and traps are appropriateThe
exactContextconfiguration should be reviewed to ensure that the precision and traps settings are appropriate for all use cases.
20-23: Function Add: Ensure proper error handlingThe
Addfunction should ensure that any errors returned byx.Add(y)are properly handled.
40-54: Function SafeSubBalance: Ensure error handling consistencyThe
SafeSubBalancefunction should ensure that error handling is consistent with other functions.
56-73: Function SafeAddBalance: Ensure error handling consistencyThe
SafeAddBalancefunction should ensure that error handling is consistent with other functions.x/group/internal/math/dec.go (1)
10-10: Updated dependency: github.com/cockroachdb/apd/v3The
apdpackage has been updated to version 3. Ensure that the new version is compatible with the existing code and does not introduce any breaking changes.docs/build/building-modules/18-decimal-handling.md (5)
51-54: LGTM!The section clearly explains the migration process and the impact of precision changes.
62-74: LGTM!The section provides clear examples of byte representation changes between
LegacyDecandDec.
76-79: LGTM!The section clearly explains the impact of the increased precision from 18 to 34 decimal places.
80-112: LGTM!The section provides a clear example of the state-breaking change in protobuf definitions.
114-128: LGTM!The section provides clear methods for converting between
LegacyDecandDec.x/group/go.mod (3)
3-3: Verify compatibility with Go 1.22.Ensure that the project is compatible with Go 1.22.
6-12: Verify dependency changes.Ensure that downgrading the dependencies does not affect compatibility and functionality. Verify the compatibility of the upgraded
github.com/cockroachdb/apddependency.Also applies to: 21-22
Line range hint
41-67: Verify indirect dependency changes.Ensure that updating the indirect dependencies does not affect compatibility and functionality.
Also applies to: 73-165
go.mod (3)
81-81: Verify new indirect dependency.Ensure that the new indirect dependency on
github.com/cockroachdb/apd/v3does not affect compatibility and functionality.
Line range hint
201-212: Verify retract directives.Ensure that the updated retract directives are correct and do not affect the functionality.
Verification successful
Retract directives are correct and do not affect functionality.
The retract directives for versions
v0.50.0andv0.46.12are syntactically correct and serve their purpose of preventing these specific versions from being used in the build process. The references found in the codebase are primarily in documentation and do not impact the functionality.
- go.mod:
v0.50.0- go.mod:
v0.46.12Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the retract directives. # Test: Ensure that the project builds successfully with the updated retract directives. go mod tidy go build ./...Length of output: 170
Script:
#!/bin/bash # Description: Verify the correctness of the retract directives by searching for references to the retracted versions in the codebase. # Search for references to the retracted version v0.50.0 rg 'v0\.50\.0' # Search for references to the retracted version v0.46.12 rg 'v0\.46\.12'Length of output: 40568
191-191: Verify replace directives.Ensure that the replace directives for
cosmossdk.io/logandcosmossdk.io/storehave been correctly removed, and the replace directive forcosmossdk.io/mathis correct.math/dec_legacy.go (47)
19-36: Ensure constant values are well-documented.The constants defined here are crucial for the precision and arithmetic operations. Ensure they are well-documented and their values are correct.
38-46: Ensure global variables are well-documented.Global variables like
precisionReuse,fivePrecision, andprecisionMultipliersare critical for the arithmetic operations. Ensure they are well-documented and initialized correctly.
55-61: Initialize precision multipliers correctly.The
initfunction initializes theprecisionMultipliersarray. Ensure that the initialization logic is correct and handles edge cases.
67-69: Use consistent naming for zero, one, and smallest decimal values.The functions
LegacyZeroDec,LegacyOneDec, andLegacySmallestDecprovide consistent naming for zero, one, and smallest decimal values. Ensure they are used consistently throughout the code.
97-109: Ensure correct precision handling inLegacyNewDecandLegacyNewDecWithPrec.The functions
LegacyNewDecandLegacyNewDecWithPreccreate newLegacyDecinstances with specified precision. Ensure that the precision handling is correct and does not lead to overflow or underflow.
125-137: Ensure correct precision handling inLegacyNewDecFromIntandLegacyNewDecFromIntWithPrec.The functions
LegacyNewDecFromIntandLegacyNewDecFromIntWithPreccreate newLegacyDecinstances fromIntwith specified precision. Ensure that the precision handling is correct and does not lead to overflow or underflow.
139-202: Handle errors correctly inLegacyNewDecFromStr.The function
LegacyNewDecFromStrcreates aLegacyDecfrom a string. Ensure that errors are handled correctly and meaningful error messages are provided.
204-211: Panic on error inLegacyMustNewDecFromStr.The function
LegacyMustNewDecFromStrcreates aLegacyDecfrom a string and panics on error. Ensure that this behavior is documented and used appropriately.
213-227: Provide meaningful method names forLegacyDecoperations.The methods
IsNil,IsZero,IsNegative,IsPositive,Equal,GT,GTE,LT,LTE,Neg,NegMut,Abs,AbsMut,Set, andCloneprovide meaningful names forLegacyDecoperations. Ensure they are used consistently throughout the code.
229-237: Return a copy of the underlyingbig.IntinBigInt.The method
BigIntreturns a copy of the underlyingbig.Int. Ensure that it does not mutate the original value.
239-246: Ensure mutative operations are thread-safe.The method
BigIntMutreturns a mutable reference to the underlyingbig.Int. Ensure that it is used in a thread-safe manner.
248-261: Ensure immutative operations are thread-safe.The methods
ImmutOp,ImmutOpInt, andImmutOpInt64perform immutative operations onLegacyDec. Ensure that they are used in a thread-safe manner.
263-267: Ensure correct precision handling inSetInt64.The method
SetInt64sets the value ofLegacyDecfrom anint64and ensures correct precision handling. Ensure that it does not lead to overflow or underflow.
269-282: Ensure correct handling of addition operations.The methods
AddandAddMutperform addition operations onLegacyDec. Ensure that they handle overflow correctly and provide meaningful error messages.
284-297: Ensure correct handling of subtraction operations.The methods
SubandSubMutperform subtraction operations onLegacyDec. Ensure that they handle overflow correctly and provide meaningful error messages.
299-314: Ensure correct handling of multiplication operations.The methods
MulandMulMutperform multiplication operations onLegacyDec. Ensure that they handle overflow correctly and provide meaningful error messages.
316-330: Ensure correct handling of multiplication truncate operations.The methods
MulTruncateandMulTruncateMutperform multiplication truncate operations onLegacyDec. Ensure that they handle overflow correctly and provide meaningful error messages.
332-346: Ensure correct handling of multiplication round-up operations.The methods
MulRoundUpandMulRoundUpMutperform multiplication round-up operations onLegacyDec. Ensure that they handle overflow correctly and provide meaningful error messages.
348-359: Ensure correct handling of multiplication withInt.The methods
MulIntandMulIntMutperform multiplication operations onLegacyDecwithInt. Ensure that they handle overflow correctly and provide meaningful error messages.
361-373: Ensure correct handling of multiplication withint64.The methods
MulInt64andMulInt64Mutperform multiplication operations onLegacyDecwithint64. Ensure that they handle overflow correctly and provide meaningful error messages.
375-393: Ensure correct handling of quotient operations.The methods
Quo,QuoMut,QuoTruncate,QuoTruncateMut,QuoRoundUp, andQuoRoundupMutperform quotient operations onLegacyDec. Ensure that they handle divide-by-zero errors and overflow correctly, providing meaningful error messages.
431-449: Ensure correct handling of quotient operations withIntandint64.The methods
QuoInt,QuoIntMut,QuoInt64, andQuoInt64Mutperform quotient operations onLegacyDecwithIntandint64. Ensure that they handle divide-by-zero errors and overflow correctly, providing meaningful error messages.
451-498: Ensure correct handling of approximate root operations.The method
ApproxRootperforms approximate root operations onLegacyDec. Ensure that it handles negative inputs, overflow, and convergence issues correctly, providing meaningful error messages.
500-523: Ensure correct handling of power operations.The methods
PowerandPowerMutperform power operations onLegacyDec. Ensure that they handle zero and negative powers, overflow, and convergence issues correctly, providing meaningful error messages.
525-529: Ensure correct handling of approximate square root operations.The method
ApproxSqrtperforms approximate square root operations onLegacyDec. Ensure that it handles negative inputs, overflow, and convergence issues correctly, providing meaningful error messages.
531-534: Ensure correct handling of integer checks.The method
IsIntegerchecks if theLegacyDecis an integer. Ensure that it handles edge cases and provides meaningful error messages.
536-542: Ensure correct handling of formatting operations.The method
Formatformats theLegacyDecas a string. Ensure that it handles edge cases and provides meaningful error messages.
544-594: Ensure correct handling of string conversion.The method
Stringconverts theLegacyDecto a string. Ensure that it handles edge cases, such as negative values and trailing zeros, correctly and provides meaningful error messages.
596-610: Ensure correct handling of float64 conversion.The methods
Float64andMustFloat64convert theLegacyDecto afloat64. Ensure that they handle conversion errors correctly and provide meaningful error messages.
621-655: Ensure correct handling of precision chopping and rounding.The methods
chopPrecisionAndRound,chopPrecisionAndRoundUp,chopPrecisionAndRoundNonMutative,RoundInt64, andRoundIntperform precision chopping and rounding operations onLegacyDec. Ensure that they handle edge cases and provide meaningful error messages.
657-707: Ensure correct handling of precision chopping and truncation.The methods
chopPrecisionAndTruncate,chopPrecisionAndTruncateNonMutative,TruncateInt64,TruncateInt, andTruncateDecperform precision chopping and truncation operations onLegacyDec. Ensure that they handle edge cases and provide meaningful error messages.
729-748: Ensure correct handling of ceiling operations.The method
Ceilreturns the smallest integer value that is greater than or equal to the givenLegacyDec. Ensure that it handles edge cases and provides meaningful error messages.
751-757: Ensure correct handling of sortable decimal values.The
LegacyMaxSortableDecandLegacyValidSortableDecconstants and theLegacySortableDecBytesfunction ensure thatLegacyDecvalues are within sortable bounds. Ensure that they handle edge cases and provide meaningful error messages.
789-795: Reuse nil values for JSON marshaling.The
nilJSONvariable reuses nil values for JSON marshaling. Ensure that it is initialized correctly and used consistently throughout the code.
798-804: Ensure correct handling of JSON marshaling.The method
MarshalJSONmarshals theLegacyDecto JSON. Ensure that it handles nil values and provides meaningful error messages.
806-826: Ensure correct handling of JSON unmarshaling.The method
UnmarshalJSONunmarshals theLegacyDecfrom JSON. Ensure that it handles nil values and provides meaningful error messages.
828-831: Ensure correct handling of YAML marshaling.The method
MarshalYAMLmarshals theLegacyDecto YAML. Ensure that it handles edge cases and provides meaningful error messages.
833-861: Ensure correct handling of protobuf marshaling.The methods
Marshal,MarshalTo, andUnmarshalimplement the gogo proto custom type interface forLegacyDec. Ensure that they handle edge cases and provide meaningful error messages.
863-889: Ensure correct handling of protobuf size calculation.The method
Sizeimplements the gogo proto custom type interface forLegacyDec. Ensure that it handles edge cases and provides meaningful error messages.
891-893: Ensure correct handling of Amino marshaling.The methods
MarshalAminoandUnmarshalAminoimplement Amino binary serialization forLegacyDec. Ensure that they handle edge cases and provide meaningful error messages.
897-909: Ensure correct handling of decimal equality checks.The function
LegacyDecsEqualchecks if two decimal arrays are equal. Ensure that it handles edge cases and provides meaningful error messages.
911-917: Ensure correct handling of minimum decimal calculation.The function
LegacyMinDecreturns the minimum decimal between twoLegacyDecvalues. Ensure that it handles edge cases and provides meaningful error messages.
919-925: Ensure correct handling of maximum decimal calculation.The function
LegacyMaxDecreturns the maximum decimal between twoLegacyDecvalues. Ensure that it handles edge cases and provides meaningful error messages.
927-931: Ensure correct handling of decimal equality assertion.The function
LegacyDecEqis intended to be used with require/assert to check decimal equality. Ensure that it handles edge cases and provides meaningful error messages.
933-937: Ensure correct handling of approximate decimal equality assertion.The function
LegacyDecApproxEqis intended to be used with require/assert to check approximate decimal equality. Ensure that it handles edge cases and provides meaningful error messages.
939-969: Ensure correct handling of decimal formatting.The function
FormatDecformats a decimal (as encoded in protobuf) into a value-rendered string following ADR-050. Ensure that it handles edge cases and provides meaningful error messages.
971-973: Ensure correct conversion fromLegacyDectoDec.The function
LegacyDecToDecconverts aLegacyDecto aDec. Ensure that it handles edge cases and provides meaningful error messages.math/dec_legacy_test.go (1)
1-25: Ensure correct initialization of test suite.The
decimalTestSuitestruct and `math/dec_test.go (14)
14-114: Comprehensive test cases forNewDecFromString.The test cases cover a wide range of scenarios, including valid and invalid decimals, scientific notation, and edge cases. This ensures robustness in the
NewDecFromStringfunction.
154-261: Comprehensive test cases forAdd.The test cases cover a wide range of scenarios, including zero, positive, negative, and extreme values. This ensures robustness in the
Addfunction.
265-397: Comprehensive test cases forSub.The test cases cover a wide range of scenarios, including zero, positive, negative, and extreme values. This ensures robustness in the
Subfunction.
597-776: Comprehensive test cases forQuoExact.The test cases cover a wide range of scenarios, including zero, positive, negative, and extreme values. This ensures robustness in the
QuoExactfunction.
780-889: Comprehensive test cases forQuoInteger.The test cases cover a wide range of scenarios, including zero, positive, negative, and extreme values. This ensures robustness in the
QuoIntegerfunction.
893-961: Comprehensive test cases forModulo.The test cases cover a wide range of scenarios, including zero, positive, negative, and extreme values. This ensures robustness in the
Modulofunction.
965-1003: Comprehensive test cases forNumDecimalPlaces.The test cases cover a wide range of scenarios, including integers, decimals with various precision, and negative values. This ensures robustness in the
NumDecimalPlacesfunction.
1007-1063: Comprehensive test cases forCmp.The test cases cover a wide range of scenarios, including equality, less than, and greater than comparisons. This ensures robustness in the
Cmpfunction.
1067-1113: Comprehensive test cases forReduce.The test cases cover a wide range of scenarios, including positive, negative, and zero values. This ensures robustness in the
Reducefunction.
1117-1224: Comprehensive test cases forMulExact.The test cases cover a wide range of scenarios, including zero, positive, negative, and extreme values. This ensures robustness in the
MulExactfunction.
1228-1248: Comprehensive test cases forToBigInt.The test cases cover a wide range of scenarios, including valid and invalid conversions. This ensures robustness in the
ToBigIntfunction.
1253-1274: Comprehensive test cases forToSdkInt.The test cases cover a wide range of scenarios, including valid and invalid conversions. This ensures robustness in the
ToSdkIntfunction.
1278-1281: Test case for invalid decimal string "iNf".The test case ensures that creating a
Decfrom the string "iNf" results in an error, which is the expected behavior.
1284-1288: Helper functionmustis correctly implemented.The
mustfunction panics if an error occurs, which is useful for simplifying test case setup.math/dec.go (25)
62-75: Thorough error handling inNewDecFromString.The method effectively handles various input formats and error conditions, ensuring robustness.
85-88: Simple and correct implementation ofNewDecFromInt64.The method correctly sets the value using
apd.Decimal.SetInt64.
96-99: Correct implementation ofNewDecWithPrec.The method correctly sets the value using
apd.Decimal.SetFinite.
114-121: Immutability and proper error handling inAdd.The method ensures immutability and proper error handling, returning a new
Decinstance.
138-146: Immutability and proper error handling inSub.The method ensures immutability and proper error handling, returning a new
Decinstance.
169-176: Proper error handling inQuo.The method ensures proper error handling and returns a new
Decinstance.
187-197: Proper error handling and rounding checks inMulExact.The method ensures proper error handling and checks for rounding conditions, returning a new
Decinstance.
219-228: Proper error handling and rounding checks inQuoExact.The method ensures proper error handling and checks for rounding conditions, returning a new
Decinstance.
245-251: Proper error handling inQuoInteger.The method ensures proper error handling and returns a new
Decinstance.
256-262: Proper error handling inModulo.The method ensures proper error handling and returns a new
Decinstance.
267-270: Proper error handling inMul.The method ensures proper error handling and returns a new
Decinstance.
275-276: Correct implementation ofInt64.The method correctly uses
apd.Decimal.Int64to perform the conversion.
281-288: Correct implementation ofBigInt.The method correctly handles the conversion and checks for non-integral values.
294-310: Correct implementation ofSdkIntTrim.The method correctly handles rounding and conversion, ensuring proper handling of both positive and negative values.
313-314: Correct implementation ofString.The method correctly uses
apd.Decimal.Textto perform the conversion.
322-323: Correct implementation ofCmp.The method correctly uses
apd.Decimal.Cmpto perform the comparison.
328-329: Correct implementation ofEqual.The method correctly uses
apd.Decimal.Cmpto perform the equality check.
333-334: Correct implementation ofIsZero.The method correctly uses
apd.Decimal.IsZeroto perform the check.
338-339: Correct implementation ofIsNegative.The method correctly uses
apd.Decimal.Negativeandapd.Decimal.IsZeroto perform the check.
343-344: Correct implementation ofIsPositive.The method correctly uses
apd.Decimal.Negativeandapd.Decimal.IsZeroto perform the check.
348-349: Correct implementation ofIsFinite.The method correctly uses
apd.Decimal.Formto perform the check.
353-358: Correct implementation ofNumDecimalPlaces.The method correctly calculates the number of decimal places based on the exponent.
363-366: Correct implementation ofReduce.The method correctly uses
apd.Decimal.Reduceto perform the operation.
369-372: Correct implementation ofMarshal.The method correctly uses
apd.Decimal.MarshalTextto perform the serialization.
377-378: Correct implementation ofUnmarshal.The method correctly uses
apd.Decimal.UnmarshalTextto perform the parsing.math/dec_rapid_test.go (4)
15-155: Comprehensive property-based tests inTestDecWithRapid.The test function covers various properties of the
Dectype, ensuring comprehensive testing.
179-185: Correct property-based test intestDecInt64.The test function correctly verifies the property that
n == NewDecFromInt64(n).Int64().
187-192: Correct property-based test intestInvalidNewDecFromString.The test function correctly verifies the property that invalid number strings result in errors.
196-205: Correct property-based test intestInvalidNewNonNegativeDecFromString.The test function correctly verifies the property that invalid or negative number strings result in errors.
Co-authored-by: Alexander Peters <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Commits
Files that changed from the base of the PR and between 0470327c41dab958ad2fcc96601a215c42d0f7f2 and 78937cc.
Files ignored due to path filters (2)
go.sumis excluded by!**/*.summath/go.sumis excluded by!**/*.sum
Files selected for processing (9)
- docs/build/building-modules/18-decimal-handling.md (1 hunks)
- go.mod (2 hunks)
- math/dec.go (1 hunks)
- math/dec_bench_test.go (1 hunks)
- math/dec_legacy.go (1 hunks)
- math/dec_legacy_test.go (1 hunks)
- math/dec_rapid_test.go (1 hunks)
- math/dec_test.go (1 hunks)
- math/go.mod (1 hunks)
Files skipped from review as they are similar to previous changes (8)
- go.mod
- math/dec.go
- math/dec_bench_test.go
- math/dec_legacy.go
- math/dec_legacy_test.go
- math/dec_rapid_test.go
- math/dec_test.go
- math/go.mod
Additional context used
Path-based instructions (1)
docs/build/building-modules/18-decimal-handling.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
LanguageTool
docs/build/building-modules/18-decimal-handling.md
[typographical] ~25-~25: Consider adding a comma after this introductory phrase.
Context: ...y useDecwithout any changes. As an example we will useDecCoinwhich is a common...(AS_A_NN_COMMA)
Additional comments not posted (6)
docs/build/building-modules/18-decimal-handling.md (6)
6-13: Introduction section looks good.The content provides a clear overview of the two decimal types and the changes involved in migrating from
LegacyDectoDec.
15-19: Why the Change? section looks good.The content clearly explains the reasons for migrating to
Dec, including enhanced precision, immutable operations, and better performance.
51-53: Modules migrating from LegacyDec to Dec section looks good.The content clearly explains the need for updating modules to use
Decand mentions that these changes are state-breaking.
55-60: Precision Handling section looks good.The content clearly explains the differences in precision handling between
LegacyDecandDec.
62-74: Byte Representation Changes Example section looks good.The content clearly explains how the byte representation of decimal values changes between
LegacyDecandDec.
114-128: Converting LegacyDec to Dec without storing the data section looks good.The content clearly provides methods for converting between
LegacyDecandDecwithout changing how the data is stored.
| ## 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.
Add a comma after the introductory phrase.
Consider adding a comma after the introductory phrase 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## 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. | |
| ## 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. |
Tools
LanguageTool
[typographical] ~25-~25: Consider adding a comma after this introductory phrase.
Context: ...y useDecwithout any changes. As an example we will useDecCoinwhich is a common...(AS_A_NN_COMMA)
|
We should have a discussion to align on encoding. I suggest having a deterministic output (no trailing zeros) and flexible input. We must explicitly reject NaN and Infinity though. Can we have the results of running benchmarks documented somewhere? |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (13)
docs/build/building-modules/18-decimal-handling.md (13)
8-8: Add a comma after the introductory phrase.Consider adding a comma after "In the Cosmos SDK" for better readability.
- In the Cosmos SDK, there are two types of decimals: `LegacyDec` and `Dec`.
8-8: Clarify the source of the new implementation.Specify that the implementation of
Decis adapted from theregen-ledgerproject for better clarity.- The implementation of `Dec` is adapted from Regen Network's `regen-ledger`, specifically from [this module](https://github.com/regen-network/regen-ledger/tree/main/types/math). + The implementation of `Dec` is adapted from Regen Network's `regen-ledger` project, specifically from [this module](https://github.com/regen-network/regen-ledger/tree/main/types/math).
10-10: Improve parallel structure in bullet points.Ensure that both bullet points use a consistent structure for better readability.
- * **Data Format**: The internal representation of decimals changes, affecting how data is stored and processed. - * **Precision Handling**: `Dec` supports flexible precision up to 34 decimal places, unlike `LegacyDec` which has a fixed precision of 18 decimal places. + * **Data Format**: The internal representation of decimals changes, affecting how data is stored and processed. + * **Precision Handling**: The `Dec` type supports flexible precision up to 34 decimal places, unlike `LegacyDec`, which has a fixed precision of 18 decimal places.
17-17: Add a comma after the introductory phrase.Consider adding a comma after "Historically" for better readability.
- Historically we have wrapped a `big.Int` to represent decimals in the Cosmos SDK and never had a decimal type. + Historically, we have wrapped a `big.Int` to represent decimals in the Cosmos SDK and never had a decimal type.Tools
LanguageTool
[typographical] ~17-~17: Consider adding a comma after ‘Historically’ for more clarity.
Context: ...and flexibility. ## Why the Change? * Historically we have wrapped abig.Intto represen...(RB_LY_COMMA)
[uncategorized] ~17-~17: Possible missing comma found.
Context: ...nt` to represent decimals in the Cosmos SDK and never had a decimal type. Finally, ...(AI_HYDRA_LEO_MISSING_COMMA)
17-17: Clarify the historical context.Specify that the historical context refers to the previous lack of a dedicated decimal type.
- Historically, we have wrapped a `big.Int` to represent decimals in the Cosmos SDK and never had a decimal type. + Historically, we have wrapped a `big.Int` to represent decimals in the Cosmos SDK because we never had a dedicated decimal type.Tools
LanguageTool
[typographical] ~17-~17: Consider adding a comma after ‘Historically’ for more clarity.
Context: ...and flexibility. ## Why the Change? * Historically we have wrapped abig.Intto represen...(RB_LY_COMMA)
[uncategorized] ~17-~17: Possible missing comma found.
Context: ...nt` to represent decimals in the Cosmos SDK and never had a decimal type. Finally, ...(AI_HYDRA_LEO_MISSING_COMMA)
20-20: Clarify performance comparison.Specify that the performance comparison is between
DecandLegacyDec.- `Dec` operations are faster and more efficient than `LegacyDec`. + `Dec` operations are faster and more efficient compared to `LegacyDec`.
24-24: Add a comma after the introductory phrase.Consider adding a comma after "If you are creating a new module or updating an existing module that has not used
LegacyDec" for better readability.- 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.
24-24: Clarify the context of the examples.Specify that the examples show how to replace
LegacyDecwithDec.- 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. + 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. The following examples show how to replace `LegacyDec` with `Dec`:
47-47: Clarify the context of the migration.Specify that the migration involves updating the module to use the new decimal type.
- When migrating from `LegacyDec` to `Dec`, you need to update your module to use the new decimal type. **These types are state breaking changes and require a migration.** + When migrating from `LegacyDec` to `Dec`, you need to update your module to use the new decimal type. **These changes are state-breaking and require a migration.**
51-51: Clarify the reason for the state-breaking change.Specify that the state-breaking change is due to the difference in precision handling.
- The reason for the state breaking change is the difference in precision handling between the two decimal types: + The reason for the state-breaking change is the difference in precision handling between the two decimal types:
58-58: Clarify the impact of the precision change.Specify that the change in data formatting and storage is a key aspect of the state-breaking transition.
- The increase in precision from 18 to 34 decimal places allows for more detailed decimal values but requires data migration. This change in how data is formatted and stored is a key aspect of why the transition is considered state-breaking. + The increase in precision from 18 to 34 decimal places allows for more detailed decimal values but requires data migration. This change in data formatting and storage is a key aspect of why the transition is considered state-breaking.
62-62: Add a comma after the introductory phrase.Consider adding a comma after "If you would like to convert a
LegacyDecto aDecwithout a state migration" for better readability.- If you would like to convert a `LegacyDec` to a `Dec` without a state migration changing how the data is handled internally within the application logic and not how it's stored or represented. + If you would like to convert a `LegacyDec` to a `Dec` without a state migration, changing how the data is handled internally within the application logic and not how it's stored or represented.Tools
LanguageTool
[uncategorized] ~62-~62: Possible missing comma found.
Context: ...LegacyDecto aDecwithout a state migration changing how the data is handled intern...(AI_HYDRA_LEO_MISSING_COMMA)
62-62: Clarify the context of the conversion methods.Specify that the methods are for converting
LegacyDectoDecwithout changing data storage or representation.- If you would like to convert a `LegacyDec` to a `Dec` without a state migration changing how the data is handled internally within the application logic and not how it's stored or represented. You can use the following methods. + If you would like to convert a `LegacyDec` to a `Dec` without a state migration, changing how the data is handled internally within the application logic and not how it's stored or represented, you can use the following methods:Tools
LanguageTool
[uncategorized] ~62-~62: Possible missing comma found.
Context: ...LegacyDecto aDecwithout a state migration changing how the data is handled intern...(AI_HYDRA_LEO_MISSING_COMMA)
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- docs/build/building-modules/18-decimal-handling.md (1 hunks)
Additional context used
Path-based instructions (1)
docs/build/building-modules/18-decimal-handling.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
LanguageTool
docs/build/building-modules/18-decimal-handling.md
[typographical] ~17-~17: Consider adding a comma after ‘Historically’ for more clarity.
Context: ...and flexibility. ## Why the Change? * Historically we have wrapped abig.Intto represen...(RB_LY_COMMA)
[uncategorized] ~17-~17: Possible missing comma found.
Context: ...nt` to represent decimals in the Cosmos SDK and never had a decimal type. Finally, ...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~62-~62: Possible missing comma found.
Context: ...LegacyDecto aDecwithout a state migration changing how the data is handled intern...(AI_HYDRA_LEO_MISSING_COMMA)
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
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:
Summary by CodeRabbit
New Features
LegacyDecto the newDectype.Dectype, supporting up to 34 decimal places.Dectype in existing modules.Documentation
LegacyDecandDec, including migration steps and examples.