Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(math): Implement custom hybrid un-/marshal model #22529

Merged
merged 3 commits into from
Dec 5, 2024
Merged

feat(math): Implement custom hybrid un-/marshal model #22529

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ac56675
Implement custom hybrid un-/marshal model
alpe Nov 14, 2024
a0a7e30
Changelog
alpe Nov 25, 2024
c951c71
Use hybrid format in string and json
alpe Nov 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions math/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ Ref: https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.j

## [Unreleased]

* [#11783](https://github.com/cosmos/cosmos-sdk/issues/11783) feat(math): Upstream GDA based decimal type

Comment on lines +39 to +40
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance the changelog entry with more details.

The current entry could be more descriptive. Consider expanding it to include:

  1. Details about the marshaling/unmarshaling feature
  2. The numerical formatting thresholds
  3. Example behavior
  4. 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)


## [math/v1.4.0](https://github.com/cosmos/cosmos-sdk/releases/tag/math/v1.4.0) - 2024-01-20

### Features
Expand Down
83 changes: 79 additions & 4 deletions math/dec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"encoding/json"
stderrors "errors"
"math/big"
"strconv"

"github.com/cockroachdb/apd/v3"

Expand Down Expand Up @@ -407,14 +408,88 @@ func (x Dec) Reduce() (Dec, int) {
return y, n
}

// Marshal serializes the decimal value into a byte slice in text format.
// This method represents the decimal in a portable and compact hybrid notation.
// Based on the exponent value, the number is formatted into decimal: -ddddd.ddddd, no exponent
// or scientific notation: -d.ddddE±dd
//
// For example, the following transformations are made:
// - 0 -> 0
// - 123 -> 123
// - 10000 -> 10000
// - -0.001 -> -0.001
// - -0.000000001 -> -1E-9
//
// Returns:
// - A byte slice of the decimal in text format.
// - An error if the decimal cannot be reduced or marshaled properly.
func (x Dec) Marshal() ([]byte, error) {
// implemented in a new PR. See: https://github.com/cosmos/cosmos-sdk/issues/22525
panic("not implemented")
var d apd.Decimal
if _, _, err := dec128Context.Reduce(&d, &x.dec); err != nil {
return nil, ErrInvalidDec.Wrap(err.Error())
}
return fmtE(d, 'E'), nil
}

// custom formatter
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)
}

// isEmptyExp checks if the adjusted exponent of the given decimal is zero.
func isEmptyExp(d apd.Decimal) bool {
var scratch [16]byte
digits := d.Coeff.Append(scratch[:0], 10)
adj := int64(d.Exponent) + int64(len(digits)) - 1
return adj == 0
}

// Unmarshal parses a byte slice containing a text-formatted decimal and stores the result in the receiver.
// It returns an error if the byte slice does not represent a valid decimal.
func (x *Dec) Unmarshal(data []byte) error {
// implemented in a new PR. See: https://github.com/cosmos/cosmos-sdk/issues/22525
panic("not implemented")
result, err := NewDecFromString(string(data))
if err != nil {
return ErrInvalidDec.Wrap(err.Error())
}

if result.dec.Form != apd.Finite {
return ErrInvalidDec.Wrap("unknown decimal form")
}

x.dec = result.dec
return nil
}

// MarshalTo encodes the receiver into the provided byte slice and returns the number of bytes written and any error encountered.
Expand Down
128 changes: 85 additions & 43 deletions math/dec_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1309,55 +1309,38 @@ func must[T any](r T, err error) T {
}

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",
},
"Trailing zeros": {
x: NewDecFromInt64(123456000),
exp: "1.23456E+8",
},
"Zero value": {
x: NewDecFromInt64(0),
exp: "0E+0",
exp: "0",
},
"-0": {
x: NewDecFromInt64(-0),
exp: "0E+0",
},
"Decimal value": {
x: must(NewDecFromString("1.30000")),
exp: "1.3E+0",
},
"Positive value": {
x: NewDecFromInt64(10),
exp: "1E+1",
exp: "0",
},
"negative 10": {
x: NewDecFromInt64(-10),
exp: "-1E+1",
"1 decimal place": {
x: must(NewDecFromString("0.1")),
exp: "0.1",
},
"9 with trailing zeros": {
x: must(NewDecFromString("9." + strings.Repeat("0", 34))),
exp: "9E+0",
"2 decimal places": {
x: must(NewDecFromString("0.01")),
exp: "0.01",
},
"negative 1 with negative exponent zeros": {
x: must(NewDecFromString("-1.000001")),
exp: "-1.000001E+0",
"3 decimal places": {
x: must(NewDecFromString("0.001")),
exp: "0.001",
},
"negative 1 with trailing zeros": {
x: must(NewDecFromString("-1." + strings.Repeat("0", 34))),
exp: "-1E+0",
"4 decimal places": {
x: must(NewDecFromString("0.0001")),
exp: "0.0001",
},
"5 decimal places": {
x: must(NewDecFromString("0.00001")),
exp: "1E-5",
exp: "0.00001",
},
"6 decimal places": {
x: must(NewDecFromString("0.000001")),
Expand All @@ -1367,17 +1350,73 @@ func TestMarshalUnmarshal(t *testing.T) {
x: must(NewDecFromString("0.0000001")),
exp: "1E-7",
},
"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",
},
Comment on lines +1354 to +1397
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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",
},

"negative 1 with negative exponent": {
x: must(NewDecFromString("-1.000001")),
exp: "-1.000001",
},
"-1.0000001 - negative 1 with negative exponent": {
x: must(NewDecFromString("-1.0000001")),
exp: "-1.0000001",
},
"3 decimal places before the comma": {
x: must(NewDecFromString("100")),
exp: "100",
},
"4 decimal places before the comma": {
x: must(NewDecFromString("1000")),
exp: "1E+3",
exp: "1000",
},
"5 decimal places before the comma": {
x: must(NewDecFromString("10000")),
exp: "1E+4",
exp: "10000",
},
"6 decimal places before the comma": {
x: must(NewDecFromString("100000")),
exp: "1E+5",
exp: "100000",
},
"7 decimal places before the comma": {
x: must(NewDecFromString("1000000")),
Expand All @@ -1388,12 +1427,12 @@ func TestMarshalUnmarshal(t *testing.T) {
exp: "1E+100000",
},
"1.1e100000": {
x: NewDecWithExp(11, 100_000),
expErr: ErrInvalidDec,
x: must(NewDecFromString("1.1e100000")),
exp: "1.1E+100000",
},
"1.e100000": {
x: NewDecWithExp(1, 100_000),
exp: "1E+100000",
"1e100001": {
x: NewDecWithExp(1, 100_001),
expErr: ErrInvalidDec,
},
}
for name, spec := range specs {
Expand All @@ -1404,9 +1443,12 @@ func TestMarshalUnmarshal(t *testing.T) {
return
}
require.NoError(t, gotErr)
unmarshalled := new(Dec)
require.NoError(t, unmarshalled.Unmarshal(marshaled))
assert.Equal(t, spec.exp, unmarshalled.dec.Text('E'))
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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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,
},

})
}
}
Loading