feat(math): Implement custom hybrid un-/marshal model #22529
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request enhance the Changes
Assessment against linked issues
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
|
This comment has been minimized.
This comment has been minimized.
alpe
requested review from
auricom,
julienrbrt,
tac0turtle,
kocubinski and
akhilkumarpilli
as code owners
alpe
force-pushed
the
alex/sam/dec-type_marshal_22525
branch
from
a44f9d1
to
ac56675
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
math/dec_test.go (2)
1319-1323: Consider adding more descriptive test case names for zero values.The test cases "Zero value" and "-0" could be more descriptive to better explain the purpose of testing different representations of zero.
Consider renaming to:
- "Zero value": { + "Zero should be normalized to '0'": { - "-0": { + "Negative zero should be normalized to '0'": {
1325-1339: Group decimal place test cases for better organization.The test cases for decimal places could be grouped together with a common prefix for better readability and organization.
Consider renaming the test cases:
- "1 decimal place": { + "decimal_places/1_digit": { - "2 decimal places": { + "decimal_places/2_digits": { - "3 decimal places": { + "decimal_places/3_digits": { - "4 decimal places": { + "decimal_places/4_digits": {math/dec.go (1)
472-477: FunctionisEmptyExpis currently unusedThe function
isEmptyExpis not referenced elsewhere in the codebase. Consider removing it to clean up the code, unless it is intended for future use.🧰 Tools
🪛 golangci-lint (1.61.0)
472-472: func
isEmptyExpis unused(unused)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
math/dec.go(2 hunks)math/dec_test.go(4 hunks)
🧰 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 (1.61.0)
math/dec.go
472-472: func isEmptyExp is unused
(unused)
🔇 Additional comments (5)
math/dec.go (5)
7-7: Import "strconv" correctly included
The strconv package is required for number formatting in the fmtE function.
411-432: Marshal method implemented correctly
The Marshal method correctly serializes the decimal value into a byte slice using the custom fmtE formatter, handling potential errors appropriately.
435-468: Implementation of custom formatter fmtE is acceptable
The fmtE function effectively formats the decimal into hybrid notation based on the exponent value, aligning with the specified requirements.
441-443: Verify adherence to exponent boundary specifications
Ensure that the condition adj > -6 && adj < 6 in the fmtE function correctly implements the intended boundaries at exponents of ±6 for switching between decimal and scientific notation, as specified in the PR objectives. Consider adding unit tests to validate behavior at the boundary values (e.g., adj == -6 and adj == 6).
479-492: Unmarshal method implemented correctly
The Unmarshal method accurately parses the byte slice into the Dec struct, handling errors appropriately and ensuring the decimal is finite.
| "1": { | ||
| x: must(NewDecFromString("1")), | ||
| exp: "1", | ||
| }, | ||
| "12": { | ||
| x: must(NewDecFromString("12")), | ||
| exp: "12", | ||
| }, | ||
| "123": { | ||
| x: must(NewDecFromString("123")), | ||
| exp: "123", | ||
| }, | ||
| "1234": { | ||
| x: must(NewDecFromString("1234")), | ||
| exp: "1234", | ||
| }, | ||
| "12345": { | ||
| x: must(NewDecFromString("12345")), | ||
| exp: "12345", | ||
| }, | ||
| "123456": { | ||
| x: must(NewDecFromString("123456")), | ||
| exp: "123456", | ||
| }, | ||
| "1234567": { | ||
| x: must(NewDecFromString("1234567")), | ||
| exp: "1.234567E+6", | ||
| }, | ||
| "12345678": { | ||
| x: must(NewDecFromString("12345678")), | ||
| exp: "12.345678E+6", | ||
| }, | ||
| "123456789": { | ||
| x: must(NewDecFromString("123456789")), | ||
| exp: "123.456789E+6", | ||
| }, | ||
| "1234567890": { | ||
| x: must(NewDecFromString("1234567890")), | ||
| exp: "123.456789E+7", | ||
| }, | ||
| "12345678900": { | ||
| x: must(NewDecFromString("12345678900")), | ||
| exp: "123.456789E+8", | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add boundary test cases for scientific notation transition.
The test cases cover various integer values but could benefit from explicit boundary test cases at the scientific notation transition points.
Add these test cases:
"boundary/decimal_to_scientific_positive": {
x: must(NewDecFromString("999999")), // Last number in decimal
exp: "999999",
},
"boundary/decimal_to_scientific_positive_next": {
x: must(NewDecFromString("1000000")), // First number in scientific
exp: "1E+6",
},
"boundary/decimal_to_scientific_negative": {
x: must(NewDecFromString("0.000001")), // Last number in decimal
exp: "0.000001",
},
"boundary/decimal_to_scientific_negative_next": {
x: must(NewDecFromString("0.0000001")), // First number in scientific
exp: "1E-7",
},
math/dec_test.go
Outdated
| assert.Equal(t, spec.exp, string(marshaled)) | ||
| // and backwards | ||
| unmarshalledDec := new(Dec) | ||
| require.NoError(t, unmarshalledDec.Unmarshal(marshaled)) | ||
| assert.Equal(t, spec.x.String(), unmarshalledDec.String()) | ||
| assert.True(t, spec.x.Equal(*unmarshalledDec)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add unmarshal error test cases.
The unmarshaling tests only verify successful cases. Consider adding test cases for invalid input.
Add these test cases:
"unmarshal/invalid_format": {
x: NewDecFromInt64(0),
exp: "invalid",
expErr: ErrInvalidDec,
},
"unmarshal/empty_input": {
x: NewDecFromInt64(0),
exp: "",
expErr: ErrInvalidDec,
},
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
math/CHANGELOG.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
math/CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
🪛 LanguageTool
math/CHANGELOG.md
[uncategorized] ~39-~39: This expression is usually spelled with a hyphen.
Context: ...-sdk/issues/11783) feat(math): Upstream GDA based decimal type ## [math/v1.4.0](https:/...
(BASED_HYPHEN)
| * [#11783](https://github.com/cosmos/cosmos-sdk/issues/11783) feat(math): Upstream GDA based decimal type | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the changelog entry with more details.
The current entry could be more descriptive. Consider expanding it to include:
- Details about the marshaling/unmarshaling feature
- The numerical formatting thresholds
- Example behavior
- Fix the hyphenation of "GDA-based"
-* [#11783](https://github.com/cosmos/cosmos-sdk/issues/11783) feat(math): Upstream GDA based decimal type
+* [#22525](https://github.com/cosmos/cosmos-sdk/issues/22525) feat(math): Implement custom hybrid marshal/unmarshal for GDA-based decimal type. Numbers are displayed in decimal format until they reach exponents of +/-6, then switch to scientific notation (e.g., 1234567 stays decimal, while 0.0000001 becomes 1E-7).Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~39-~39: This expression is usually spelled with a hyphen.
Context: ...-sdk/issues/11783) feat(math): Upstream GDA based decimal type ## [math/v1.4.0](https:/...
(BASED_HYPHEN)
julienrbrt
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
math/dec_examples_test.go (1)
Line range hint
1-311: Add examples for Marshal/Unmarshal functionalityThe example tests are missing coverage for the new marshaling functionality. Consider adding an
ExampleDec_MarshalandExampleDec_Unmarshalto demonstrate:
- Decimal to scientific notation transition at +/-6 exponents
- Handling of various number formats (regular decimals, scientific notation)
- Error cases for invalid inputs
Here's a suggested example test:
func ExampleDec_Marshal() { examples := []string{ "1.23", // Regular decimal "1000000", // At +6 boundary "0.000001", // At -6 boundary "1.23E+7", // Beyond +6 "1.23E-7", // Beyond -6 } for _, s := range examples { d, err := NewDecFromString(s) if err != nil { panic(err) } bytes, err := d.Marshal() if err != nil { panic(err) } fmt.Printf("%s -> %s\n", s, string(bytes)) } // Output: // 1.23 -> 1.23 // 1000000 -> 1000000 // 0.000001 -> 0.000001 // 1.23E+7 -> 1.23E+7 // 1.23E-7 -> 1.23E-7 }math/dec.go (1)
411-432: Consider enhancing error message specificityWhile the implementation is correct, the error message could be more specific about the reduction failure.
Consider this improvement:
- return nil, ErrInvalidDec.Wrap(err.Error()) + return nil, ErrInvalidDec.Wrap(fmt.Sprintf("failed to reduce decimal: %v", err))math/dec_test.go (1)
Line range hint
1320-1452: LGTM! Comprehensive test coverage for marshal/unmarshal functionality.The test cases thoroughly cover:
- Zero and negative zero handling
- Decimal place variations
- Scientific notation transitions
- Edge cases and error conditions
Consider grouping related test cases together with descriptive comments to improve readability. For example:
// Basic zero handling "Zero value": {...}, "-0": {...}, // Decimal places (0.1 to 0.000001) "1 decimal place": {...}, "2 decimal places": {...}, // Scientific notation transition "6 decimal places": {...}, "7 decimal places": {...},
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
math/dec.go(5 hunks)math/dec_examples_test.go(1 hunks)math/dec_test.go(6 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
math/dec.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
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_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"
🔇 Additional comments (7)
math/dec_examples_test.go (1)
311-311: LGTM: Output format aligns with the new marshaling rules
The modified output "0E-35" correctly follows the scientific notation format for numbers with exponents beyond +/-6, which is consistent with the PR's marshaling implementation.
math/dec.go (4)
303-303: LGTM: Appropriate use of Text('f') for decimal notation
The change from String() to Text('f') ensures consistent decimal notation without exponents, which is more appropriate for BigInt conversion.
338-338: LGTM: Consistent with hybrid notation requirements
The implementation correctly uses the new fmtE helper function to provide hybrid notation based on the exponent value.
472-485: LGTM: Robust implementation with proper validation
The implementation correctly handles parsing and validation, ensuring only finite decimals are accepted.
506-506: LGTM: Consistent with other serialization methods
The change appropriately uses the new fmtE helper function, maintaining consistency with other serialization methods.
math/dec_test.go (2)
154-158: LGTM! Expected values updated to match new decimal format.
The updated expected values for max/min int64 correctly reflect the new hybrid decimal format, maintaining 6 decimal places of precision before switching to scientific notation.
1249-1259: LGTM! Comprehensive test coverage for BigInt conversion.
The test implementation properly covers:
- Successful conversion cases
- Error handling for non-integral values
- Proper assertion of results and errors
| // fmtE formats a decimal number into a byte slice in scientific notation or fixed-point notation depending on the exponent. | ||
| // If the adjusted exponent is between -6 and 6 inclusive, it uses fixed-point notation, otherwise it uses scientific notation. | ||
| func fmtE(d apd.Decimal, fmt byte) []byte { | ||
| var scratch, dest [16]byte | ||
| buf := dest[:0] | ||
| digits := d.Coeff.Append(scratch[:0], 10) | ||
| totalDigits := int64(len(digits)) | ||
| adj := int64(d.Exponent) + totalDigits - 1 | ||
| if adj > -6 && adj < 6 { | ||
| return []byte(d.Text('f')) | ||
| } | ||
| switch { | ||
| case totalDigits > 5: | ||
| beforeComma := digits[0 : totalDigits-6] | ||
| adj -= int64(len(beforeComma) - 1) | ||
| buf = append(buf, beforeComma...) | ||
| buf = append(buf, '.') | ||
| buf = append(buf, digits[totalDigits-6:]...) | ||
| case totalDigits > 1: | ||
| buf = append(buf, digits[0]) | ||
| buf = append(buf, '.') | ||
| buf = append(buf, digits[1:]...) | ||
| default: | ||
| buf = append(buf, digits[0:]...) | ||
| } | ||
|
|
||
| buf = append(buf, fmt) | ||
| var ch byte | ||
| if adj < 0 { | ||
| ch = '-' | ||
| adj = -adj | ||
| } else { | ||
| ch = '+' | ||
| } | ||
| buf = append(buf, ch) | ||
| return strconv.AppendInt(buf, adj, 10) |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Based on the search results, I can now provide a final response:
Buffer size is sufficient, but input validation is needed
-
The fixed buffer size of 16 bytes is actually sufficient because:
- The function handles scientific notation for large numbers by using the exponent format
- Test cases show numbers with up to 35 decimal places being handled correctly (e.g., "0.12345678901234567890123456789012345")
-
However, the lack of input validation for non-finite forms (NaN, Infinity) should be addressed as suggested.
🔗 Analysis chain
Verify buffer sizes and add input validation
Two potential issues to consider:
- The fixed buffer sizes (
[16]byte) might be insufficient for large numbers - The function lacks validation for the input decimal's form (finite, infinite, NaN)
Consider these improvements:
func fmtE(d apd.Decimal, fmt byte) []byte {
+ if d.Form != apd.Finite {
+ return []byte("NaN")
+ }
var scratch, dest [16]byte🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for large number handling in tests
rg -A 2 "NewDecFromString.*9{15,}"
Length of output: 36
Script:
#!/bin/bash
# Search for tests related to decimal formatting
rg -l "fmtE.*test"
# Search for large number test cases in decimal-related tests
rg "Test.*Dec.*" -A 5
# Look for any existing decimal formatting tests
ast-grep --pattern 'func Test$_($_) {
$$$
fmtE($$$)
$$$
}'
Length of output: 45683
Script:
#!/bin/bash
# Search for large decimal test cases
rg -A 3 "NewDecFromString.*[0-9]{15,}" math/
# Look for any decimal formatting tests
rg -A 5 "TestFmt.*Dec" math/
# Check if there are any existing panic tests for decimal formatting
rg -A 3 "panic.*fmtE" math/
Length of output: 5618
|
I would keep this open for some more days to give others a chance to 👀 |
|
cc @testinginprod @facundomedica @tac0turtle |
Description
Closes: #22525
Implement custom hybrid marshal/ unmarshalling for Dec type.
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
Summary by CodeRabbit
New Features
Bug Fixes
Tests