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

Jump to bottom

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+177 −63
Open

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

Show file tree
Hide file tree
Changes from all 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
82 changes: 75 additions & 7 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 @@ -299,7 +300,7 @@ func (x Dec) Int64() (int64, error) {
// fit precisely into an *big.Int.
func (x Dec) BigInt() (*big.Int, error) {
y, _ := x.Reduce()
z, ok := new(big.Int).SetString(y.String(), 10)
z, ok := new(big.Int).SetString(y.Text('f'), 10)
if !ok {
return nil, ErrNonIntegral
}
Expand Down Expand Up @@ -334,7 +335,7 @@ func (x Dec) SdkIntTrim() (Int, error) {

// String formatted in decimal notation: '-ddddd.dddd', no exponent
func (x Dec) String() string {
return x.dec.Text('f')
return string(fmtE(x.dec, 'E'))
}

// Text converts the floating-point number x to a string according
Expand Down Expand Up @@ -407,14 +408,81 @@ 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
}

// 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)
Comment on lines +434 to +469
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 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

  1. 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")

  2. 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:

  1. The fixed buffer sizes ([16]byte) might be insufficient for large numbers
  2. 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

}

// 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 All @@ -435,7 +503,7 @@ func (x Dec) Size() int {

// MarshalJSON serializes the Dec struct into a JSON-encoded byte slice using scientific notation.
func (x Dec) MarshalJSON() ([]byte, error) {
return json.Marshal(x.dec.Text('E'))
return json.Marshal(fmtE(x.dec, 'E'))
}

// UnmarshalJSON implements the json.Unmarshaler interface for the Dec type, converting JSON strings to Dec objects.
Expand Down
2 changes: 1 addition & 1 deletion math/dec_examples_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,7 @@ func ExampleDec_MulExact() {
// 2.50
// exponent out of range: invalid decimal
// unexpected rounding
// 0.00000000000000000000000000000000000
// 0E-35
// 0
}

Expand Down
153 changes: 98 additions & 55 deletions math/dec_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package math
import (
"fmt"
"math"
"strconv"
"strings"
"testing"

Expand Down Expand Up @@ -152,11 +151,11 @@ func TestNewDecFromInt64(t *testing.T) {
},
"max value": {
src: math.MaxInt64,
exp: strconv.Itoa(math.MaxInt64),
exp: "9223372036854.775807E+6",
},
"min value": {
src: math.MinInt64,
exp: strconv.Itoa(math.MinInt64),
exp: "9223372036854.775808E+6",
},
}
for name, spec := range specs {
Expand Down Expand Up @@ -1247,15 +1246,17 @@ func TestToBigInt(t *testing.T) {
{"12345.6", "", ErrNonIntegral},
}
for idx, tc := range tcs {
a, err := NewDecFromString(tc.intStr)
require.NoError(t, err)
b, err := a.BigInt()
if tc.isError == nil {
require.NoError(t, err, "test_%d", idx)
require.Equal(t, tc.out, b.String(), "test_%d", idx)
} else {
require.ErrorIs(t, err, tc.isError, "test_%d", idx)
}
t.Run(fmt.Sprintf("%d", idx), func(t *testing.T) {
a, err := NewDecFromString(tc.intStr)
require.NoError(t, err)
b, err := a.BigInt()
if tc.isError == nil {
require.NoError(t, err, "test_%d", idx)
require.Equal(t, tc.out, b.String(), "test_%d", idx)
} else {
require.ErrorIs(t, err, tc.isError, "test_%d", idx)
}
})
}
}

Expand Down Expand Up @@ -1309,55 +1310,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 +1351,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 +1428,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 +1444,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.exp, unmarshalledDec.String())
assert.True(t, spec.x.Equal(*unmarshalledDec))
})
}
}
Loading