From 4db99fc1a56919efc87ac73f2db662b963de8be9 Mon Sep 17 00:00:00 2001 From: Matt Kocubinski Date: Wed, 22 Mar 2023 14:48:36 -0500 Subject: [PATCH] fix(math): Dec marshal shouldn't have side effects (#15506) ## Description Ref: https://github.com/cosmos/cosmos-sdk/issues/15170 Included in: #15515 Required to resolve a discrepancy between the sign mode being implemented in #15170 and the existing legacy one. When calling tx builder [SetMsgs](https://github.com/cosmos/cosmos-sdk/blob/4a6a1e3cb8de459891cb0495052589673d14ef51/x/auth/tx/builder.go#L208) messages are coerced to an any type in [NewAnyWithValue](https://github.com/cosmos/cosmos-sdk/blob/4a6a1e3cb8de459891cb0495052589673d14ef51/codec/types/any.go#L61). This marshals a message as proto bytes. If a message has cosmos.Dec typed fields, and they are set to nil, the will be set to 0 during seriazliation in the [Dec.Marshal](https://github.com/cosmos/cosmos-sdk/blob/88909d6f2b86dee6969a76e1e0e3c106fe8fd20c/math/dec.go#L801) implementation. This side effect *changes* the output of a JSON marshal before and after the call to Dec.Marshal, as shown below. ```go rates := &stakingtypes.CommissionRates{} MarshalJson(rates) // {"rate":"0","max_rate":"0","max_change_rate":"0"} MarshalProto(rates) MarshalJson(rates) //{"rate":"0.000000000000000000","max_rate":"0.000000000000000000","max_change_rate":"0.000000000000000000"} ``` An integration is test is also committed asserting the (now fixed) behavior above. Its asserts should be changed once a release of math is made and `/tests/go.mod` is updated. --- ### 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... * [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title * [ ] added `!` to the type prefix if API or client breaking change * [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting)) * [ ] provided a link to the relevant issue or specification * [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/docs/building-modules) * [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing) * [ ] added a changelog entry to `CHANGELOG.md` * [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) * [ ] updated the relevant documentation or specification * [ ] reviewed "Files changed" and left comments if necessary * [ ] confirmed all CI checks have passed ### Reviewers 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.* I have... * [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title * [ ] confirmed `!` in the type prefix if API or client breaking change * [ ] confirmed all author checklist items have been addressed * [ ] reviewed state machine logic * [ ] reviewed API design and naming * [ ] reviewed documentation is accurate * [ ] reviewed tests and test coverage * [ ] manually tested (if applicable) --- math/CHANGELOG.md | 7 +++++++ math/dec.go | 14 +++++++------ tests/integration/aminojson/aminojson_test.go | 20 +++++++++++++++++++ 3 files changed, 35 insertions(+), 6 deletions(-) diff --git a/math/CHANGELOG.md b/math/CHANGELOG.md index 43e36e120b..3d97302b88 100644 --- a/math/CHANGELOG.md +++ b/math/CHANGELOG.md @@ -34,6 +34,12 @@ Ref: https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.j # Changelog +## [Unreleased] + +### Bug Fixes + +* [#15506](https://github.com/cosmos/cosmos-sdk/issues/16605) fix: Dec marshal shouldn't have side effects + ## [math/v1.0.0-rc.0](https://github.com/cosmos/cosmos-sdk/releases/tag/math/v1.0.0-rc.0) - 2023-03-13 ### Features @@ -43,6 +49,7 @@ Ref: https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.j ### Bug Fixes * [#14922](https://github.com/cosmos/cosmos-sdk/issues/14922) check for negative precision +* [#15506](https://github.com/cosmos/cosmos-sdk/issues/16605) fix: Dec marshal shouldn't have side effects ### Testing diff --git a/math/dec.go b/math/dec.go index 844b3039af..8875dfdd70 100644 --- a/math/dec.go +++ b/math/dec.go @@ -799,19 +799,21 @@ func (d LegacyDec) MarshalYAML() (interface{}, error) { // Marshal implements the gogo proto custom type interface. func (d LegacyDec) Marshal() ([]byte, error) { - if d.i == nil { - d.i = new(big.Int) + i := d.i + if i == nil { + i = new(big.Int) } - return d.i.MarshalText() + return i.MarshalText() } // MarshalTo implements the gogo proto custom type interface. func (d *LegacyDec) MarshalTo(data []byte) (n int, err error) { - if d.i == nil { - d.i = new(big.Int) + i := d.i + if i == nil { + i = new(big.Int) } - if d.i.Cmp(zeroInt) == 0 { + if i.Cmp(zeroInt) == 0 { copy(data, []byte{0x30}) return 1, nil } diff --git a/tests/integration/aminojson/aminojson_test.go b/tests/integration/aminojson/aminojson_test.go index ad8168a0fe..0a2abe15f3 100644 --- a/tests/integration/aminojson/aminojson_test.go +++ b/tests/integration/aminojson/aminojson_test.go @@ -665,6 +665,26 @@ func TestSendAuthorization(t *testing.T) { require.Equal(t, string(legacyAminoJson), string(aminoJson)) } +func TestDecimalMutation(t *testing.T) { + encCfg := testutil.MakeTestEncodingConfig(staking.AppModuleBasic{}) + rates := &stakingtypes.CommissionRates{} + rateBz, _ := encCfg.Amino.MarshalJSON(rates) + require.Equal(t, `{"rate":"0","max_rate":"0","max_change_rate":"0"}`, string(rateBz)) + _, err := gogoproto.Marshal(rates) + require.NoError(t, err) + rateBz, _ = encCfg.Amino.MarshalJSON(rates) + + // these assertions show behavior prior to the merge of https://github.com/cosmos/cosmos-sdk/pull/15506 + // and should be updated to reflect the new behavior once a release of math is made and updated in ./tests/go.mod + require.NotEqual(t, `{"rate":"0","max_rate":"0","max_change_rate":"0"}`, string(rateBz)) + require.Equal(t, + `{"rate":"0.000000000000000000","max_rate":"0.000000000000000000","max_change_rate":"0.000000000000000000"}`, + string(rateBz)) + + // new behavior + //require.Equal(t, `{"rate":"0","max_rate":"0","max_change_rate":"0"}`, string(rateBz)) +} + func postFixPulsarMessage(msg proto.Message) { switch m := msg.(type) { case *authapi.ModuleAccount: