feat(math): Upstream GDA based decimal type - errors #20827
Conversation
|
Warning Review failedFailed to post review comments. Configuration used: .coderabbit.yml Files ignored due to path filters (3)
Files selected for processing (12)
Files not summarized due to errors (2)
Files not reviewed due to errors (1)
Files skipped from review due to trivial changes (1)
Additional context usedPath-based instructions (9)
LanguageTool
Markdownlint
Additional comments not posted (106)
Comments failed to post (6)
WalkthroughWalkthroughThe changes focus on replacing the older Changes
Sequence Diagram(s)No sequence diagrams are needed as the changes are mostly internal enhancements and refactoring without significant changes to the control flow. Possibly related issues
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
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)
Files not summarized due to errors (2)
- math/dec_legacy_test.go: Error: Message exceeds token limit
- math/dec_test.go: Error: Message exceeds token limit
Files not reviewed due to errors (1)
- math/dec_rapid_test.go (no review received)
Files skipped from review due to trivial changes (1)
- math/go.mod
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.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.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_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_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"
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)
[uncategorized] ~116-~116: Possible missing comma found.
Context: ...LegacyDecto aDecwithout a state migration changing how the data is handled intern...(AI_HYDRA_LEO_MISSING_COMMA)
Markdownlint
docs/build/building-modules/18-decimal-handling.md
45-45: Column: 1
Hard tabs(MD010, no-hard-tabs)
27-27: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
130-130: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
Additional comments not posted (106)
math/math.go (4)
20-23: LGTM!The
Addfunction correctly adds twoDecvalues and returns the result. The error handling is appropriate.
25-38: LGTM!The
SubNonNegativefunction correctly subtractsyfromxand returns an error if the result is negative. The error handling and logic are appropriate.
40-54: LGTM!The
SafeSubBalancefunction correctly subtractsyfromxwith arbitrary precision and returns an error if the result is negative. The error handling and logic are appropriate.
56-73: LGTM!The
SafeAddBalancefunction correctly addsxandywith arbitrary precision and returns an error if either value is negative. The error handling and logic are appropriate.x/group/internal/math/dec.go (7)
Line range hint
17-23: LGTM!The
NewPositiveDecFromStringfunction correctly creates a newDecfrom a string and ensures it is positive. The error handling and logic are appropriate.
Line range hint
25-31: LGTM!The
NewNonNegativeDecFromStringfunction correctly creates a newDecfrom a string and ensures it is non-negative. The error handling and logic are appropriate.
Line range hint
37-46: LGTM!The
NewDecFromStringfunction correctly creates a newDecfrom a string and ensures it is a finite number. The error handling and logic are appropriate.
Line range hint
60-64: LGTM!The
Addfunction correctly adds twoDecvalues and returns the result with error handling for overflow. The logic and error handling are appropriate.
Line range hint
66-70: LGTM!The
Subfunction correctly subtractsyfromxand returns the result with error handling for overflow. The logic and error handling are appropriate.
Line range hint
87-92: LGTM!The
Quofunction correctly dividesxbyyand returns the result with error handling for overflow. The logic and error handling are appropriate.
Line range hint
94-106: LGTM!The
SubNonNegativefunction correctly subtractsyfromxand returns an error if the result is negative. The error handling and logic are appropriate.docs/build/building-modules/18-decimal-handling.md (7)
1-14: LGTM!The introduction section clearly explains the transition from
LegacyDectoDecand the reasons for the change. The content is clear and informative.
15-19: LGTM! Consider adding a comma.The section explains the benefits of using
DecoverLegacyDec. The content is clear and informative. Consider adding a comma after "concurrent use".- `Dec` operations are safer for concurrent use as they do not mutate the original values. + `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)
21-25: LGTM! Consider adding a comma.The section explains how to use
Decin new modules and provides an example. Consider adding 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)
51-53: LGTM!The section explains the steps required to migrate from
LegacyDectoDec. The content is clear and informative.
55-61: LGTM!The section explains the differences in precision handling between
LegacyDecandDec. The content is clear and informative.
62-74: LGTM!The section provides examples of byte representation changes due to different precision handling. The content is clear and informative.
114-128: LGTM! Consider adding a comma.The section explains how to convert
LegacyDectoDecwithout state migration and provides example code. Consider adding a comma after "without a state migration".- 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] ~116-~116: Possible missing comma found.
Context: ...LegacyDecto aDecwithout a state migration changing how the data is handled intern...(AI_HYDRA_LEO_MISSING_COMMA)
x/group/go.mod (2)
Line range hint
3-36: LGTM!The dependency updates in the
requiresection appear correct and consistent with the rest of the changes in the PR.
Line range hint
167-176: LGTM!The module replacements in the
replacesection appear correct and consistent with the rest of the changes in the PR.math/dec.go (17)
78-97: Ensure edge cases are handled and constraints are applied correctly.The
NewDecFromStringmethod correctly handles edge cases such as NaN, signaling NaN, and infinite values. It also applies constraints provided as variadic arguments. Ensure that all constraints are properly tested.
101-104: LGTM!The
NewDecFromInt64method correctly initializes aDecfrom an int64 value.
107-111: LGTM!The
NewDecWithPrecmethod correctly initializes aDecwith a specified coefficient and exponent.
114-124: LGTM!The
Addmethod correctly performs an addition operation between twoDecvalues and handles errors appropriately.
151-163: LGTM!The
MulExactmethod correctly performs a multiplication operation without rounding and handles errors appropriately.
179-187: LGTM!The
QuoIntegermethod correctly performs an integer division operation between twoDecvalues and handles errors appropriately.
206-209: LGTM!The
Int64method correctly converts aDecto an int64 value.
212-222: LGTM!The
BigIntmethod correctly converts aDecto a*big.Intvalue and handles errors appropriately.
224-242: LGTM!The
SdkIntTrimmethod correctly rounds aDectowards zero and converts it tosdkmath.Int. It handles negative values appropriately.
245-246: LGTM!The
Stringmethod correctly converts aDecto its string representation.
249-255: LGTM!The
Cmpmethod correctly compares twoDecvalues and returns the result.
258-259: LGTM!The
Equalmethod correctly checks if twoDecvalues are equal.
262-264: LGTM!The
IsZeromethod correctly checks if aDecvalue is zero.
267-269: LGTM!The
IsNegativemethod correctly checks if aDecvalue is negative.
272-274: LGTM!The
IsPositivemethod correctly checks if aDecvalue is positive.
198-203: Fix error wrapping inMulmethod.The
Mulmethod incorrectly wraps the error. Remove the wrapping.- return z, errors.Wrap(err, "decimal multiplication error") + return z, nilLikely invalid or redundant comment.
190-195: Fix error wrapping inRemmethod.The
Remmethod incorrectly wraps the error. Remove the wrapping.- return z, errors.Wrap(err, "decimal remainder error") + return z, nilLikely invalid or redundant comment.
go.mod (2)
81-81: Dependency update:github.com/cockroachdb/apd/v3The dependency for
github.com/cockroachdb/apd/v3has been updated to versionv3.2.1. Ensure compatibility with the rest of the codebase.
191-191: Module replacement:cosmossdk.io/mathThe module
cosmossdk.io/mathis replaced with a local path./math. Ensure this replacement is temporary and will be updated with the final version.math/dec_bench_test.go (4)
1-7: Ensure sufficient benchmark coverage.The benchmark tests cover various arithmetic operations and serialization for both
LegacyDecandDec. Ensure that all critical operations are benchmarked.
9-63: LGTM!The benchmark
BenchmarkCompareLegacyDecAndNewDecQuotientcorrectly compares the performance of division operations betweenLegacyDecandDec.
65-120: LGTM!The benchmark
BenchmarkCompareLegacyDecAndNewDecSumcorrectly compares the performance of addition operations betweenLegacyDecandDec.
122-179: LGTM!The benchmark
BenchmarkCompareLegacyDecAndNewDecSubcorrectly compares the performance of subtraction operations betweenLegacyDecandDec.math/dec_test.go (13)
14-124: LGTM! Comprehensive test coverage forNewDecFromString.The test cases cover various scenarios for the
NewDecFromStringfunction, ensuring robust validation of expected outcomes.
128-160: LGTM! Comprehensive test coverage forNewDecFromInt64.The test cases cover various scenarios for the
NewDecFromInt64function, ensuring robust validation of expected outcomes.
164-271: LGTM! Comprehensive test coverage forAdd.The test cases cover various scenarios for the
Addfunction, ensuring robust validation of expected outcomes.
275-408: LGTM! Comprehensive test coverage forSub.The test cases cover various scenarios for the
Subfunction, ensuring robust validation of expected outcomes.
412-506: LGTM! Comprehensive test coverage forQuo.The test cases cover various scenarios for the
Quofunction, ensuring robust validation of expected outcomes.
510-604: LGTM! Comprehensive test coverage forQuoExact.The test cases cover various scenarios for the
QuoExactfunction, ensuring robust validation of expected outcomes.
608-702: LGTM! Comprehensive test coverage forQuoInteger.The test cases cover various scenarios for the
QuoIntegerfunction, ensuring robust validation of expected outcomes.
718-764: LGTM! Comprehensive test coverage forReduce.The test cases cover various scenarios for the
Reducefunction, ensuring robust validation of expected outcomes.
768-842: LGTM! Comprehensive test coverage forMulExact.The test cases cover various scenarios for the
MulExactfunction, ensuring robust validation of expected outcomes.
846-867: LGTM! Comprehensive test coverage forBigInt.The test cases cover various scenarios for the
BigIntfunction, ensuring robust validation of expected outcomes.
871-891: LGTM! Comprehensive test coverage forSdkIntTrim.The test cases cover various scenarios for the
SdkIntTrimfunction, ensuring robust validation of expected outcomes.
896-899: LGTM! Test coverage for invalid decimal string "inf".The test case ensures that invalid decimal strings like "inf" are correctly handled.
902-906: LGTM! Simple and effective error handling helper.The
mustfunction is correctly implemented to panic if an error occurs.math/dec_legacy.go (20)
1-11: LGTM!The package and import statements are correct and include all necessary packages.
13-36: LGTM!The
LegacyDecstruct and constants are well-defined and provide necessary precision and configuration.
38-61: LGTM!The variable declarations and
initfunction are correct and initialize necessary precision multipliers and other constants.
63-95: LGTM!The utility functions for precision multipliers are well-defined and provide necessary precision handling.
97-137: LGTM!The
LegacyNewDecand related functions are well-defined and provide necessary constructors forLegacyDec.
139-211: LGTM!The
LegacyNewDecFromStrand related functions are well-defined and handle string conversion toLegacyDeccorrectly.
213-227: LGTM!The
LegacyDecmethods for checking value properties are well-defined and provide necessary value property checks and operations.
229-246: LGTM!The
BigIntandBigIntMutmethods are well-defined and provide necessary conversions tobig.Int.
248-261: LGTM!The
ImmutOp,ImmutOpInt, andImmutOpInt64methods are well-defined and provide necessary immutable operations forLegacyDec.
263-267: LGTM!The
SetInt64method is well-defined and provides necessary conversion fromint64toLegacyDec.
269-373: LGTM!The arithmetic operation methods are well-defined and provide necessary arithmetic operations for
LegacyDec.
375-449: LGTM!The quotient operation methods are well-defined and provide necessary quotient operations for
LegacyDec.
451-529: LGTM!The
ApproxRoot,Power,PowerMut, andApproxSqrtmethods are well-defined and provide necessary root and power operations forLegacyDec.
531-593: LGTM!The
IsInteger,Format, andStringmethods are well-defined and provide necessary string and format operations forLegacyDec.
596-610: LGTM!The
Float64andMustFloat64methods are well-defined and provide necessary float conversion operations forLegacyDec.
612-707: LGTM!The precision chopping methods are well-defined and provide necessary precision chopping operations for
LegacyDec.
684-727: LGTM!The rounding and truncation methods are well-defined and provide necessary rounding and truncation operations for
LegacyDec.
729-748: LGTM!The
Ceilmethod is well-defined and provides necessary ceiling operation forLegacyDec.
751-787: LGTM!The
LegacyMaxSortableDec,LegacyValidSortableDec, andLegacySortableDecBytesmethods are well-defined and provide necessary sortable operations forLegacyDec.
789-893: LGTM!The JSON and YAML marshal/unmarshal methods are well-defined and provide necessary serialization and deserialization operations for
LegacyDec.math/dec_legacy_test.go (30)
23-25: LGTM!The test suite initialization is correctly implemented.
27-48: LGTM!The test cases for approximate equality are well-defined and cover various scenarios.
50-56: LGTM!The helper function for creating
LegacyDecvalues from strings is correctly implemented.
58-119: LGTM!The test cases for creating
LegacyDecvalues from strings are comprehensive and well-defined.
122-139: LGTM!The test cases for the string representation of
LegacyDecvalues are well-defined and cover various scenarios.
141-161: LGTM!The test cases for the float64 conversion of
LegacyDecvalues are well-defined and cover various scenarios.
163-196: LGTM!The test cases for comparisons of
LegacyDecvalues are well-defined and cover various scenarios.
199-219: LGTM!The test cases for the equality of slices of
LegacyDecvalues are well-defined and cover various scenarios.
221-290: LGTM!The test cases for arithmetic operations on
LegacyDecvalues are comprehensive and well-defined.
293-306: LGTM!The test cases for
MulRoundUpandMulTruncatemethods are well-defined and cover edge cases.
308-331: LGTM!The test cases for the banker’s rounding method are well-defined and cover various scenarios.
334-357: LGTM!The test cases for truncation behavior are well-defined and cover various scenarios.
360-371: LGTM!The test case for string conversion involving large numbers is well-defined and correctly implemented.
373-387: LGTM!The test cases for multiplication with integers are well-defined and cover various scenarios.
390-409: LGTM!The test cases for the ceiling method are well-defined and cover various scenarios.
411-417: LGTM!The test case for overflow scenarios in the ceiling method is well-defined and correctly implemented.
419-443: LGTM!The test cases for the power method are well-defined and cover various scenarios.
446-473: LGTM!The test cases for the approximate root method are well-defined and cover various scenarios.
476-498: LGTM!The test cases for the approximate square root method are well-defined and cover various scenarios.
501-524: LGTM!The test cases for the sortable byte representation are well-defined and cover various scenarios.
526-610: LGTM!The test cases for encoding and decoding
LegacyDecvalues are well-defined and cover various scenarios.
613-619: LGTM!The test cases for different orders of operations are well-defined and correctly implemented.
621-650: LGTM!The benchmark cases for the
MarshalTomethod are well-defined and correctly implemented.
654-667: LGTM!The benchmark case for the
QuoMutmethod is well-defined and correctly implemented.
669-682: LGTM!The benchmark case for the
QuoTruncateMutmethod is well-defined and correctly implemented.
684-695: LGTM!The benchmark case for the
ApproxSqrtmethod on a Mersenne prime is well-defined and correctly implemented.
698-711: LGTM!The benchmark case for the
QuoRoundupMutmethod is well-defined and correctly implemented.
713-729: LGTM!The test cases for the
FormatDecmethod are well-defined and cover various scenarios.
731-757: LGTM!The test cases for handling invalid decimal strings are well-defined and correctly implemented.
760-763: LGTM!The test case for negative precision values is well-defined and correctly implemented.
Description
Closes: #XXXX
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.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Decdecimal type, providing enhanced precision and performance overLegacyDec.LegacyDecandDec.Dec.Documentation
LegacyDectoDecwith emphasis on precision, format, and performance improvements.Tests
LegacyDecandDecperformance.Decoperations.Chores
Dectype.