Skip to content

Commit

Permalink
fix: correctly coalesce coins even with repeated denominations
Browse files Browse the repository at this point in the history
  • Loading branch information
jaeseung-bae committed Mar 27, 2024
1 parent 05ff4ea commit fe9ebec
Show file tree
Hide file tree
Showing 30 changed files with 204 additions and 136 deletions.
3 changes: 2 additions & 1 deletion client/keys/delete.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ private keys stored in a ledger device cannot be deleted with the CLI.
for _, name := range args {
info, err := clientCtx.Keyring.Key(name)
if err != nil {
return err
cmd.PrintErrf("key %s not found\n", name)
continue
}

// confirm deletion, unless -y is passed
Expand Down
3 changes: 1 addition & 2 deletions client/keys/delete_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,7 @@ func Test_runDeleteCmd(t *testing.T) {
ctx := context.WithValue(context.Background(), client.ClientContextKey, &clientCtx)

err = cmd.ExecuteContext(ctx)
require.Error(t, err)
require.EqualError(t, err, "blah.info: key not found")
require.NoError(t, err)

// User confirmation missing
cmd.SetArgs([]string{
Expand Down
2 changes: 1 addition & 1 deletion simapp/test_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,7 @@ func TestAddr(addr, bech string) (sdk.AccAddress, error) {
func CheckBalance(t *testing.T, app *SimApp, addr sdk.AccAddress, balances sdk.Coins) {
t.Helper()
ctxCheck := app.BaseApp.NewContext(true, tmproto.Header{})
require.True(t, balances.IsEqual(app.BankKeeper.GetAllBalances(ctxCheck, addr)))
require.True(t, balances.Equal(app.BankKeeper.GetAllBalances(ctxCheck, addr)))
}

// SignCheckDeliver checks a generated signed transaction and simulates a
Expand Down
10 changes: 9 additions & 1 deletion store/rootmulti/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -984,8 +984,16 @@ func commitStores(version int64, storeMap map[types.StoreKey]types.CommitKVStore
storeInfos := make([]types.StoreInfo, 0, len(storeMap))

for key, store := range storeMap {
commitID := store.Commit()
last := store.LastCommitID()

// If a commit event execution is interrupted, a new iavl store's version will be larger than the rootmulti's metadata, when the block is replayed, we should avoid committing that iavl store again.
var commitID types.CommitID
if last.Version >= version {
last.Version = version
commitID = last
} else {
commitID = store.Commit()
}
if store.GetStoreType() == types.StoreTypeTransient {
continue
}
Expand Down
75 changes: 75 additions & 0 deletions store/rootmulti/store_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -858,3 +858,78 @@ func TestSetIAVLDIsableFastNode(t *testing.T) {
multi.SetIAVLDisableFastNode(false)
require.Equal(t, multi.iavlDisableFastNode, false)
}

type commitKVStoreStub struct {
types.CommitKVStore
Committed int
}

func (stub *commitKVStoreStub) Commit() types.CommitID {
commitID := stub.CommitKVStore.Commit()
stub.Committed += 1
return commitID
}

func prepareStoreMap() map[types.StoreKey]types.CommitKVStore {
var db dbm.DB = dbm.NewMemDB()
store := NewStore(db, log.NewNopLogger())
store.MountStoreWithDB(types.NewKVStoreKey("iavl1"), types.StoreTypeIAVL, nil)
store.MountStoreWithDB(types.NewKVStoreKey("iavl2"), types.StoreTypeIAVL, nil)
store.MountStoreWithDB(types.NewTransientStoreKey("trans1"), types.StoreTypeTransient, nil)
err := store.LoadLatestVersion()
if err != nil {
panic(err)
}
return map[types.StoreKey]types.CommitKVStore{
testStoreKey1: &commitKVStoreStub{
CommitKVStore: store.GetStoreByName("iavl1").(types.CommitKVStore),
},
testStoreKey2: &commitKVStoreStub{
CommitKVStore: store.GetStoreByName("iavl2").(types.CommitKVStore),
},
testStoreKey3: &commitKVStoreStub{
CommitKVStore: store.GetStoreByName("trans1").(types.CommitKVStore),
},
}
}

func TestCommitStores(t *testing.T) {
testCases := []struct {
name string
committed int
exptectCommit int
}{
{
"when upgrade not get interrupted",
0,
1,
},
{
"when upgrade get interrupted once",
1,
0,
},
{
"when upgrade get interrupted twice",
2,
0,
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
storeMap := prepareStoreMap()
store := storeMap[testStoreKey1].(*commitKVStoreStub)
for i := tc.committed; i > 0; i-- {
store.Commit()
}
store.Committed = 0
var version int64 = 1
res := commitStores(version, storeMap)
for _, s := range res.StoreInfos {
require.Equal(t, version, s.CommitId.Version)
}
require.Equal(t, version, res.Version)
require.Equal(t, tc.exptectCommit, store.Committed)
})
}
}
74 changes: 22 additions & 52 deletions types/coin.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,12 +87,9 @@ func (coin Coin) IsLT(other Coin) bool {
}

// IsEqual returns true if the two sets of Coins have the same value
// Deprecated: Use Coin.Equal instead.
func (coin Coin) IsEqual(other Coin) bool {
if coin.Denom != other.Denom {
panic(fmt.Sprintf("invalid coin denominations; %s, %s", coin.Denom, other.Denom))
}

return coin.Amount.Equal(other.Amount)
return coin.Equal(other)
}

// Add adds amounts of two coins with same denom. If the coins differ in denom then
Expand Down Expand Up @@ -295,7 +292,7 @@ func (coins Coins) Add(coinsB ...Coin) Coins {
// denomination and addition only occurs when the denominations match, otherwise
// the coin is simply added to the sum assuming it's not zero.
// The function panics if `coins` or `coinsB` are not sorted (ascending).
func (coins Coins) safeAdd(coinsB Coins) Coins {
func (coins Coins) safeAdd(coinsB Coins) (coalesced Coins) {
// probably the best way will be to make Coins and interface and hide the structure
// definition (type alias)
if !coins.isSorted() {
Expand All @@ -305,51 +302,24 @@ func (coins Coins) safeAdd(coinsB Coins) Coins {
panic("Wrong argument: coins must be sorted")
}

sum := ([]Coin)(nil)
indexA, indexB := 0, 0
lenA, lenB := len(coins), len(coinsB)

for {
if indexA == lenA {
if indexB == lenB {
// return nil coins if both sets are empty
return sum
}

// return set B (excluding zero coins) if set A is empty
return append(sum, removeZeroCoins(coinsB[indexB:])...)
} else if indexB == lenB {
// return set A (excluding zero coins) if set B is empty
return append(sum, removeZeroCoins(coins[indexA:])...)
uniqCoins := make(map[string]Coins, len(coins)+len(coinsB))
// Traverse all the coins for each of the coins and coinsB.
for _, cL := range []Coins{coins, coinsB} {
for _, c := range cL {
uniqCoins[c.Denom] = append(uniqCoins[c.Denom], c)
}
}

coinA, coinB := coins[indexA], coinsB[indexB]

switch strings.Compare(coinA.Denom, coinB.Denom) {
case -1: // coin A denom < coin B denom
if !coinA.IsZero() {
sum = append(sum, coinA)
}

indexA++

case 0: // coin A denom == coin B denom
res := coinA.Add(coinB)
if !res.IsZero() {
sum = append(sum, res)
}

indexA++
indexB++

case 1: // coin A denom > coin B denom
if !coinB.IsZero() {
sum = append(sum, coinB)
}

indexB++
for denom, cL := range uniqCoins {
comboCoin := Coin{Denom: denom, Amount: NewInt(0)}
for _, c := range cL {
comboCoin = comboCoin.Add(c)
}
if !comboCoin.IsZero() {
coalesced = append(coalesced, comboCoin)
}
}
return coalesced.Sort()
}

// DenomsSubsetOf returns true if receiver's denom set
Expand Down Expand Up @@ -404,7 +374,7 @@ func (coins Coins) SafeSub(coinsB Coins) (Coins, bool) {
// a.IsAllLTE(a.Max(b))
// b.IsAllLTE(a.Max(b))
// a.IsAllLTE(c) && b.IsAllLTE(c) == a.Max(b).IsAllLTE(c)
// a.Add(b...).IsEqual(a.Min(b).Add(a.Max(b)...))
// a.Add(b...).Equal(a.Min(b).Add(a.Max(b)...))
//
// E.g.
// {1A, 3B, 2C}.Max({4A, 2B, 2C} == {4A, 3B, 2C})
Expand Down Expand Up @@ -450,7 +420,7 @@ func (coins Coins) Max(coinsB Coins) Coins {
// a.Min(b).IsAllLTE(a)
// a.Min(b).IsAllLTE(b)
// c.IsAllLTE(a) && c.IsAllLTE(b) == c.IsAllLTE(a.Min(b))
// a.Add(b...).IsEqual(a.Min(b).Add(a.Max(b)...))
// a.Add(b...).Equal(a.Min(b).Add(a.Max(b)...))
//
// E.g.
// {1A, 3B, 2C}.Min({4A, 2B, 2C} == {1A, 2B, 2C})
Expand Down Expand Up @@ -593,8 +563,8 @@ func (coins Coins) IsZero() bool {
return true
}

// IsEqual returns true if the two sets of Coins have the same value
func (coins Coins) IsEqual(coinsB Coins) bool {
// Equal returns true if the two sets of Coins have the same value
func (coins Coins) Equal(coinsB Coins) bool {
if len(coins) != len(coinsB) {
return false
}
Expand All @@ -603,7 +573,7 @@ func (coins Coins) IsEqual(coinsB Coins) bool {
coinsB = coinsB.Sort()

for i := 0; i < len(coins); i++ {
if !coins[i].IsEqual(coinsB[i]) {
if !coins[i].Equal(coinsB[i]) {
return false
}
}
Expand Down
Loading

0 comments on commit fe9ebec

Please sign in to comment.