fix(vbank): ensure that multiple balance updates are sorted #3435
Conversation
golang/cosmos/x/vbank/vbank_test.go
Outdated
| @@ -99,6 +103,7 @@ func Test_marshalBalanceUpdate(t *testing.T) { | |||
| addressToBalance map[string]sdk.Coins | |||
| want balances | |||
| wantErr bool | |||
| encoded []byte | |||
There was a problem hiding this comment.
This field seems to be unused.
| @@ -82,7 +82,11 @@ func decodeBalances(encoded []byte) (balances, uint64, error) { | |||
| return nil, 0, fmt.Errorf("bad balance update type: %s", balanceUpdate.Type) | |||
| } | |||
| b := newBalances() | |||
| for _, u := range balanceUpdate.Updated { | |||
| fmt.Printf("updated balances %v\n", balanceUpdate.Updated) | |||
There was a problem hiding this comment.
The non-determinism of update order is what led to the following commit in the PR that created these tests: 854fe21
In the long run, that commit should be undone and this file should switch from want values in the the map-of-maps representation - elegant as it is - to the vbankBalanceUpdate structure, or perhaps the raw encoded JSON - depending on whether differences in encoded JSON, e.g. different whitespace, would trigger a nondeterminism failure.
That could possibly be delayed to a follow-on PR. If so, then for now please abstract this plus the Type field check above into a balanceUpdateValid(vbu vbankBalanceUpdate) bool utility function, and file an issue for going back to determinism-sensitive want values.
There was a problem hiding this comment.
I introduced validateBalanceUpdate, which returns an error if there's a problem, nil otherwise.
I'm not sure I want determinism-sensitive want values. I like the fact that the test creates random private/pubkeys, and it would make the test less clear to write dynamic want values (since their update order depends on the lexicographic ordering of the pubkey addresses).
| @@ -495,7 +502,7 @@ func Test_EndBlock_Events(t *testing.T) { | |||
| } | |||
| gotMsg, gotNonce, err := decodeBalances([]byte(msgsSent[0])) | |||
| if err != nil { | |||
| t.Errorf("decode balances error = %v", err) | |||
| t.Fatalf("decode balances error = %v", err) | |||
There was a problem hiding this comment.
Cool! Please note that this PR fixes #3425.
Fixes #3425
@mhofman identified another nondeterminism in
x/vbank: balance update messages sent to JS that contained more than 1 update were giving an updates array whose order was determined by the Golang iteration order randomizer.This change sorts the updates array and tests for regressions.