feat(math): Upstream GDA based decimal type #21982
Conversation
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant changes to the Cosmos SDK's decimal handling by replacing the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
alpe
force-pushed
the
alex/sam/dec-type
branch
from
26f16eb
to
40082ff
Compare
alpe
force-pushed
the
alex/sam/dec-type
branch
from
40082ff
to
7cae678
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Co-authored-by: Alexander Peters <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 32
🧹 Outside diff range and nitpick comments (33)
math/go.mod (1)
40-41: Document retraction rationaleThe retraction notice for versions [v1.1.0, v1.1.1] should be documented more explicitly.
Consider adding a comment explaining the specific issue with the
math.Int{}.Size()implementation that led to this retraction.math/math.go (5)
1-12: Enhance package documentation.While the current documentation provides a basic overview, consider adding more details about:
- The precision guarantees provided by the package
- Common use cases and examples
- Any assumptions or limitations
- The relationship with the new decimal type implementation
14-19: Document exactContext configuration.The
exactContextconfiguration is crucial for decimal operations. Please add documentation explaining:
- Why precision is set to 0
- The significance of the trap settings
- The implications of these settings on decimal operations
21-24: Improve Add function documentation.Please enhance the documentation to include:
- Error conditions that might occur
- Whether negative numbers are allowed
- Any precision guarantees
26-55: Consider consolidating subtraction functions.There are two similar functions for subtraction (
SubNonNegativeandSafeSubBalance) with different implementations and error handling approaches. Consider:
- Using
exactContextconsistently across both functions- Consolidating them into a single function with a parameter for error behavior
- Documenting the differences and use cases for each function
57-74: Optimize error message construction.Consider pre-allocating the error message to avoid string concatenation in the error path:
- return z, errors.Wrap( - sdkerrors.ErrInvalidRequest, - fmt.Sprintf("AddBalance() requires two non-negative Dec parameters, but received %s and %s", x, y)) + const msg = "AddBalance() requires two non-negative Dec parameters, but received %s and %s" + return z, errors.Wrapf(sdkerrors.ErrInvalidRequest, msg, x, y)x/group/internal/math/dec.go (2)
139-139: Enhance error message with actual valuesThe error message could be more helpful by including the actual values involved in the subtraction.
Consider updating the error message to include more context:
- return z, fmt.Errorf("result negative during non-negative subtraction") + return z, fmt.Errorf("result negative during non-negative subtraction: %s - %s = %s", x, y, z)
Line range hint
1-3: Consider adding usage examples in documentationWhile the package documentation is clear, it would be helpful to add examples showing proper usage of the
Dectype and its operations, especially given that this is a wrapper aroundapd.Decimalwith specific safety guarantees.docs/build/building-modules/18-decimal-handling.md (4)
8-14: Add SDK version compatibility information.The introduction effectively explains the decimal types, but it would be helpful to specify:
- Which SDK version introduced the new
Dectype- Which versions still support
LegacyDec- The planned deprecation timeline for
LegacyDec(if any)
17-17: Improve sentence structure with proper punctuation.The sentence would be clearer with proper comma placement:
-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)
26-40: Enhance code examples with explanations.The code examples would be more helpful if each example included:
- A brief explanation of what the method does
- Any behavioral differences between the old and new implementations
- Performance implications of the change
62-62: Fix sentence structure for clarity.The sentence needs better punctuation:
-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, but 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)
math/dec_examples_test.go (3)
5-36: Add documentation about decimal precision and scaleWhile the example demonstrates various initialization methods well, it would be helpful to add comments explaining:
- The maximum supported precision
- The decimal scale/precision guarantees
- Any limitations on the exponent range
127-314: Add performance characteristics documentationFor arithmetic operations (Quo, QuoExact, QuoInteger, Mul, MulExact), consider adding comments about:
- Time complexity
- Memory allocation behavior
- Performance implications of exact vs. non-exact operations
This would help developers choose the appropriate method for their use case.
1-324: Add examples for common decimal use casesConsider adding examples for:
- Currency/monetary calculations
- Percentage calculations
- Rounding operations
- Comparison operations (Equal, Greater, Less)
These are common use cases that would help developers understand how to use the Dec type effectively.x/group/go.mod (1)
Line range hint
185-185: Track TODO for module separation completionThe comment indicates these replacements are temporary until module separation is complete. This should be tracked to ensure cleanup after the modules are spun out.
Would you like me to create a GitHub issue to track the removal of these replace directives after module separation is complete?
math/dec_bench_test.go (3)
183-184: Correct typo: 'multiplicant' should be 'multiplicand'In the benchmark specifications for multiplication, the term 'multiplicant' is used, but the correct term is 'multiplicand'.
Apply this diff to correct the terminology:
-specs := map[string]struct { - multiplier, multiplicant string +specs := map[string]struct { + multiplier, multiplicand stringUpdate all instances accordingly:
- multiplier: "100", multiplicant: "5", + multiplier: "100", multiplicand: "5",Also applies to: 186-215
3-7: Group imports according to Go conventionsPer the Uber Go Style Guide, imports should be grouped into standard library packages, third-party packages, and local packages, separated by blank lines.
Apply this diff to group imports properly:
import ( "testing" + "github.com/stretchr/testify/require" )
67-88: Enhance benchmark coverage with additional edge casesTo thoroughly evaluate performance, consider including edge cases such as zero values, negative numbers, and extremely large or small decimals.
For example, add test cases like:
"zero values": { summands: []string{"0", "0"}, }, "negative numbers": { summands: []string{"-1", "-1000", "-123456789012345678"}, },math/dec_rapid_test.go (3)
13-13: Refine or remove the general comment about RapidThe comment
// Rapid is a Go library for property-based testing.is quite general. Consider removing it or updating it to explain the purpose of theTestDecWithRapidfunction more specifically.
504-541: Handle special float values infloatDecimalPlacesfunctionThe
floatDecimalPlacesfunction may not correctly handle special float values likeNaNorInf. Before processing, ensure these values are filtered out or properly handled to prevent unexpected behavior.Apply this diff to check for special values:
func floatDecimalPlaces(t *rapid.T, f float64) uint32 { + if math.IsNaN(f) || math.IsInf(f, 0) { + t.Fatalf("Cannot determine decimal places for special float value: %g", f) + } reScientific := regexp.MustCompile(`^\-?(?:[[:digit:]]+(?:\.([[:digit:]]+))?|\.([[:digit:]]+))(?:e?(?:\+?([[:digit:]]+)|(-[[:digit:]]+)))?$`) fStr := fmt.Sprintf("%g", f)
473-478: Use epsilon comparison for floating-point zero checksDirectly comparing floating-point numbers to zero can lead to precision issues due to the nature of floating-point arithmetic. Consider using a small epsilon value for comparison to improve test robustness.
Apply this diff to use epsilon comparison:
func testIsZero(t *rapid.T) { floatAndDec := genFloatAndDec.Draw(t, "floatAndDec") f, dec := floatAndDec.float, floatAndDec.dec - require.Equal(t, f == 0, dec.IsZero()) + epsilon := 1e-9 + require.Equal(t, math.Abs(f) < epsilon, dec.IsZero()) }math/dec.go (3)
432-443: Simplify JSON unmarshaling inUnmarshalJSONmethod.You can streamline the
UnmarshalJSONmethod by directly unmarshaling intox.dec, reducing complexity and improving performance.Apply this diff to simplify the method:
func (x *Dec) UnmarshalJSON(data []byte) error { - var text string - err := json.Unmarshal(data, &text) - if err != nil { - return err - } - val, err := NewDecFromString(text) - if err != nil { - return err - } - *x = val + return x.dec.UnmarshalJSON(data) }
306-325: Simplify theSdkIntTrimmethod for clarity and efficiency.The
SdkIntTrimmethod is complex and might impact performance due to multiple big integer operations. Simplify the logic or add detailed comments explaining each step.Consider refactoring the method to make it more efficient and easier to understand.
333-347: Align function comments with Go style guidelines.The comment for the
Textmethod doesn't start with the method name, which deviates from the Go style guidelines.Update the comment to follow the recommended style:
-// Text converts the floating-point number x to a string according +// Text converts the floating-point number `x` to a string accordingmath/legacy_dec.go (7)
804-805: Handle errors returned byMarshalTextandjson.MarshalIn the
initfunction, the errors fromempty.MarshalText()andjson.Marshal()are being ignored. Ignoring errors can lead to unnoticed failures.Consider checking and handling the errors:
empty := new(big.Int) -bz, _ := empty.MarshalText() +bz, err := empty.MarshalText() +if err != nil { + panic(fmt.Errorf("failed to marshal empty big.Int: %w", err)) +} -nilJSON, _ = json.Marshal(string(bz)) +nilJSON, err = json.Marshal(string(bz)) +if err != nil { + panic(fmt.Errorf("failed to marshal nil JSON: %w", err)) +}
470-473: Return an error for zero root inApproxRootThe
ApproxRootmethod returns1when the root is zero. Mathematically, the zeroth root is undefined and should result in an error.Modify the method to return an error when the root is zero:
if root == 0 { - // Return 1 as root 0 of any number is considered 1. - return LegacyOneDec(), nil + return LegacyDec{}, fmt.Errorf("cannot compute zeroth root") }
182-183: Correct the error message inLegacyNewDecFromStrfor precision exceedingThe error message when the decimal exceeds maximum precision may have incorrect calculation for the number of excess decimal places.
Update the error message to accurately reflect the precision issue:
return LegacyDec{}, fmt.Errorf( - "value '%s' exceeds max precision by %d decimal places: max precision %d", - str, LegacyPrecision-lenDecs, LegacyPrecision) + "value '%s' has %d decimal places, which exceeds the max precision of %d", + str, lenDecs, LegacyPrecision)
938-941: Simplify the return values ofLegacyDecEqThe
LegacyDecEqfunction returns multiple values, including*testing.T, which is unconventional. In Go tests, helper functions typically don't return*testing.T.Consider refactoring the function to improve clarity and compliance with Go testing practices:
-func LegacyDecEq(t *testing.T, exp, got LegacyDec) (*testing.T, bool, string, string, string) { +func LegacyDecEq(t *testing.T, exp, got LegacyDec) { t.Helper() - return t, exp.Equal(got), "expected:\t%v\ngot:\t\t%v", exp.String(), got.String() + if !exp.Equal(got) { + t.Fatalf("expected: %v, got: %v", exp.String(), got.String()) + } }
16-16: Evaluate the necessity of using a pointer tobig.IntinLegacyDecThe
LegacyDecstruct uses a pointer tobig.Int, but sincebig.Intis already a pointer-like type, this may introduce unnecessary complexity.Consider using
big.Intdirectly to simplify the code and reduce the risk of nil pointer dereferences:-type LegacyDec struct { - i *big.Int +type LegacyDec struct { + i big.Int }
803-806: Handle potential errors ininitfunction when marshallingThe errors from
MarshalTextandjson.Marshalare ignored. This could mask issues during initialization.Amend the
initfunction to handle errors:func init() { empty := new(big.Int) - bz, _ := empty.MarshalText() + bz, err := empty.MarshalText() + if err != nil { + panic(fmt.Sprintf("failed to marshal empty big.Int: %v", err)) + } - nilJSON, _ = json.Marshal(string(bz)) + nilJSON, err = json.Marshal(string(bz)) + if err != nil { + panic(fmt.Sprintf("failed to marshal nil JSON: %v", err)) + } }
249-261: Assess the mutability pattern used inImmutOpmethodsThe
ImmutOpmethods clone the receiver before applying operations. This pattern might be unnecessary if the methods themselves do not mutate the receiver.Review whether cloning is essential, or if the methods can be restructured to avoid unnecessary cloning.
math/dec_test.go (1)
1315-1412: Skipped testTestMarshalUnmarshalcontains active test casesThe test function
TestMarshalUnmarshalis currently skipped usingt.Skip("not supported, yet"), but it includes test cases that will not be executed. If the functionality is not yet supported, consider commenting out the test cases or implementing the required functionality to enable the tests.Apply this diff to comment out the unused test cases:
func TestMarshalUnmarshal(t *testing.T) { t.Skip("not supported, yet") - specs := map[string]struct { - x Dec - exp string - expErr error - }{ - "No trailing zeros": { - x: NewDecFromInt64(123456), - exp: "1.23456E+5", - }, - // ... additional test cases ... - } - for name, spec := range specs { - t.Run(name, func(t *testing.T) { - marshaled, gotErr := spec.x.Marshal() - if spec.expErr != nil { - require.ErrorIs(t, gotErr, spec.expErr) - return - } - require.NoError(t, gotErr) - unmarshalled := new(Dec) - require.NoError(t, unmarshalled.Unmarshal(marshaled)) - assert.Equal(t, spec.exp, unmarshalled.dec.Text('E')) - }) - } }Alternatively, if the functionality is ready, remove the
t.Skipcall to enable the tests.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (5)
math/go.sumis excluded by!**/*.sumsimapp/go.sumis excluded by!**/*.sumsimapp/v2/go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sumx/group/go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
.github/workflows/test.yml(1 hunks)docs/build/building-modules/18-decimal-handling.md(1 hunks)math/dec.go(1 hunks)math/dec_bench_test.go(1 hunks)math/dec_examples_test.go(1 hunks)math/dec_rapid_test.go(1 hunks)math/dec_test.go(1 hunks)math/go.mod(1 hunks)math/int_test.go(0 hunks)math/legacy_dec.go(1 hunks)math/legacy_dec_test.go(1 hunks)math/math.go(1 hunks)math/uint_test.go(0 hunks)simapp/go.mod(1 hunks)simapp/v2/go.mod(1 hunks)tests/go.mod(1 hunks)x/group/go.mod(1 hunks)x/group/internal/math/dec.go(4 hunks)
💤 Files with no reviewable changes (2)
- math/int_test.go
- math/uint_test.go
🧰 Additional context used
📓 Path-based instructions (11)
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_examples_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/legacy_dec.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
math/legacy_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/math.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
tests/go.mod (1)
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
x/group/internal/math/dec.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
📓 Learnings (1)
.github/workflows/test.yml (2)
Learnt from: julienrbrt
PR: cosmos/cosmos-sdk#20888
File: .github/workflows/test.yml:466-466
Timestamp: 2024-10-08T15:31:05.486Z
Learning: For the `test-schema` job in the `.github/workflows/test.yml` file, use Go version 1.12 as specified by the user.
Learnt from: julienrbrt
PR: cosmos/cosmos-sdk#20888
File: .github/workflows/test.yml:466-466
Timestamp: 2024-07-05T08:22:05.859Z
Learning: For the `test-schema` job in the `.github/workflows/test.yml` file, use Go version 1.12 as specified by the user.
🪛 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 a big.Int to 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: ... LegacyDec to a Dec without a state migration changing how the data is handled intern...
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (53)
math/go.mod (4)
13-29: Review indirect dependency versions
The extensive list of new indirect dependencies suggests a significant expansion of the module's dependency tree. Some key observations:
- Security-critical dependencies (crypto, grpc) are at recent versions
- All dependencies use specific versions or commits rather than floating versions
32-38: Verify cosmos-sdk version compatibility
The addition of github.com/cosmos/cosmos-sdk v0.50.10 as a dependency requires careful consideration to avoid circular dependencies.
6-8: Verify apd/v3 integration impact
The addition of github.com/cockroachdb/apd/v3 as a primary dependency is a significant change that aligns with the PR objective of implementing a new decimal type.
3-4: Verify Go 1.23 compatibility across the ecosystem
The upgrade to Go 1.23 is significant and requires careful consideration.
x/group/internal/math/dec.go (4)
5-10: LGTM: Import changes align with dependency updates
The upgrade to apd/v3 and addition of required imports are appropriate for the changes made in this file.
26-29: LGTM: Improved error handling
The standardization of error handling using errors.ErrInvalidDecString provides better consistency and more descriptive error messages.
37-40: LGTM: Consistent error handling pattern
The error handling follows the same pattern as NewPositiveDecFromString, maintaining consistency throughout the package.
54-58: LGTM: Robust error handling for decimal validation
The error handling properly validates finite decimal values and provides clear error messages using the standardized error type.
math/dec_examples_test.go (1)
38-81: Ensure error messages in output comments match actual errors
The error message in the output comments (line 77) shows:
add: exponent out of range: invalid decimal
But this format should be verified to match the actual error returned by the Dec type. Consider adding a comment explaining the error format for consistency.
✅ Verification successful
Let me search for the actual error message implementation to verify the format.
Let me check the error registration and formatting in the math package.
Error messages in output comments are correct
Looking at the implementation in math/dec.go, the error messages in the test output are consistent with the actual error format. The Add method wraps any internal error with ErrInvalidDec using ErrInvalidDec.Wrap(err.Error()), which produces the exact format shown in the test output:
add: exponent out of range: invalid decimal
The error message format follows the pattern where:
- "add" comes from the operation name
- "exponent out of range" is the internal error message
- "invalid decimal" comes from
ErrInvalidDec
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other error message formats in the codebase
rg "exponent out of range" --type go
Length of output: 758
Script:
#!/bin/bash
# Search for error message definition in the Dec type
ast-grep --pattern 'func ($dec *Dec) Add($_ $_) ($_, error) {
$$$
}'
# Also search for any error-related constants or variables
rg "invalid decimal" --type go
Length of output: 1951
Script:
#!/bin/bash
# Check the content of math/dec.go for error definitions and Add implementation
cat math/dec.go
Length of output: 15451
x/group/go.mod (1)
21-21: Verify consistency of apd v3 upgrade across dependent modules
The upgrade from github.com/cockroachdb/apd/v2 to github.com/cockroachdb/apd/v3 is a major version bump that may introduce breaking changes. This aligns with the PR objective of transitioning to a new decimal type.
Let's verify the version consistency across the codebase:
✅ Verification successful
Upgrade to apd/v3 is consistently applied across core modules
The verification shows that:
- All core modules (math, simapp, tests, x/group) consistently use apd/v3 v3.2.1
- The only v2 references are in go.sum files of tools (confix, hubl) and system tests, which are not part of the core functionality
- The actual code imports (*.go files) exclusively use apd/v3
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining v2 references and confirm v3 usage
# Expected: All references should be to v3, no v2 references should remain
echo "Checking for any remaining apd v2 references:"
rg "github.com/cockroachdb/apd/v2"
echo -e "\nConfirming consistent v3 usage:"
rg "github.com/cockroachdb/apd/v3"
Length of output: 3352
tests/go.mod (1)
86-86: Verify compatibility with APD v3 major version upgrade.
The upgrade from github.com/cockroachdb/apd v2 to v3 is a major version change that may introduce breaking changes. Please ensure:
- All decimal operations in tests are updated to be compatible with v3 API
- Test coverage includes edge cases specific to v3 behavior
✅ Verification successful
Let me analyze the results and generate additional verification scripts to check the decimal type implementation changes.
Based on the comprehensive analysis of the code, I can now provide a final response regarding the APD v3 upgrade verification.
APD v3 upgrade is properly implemented with full compatibility
The implementation in math/dec.go shows proper usage of APD v3 with:
- Correct import of
github.com/cockroachdb/apd/v3 - Full test coverage in
math/dec_test.goverifying all decimal operations - Proper documentation in
docs/building-modules/18-decimal-handling.mdexplaining the migration
Key verifications:
- All APD v3 API calls are correctly used with proper error handling
- Decimal precision is maintained at 34 digits using
dec128Context - No breaking changes in the public API
- Comprehensive test suite covering edge cases and precision requirements
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining v2 imports and potential incompatibilities
# Test 1: Search for any remaining v2 imports
echo "Checking for any remaining v2 imports..."
rg "github.com/cockroachdb/apd/v2"
# Test 2: Look for decimal-related test files that might need updates
echo "Finding decimal-related test files..."
fd -e go -e md -e txt . | rg -i "decimal|dec"
# Test 3: Search for potential APD usage patterns that might need updates
echo "Checking for APD usage patterns..."
rg "apd\.(New|Basic|Context|Decimal|Cond)"
Length of output: 6860
Script:
#!/bin/bash
# Check the actual implementation changes in decimal-related files
# Check the implementation in math/dec.go for v3 specific changes
echo "Checking math/dec.go implementation..."
cat math/dec.go
# Check for any test updates related to v3 compatibility
echo "Checking decimal-related test updates..."
cat math/dec_test.go
# Look for any migration guides or documentation
echo "Checking for migration documentation..."
cat docs/build/building-modules/18-decimal-handling.md
Length of output: 53718
simapp/go.mod (1)
Line range hint 78-84: LGTM! CockroachDB APD v3 dependency added correctly.
The addition of github.com/cockroachdb/apd/v3 v3.2.1 and its related dependencies aligns with the PR objective to introduce a new decimal type. The version chosen is appropriate and stable.
Let's verify the version compatibility across the codebase:
✅ Verification successful
Version compatibility check completed - Minor inconsistency found
There's a minor version inconsistency in the codebase:
- Main modules correctly use
github.com/cockroachdb/apd/v3 v3.2.1 - However, some tool modules (
tools/hubl,tools/confix,tests/systemtests) still use olderapd/v2 v2.0.2
Since these are independent tool modules and not part of the main application logic, this doesn't affect the correctness of the decimal type implementation. The v3 dependency is consistently used across all relevant application modules.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any version conflicts or inconsistencies with cockroachdb/apd
# across the codebase
# Test 1: Check for any other versions of cockroachdb/apd
rg -g 'go.mod' "github.com/cockroachdb/apd"
# Test 2: Check for any direct imports of the package
rg "\"github.com/cockroachdb/apd\""
Length of output: 575
Script:
#!/bin/bash
# Let's check for imports of both v2 and v3 versions to ensure consistency
rg "\"github.com/cockroachdb/apd/v[23]\""
# Also check for any potential version conflicts in go.sum files
rg -g 'go.sum' "github.com/cockroachdb/apd/v[23]"
Length of output: 2696
simapp/v2/go.mod (1)
84-84: LGTM! Verify compatibility with existing decimal operations.
The addition of github.com/cockroachdb/apd/v3 aligns with the PR objective of introducing a new decimal type. Since this is a major version upgrade from v2, ensure all modules using decimal operations are compatible with the new version.
Let's verify the usage of the old APD v2 package to ensure complete migration:
✅ Verification successful
Let me gather more information to verify if the v2 references in go.sum files are actual dependencies or just historical entries.
Let me check the actual usage of APD v2 and v3 in the codebase to verify the migration status.
LGTM! The migration to APD v3 is complete.
The codebase shows consistent usage of github.com/cockroachdb/apd/v3 across all relevant files:
- math/dec.go
- math/math.go
- x/group/internal/math/dec.go
- schema/testing/diff.go
- schema/testing/fmt.go
All apd.Decimal usages are properly using the v3 import, and there are no remaining references to APD v2 in the actual code. The only v2 references found earlier were in go.sum files which are historical entries and don't affect the actual dependency usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old APD v2 package
# to ensure complete migration to v3.
# Test: Search for imports of the old package
rg -l 'github.com/cockroachdb/apd/v2'
# Test: Search for any remaining v2 decimal type usage
rg 'apd\.Decimal'
Length of output: 948
Script:
#!/bin/bash
# Check the actual module dependencies in go.mod files
fd 'go.mod$' --exec cat {}
# Check if there are any direct imports of apd v2 in the source code
rg -l 'import.*"github.com/cockroachdb/apd/v2"'
Length of output: 313342
Script:
#!/bin/bash
# Check for actual imports and usage of APD v2 and v3
rg -A 2 'github.com/cockroachdb/apd/v[23]' --type go
# Also check for any APD v2 specific types or methods
rg 'apd\.Decimal' --type go
Length of output: 1516
.github/workflows/test.yml (1)
Line range hint 466-466: Maintain Go 1.12 for test-schema job.
According to the learnings from PR #20888, the test-schema job should use Go version 1.12.
Apply this change to comply with the requirement:
- go-version: "1.12"
+ go-version: "1.12"
cache: true✅ Verification successful
Let me gather more context to see the specific Go version being used.
Go version 1.12 is already correctly set for test-schema job
The test-schema job is already using Go version 1.12 as required, which matches the learnings from PR #20888. No changes are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if test-schema job is using Go 1.12 across all workflow files
rg -A 5 'test-schema:' .github/workflows/
Length of output: 338
Script:
#!/bin/bash
# Get more context around the test-schema job setup
rg -A 10 'test-schema:' .github/workflows/test.yml
Length of output: 356
math/legacy_dec.go (2)
805-806: Ensure consistent initialization of nilJSON
In the init function, nilJSON is initialized using a marshalled empty string. Confirm that this is consistent with the expected JSON representation of a nil LegacyDec.
Check if nilJSON matches the JSON encoding used elsewhere when a LegacyDec is nil.
#!/bin/bash
# Description: Verify that `nilJSON` matches expected nil JSON representations in the codebase.
# Expected result: Consistent usage of `nilJSON`.
rg 'nilJSON' -A 5809-813: Ensure consistent handling of nil big.Int in MarshalJSON
In the MarshalJSON method, if d.i is nil, it returns nilJSON. This may lead to inconsistent JSON outputs.
Verify that returning nilJSON is the intended behavior and that it doesn't cause issues with JSON marshalling elsewhere in the codebase.
✅ Verification successful
The nil handling in MarshalJSON is correct and consistent
The implementation is correct for several reasons:
- The
nilJSONvalue is properly initialized ininit()to represent an emptybig.Intas JSON - The nil check is consistent with the type's documentation which states "never use new(Dec) or else we will panic unmarshalling"
- The complementary
UnmarshalJSONmethod properly initializes the internalbig.Intwhen it's nil - Other numeric types in the codebase (like
IntandUint) handle nil cases similarly - There are comprehensive tests including specific cases for JSON marshaling/unmarshaling
The current implementation ensures consistent JSON representation of LegacyDec values, including proper handling of nil cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usages of LegacyDec.MarshalJSON to ensure nil handling is appropriate.
# Expected result: All usages correctly handle potential nil JSON outputs.
rg 'LegacyDec\.\w+\s*\(.*\)' -g '*.go' -A 5 | grep -i 'MarshalJSON'
Length of output: 1742
Script:
#!/bin/bash
# Let's do a more comprehensive analysis
# Check how nilJSON is used and initialized
rg "nilJSON" -A 5 -B 5
# Look for other JSON marshaling implementations in the codebase
ast-grep --pattern 'func ($_ $_) MarshalJSON() ([]byte, error) { $$$ }'
# Check for UnmarshalJSON implementation to understand the complete serialization pattern
rg "UnmarshalJSON.*LegacyDec" -A 10
# Look for tests related to JSON marshaling of LegacyDec
rg "TestLegacyDec.*JSON" -A 10
Length of output: 7563
Script:
#!/bin/bash
# Let's check the initialization and handling of nil values in other numeric types
# Check how nil is handled in UnmarshalJSON for LegacyDec
rg "func.*UnmarshalJSON.*LegacyDec" -A 5 -B 1
# Look for test cases specifically related to LegacyDec JSON marshaling
rg "func.*Test.*LegacyDec.*JSON" -A 10
# Check how other numeric types handle nil in their MarshalJSON
rg "func.*MarshalJSON.*nil" -A 3 -B 1
Length of output: 128
Script:
#!/bin/bash
# Let's look at the complete LegacyDec type implementation and its tests
# Get the full LegacyDec type implementation
rg "type LegacyDec struct" -A 10 -B 5 math/legacy_dec.go
# Get the UnmarshalJSON implementation
rg "func \(d \*LegacyDec\) UnmarshalJSON" -A 10 math/legacy_dec.go
# Look for test files related to LegacyDec
fd "legacy_dec_test.go"
# If test file exists, get its content
rg "func Test" -A 5 math/legacy_dec_test.go
Length of output: 1867
math/dec_test.go (3)
14-133: Comprehensive test coverage in TestNewDecFromString
The test cases in TestNewDecFromString cover a wide range of inputs, including valid decimals, negative values, scientific notation, edge cases, and error conditions. This thorough testing ensures that NewDecFromString handles various scenarios correctly.
1276-1297: Adequate test coverage in TestToSdkInt
The TestToSdkInt function includes test cases for various scenarios, including large integers, floating-point values, and negative numbers. This comprehensive testing ensures that SdkIntTrim correctly truncates decimals and handles different edge cases.
1301-1304: Effective testing of invalid input in TestInfDecString
The TestInfDecString function appropriately tests the parsing of invalid decimal strings like "iNf", ensuring that the NewDecFromString function correctly returns an error for such inputs.
math/legacy_dec_test.go (34)
1-2: LGTM!
The package declaration and import statements are correct.
19-21: LGTM!
The decimalTestSuite struct definition is correct, embedding the suite.Suite type.
23-25: LGTM!
The TestDecimalTestSuite function correctly sets up and runs the test suite.
27-48: LGTM!
The TestDecApproxEq function correctly tests the math.LegacyDecApproxEq function with various test cases, asserting the expected results.
50-56: LGTM!
The mustNewDecFromStr helper method is correctly defined, creating a math.LegacyDec from a string and panicking on error.
58-120: LGTM!
The TestNewDecFromStr function thoroughly tests the math.LegacyNewDecFromStr function with various test cases, asserting the expected results and error conditions. The test cases cover a wide range of scenarios, including valid and invalid decimal strings, negative values, and edge cases.
122-139: LGTM!
The TestDecString function correctly tests the string representation of math.LegacyDec instances, comparing the actual output with the expected formatted strings.
141-161: LGTM!
The TestDecFloat64 function correctly tests the conversion of math.LegacyDec instances to float64, validating the results and error handling.
163-197: LGTM!
The TestEqualities function thoroughly tests the comparison operations (greater than, less than, and equality) between math.LegacyDec instances, asserting the expected results for various test cases.
199-219: LGTM!
The TestDecsEqual function correctly tests the equality of arrays of math.LegacyDec instances, covering scenarios with equal and unequal arrays, as well as arrays of different lengths.
221-291: LGTM!
The TestArithmetic function comprehensively tests the arithmetic operations (addition, subtraction, multiplication, and division) of math.LegacyDec instances, comparing the actual results with the expected values for various test cases. The test cases cover a wide range of scenarios, including positive and negative values, different precisions, and edge cases like division by zero.
293-306: LGTM!
The TestMulRoundUp_RoundingAtPrecisionEnd function correctly tests the rounding behavior of math.LegacyDec multiplication when the result is at the precision limit, verifying the expected rounded-up and truncated values.
308-332: LGTM!
The TestBankerRoundChop function correctly tests the banker's rounding behavior of math.LegacyDec instances, comparing the actual rounded integer values with the expected values for various test cases.
334-358: LGTM!
The TestTruncate function correctly tests the truncation behavior of math.LegacyDec instances, comparing the actual truncated integer values with the expected values for various test cases.
360-371: LGTM!
The TestStringOverflow function correctly tests the string representation of math.LegacyDec instances with large values, ensuring that the resulting string matches the expected value without overflow.
373-388: LGTM!
The TestDecMulInt function correctly tests the multiplication of math.LegacyDec instances with math.Int values, comparing the actual results with the expected values for various test cases.
390-409: LGTM!
The TestDecCeil function correctly tests the ceiling function of math.LegacyDec instances, comparing the actual ceiling values with the expected values for various test cases.
411-417: LGTM!
The TestCeilOverflow function correctly tests the panic behavior when calling the Ceil method on a math.LegacyDec instance with a value that exceeds the maximum representable size.
419-444: LGTM!
The TestPower function correctly tests the power function of math.LegacyDec instances, comparing the actual results with the expected values for various test cases. It also verifies the consistency between the immutable and mutable power methods.
446-474: LGTM!
The TestApproxRoot function correctly tests the approximate root function of math.LegacyDec instances, comparing the actual results with the expected values for various test cases. It covers scenarios with different root values and precision levels.
476-499: LGTM!
The TestApproxSqrt function correctly tests the approximate square root function of math.LegacyDec instances, comparing the actual results with the expected values for various test cases. It covers scenarios with different input values and precision levels.
501-524: LGTM!
The TestDecSortableBytes function correctly tests the math.LegacySortableDecBytes function, comparing the actual byte representations with the expected values for various test cases. It also verifies the panic behavior when the input value exceeds the maximum representable size.
526-611: LGTM!
The TestDecEncoding function thoroughly tests the encoding and decoding of math.LegacyDec instances using various formats (raw bytes, JSON, YAML). It compares the actual encoded values with the expected values and ensures that the decoded values match the original instances.
621-650: LGTM!
The BenchmarkMarshalTo function correctly benchmarks the MarshalTo method of math.LegacyDec instances, measuring the performance for different input values and comparing the actual encoded bytes with the expected values.
654-667: LGTM!
The BenchmarkLegacyQuoMut function correctly benchmarks the QuoMut method of math.LegacyDec instances, measuring the performance for a specific input value.
669-682: LGTM!
The BenchmarkLegacyQuoTruncateMut function correctly benchmarks the QuoTruncateMut method of math.LegacyDec instances, measuring the performance for a specific input value.
684-696: LGTM!
The BenchmarkLegacySqrtOnMersennePrime function correctly benchmarks the ApproxSqrt method of math.LegacyDec instances, measuring the performance for a specific input value (Mersenne prime).
698-711: LGTM!
The BenchmarkLegacyQuoRoundupMut function correctly benchmarks the QuoRoundupMut method of math.LegacyDec instances, measuring the performance for a specific input value.
713-728: LGTM!
The TestFormatDec function correctly tests the math.FormatDec function, comparing the actual formatted output with the expected values loaded from a JSON file.
730-756: LGTM!
The TestFormatDecNonDigits function correctly tests the error handling of the math.FormatDec function when the input contains non-digit characters. It verifies that the expected error message is returned and that the output string is empty.
758-762: LGTM!
The TestNegativePrecisionPanic function correctly tests the panic behavior when creating a math.LegacyDec instance with a negative precision value.
764-782: LGTM!
The TestConvertToBigIntMutativeForLegacyDec function correctly tests the mutative behavior of the BigIntMut method of math.LegacyDec instances. It verifies that modifying the returned big.Int pointer affects the original math.LegacyDec instance, while modifying the big.Int pointer returned by BigInt does not affect the original instance.
784-1018: LGTM!
The TestQuoMut function thoroughly tests the QuoRoundupMut and QuoTruncateMut methods of math.LegacyDec instances with various test cases. It covers scenarios with different dividend and divisor values, including edge cases like dividing by zero. The test cases verify the expected rounded-up and truncated results and ensure that the methods panic when dividing by zero.
614-619: Consider the impact of operation order on the codebase.
The test case demonstrates that different orders of operations can lead to different results when using math.LegacyDec. This behavior should be carefully considered and documented to avoid potential bugs or inconsistencies in the codebase.
✅ Verification successful
Operation order is intentionally tested and documented
The test case TestOperationOrders explicitly verifies that operation order matters for decimal arithmetic, as shown by:
- The test checks that
n1.Mul(n2).Quo(n2)equals 10 (the expected mathematical result) - The test verifies that
n1.Mul(n2).Quo(n2)is NOT equal ton1.Quo(n2).Mul(n2), demonstrating that order affects the result - The test name itself indicates this is intentional behavior
This is not a bug but rather expected behavior that is being explicitly tested. The test ensures that decimal arithmetic operations maintain proper order of operations, which is critical for financial calculations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for instances where the order of operations might impact the result.
# Test: Search for patterns like `a.Mul(b).Quo(c)` or `a.Quo(b).Mul(c)`.
# Expect: Review the results to identify potential issues.
rg -A 5 $'\\.(Mul|Quo)\\(.*\\)\\.\\(Mul|Quo\\)\\('
Length of output: 44
Script:
#!/bin/bash
# Search for chained operations with Mul and Quo in the codebase
rg -U "\.Mul\(.*?\)\.Quo\(|\.Quo\(.*?\)\.Mul\(" -A 3
# Search for any documentation or comments about operation order
rg -i "operation.*order|arithmetic.*order" -A 2
# Look for other test cases involving Mul and Quo operations
ast-grep --pattern 'func $_($_ *testing.$_) {
$$$
.Mul($_)
$$$
.Quo($_)
$$$
}'
Length of output: 266511
Script:
#!/bin/bash
# Search for chained operations with Mul and Quo in test files
rg -U "\.Mul\(.*?\)\.Quo\(|\.Quo\(.*?\)\.Mul\(" -A 3 --glob "**/*_test.go"
# Search for any documentation or comments about operation order in test files
rg -i "operation.*order|arithmetic.*order" -A 2 --glob "**/*_test.go"
# Look for test cases involving Mul and Quo operations
ast-grep --pattern 'func ($_ *testing.$_) {
$$$
.Mul($_)
$$$
.Quo($_)
$$$
}'
Length of output: 1325
|
|
||
| if d.IsNegative() { | ||
| absRoot, err := d.Neg().ApproxRoot(root) | ||
| return absRoot.NegMut(), err | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Negative inputs in ApproxRoot are handled incorrectly for even roots
The current implementation negates negative inputs, computes the root, and then negates the result back. This is mathematically incorrect for even roots as they don't have real solutions for negative numbers. For example:
- √(-4) is not equal to -√4
- ⁴√(-16) is not equal to -⁴√16
The function should return an error for negative inputs when the root is even.
🔗 Analysis chain
Double-check the handling of negative inputs in ApproxRoot
When d is negative, ApproxRoot calls itself recursively after negating d. This could lead to unexpected behavior for even roots.
Ensure that taking roots of negative numbers is handled correctly, especially considering complex numbers are not supported.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if `ApproxRoot` correctly handles negative inputs for even roots.
# Expected result: The function returns an error or handles odd and even roots appropriately.
rg 'func\s+\(d LegacyDec\)\s+ApproxRoot\(' -A 20
Length of output: 1742
Script:
#!/bin/bash
# Let's get more context around the ApproxRoot implementation
rg -A 50 'func \(d LegacyDec\) ApproxRoot\(' math/legacy_dec.go
Length of output: 1114
There was a problem hiding this comment.
Actionable comments posted: 30
🧹 Outside diff range and nitpick comments (27)
math/go.mod (2)
32-38: Consider explicit versioning for cosmossdk.io/errors.While adding
cosmossdk.io/errors v1.0.1is appropriate, consider pinning the cosmos-sdk version to a specific commit hash for better reproducibility, especially since it's a direct dependency.- github.com/cosmos/cosmos-sdk v0.50.10 + github.com/cosmos/cosmos-sdk v0.50.10-0.20240404231335-abcdef123456
Line range hint
41-42: Document the math.Int{}.Size() implementation issue.The retraction notice for versions [v1.1.0, v1.1.1] should be accompanied by a comment explaining the specific issue with the math.Int{}.Size() implementation for future reference.
// Issue with math.Int{}.Size() implementation. +// Versions v1.1.0 and v1.1.1 contained incorrect size calculations for large integers. retract [v1.1.0, v1.1.1]math/math.go (2)
14-19: Add documentation for exactContext configurationThe
exactContextconfiguration is critical for decimal operations. Consider adding documentation explaining:
- Why precision is set to 0
- The implications of the trap configuration
- The reasoning behind using DefaultTraps | Inexact | Rounded
21-24: Consider removing the Add wrapper functionThis wrapper function doesn't add any value beyond the underlying
Dec.Addmethod. Consider removing it to reduce API surface area unless there's a specific reason for its existence.x/group/internal/math/dec.go (2)
26-29: Consider adding more context to error messages.While the error handling is correct, consider adding more context to help with debugging:
- return Dec{}, errors.ErrInvalidDecString.Wrap(err.Error()) + return Dec{}, errors.ErrInvalidDecString.Wrapf("failed to parse decimal string %q: %v", s, err)
37-40: Apply consistent error message improvements.For consistency with the previous function:
- return Dec{}, errors.ErrInvalidDecString.Wrap(err.Error()) + return Dec{}, errors.ErrInvalidDecString.Wrapf("failed to parse decimal string %q: %v", s, err)docs/build/building-modules/18-decimal-handling.md (3)
10-11: Enhance bullet point formatting for better readability.Consider restructuring the bullet points to be more descriptive:
-* **Data Format**: The internal representation of decimals changes, affecting how data is stored and processed. +* **Data Format Changes**: The internal representation of decimals is modified, which affects: + * How data is stored in the state + * How decimal values are processed
17-17: Add comma 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)
26-40: Use standard diff notation for code examples.The current
++and--notation is non-standard. Consider using the conventional+and-for diff blocks:-math.NewLegacyDecFromInt64(100) +math.NewDecFromInt64(100) -math.LegacyNewDecWithPrec(100, 18) +math.NewDecWithPrec(100, 18)math/dec_examples_test.go (4)
5-36: Add examples for error cases in decimal initializationWhile the current examples effectively demonstrate successful initialization cases, consider adding examples showing how the Dec type handles invalid inputs (e.g., malformed strings, out of range values) to make the documentation more comprehensive.
164-198: Add documentation explaining QuoExact vs QuoConsider adding a comment explaining how QuoExact differs from regular Quo in terms of precision and when each should be used.
238-267: Add examples with negative numbersConsider adding multiplication examples with negative numbers to demonstrate sign handling.
269-314: Improve error message for unexpected roundingThe error message "unexpected rounding" on line 311 could be more descriptive by including the actual precision that caused the rounding.
tests/go.mod (1)
Line range hint
3-3: Fix invalid Go versionThe specified Go version
1.23.1is invalid as it doesn't exist. The latest stable version of Go is 1.22.x.Apply this change:
-go 1.23.1 +go 1.22math/dec_bench_test.go (2)
270-275: Avoid repetitive error checks in marshal/unmarshal benchmarkingThe repeated use of
require.NoErrorfor every operation can be streamlined to enhance readability.Consider refactoring to use a helper function or handling errors directly:
bz, err := src.Marshal() -require.NoError(b, err) +if err != nil { + b.Fatalf("Marshal error: %v", err) +} var d Dec -require.NoError(b, d.Unmarshal(bz)) +if err := d.Unmarshal(bz); err != nil { + b.Fatalf("Unmarshal error: %v", err) +}
282-295: Define variables within benchmark sub-functionsDefining variables like
legacyB1andnewB1at the top level of the benchmark function can lead to issues if benchmarks are run in parallel. It's better to define them within each sub-benchmark to prevent unintended sharing.Apply this diff to move variable definitions inside sub-functions:
func BenchmarkCompareLegacyDecAndNewDecQuoInteger(b *testing.B) { - legacyB1 := LegacyNewDec(100) - newB1 := NewDecFromInt64(100) b.Run("LegacyDec", func(b *testing.B) { + legacyB1 := LegacyNewDec(100) for i := 0; i < b.N; i++ { _ = legacyB1.Quo(LegacyNewDec(1)) } }) b.Run("NewDec", func(b *testing.B) { + newB1 := NewDecFromInt64(100) for i := 0; i < b.N; i++ { _, _ = newB1.QuoInteger(NewDecFromInt64(1)) } }) }math/dec_rapid_test.go (3)
13-13: Remove unnecessary comment explaining imported packageThe comment
// Rapid is a Go library for property-based testing.is unnecessary as it states what the imported package does, which can be found in the package documentation.Apply this diff to remove the comment:
-// Rapid is a Go library for property-based testing.
60-150: Separate unit tests into individual test functionsTo improve test organization and readability, consider moving the unit tests starting from line 60 into separate test functions outside of
TestDecWithRapid. This aligns with Go testing best practices and the Uber Go style guide, enhancing maintainability.
504-541: Simplify decimal place calculation infloatDecimalPlacesThe function
floatDecimalPlacesuses a complex regular expression and manual parsing to determine the number of decimal places, which can be error-prone and difficult to maintain. Consider leveraging theapdlibrary's capabilities or simplifying the logic to enhance reliability and readability.math/dec.go (2)
352-354: Add documentation for theTextmethodThe
Textmethod has a detailed comment explaining format options, but the function signature lacks a proper GoDoc comment.Consider adding a GoDoc comment to the function signature for consistency and clarity.
Apply this diff:
+// Text converts the Dec to a string according to the given format. func (x Dec) Text(format byte) string { return x.dec.Text(format) }
433-434: Ensure proper JSON encoding inMarshalJSONThe
MarshalJSONmethod usesx.dec.Text('E'), which may not produce the desired JSON representation for all decimal values.Consider using
x.String()or specifying the format to ensure consistency in JSON encoding.Apply this diff:
func (x Dec) MarshalJSON() ([]byte, error) { - return json.Marshal(x.dec.Text('E')) + return json.Marshal(x.String()) }This change will use the standard decimal notation for JSON output, which might be more readable and consistent.
math/legacy_dec.go (3)
566-568: Properly handle error fromd.i.MarshalText()In the
String()method, ifd.i.MarshalText()returns an error, the method currently returns an empty string, potentially suppressing important errors.Consider propagating the error or at least logging it to aid in debugging.
- if err != nil { - return "" - } + if err != nil { + return fmt.Sprintf("error: %v", err) + }Alternatively, modify the method to return
(string, error)to properly handle the error.
460-468: Avoid using panics and recover for control flowIn the
ApproxRootmethod, a deferred function is used to recover from panics and convert them into errors. Using panic and recover for control flow is generally discouraged as it can make the code harder to read and maintain.Consider refactoring the code to avoid panics and handle errors explicitly.
939-941: Simplify testing helper functionsThe functions
LegacyDecEqandLegacyDecApproxEqare intended for testing but return multiple values, including the testing objectt. This pattern is unconventional and can make tests harder to read.Consider simplifying these functions to return a single
boolor use existing testing libraries to improve code clarity.Also applies to: 943-947
math/legacy_dec_test.go (3)
216-217: Fix typo in error message: 'decional' should be 'decimal'There's a typo in the error messages for the test cases; "decional arrays" should be corrected to "decimal arrays" for clarity.
Apply the following changes to fix the typo:
- s.Require().Equal(tc.eq, math.LegacyDecsEqual(tc.d1s, tc.d2s), "equality of decional arrays is incorrect, tc %d", tcIndex) + s.Require().Equal(tc.eq, math.LegacyDecsEqual(tc.d1s, tc.d2s), "equality of decimal arrays is incorrect, tc %d", tcIndex) - s.Require().Equal(tc.eq, math.LegacyDecsEqual(tc.d2s, tc.d1s), "equality of decional arrays is incorrect (converse), tc %d", tcIndex) + s.Require().Equal(tc.eq, math.LegacyDecsEqual(tc.d2s, tc.d1s), "equality of decimal arrays is incorrect (converse), tc %d", tcIndex)
51-56: Avoid unnecessary named return value in 'mustNewDecFromStr'The function
mustNewDecFromStruses a named return valued, which is not necessary here. Removing the named return can improve code readability.Here's the proposed change:
- func (s *decimalTestSuite) mustNewDecFromStr(str string) (d math.LegacyDec) { + func (s *decimalTestSuite) mustNewDecFromStr(str string) math.LegacyDec { d, err := math.LegacyNewDecFromStr(str) s.Require().NoError(err) return d }
693-695: Simplify resetting 'sink' to nilThe assignment
sink = (interface{})(nil)includes an unnecessary type conversion. It can be simplified by directly assigningniltosink.Apply this change:
- sink = (interface{})(nil) + sink = nil
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (5)
math/go.sumis excluded by!**/*.sumsimapp/go.sumis excluded by!**/*.sumsimapp/v2/go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sumx/group/go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
.github/workflows/test.yml(1 hunks)docs/build/building-modules/18-decimal-handling.md(1 hunks)math/dec.go(1 hunks)math/dec_bench_test.go(1 hunks)math/dec_examples_test.go(1 hunks)math/dec_rapid_test.go(1 hunks)math/dec_test.go(1 hunks)math/go.mod(1 hunks)math/int_test.go(0 hunks)math/legacy_dec.go(1 hunks)math/legacy_dec_test.go(1 hunks)math/math.go(1 hunks)math/uint_test.go(0 hunks)simapp/go.mod(1 hunks)simapp/v2/go.mod(1 hunks)tests/go.mod(1 hunks)x/group/go.mod(1 hunks)x/group/internal/math/dec.go(4 hunks)
💤 Files with no reviewable changes (2)
- math/int_test.go
- math/uint_test.go
🧰 Additional context used
📓 Path-based instructions (11)
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_examples_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/legacy_dec.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
math/legacy_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/math.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
tests/go.mod (1)
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
x/group/internal/math/dec.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 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 a big.Int to represen...
(RB_LY_COMMA)
🔇 Additional comments (23)
math/go.mod (2)
6-9: Primary dependency addition looks good.
The addition of github.com/cockroachdb/apd/v3 v3.2.1 aligns perfectly with the PR's objective to introduce a new decimal type. The version is stable (v3.2.1) and the dependency is appropriately placed in the primary require block.
13-30: Verify indirect dependency version compatibility.
The extensive list of indirect dependencies includes critical components like protobuf and crypto libraries. While these are marked as indirect, some are core dependencies that warrant careful version management.
math/math.go (1)
1-12: LGTM: Well-structured package documentation and imports
The package documentation clearly states its purpose, and the imports are organized appropriately.
x/group/internal/math/dec.go (2)
5-10: LGTM! Import changes are well-organized.
The imports follow the standard Go grouping convention and align with the updated error handling approach.
54-58: Verify error handling coverage.
The error handling looks good, but let's ensure we have test coverage for these error cases.
✅ Verification successful
Error handling coverage is comprehensive
The error cases are well-tested with both specific test cases and property-based testing:
- Unit tests cover invalid inputs like "inf", "Infinite", "NaN", and "foo"
- Property-based testing using rapid.StringMatching("[[:alpha:]]+") tests invalid string inputs systematically
- Both parsing errors and non-finite decimal cases are covered
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test cases covering invalid decimal strings and non-finite decimals
rg -l "NewDecFromString.*Invalid|NewDecFromString.*Inf|NewDecFromString.*NaN" --type go
Length of output: 146
Script:
#!/bin/bash
# Let's examine the actual test cases in these files
rg "NewDecFromString.*Invalid|NewDecFromString.*Inf|NewDecFromString.*NaN" -B 2 -A 2 x/group/internal/math/dec_test.go math/dec_rapid_test.go
Length of output: 960
Script:
#!/bin/bash
# Let's check the rapid test implementation to ensure comprehensive coverage
rg "testInvalidNewDecFromString" -A 10 math/dec_rapid_test.go
Length of output: 883
math/dec_examples_test.go (3)
38-81: LGTM! Comprehensive example with good error handling
The example effectively demonstrates addition operations, including edge cases and proper error handling for exponent limits.
127-162: LGTM! Well-structured division examples
Good coverage of division operations including edge cases and proper error handling for division by zero.
200-236: LGTM! Clear examples of integer division
Good demonstration of integer division behavior including truncation cases.
x/group/go.mod (1)
21-21: Verify impact of major version upgrade of cockroachdb/apd.
The upgrade from apd/v2 to apd/v3 is a major version bump that likely includes breaking changes. While this aligns with the PR objectives to introduce a new decimal type, please ensure all usages of the library have been updated accordingly.
Let's verify if there are any remaining v2 references or if other related dependencies need updates:
✅ Verification successful
The previous results showed multiple modules using apd and some go.sum files with v2 references. Let's verify the versions across all modules and check for any code that might still be using v2-specific APIs.
The previous results show consistent v3 usage in go.mod files, but let's verify the actual code imports to ensure they're all using v3 as well.
Upgrade to apd/v3 is consistent across the codebase
The verification shows that:
- All modules using apd are consistently using v3.2.1
- All Go source files are properly importing apd/v3
- No remaining references to apd/v2 in the source code
- The upgrade has been applied uniformly across all dependent packages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining v2 references
rg -l "github.com/cockroachdb/apd/v2"
# Check if other modules also use apd to ensure version consistency
fd -e mod -x grep -l "github.com/cockroachdb/apd"
Length of output: 300
Script:
#!/bin/bash
# Check the apd version in all go.mod files that use it
for mod in $(fd -e mod); do
echo "=== $mod ==="
grep "github.com/cockroachdb/apd" "$mod"
done
# Check for any code references to apd v2 specific types or methods
rg -l "apd\.Decimal|apd\.New|apd\.Context" --type go
Length of output: 7821
Script:
#!/bin/bash
# Check the actual code in files using apd to verify they're using v3
echo "=== x/group/internal/math/dec.go ==="
grep -A 5 "github.com/cockroachdb/apd" x/group/internal/math/dec.go
echo "=== math/dec.go ==="
grep -A 5 "github.com/cockroachdb/apd" math/dec.go
echo "=== schema/testing/fmt.go ==="
grep -A 5 "github.com/cockroachdb/apd" schema/testing/fmt.go
# Check for any v2-specific imports
rg "github.com/cockroachdb/apd/v2" --type go
Length of output: 774
tests/go.mod (2)
Line range hint 250-283: LGTM: Replace directives are properly configured
The replace directives correctly point to local SDK modules, which is essential for testing against the latest changes.
86-86: Verify compatibility with cockroachdb/apd v3 upgrade
The upgrade from apd/v2 to apd/v3 is a major version change that could introduce breaking changes. This aligns with the PR objective to introduce a new decimal type, but we should verify the impact.
Let's check for any v2 to v3 migration issues:
✅ Verification successful
Let me gather more information about the actual usage of the apd package to verify the impact.
The apd/v3 upgrade is properly implemented and safe
The upgrade from apd/v2 to apd/v3 has been consistently applied across the codebase. Key findings:
- The main usage is in
math/dec.goandx/group/internal/math/dec.gofor decimal operations - All direct dependencies in go.mod files have been updated to v3.2.1
- No remaining v2 imports in actual code, only legacy entries in some go.sum files
- Documentation in
docs/build/building-modules/18-decimal-handling.mdconfirms intentional usage of apd for decimal calculations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining v2 imports of apd
rg "github.com/cockroachdb/apd/v2"
# Search for apd usage patterns that might need updates
ast-grep --pattern 'import "$_/apd/v2"'
Length of output: 6250
Script:
#!/bin/bash
# Search for actual usage of apd package
rg -A 3 "cockroachdb/apd"
# Check go.mod files for apd dependencies
find . -name "go.mod" -exec grep -l "cockroachdb/apd" {} \;
Length of output: 10237
simapp/go.mod (1)
78-78: Verify the impact of upgrading cockroachdb/apd to v3.
The upgrade to github.com/cockroachdb/apd/v3 v3.2.1 aligns with the PR objective of introducing a new decimal type. However, as this is a major version upgrade (v2 to v3), it's important to verify the impact.
Let's check for any potential breaking changes in the usage:
✅ Verification successful
Let me gather more information about the specific usage to understand the impact of the v3 upgrade.
The upgrade to apd/v3 is properly implemented and safe
The verification shows that:
- All direct imports are using
github.com/cockroachdb/apd/v3 - The v2 references only appear in old
go.sumfiles which is expected - The code has been properly adapted to use v3 API with appropriate error handling and context settings
- The implementation maintains safety by avoiding mutations and using new decimal instances for operations
The upgrade is well-implemented with proper error handling and follows best practices for decimal arithmetic operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any direct usage of the apd package to verify compatibility
# with the v3 upgrade
# Search for imports
rg -l 'github.com/cockroachdb/apd'
# Search for any direct usage of apd types or functions
ast-grep --pattern 'apd.$_'
Length of output: 835
Script:
#!/bin/bash
# Check the content of files that directly use apd types
cat x/group/internal/math/dec.go math/dec.go math/math.go schema/testing/fmt.go
# Check if there are any v2 imports still present
rg "github.com/cockroachdb/apd/v2"
Length of output: 23301
simapp/v2/go.mod (2)
Line range hint 85-91: LGTM: Required cockroachdb dependencies are properly included.
The additional cockroachdb dependencies (errors, fifo, logtags, pebble, redact, tokenbucket) are correctly included as indirect dependencies. These are required by the apd v3 library and their versions are compatible.
84-84: Verify the impact of upgrading cockroachdb/apd to v3.
The upgrade from apd v2 to v3 is a major version change that may introduce breaking changes. This aligns with the PR objective to introduce a new decimal type, but we should verify the impact.
Let's check for any usage of the old apd v2 package:
✅ Verification successful
Let me gather more information to verify if these usages are from v2 or v3 by checking the imports in these files.
Let me try a different approach to check the imports.
The upgrade to cockroachdb/apd v3 is properly implemented
All files using the apd package are correctly importing v3 (github.com/cockroachdb/apd/v3). The presence of apd in go.sum files is expected as they track dependency history. The codebase shows consistent usage of v3 APIs with proper imports, indicating a complete migration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining usage of apd v2 to ensure complete migration
# Test: Search for imports of the old package. Expect: No results
rg -l 'github.com/cockroachdb/apd/v2'
# Test: Search for any v2 specific types/functions. Expect: No results
rg 'apd\.Decimal|apd\.New|apd\.Context'
Length of output: 1657
Script:
#!/bin/bash
# Check imports in the files that showed apd usage
rg -p "import.*cockroachdb/apd" x/group/internal/math/dec.go schema/testing/fmt.go schema/testing/diff.go math/dec.go math/math.go
Length of output: 132
Script:
#!/bin/bash
# Check the content of files showing apd usage to see import statements
echo "=== x/group/internal/math/dec.go ==="
head -n 20 x/group/internal/math/dec.go
echo "=== schema/testing/fmt.go ==="
head -n 20 schema/testing/fmt.go
echo "=== schema/testing/diff.go ==="
head -n 20 schema/testing/diff.go
echo "=== math/dec.go ==="
head -n 20 math/dec.go
echo "=== math/math.go ==="
head -n 20 math/math.go
Length of output: 3001
.github/workflows/test.yml (2)
Line range hint 1-24: LGTM: Well-structured test execution pattern.
The workflow follows a consistent and robust pattern for test execution:
- Proper code checkout
- Go environment setup with caching
- Efficient change detection
- Comprehensive test execution with coverage
- Integration with SonarCloud for code quality analysis
Line range hint 8-15: LGTM: Efficient job configurations and resource management.
The workflow demonstrates excellent practices:
- Proper concurrency control to prevent redundant runs
- Logical job organization by module type
- Efficient cache usage for dependencies
- Clear job dependencies and execution flow
math/dec_bench_test.go (1)
350-351: 🛠️ Refactor suggestion
Handle error returned by Sub function
In the NewDec benchmark for subtraction, the error returned by Sub is being ignored. According to the Uber Go Style Guide, errors should not be silently ignored but properly handled to ensure reliability.
Apply this diff to handle the error:
for i := 0; i < b.N; i++ {
- _, _ = newB1.Sub(newB2)
+ diff, err := newB1.Sub(newB2)
+ if err != nil {
+ b.Fatalf("Subtraction error: %v", err)
+ }
}Likely invalid or redundant comment.
math/dec.go (6)
436-447: Handle edge cases in UnmarshalJSON
In the UnmarshalJSON method, the variable text is declared without initialization. Additionally, error handling could be improved.
Ensure that the JSON unmarshaling handles all possible input cases, including invalid data.
Run the following script to check for potential issues:
#!/bin/bash
# Description: Find all usages of `UnmarshalJSON` and check for proper error handling.
rg 'UnmarshalJSON' math/Consider adding validation for the input data and handling unexpected input gracefully.
416-422:
Handle potential panics when calling Marshal in MarshalTo
The MarshalTo method calls x.Marshal(), which currently panics. Since Marshal is not implemented and returns an error, MarshalTo should handle this error appropriately to prevent unexpected panics.
Ensure that MarshalTo handles the error from Marshal properly. Apply this diff:
func (x Dec) MarshalTo(data []byte) (n int, err error) {
bz, err := x.Marshal()
if err != nil {
return 0, err
}
return copy(data, bz), nil
}Also, after implementing the Marshal method to return an error instead of panicking, ensure that MarshalTo correctly propagates this error.
Likely invalid or redundant comment.
246-249:
Simplify error handling in Mul method
In the Mul method, you don't need to wrap err when it's nil. The function should return nil directly if there's no error.
Apply this diff to fix the issue:
func (x Dec) Mul(y Dec) (Dec, error) {
var z Dec
if _, err := dec128Context.Mul(&z.dec, &x.dec, &y.dec); err != nil {
return z, ErrInvalidDec.Wrap(err.Error())
}
- return z, errors.Wrap(err, "decimal multiplication error")
+ return z, nil
}Likely invalid or redundant comment.
235-239:
Avoid wrapping nil error in QuoInteger
In the QuoInteger method, at line 239, you're wrapping err which is nil. Since there's no error, you should return nil directly.
Apply this diff to fix the issue:
func (x Dec) QuoInteger(y Dec) (Dec, error) {
var z Dec
_, err := dec128Context.QuoInteger(&z.dec, &x.dec, &y.dec)
if err != nil {
return z, ErrInvalidDec.Wrap(err.Error())
}
- return z, errors.Wrap(err, "decimal integer quotient error")
+ return z, nil
}Likely invalid or redundant comment.
262-270:
Avoid wrapping nil error in MulExact method
In the MulExact method, after handling potential errors and rounding conditions, you wrap err which is nil. There's no need to wrap a nil error.
Apply this diff to fix the issue:
func (x Dec) MulExact(y Dec) (Dec, error) {
var z Dec
condition, err := dec128Context.Mul(&z.dec, &x.dec, &y.dec)
if err != nil {
return z, ErrInvalidDec.Wrap(err.Error())
}
if condition.Rounded() {
return z, ErrUnexpectedRounding
}
- return z, errors.Wrap(err, "decimal multiplication error")
+ return z, nil
}Likely invalid or redundant comment.
306-329: Review error handling and logic in SdkIntTrim method
The SdkIntTrim method includes complex logic for adjusting the coefficient based on the exponent. There's a risk of integer overflow or incorrect calculations.
Please verify that the logic correctly handles all possible values of Dec, especially large exponents and coefficients. Consider adding unit tests to cover edge cases.
Run the following script to identify potential issues:
This will help ensure that the method is robust and that all edge cases are tested.
✅ Verification successful
The SdkIntTrim implementation is robust and well-tested
The implementation correctly handles:
- Integer overflow checks using
MaxBitLenconstant (256 bits) - Proper error handling with
ErrNonIntegralfor non-integral values - Comprehensive test cases covering:
- Decimal to integer conversions
- Large numbers up to max allowed value
- Negative numbers
- Zero values
- Edge cases with different decimal places
The test cases in TestToSdkInt demonstrate that the method properly handles rounding towards zero and validates against the maximum allowed integer size. The error handling for overflow cases is also properly implemented and tested.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of `SdkIntTrim` and ensure proper error handling.
# Search for `SdkIntTrim` method implementations and calls.
rg 'SdkIntTrim'
# Check for unit tests that cover `SdkIntTrim`.
rg -A 5 'func TestSdkIntTrim' math/
Length of output: 1514
Script:
#!/bin/bash
# Let's search for test cases related to SdkIntTrim in dec_test.go
rg -A 10 'SdkIntTrim' math/dec_test.go
# Let's also check for any error handling patterns in the tests
rg 'ErrNonIntegral' math/dec_test.go
# Check the implementation of Reduce() method since it's used in SdkIntTrim
ast-grep --pattern 'func (x Dec) Reduce() (Dec, error) {
$$$
}'
Length of output: 378
Script:
#!/bin/bash
# Let's look at the complete test file content to understand test coverage
cat math/dec_test.go
# Let's also check for any test table definitions that might contain SdkIntTrim test cases
rg -B 5 -A 15 'struct.*{.*SdkIntTrim' math/dec_test.go
# Check for any constants or variables used in testing large numbers
rg 'MaxBitLen|ErrNonIntegral' math/
Length of output: 36123
* main: (31 commits) docs: update links for https security protocol (#22514) build(deps): Bump github.com/bytedance/sonic from 1.12.3 to 1.12.4 in /log (#22513) feat(x/protocolpool)!: allow any coins in continuous funds (#21916) docs: Update protobuf tx signing message format outer link (#22510) test(accounts): fix integration tests (#22418) chore(x): fix some typos in comment (#22508) build(deps): Bump cosmossdk.io/log from 1.4.1 to 1.5.0 (#22487) build(deps): Bump cosmossdk.io/core from 1.0.0-alpha.5 to 1.0.0-alpha.6 (#22502) build(deps): Bump golang.org/x/crypto from 0.28.0 to 0.29.0 (#22480) docs(adr75): server v2 (#21069) fix(server/v2): improve server stop (#22455) chore: prepare core changelog (#22495) refactor(store/v2): simplify genesis flow (#22435) build(deps): Bump google.golang.org/grpc from 1.67.1 to 1.68.0 (#22486) build(deps): Bump golang.org/x/sync from 0.8.0 to 0.9.0 (#22482) feat(x/circuit): Allow msg Reset with empty msgURL (#22459) build(deps): Bump actions/xxx-artifact from v3 to v4 (#22468) feat(stf/branch): simplify merged iterator (#22131) refactor(log): disable coloring in testing logger (#22466) chore(x/tx): update changelog to alpha.2 (#22465) ...
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
docs/build/building-modules/18-decimal-handling.md (3)
6-21: Add SDK version information for better context.Consider adding the SDK version where
Decwas introduced and whenLegacyDecwill be deprecated (if planned).
27-48: Specify language for the code block.Add
goto the code fence for proper syntax highlighting:-``` +```go🧰 Tools
🪛 Markdownlint
27-27: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
69-79: Add code comments to explain conversion logic.Consider adding comments to explain:
- The string conversion approach used
- Why this approach maintains data integrity
- Any precision considerations
Example:
// LegacyDecToDec converts a LegacyDec to Dec by using string representation // to maintain exact decimal places without precision loss func LegacyDecToDec(ld LegacyDec) (Dec, error) { return NewDecFromString(ld.String()) }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
math/go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
docs/build/building-modules/18-decimal-handling.md(1 hunks)math/dec_rapid_test.go(1 hunks)math/go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- math/dec_rapid_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"
🪛 Markdownlint
docs/build/building-modules/18-decimal-handling.md
27-27: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (1)
docs/build/building-modules/18-decimal-handling.md (1)
1-5: LGTM! Document structure and metadata are well-organized.
|
I would add the changelog with #22525 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
math/dec.go (3)
13-13: Add documentation for the interface implementation marker.The line declaring interface implementation conformance should be documented to explain its purpose.
+// Ensure Dec implements the customProtobufType interface var _ customProtobufType = &Dec{}
94-98: Add error handling for extreme values in NewDecFromInt64.The function should handle potential overflow cases when converting large int64 values.
func NewDecFromInt64(x int64) Dec { var res Dec - res.dec.SetInt64(x) + err := res.dec.SetInt64(x) + if err != nil { + panic(fmt.Sprintf("unexpected error in NewDecFromInt64: %v", err)) + } return res }
309-311: Enhance documentation for SdkIntTrim rounding behavior.The documentation should explicitly state the rounding behavior for values between integers.
// SdkIntTrim rounds the decimal number towards zero to the nearest integer, then converts and returns it as `sdkmath.Int`. // It handles both positive and negative values correctly by truncating towards zero. // This function returns an ErrNonIntegral error if the resulting integer is larger than the maximum value that `sdkmath.Int` can represent. +// For example: +// - 1.7 becomes 1 +// - -1.7 becomes -1 +// - 1.2 becomes 1 +// - -1.2 becomes -1math/dec_test.go (2)
1304-1309: Add documentation for the must helper functionThe
musthelper function lacks documentation explaining its purpose and panic behavior.Add a doc comment explaining the function's purpose and panic behavior:
+// must is a helper that wraps a call returning (T, error) and panics if the error is non-nil. +// It's intended for use in tests only. func must[T any](r T, err error) T { if err != nil { panic(err) } return r }
1312-1409: Implement the skipped marshal/unmarshal testsThe marshal/unmarshal tests are currently skipped with a TODO comment. These tests are important for ensuring proper serialization behavior.
Would you like me to help implement these tests or create a GitHub issue to track this task?
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
math/go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
math/dec.go(1 hunks)math/dec_examples_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 (2)
- math/dec_examples_test.go
- math/go.mod
🧰 Additional context used
📓 Path-based instructions (2)
math/dec.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
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"
🪛 golangci-lint
math/dec_test.go
1284-1284: printf: non-constant format string in call to fmt.Sprintf
(govet)
🔇 Additional comments (2)
math/dec.go (1)
212-220: Remove unnecessary error wrapping in QuoExact.
The error wrapping at the end of the function is unnecessary as err is already checked and is nil at this point.
func (x Dec) QuoExact(y Dec) (Dec, error) {
var z Dec
condition, err := dec128Context.Quo(&z.dec, &x.dec, &y.dec)
if err != nil {
return z, ErrInvalidDec.Wrap(err.Error())
}
if condition.Rounded() {
return z, ErrUnexpectedRounding
}
- return z, errors.Wrap(err, "decimal quotient error")
+ return z, nil
}math/dec_test.go (1)
38-38: Replace calculated expected values with hard-coded constants
Using functions under test (Add, Sub) to generate expected values can mask potential bugs because if the method under test has an error, the test may not detect it.
Also applies to: 42-42, 255-255, 371-371
| func (x Dec) Marshal() ([]byte, error) { | ||
| // implemented in a new PR. See: https://github.com/cosmos/cosmos-sdk/issues/22525 | ||
| panic("not implemented") |
There was a problem hiding this comment.
TODO implementation causes Size method to panic.
The Marshal method is not implemented and will panic, which also affects the Size method that depends on it.
Would you like me to help implement the Marshal and Unmarshal methods as mentioned in issue #22525?
| t.Run(tc[0], func(t *testing.T) { | ||
| out, err := math.FormatDec(tc[0]) | ||
| for _, tc := range tcs { | ||
| t.Run(fmt.Sprintf(tc.src), func(t *testing.T) { |
There was a problem hiding this comment.
Use a constant format string in fmt.Sprintf
The format string in fmt.Sprintf(tc.src) is non-constant. This could lead to potential security issues if the source string contains format specifiers.
Apply this diff to fix the issue:
-t.Run(fmt.Sprintf(tc.src), func(t *testing.T) {
+t.Run(fmt.Sprintf("%s", tc.src), func(t *testing.T) {📝 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.
| t.Run(fmt.Sprintf(tc.src), func(t *testing.T) { | |
| t.Run(fmt.Sprintf("%s", tc.src), func(t *testing.T) { |
🧰 Tools
🪛 golangci-lint
1284-1284: printf: non-constant format string in call to fmt.Sprintf
(govet)
There was a problem hiding this comment.
Copilot reviewed 17 out of 23 changed files in this pull request and generated no suggestions.
Files not reviewed (6)
- math/go.mod: Language not supported
- simapp/go.mod: Language not supported
- simapp/v2/go.mod: Language not supported
- tests/go.mod: Language not supported
- x/group/go.mod: Language not supported
- math/dec_examples_test.go: Evaluated as low risk
Comments skipped due to low confidence (2)
x/group/internal/math/dec.go:27
- [nitpick] The error message 'decimal subtraction error' is not very descriptive. Consider changing it to 'error during decimal subtraction'.
return z, errorsmod.Wrap(err, "decimal subtraction error")
math/uint_test.go:345
- Removing this line could lead to unintended behavior due to variable capture in goroutines. Consider re-adding
value := valueto avoid this issue.
value := value
* main: build(deps): Bump cosmossdk.io/math from 1.3.0 to 1.4.0 (#22580) fix(server/v2/api/telemetry): enable global metrics (#22571) refactor(server/v2/cometbft): add `codec.Codec` and clean-up APIs (#22566) feat(core/coretesting): make memDB satisfy db.Db interface (#22570) Merge commit from fork fix(server(/v2)): fix fallback genesis path (#22564) fix: only overwrite context chainID when necessary (#22568) docs(client): Update setFeeGranter and setFeePayer comments (#22526) fix(baseapp): populate header info in `NewUncachedContext` (#22557) build(deps): Bump buf.build/gen/go/cometbft/cometbft/protocolbuffers/go from 1.35.1-20240701160653-fedbb9acfd2f.1 to 1.35.2-20240701160653-fedbb9acfd2f.1 in /api (#22551) build(deps): Bump github.com/creachadair/atomicfile from 0.3.5 to 0.3.6 in /tools/confix (#22552) docs: Update reference of Approximation (#22550) fix(server/v2/comebft): wire missing services + fix simulation (#21964) ci: fix permissions for GITHUB_TOKEN on dependabot workflows (#22547) ci: fix permissions for GITHUB_TOKEN in spell check workflow (#22545) build(deps): Bump google.golang.org/protobuf from 1.35.1 to 1.35.2 (#22537) fix(cosmovisor): premature upgrade on restart (#22528) fix(store/v2/pebble): handle version 0 in keys (#22524) refactor(server/v2/telemetry): swap redirects (#22520) docs: Update content in CODE_OF_CONDUCT.md (#22518)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (8)
tests/go.mod (1)
Line range hint
3-3: Invalid Go version specifiedThe Go version
1.23.1is not valid as it does not exist. The latest stable version of Go is 1.22.1.Apply this diff to fix the Go version:
-go 1.23.1 +go 1.22math/legacy_dec_test.go (5)
51-141: Consider enhancing error message assertionsWhile the string conversion tests are comprehensive, consider adding specific error message assertions for invalid inputs to ensure the correct error types are being returned.
Example enhancement for error cases:
if tc.expErr { - s.Require().NotNil(err, "error expected, decimalStr %v, tc %v", tc.decimalStr, tcIndex) + s.Require().Error(err, "error expected, decimalStr %v, tc %v", tc.decimalStr, tcIndex) + s.Require().Contains(err.Error(), "expected error message", "incorrect error message for %v, tc %v", tc.decimalStr, tcIndex) }
143-221: Enhance float64 conversion test coverageConsider adding test cases for:
- Values that are too large for float64
- NaN and Infinity handling
- Precision loss scenarios
Example additional test cases:
tests := []struct { d math.LegacyDec want float64 + expectErr bool }{ // ... existing cases ... + {math.LegacyNewDecFromBigIntWithPrec(maxInt, 0), 0, true}, // Too large for float64 + {math.LegacyNewDecWithPrec(-1, 1000), 0, true}, // Too small for float64 }
223-293: Consider adding systematic boundary tests for arithmetic operationsWhile the arithmetic operation tests are comprehensive, consider adding systematic boundary tests for:
- Operations near max/min decimal values
- Precision boundary cases
- Overflow scenarios
Example additional test cases:
tests := []struct { d1, d2 math.LegacyDec expMul, expMulTruncate, expMulRoundUp math.LegacyDec expQuo, expQuoRoundUp, expQuoTruncate math.LegacyDec expAdd, expSub math.LegacyDec }{ // ... existing cases ... + { + math.LegacyMaxDec(), + math.LegacyOneDec(), + math.LegacyMaxDec(), + math.LegacyMaxDec(), + math.LegacyMaxDec(), + math.LegacyMaxDec(), + math.LegacyMaxDec(), + math.LegacyMaxDec(), + nil, // expect panic + math.LegacyMaxDec().Sub(math.LegacyOneDec()), + }, }
1035-1073: Improve documentation for asymmetric behavior testThe test for asymmetric behavior between zero and empty decimal values could benefit from clearer documentation explaining:
- The expected behavior differences
- The implications for API consumers
- Potential migration considerations
Add detailed comments explaining the asymmetry:
func Test_DocumentLegacyAsymmetry(t *testing.T) { + // This test documents an important asymmetry in the legacy decimal type: + // 1. A zero decimal and an empty decimal are semantically different + // 2. They serialize differently in JSON but identically in protobuf + // 3. This can lead to unexpected behavior when round-tripping values + // + // API consumers should be aware that: + // - Always initialize decimals using NewDec() or ZeroDec() + // - Never use empty decimal struct literals + // - Validate decimal values after deserialization
1075-1329: Consider restructuring boundary test data for better maintainabilityThe boundary tests could be more maintainable by:
- Moving test constants to a separate test constants file
- Using table-driven subtests consistently
- Adding helper functions for common test setup
Example restructuring:
+// testdata/dec_test_constants.go +const ( + maxValidDecNumber = "115792089237316195423570985008687907853269984665640564039457584007913129639935999999999999999999" +) + +type boundaryTestCase struct { + name string + setup func() math.LegacyDec + op func(math.LegacyDec) math.LegacyDec + wantErr bool + want string +} + +func getBoundaryTestCases() []boundaryTestCase { + return []boundaryTestCase{ + // ... test cases ... + } +}math/legacy_dec.go (2)
217-231: Adjust comments for exported methods to follow Go conventionsAccording to the Go conventions, comments for exported methods should be placed above the method definition and start with the method name. This improves readability and consistency.
Apply the following changes to correct the comment placement and style:
- func (d LegacyDec) IsNil() bool { return d.i == nil } // is decimal nil + // IsNil returns true if the decimal is nil. + func (d LegacyDec) IsNil() bool { return d.i == nil } - func (d LegacyDec) IsZero() bool { return (d.i).Sign() == 0 } // is equal to zero + // IsZero returns true if the decimal equals zero. + func (d LegacyDec) IsZero() bool { return d.i.Sign() == 0 } - func (d LegacyDec) IsNegative() bool { return (d.i).Sign() == -1 } // is negative + // IsNegative returns true if the decimal is negative. + func (d LegacyDec) IsNegative() bool { return d.i.Sign() == -1 } - func (d LegacyDec) IsPositive() bool { return (d.i).Sign() == 1 } // is positive + // IsPositive returns true if the decimal is positive. + func (d LegacyDec) IsPositive() bool { return d.i.Sign() == 1 } - func (d LegacyDec) Equal(d2 LegacyDec) bool { return (d.i).Cmp(d2.i) == 0 } // equal decimals + // Equal returns true if the decimal is equal to d2. + func (d LegacyDec) Equal(d2 LegacyDec) bool { return d.i.Cmp(d2.i) == 0 } - func (d LegacyDec) GT(d2 LegacyDec) bool { return (d.i).Cmp(d2.i) > 0 } // greater than + // GT returns true if the decimal is greater than d2. + func (d LegacyDec) GT(d2 LegacyDec) bool { return d.i.Cmp(d2.i) > 0 } - func (d LegacyDec) GTE(d2 LegacyDec) bool { return (d.i).Cmp(d2.i) >= 0 } // greater than or equal + // GTE returns true if the decimal is greater than or equal to d2. + func (d LegacyDec) GTE(d2 LegacyDec) bool { return d.i.Cmp(d2.i) >= 0 } - func (d LegacyDec) LT(d2 LegacyDec) bool { return (d.i).Cmp(d2.i) < 0 } // less than + // LT returns true if the decimal is less than d2. + func (d LegacyDec) LT(d2 LegacyDec) bool { return d.i.Cmp(d2.i) < 0 } - func (d LegacyDec) LTE(d2 LegacyDec) bool { return (d.i).Cmp(d2.i) <= 0 } // less than or equal + // LTE returns true if the decimal is less than or equal to d2. + func (d LegacyDec) LTE(d2 LegacyDec) bool { return d.i.Cmp(d2.i) <= 0 } - func (d LegacyDec) Neg() LegacyDec { return LegacyDec{new(big.Int).Neg(d.i)} } // reverse the decimal sign + // Neg returns the negative of the decimal. + func (d LegacyDec) Neg() LegacyDec { return LegacyDec{new(big.Int).Neg(d.i)} } - func (d LegacyDec) NegMut() LegacyDec { d.i.Neg(d.i); return d } // reverse the decimal sign, mutable + // NegMut negates the decimal in place and returns it. + func (d LegacyDec) NegMut() LegacyDec { d.i.Neg(d.i); return d } - func (d LegacyDec) Abs() LegacyDec { return LegacyDec{new(big.Int).Abs(d.i)} } // absolute value + // Abs returns the absolute value of the decimal. + func (d LegacyDec) Abs() LegacyDec { return LegacyDec{new(big.Int).Abs(d.i)} } - func (d LegacyDec) AbsMut() LegacyDec { d.i.Abs(d.i); return d } // absolute value, mutable + // AbsMut sets the decimal to its absolute value and returns it. + func (d LegacyDec) AbsMut() LegacyDec { d.i.Abs(d.i); return d } - func (d LegacyDec) Set(d2 LegacyDec) LegacyDec { d.i.Set(d2.i); return d } // set to existing dec value + // Set assigns the value of d2 to the decimal and returns it. + func (d LegacyDec) Set(d2 LegacyDec) LegacyDec { d.i.Set(d2.i); return d } - func (d LegacyDec) Clone() LegacyDec { return LegacyDec{new(big.Int).Set(d.i)} } // clone new dec + // Clone returns a new LegacyDec that is a copy of the decimal. + func (d LegacyDec) Clone() LegacyDec { return LegacyDec{new(big.Int).Set(d.i)} }
628-658: SimplifychopPrecisionAndRoundby removing unnecessary recursionThe
chopPrecisionAndRoundfunction handles negative numbers by negating the input, calling itself recursively, and then negating the result. This can be simplified to avoid recursion, improving readability and performance.Refactor the function as follows:
func chopPrecisionAndRound(d *big.Int) *big.Int { // Remove the negative sign and record it isNegative := d.Sign() == -1 - if isNegative { - d.Neg(d) - } + absD := new(big.Int).Abs(d) // get the truncated quotient and remainder - quo, rem := d, big.NewInt(0) - quo, rem = quo.QuoRem(d, precisionReuse, rem) + quo, rem := new(big.Int).QuoRem(absD, precisionReuse, new(big.Int)) result := quo if rem.Sign() != 0 { switch rem.Cmp(fivePrecision) { case -1: // Do nothing, result is correct case 1: result = quo.Add(quo, oneInt) default: // bankers rounding if quo.Bit(0) != 0 { result = quo.Add(quo, oneInt) } } } if isNegative { result.Neg(result) } return result }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (4)
simapp/go.sumis excluded by!**/*.sumsimapp/v2/go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sumx/group/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
math/go.mod(1 hunks)math/legacy_dec.go(1 hunks)math/legacy_dec_test.go(1 hunks)simapp/go.mod(1 hunks)simapp/v2/go.mod(1 hunks)tests/go.mod(1 hunks)x/group/go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- math/go.mod
- simapp/go.mod
- simapp/v2/go.mod
- x/group/go.mod
🧰 Additional context used
📓 Path-based instructions (3)
math/legacy_dec.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
math/legacy_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"
tests/go.mod (1)
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
🪛 golangci-lint
math/legacy_dec_test.go
635-635: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
639-639: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
🔇 Additional comments (5)
tests/go.mod (2)
Line range hint 1-300: LGTM for remaining changes
The rest of the changes look good:
- All new dependencies are properly versioned
- Replace directives are correctly configured for local testing
- Module structure follows Go standards
98-98: Verify APD version compatibility
The upgrade to github.com/cockroachdb/apd/v3 v3.2.1 aligns with the PR objectives to introduce a new decimal type. However, let's verify there are no breaking changes that could affect the implementation.
✅ Verification successful
Let me gather more specific information about the APD usage to verify version compatibility.
APD v3.2.1 upgrade is compatible and consistently used
The verification shows that the APD v3.2.1 upgrade is safe and properly aligned across the codebase:
- All main modules consistently use
github.com/cockroachdb/apd/v3 v3.2.1 - The implementation is used primarily in math and decimal handling modules
- No breaking changes are evident in the usage patterns
- The upgrade aligns with the documented decimal handling approach in
docs/build/building-modules/18-decimal-handling.md
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for breaking changes in APD v3.2.1
# Search for all APD usage in the codebase
echo "Searching for APD usage patterns..."
rg -l "github.com/cockroachdb/apd"
# Look for specific APD type usage
echo "Checking APD type usage..."
ast-grep --pattern 'apd.$_'
Length of output: 918
Script:
#!/bin/bash
# Check the specific APD imports and version references
echo "Checking APD import statements..."
rg "github.com/cockroachdb/apd" -A 2
# Check decimal-related function signatures and usages
echo "Checking decimal-related implementations..."
ast-grep --pattern 'func $_($_ *apd.Decimal) $_'
# Look for APD context configurations
echo "Checking APD context configurations..."
ast-grep --pattern 'apd.Context{'
Length of output: 8277
math/legacy_dec_test.go (1)
1-49: Well-structured test suite with good basic test coverage!
The test suite is well-organized using testify's suite package and includes comprehensive basic test cases for decimal approximation. The test structure follows good practices with clear naming conventions.
math/legacy_dec.go (2)
836-864:
Handle errors returned by Marshal in MarshalTo
In MarshalTo, the error returned by d.Marshal() is not properly handled. Ignoring errors can lead to unexpected behaviors or panics. It's important to check and handle all returned errors.
Apply this diff to handle the error:
func (d *LegacyDec) MarshalTo(data []byte) (n int, err error) {
i := d.i
if i == nil {
i = new(big.Int)
}
if i.Sign() == 0 {
copy(data, []byte{0x30})
return 1, nil
}
- bz, err := d.Marshal()
+ bz, marshalErr := d.Marshal()
- if err != nil {
- return 0, err
+ if marshalErr != nil {
+ return 0, marshalErr
}
copy(data, bz)
return len(bz), nil
}Likely invalid or redundant comment.
605-613: 🛠️ Refactor suggestion
Avoid panicking in MustFloat64; return an error instead
The MustFloat64 method panics if the conversion fails. Panicking is discouraged in libraries as it can crash applications using the library. Instead, consider returning an error to allow the caller to handle it gracefully.
Modify the method to return an error:
-func (d LegacyDec) MustFloat64() float64 {
+func (d LegacyDec) MustFloat64() (float64, error) {
if value, err := strconv.ParseFloat(d.String(), 64); err != nil {
- panic(err)
+ return 0, err
} else {
- return value
+ return value, nil
}
}Likely invalid or redundant comment.
| func (d LegacyDec) AddMut(d2 LegacyDec) LegacyDec { | ||
| d.i.Add(d.i, d2.i) | ||
|
|
||
| d.assertInValidRange() | ||
| return d | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use pointer receiver for methods that mutate the receiver
Methods that modify the receiver should use a pointer receiver to make the mutation explicit and to prevent unintended copies. Currently, AddMut has a value receiver but mutates the receiver's internal state, which can lead to confusion and bugs.
Apply this change to the AddMut method:
-func (d LegacyDec) AddMut(d2 LegacyDec) LegacyDec {
+func (d *LegacyDec) AddMut(d2 LegacyDec) LegacyDec {
d.i.Add(d.i, d2.i)
d.assertInValidRange()
- return d
+ return *d
}Similarly, update other mutative methods to use pointer receivers:
// SubMut mutable subtraction
-func (d LegacyDec) SubMut(d2 LegacyDec) LegacyDec {
+func (d *LegacyDec) SubMut(d2 LegacyDec) LegacyDec {
d.i.Sub(d.i, d2.i)
d.assertInValidRange()
- return d
+ return *d
}
// MulMut mutable multiplication
-func (d LegacyDec) MulMut(d2 LegacyDec) LegacyDec {
+func (d *LegacyDec) MulMut(d2 LegacyDec) LegacyDec {
d.i.Mul(d.i, d2.i)
chopped := chopPrecisionAndRound(d.i)
*d.i = *chopped
d.assertInValidRange()
- return d
+ return *d
}
// NegMut reverses the decimal sign, mutable
-func (d LegacyDec) NegMut() LegacyDec {
+func (d *LegacyDec) NegMut() LegacyDec {
d.i.Neg(d.i)
- return d
+ return *d
}
// AbsMut sets the decimal to its absolute value, mutable
-func (d LegacyDec) AbsMut() LegacyDec {
+func (d *LegacyDec) AbsMut() LegacyDec {
d.i.Abs(d.i)
- return d
+ return *d
}📝 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.
| func (d LegacyDec) AddMut(d2 LegacyDec) LegacyDec { | |
| d.i.Add(d.i, d2.i) | |
| d.assertInValidRange() | |
| return d | |
| } | |
| func (d *LegacyDec) AddMut(d2 LegacyDec) LegacyDec { | |
| d.i.Add(d.i, d2.i) | |
| d.assertInValidRange() | |
| return *d | |
| } |
* main: feat(serverv2): add benchmarks of (old) cacheKV vs branch (#22497) fix: match comets bls implmentation (#22613) feat(math): Upstream GDA based decimal type (#21982) fix(client/v2/autocli): prevent duplicate addition of customCommands (#22576) build(deps): Bump cosmossdk.io/math from 1.3.0 to 1.4.0 (#22580)
Description
Rebased #20536
See: #11783
Left to do:
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
LegacyDectoDec, including migration processes.Dectype for improved decimal representation with enhanced performance and flexibility.Decinstances, ensuring robust error handling.Bug Fixes
Tests
Dectype, ensuring performance and usability.Dectype under various scenarios.Documentation