LaconicNetwork/cosmos-sdk
7
0
Fork 0
Code Issues Pull Requests Packages Projects Releases 2 Wiki Activity

fix(math): Dec marshal shouldn't have side effects (#15506)

Browse Source 
## 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](4a6a1e3cb8/x/auth/tx/builder.go (L208)) messages are coerced to an any type in [NewAnyWithValue](4a6a1e3cb8/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](88909d6f2b/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)
This commit is contained in:
Matt Kocubinski 2023-03-22 14:48:36 -05:00 committed by GitHub
parent d53b29b1d1
commit 4db99fc1a5
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 35 additions and 6 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
math/CHANGELOG.md
View File

@ -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

14
math/dec.go
View File

@ -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
}

20
tests/integration/aminojson/aminojson_test.go
View File

@ -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: