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.

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

types: Dec.MarshalJSON now marshals as a normal decimal string #2506

Merged
merged 6 commits into from
Oct 21, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
2 changes: 2 additions & 0 deletions PENDING.md
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,8 @@ BREAKING CHANGES
* [x/stake] \#2412 Added an unbonding validator queue to EndBlock to automatically update validator.Status when finished Unbonding
* [x/stake] \#2500 Block conflicting redelegations until we add an index
* [x/params] Global Paramstore refactored
* [types] \#2506 sdk.Dec MarshalJSON now marshals as a normal Decimal, with 10 digits of decimal precision


* Tendermint
* Update tendermint version from v0.23.0 to v0.25.0, notable changes
Expand Down
33 changes: 29 additions & 4 deletions types/decimal.go
Original file line number Diff line number Diff line change
Expand Up @@ -407,17 +407,36 @@ func (d *Dec) UnmarshalAmino(text string) (err error) {
return nil
}

// MarshalJSON defines custom encoding scheme
// MarshalJSON marshals the decimal
func (d Dec) MarshalJSON() ([]byte, error) {
if d.Int == nil {
return nilJSON, nil
}

bz, err := d.Int.MarshalText()
if err != nil {
return nil, err
}
return json.Marshal(string(bz))
var bzWDec []byte
// TODO: Remove trailing zeros
// case 1, pure decimal
if len(bz) <= 10 {
bzWDec = make([]byte, 12)
// 0. prefix
bzWDec[0] = byte('0')
bzWDec[1] = byte('.')
// set relevant digits to 0
for i := 0; i < 10-len(bz); i++ {
bzWDec[i+2] = byte('0')
}
// set last few digits
copy(bzWDec[2+(10-len(bz)):], bz)
} else {
bzWDec = make([]byte, len(bz)+1)
copy(bzWDec, bz[:len(bz)-10])
bzWDec[len(bz)-10] = byte('.')
copy(bzWDec[len(bz)-9:], bz[len(bz)-10:])
}
return json.Marshal(string(bzWDec))
Copy link
Contributor

Choose a reason for hiding this comment

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

All this logic already exists using Decimal.String() - why are we not reusing it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh woops, didn't see that lol

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I think this implementation is more efficient (albeit harder to grok). I'd actually propose making a follow-up issue to compare efficiencies via benchmarks and converge on using one or the other.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yo - If I had to guess I'd say you're correct why don't we:

  • Replace the .String() code with your new updated logic
  • Use .String() in JSON Marshalling here
  • copy the old code into a #postlaunch issue for later benchmarking

:)

how does that sound?

}

// UnmarshalJSON defines custom decoding scheme
Expand All @@ -431,7 +450,13 @@ func (d *Dec) UnmarshalJSON(bz []byte) error {
if err != nil {
return err
}
return d.Int.UnmarshalText([]byte(text))
// TODO: Reuse dec allocation
newDec, err := NewDecFromStr(text)
if err != nil {
return err
}
d.Int = newDec.Int
return nil
}

//___________________________________________________________________________________
Expand Down
40 changes: 40 additions & 0 deletions types/decimal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import (
"math/big"
"testing"

"github.com/stretchr/testify/assert"

"github.com/cosmos/cosmos-sdk/codec"
"github.com/stretchr/testify/require"
)
Expand Down Expand Up @@ -248,6 +250,44 @@ func TestToLeftPadded(t *testing.T) {

var cdc = codec.New()

func TestDecMarshalJSON(t *testing.T) {
decimal := func(i int64) Dec {
d := NewDec(0)
d.Int = new(big.Int).SetInt64(i)
return d
}
tests := []struct {
name string
d Dec
want string
wantErr bool // if wantErr = false, will also attempt unmarshaling
}{
{"zero", decimal(0), "\"0.0000000000\"", false},
{"one", decimal(1), "\"0.0000000001\"", false},
{"ten", decimal(10), "\"0.0000000010\"", false},
{"12340", decimal(12340), "\"0.0000012340\"", false},
{"zeroInt", NewDec(0), "\"0.0000000000\"", false},
{"oneInt", NewDec(1), "\"1.0000000000\"", false},
{"tenInt", NewDec(10), "\"10.0000000000\"", false},
{"12340Int", NewDec(12340), "\"12340.0000000000\"", false},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, err := tt.d.MarshalJSON()
if (err != nil) != tt.wantErr {
t.Errorf("Dec.MarshalJSON() error = %v, wantErr %v", err, tt.wantErr)
return
}
if !tt.wantErr {
assert.Equal(t, tt.want, string(got), "incorrect marshalled value")
unmarshalledDec := NewDec(0)
unmarshalledDec.UnmarshalJSON(got)
assert.Equal(t, tt.d, unmarshalledDec, "incorrect unmarshalled value")
}
})
}
}

func TestZeroDeserializationJSON(t *testing.T) {
d := Dec{new(big.Int)}
err := cdc.UnmarshalJSON([]byte(`"0"`), &d)
Expand Down
6 changes: 3 additions & 3 deletions x/distribution/keeper/key.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@ var (
ProposerKey = []byte{0x04} // key for storing the proposer operator address

// params store
ParamStoreKeyCommunityTax = []byte("community-tax")
ParamStoreKeyBaseProposerReward = []byte("base-proposer-reward")
ParamStoreKeyBonusProposerReward = []byte("bonus-proposer-reward")
ParamStoreKeyCommunityTax = []byte("CommunityTax")
ParamStoreKeyBaseProposerReward = []byte("BaseProposerReward")
ParamStoreKeyBonusProposerReward = []byte("BonusProposerReward")
)

const (
Expand Down