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

fix: Format decimals correctly for signmode textual #15129

Merged
merged 12 commits into from
Feb 27, 2023
Merged

fix: Format decimals correctly for signmode textual #15129

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
c01e67d
fix: Format decimals correctly
facundomedica Feb 22, 2023
21b12cf
remove test print
facundomedica Feb 22, 2023
619246f
use math instead of string
facundomedica Feb 23, 2023
507c3a1
Update x/tx/textual/dec.go
facundomedica Feb 23, 2023
ac6ebe3
fix lint
facundomedica Feb 23, 2023
68fce11
Merge branch 'main' into facu/textual-dec-workaround
facundomedica Feb 23, 2023
57fcc1b
lint
facundomedica Feb 23, 2023
3ee7a84
Merge branch 'main' into facu/textual-dec-workaround
facundomedica Feb 23, 2023
d92ce05
Merge branch 'main' into facu/textual-dec-workaround
facundomedica Feb 24, 2023
63f2343
Merge branch 'main' into facu/textual-dec-workaround
facundomedica Feb 24, 2023
0b230d5
Merge branch 'main' into facu/textual-dec-workaround
facundomedica Feb 26, 2023
d880661
Merge branch 'main' into facu/textual-dec-workaround
facundomedica Feb 27, 2023
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
26 changes: 25 additions & 1 deletion x/tx/textual/dec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,31 @@ type decValueRenderer struct{}
var _ ValueRenderer = decValueRenderer{}

func (vr decValueRenderer) Format(_ context.Context, v protoreflect.Value) ([]Screen, error) {
formatted, err := math.FormatDec(v.String())
decStr := v.String()

// If the decimal doesn't contain a point, we assume it's a badly formatted
// value. So we assume it's a decimal with 18 decimal places and add the decimal
// in the right place.
if !strings.Contains(decStr, ".") {
isNeg := false
if strings.HasPrefix(decStr, "-") {
decStr = strings.TrimPrefix(decStr, "-")
isNeg = true
}

if len(decStr) < 19 {
decStr = "0." + strings.Repeat("0", 18-len(decStr)) + decStr
} else {
whole, decimal := decStr[:len(decStr)-18], decStr[18:]
decStr = whole + "." + decimal
}

if isNeg {
decStr = "-" + decStr
}
}
facundomedica marked this conversation as resolved.
Show resolved Hide resolved

formatted, err := math.FormatDec(decStr)
if err != nil {
return nil, err
}
Expand Down
35 changes: 34 additions & 1 deletion x/tx/textual/dec_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,17 @@
package textual_test

import (
"context"
"encoding/json"
"math/big"
"os"
"strings"
"testing"

"github.com/stretchr/testify/require"
"google.golang.org/protobuf/reflect/protoreflect"

"cosmossdk.io/math"
"cosmossdk.io/x/tx/textual"
)

Expand All @@ -27,7 +31,36 @@ func TestDecJsonTestcases(t *testing.T) {
r, err := textual.GetFieldValueRenderer(fieldDescriptorFromName("SDKDEC"))
require.NoError(t, err)

checkNumberTest(t, r, protoreflect.ValueOf(tc[0]), tc[1])
checkDecTest(t, r, protoreflect.ValueOf(tc[0]), tc[1])
})
}
}

func checkDecTest(t *testing.T, r textual.ValueRenderer, pv protoreflect.Value, expected string) {
screens, err := r.Format(context.Background(), pv)
require.NoError(t, err)
require.Len(t, screens, 1)
require.Zero(t, screens[0].Indent)
require.False(t, screens[0].Expert)

require.Equal(t, expected, screens[0].Content)

// Round trip.
value, err := r.Parse(context.Background(), screens)
require.NoError(t, err)

v1, err := math.LegacyNewDecFromStr(value.String())
require.NoError(t, err)

decStr := pv.String()
if !strings.Contains(decStr, ".") {
n, ok := new(big.Int).SetString(decStr, 10)
require.True(t, ok)
decStr = math.LegacyNewDecFromBigIntWithPrec(n, 18).String()
}

v, err := math.LegacyNewDecFromStr(decStr)
require.NoError(t, err)

require.Truef(t, v.Equal(v1), "%s != %s", v, v1)
}
6 changes: 3 additions & 3 deletions x/tx/textual/int_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,8 @@ func checkNumberTest(t *testing.T, r textual.ValueRenderer, pv protoreflect.Valu
screens, err := r.Format(context.Background(), pv)
require.NoError(t, err)
require.Len(t, screens, 1)
require.Equal(t, 0, screens[0].Indent)
require.Equal(t, false, screens[0].Expert)
require.Zero(t, screens[0].Indent)
require.False(t, screens[0].Expert)

require.Equal(t, expected, screens[0].Content)

Expand All @@ -78,5 +78,5 @@ func checkNumberTest(t *testing.T, r textual.ValueRenderer, pv protoreflect.Valu
v1, err := math.LegacyNewDecFromStr(value.String())
require.NoError(t, err)

require.True(t, v.Equal(v1))
require.Truef(t, v.Equal(v1), "%s != %s", v, v1)
}
68 changes: 60 additions & 8 deletions x/tx/textual/internal/testdata/decimals.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
[
["0", "0"],
["1", "1"],
["12", "12"],
["123", "123"],
["1234", "1'234"],
["0.0", "0"],
["1.0", "1"],
Copy link
Contributor

Choose a reason for hiding this comment

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

I remember when we introduced gov v1, we dropped the customtype to sdk.Dec, see here or tally. This means in state, the string is stored as a "normal" decimal.

What we need to make sure is that the string does always have a .. For example, we should test this manually with Ledger, using a weighted vote of 1, and make sure it shows 1 and not 1e-18.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will test that 👌

Copy link
Member Author

Choose a reason for hiding this comment

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

...Options (1/1)xWeightedVoteOption object�fOptionoVOTE_OPTION_YES�fWeighta1�nEnd of Options�nEnd of Message�iGas limitg200'000...

WhatsApp Image 2023-02-23 at 3 17 02 PM

Looks fine 👌 Also for bank send it looks good.

["12.0", "12"],
["123.0", "123"],
["1234.0", "1'234"],
["0.1", "0.1"],
["0.01", "0.01"],
["0.001", "0.001"],
Expand Down Expand Up @@ -41,7 +41,59 @@
["0.000000000000000010", "0.00000000000000001"],
["0.000000000000000001", "0.000000000000000001"],
["-10.0", "-10"],
["-10000", "-10'000"],
["-9999", "-9'999"],
["-999999999999", "-999'999'999'999"]
["-10000.0", "-10'000"],
["-9999.0", "-9'999"],
["-999999999999.0", "-999'999'999'999"],
["1000000000000000000000000", "1'000'000"],
["1000000000000000000000000", "1'000'000"],
["100000000000000000000000", "100'000"],
["10000000000000000000000", "10'000"],
["1000000000000000000000", "1'000"],
["100000000000000000000", "100"],
["10000000000000000000", "10"],
["1000000000000000000", "1"],
["100000000000000000", "0.1"],
["10000000000000000", "0.01"],
["1000000000000000", "0.001"],
["100000000000000", "0.0001"],
["10000000000000", "0.00001"],
["1000000000000", "0.000001"],
["100000000000", "0.0000001"],
["10000000000", "0.00000001"],
["1000000000", "0.000000001"],
["100000000", "0.0000000001"],
["10000000", "0.00000000001"],
["1000000", "0.000000000001"],
["100000", "0.0000000000001"],
["10000", "0.00000000000001"],
["1000", "0.000000000000001"],
["100", "0.0000000000000001"],
["10", "0.00000000000000001"],
["1", "0.000000000000000001"],
["-1000000000000000000000000", "-1'000'000"],
["-1000000000000000000000000", "-1'000'000"],
["-100000000000000000000000", "-100'000"],
["-10000000000000000000000", "-10'000"],
["-1000000000000000000000", "-1'000"],
["-100000000000000000000", "-100"],
["-10000000000000000000", "-10"],
["-1000000000000000000", "-1"],
["-100000000000000000", "-0.1"],
["-10000000000000000", "-0.01"],
["-1000000000000000", "-0.001"],
["-100000000000000", "-0.0001"],
["-10000000000000", "-0.00001"],
["-1000000000000", "-0.000001"],
["-100000000000", "-0.0000001"],
["-10000000000", "-0.00000001"],
["-1000000000", "-0.000000001"],
["-100000000", "-0.0000000001"],
["-10000000", "-0.00000000001"],
["-1000000", "-0.000000000001"],
["-100000", "-0.0000000000001"],
["-10000", "-0.00000000000001"],
["-1000", "-0.000000000000001"],
["-100", "-0.0000000000000001"],
["-10", "-0.00000000000000001"],
["-1", "-0.000000000000000001"]
]