diff --git a/CHANGELOG.md b/CHANGELOG.md index ca02a54a89..49b19e0672 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -208,7 +208,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (cli) [#15299](https://github.com/cosmos/cosmos-sdk/pull/15299) remove `--amino` flag from `sign` and `multi-sign` commands. Amino `StdTx` has been deprecated for a while. Amino JSON signing still works as expected. ### Bug Fixes - +* (x/staking) [#16043](https://github.com/cosmos/cosmos-sdk/pull/16043) Call `AfterUnbondingInitiated` hook for new unbonding entries only and fix `UnbondingDelegation` entries handling * (types) [#16010](https://github.com/cosmos/cosmos-sdk/pull/16010) Let `module.CoreAppModuleBasicAdaptor` fallback to legacy genesis handling. * (x/group) [#16017](https://github.com/cosmos/cosmos-sdk/pull/16017) Correctly apply account number in group v2 migration. * (types) [#15691](https://github.com/cosmos/cosmos-sdk/pull/15691) Make `Coin.Validate()` check that `.Amount` is not nil. diff --git a/x/staking/keeper/delegation.go b/x/staking/keeper/delegation.go index 49968aff99..96cb91bece 100644 --- a/x/staking/keeper/delegation.go +++ b/x/staking/keeper/delegation.go @@ -335,21 +335,25 @@ func (k Keeper) SetUnbondingDelegationEntry( ) types.UnbondingDelegation { ubd, found := k.GetUnbondingDelegation(ctx, delegatorAddr, validatorAddr) id := k.IncrementUnbondingID(ctx) + isNewUbdEntry := true if found { - ubd.AddEntry(creationHeight, minTime, balance, id) + isNewUbdEntry = ubd.AddEntry(creationHeight, minTime, balance, id) } else { ubd = types.NewUnbondingDelegation(delegatorAddr, validatorAddr, creationHeight, minTime, balance, id) } k.SetUnbondingDelegation(ctx, ubd) - // Add to the UBDByUnbondingOp index to look up the UBD by the UBDE ID - k.SetUnbondingDelegationByUnbondingID(ctx, ubd, id) + // only call the hook for new entries since + // calls to AfterUnbondingInitiated are not idempotent + if isNewUbdEntry { + // Add to the UBDByUnbondingOp index to look up the UBD by the UBDE ID + k.SetUnbondingDelegationByUnbondingID(ctx, ubd, id) - if err := k.Hooks().AfterUnbondingInitiated(ctx, id); err != nil { - k.Logger(ctx).Error("failed to call after unbonding initiated hook", "error", err) + if err := k.Hooks().AfterUnbondingInitiated(ctx, id); err != nil { + k.Logger(ctx).Error("failed to call after unbonding initiated hook", "error", err) + } } - return ubd } diff --git a/x/staking/keeper/delegation_test.go b/x/staking/keeper/delegation_test.go index c609ef0639..06b03cbdd3 100644 --- a/x/staking/keeper/delegation_test.go +++ b/x/staking/keeper/delegation_test.go @@ -1063,3 +1063,123 @@ func (s *KeeperTestSuite) TestRedelegateFromUnbondedValidator() { red, found := keeper.GetRedelegation(ctx, addrDels[0], addrVals[0], addrVals[1]) require.False(found, "%v", red) } + +func (s *KeeperTestSuite) TestUnbondingDelegationAddEntry() { + require := s.Require() + + delAddrs, valAddrs := createValAddrs(1) + for _, addr := range delAddrs { + s.accountKeeper.EXPECT().StringToBytes(addr.String()).Return(addr, nil).AnyTimes() + s.accountKeeper.EXPECT().BytesToString(addr).Return(addr.String(), nil).AnyTimes() + } + + delAddr := delAddrs[0] + valAddr := valAddrs[0] + creationHeight := int64(10) + ubd := stakingtypes.NewUnbondingDelegation( + delAddr, + valAddr, + creationHeight, + time.Unix(0, 0).UTC(), + math.NewInt(10), + 0, + ) + var initialEntries []stakingtypes.UnbondingDelegationEntry + initialEntries = append(initialEntries, ubd.Entries...) + require.Len(initialEntries, 1) + + isNew := ubd.AddEntry(creationHeight, time.Unix(0, 0).UTC(), math.NewInt(5), 1) + require.False(isNew) + require.Len(ubd.Entries, 1) // entry was merged + require.NotEqual(initialEntries, ubd.Entries) + require.Equal(creationHeight, ubd.Entries[0].CreationHeight) + require.Equal(initialEntries[0].UnbondingId, ubd.Entries[0].UnbondingId) // unbondingID remains unchanged + require.Equal(ubd.Entries[0].Balance, math.NewInt(15)) // 10 from previous + 5 from merged + + newCreationHeight := int64(11) + isNew = ubd.AddEntry(newCreationHeight, time.Unix(1, 0).UTC(), math.NewInt(5), 2) + require.True(isNew) + require.Len(ubd.Entries, 2) // entry was appended + require.NotEqual(initialEntries, ubd.Entries) + require.Equal(creationHeight, ubd.Entries[0].CreationHeight) + require.Equal(newCreationHeight, ubd.Entries[1].CreationHeight) + require.Equal(ubd.Entries[0].Balance, math.NewInt(15)) + require.Equal(ubd.Entries[1].Balance, math.NewInt(5)) + require.NotEqual(ubd.Entries[0].UnbondingId, ubd.Entries[1].UnbondingId) // appended entry has a new unbondingID +} + +func (s *KeeperTestSuite) TestSetUnbondingDelegationEntry() { + ctx, keeper := s.ctx, s.stakingKeeper + require := s.Require() + + delAddrs, valAddrs := createValAddrs(1) + for _, addr := range delAddrs { + s.accountKeeper.EXPECT().StringToBytes(addr.String()).Return(addr, nil).AnyTimes() + s.accountKeeper.EXPECT().BytesToString(addr).Return(addr.String(), nil).AnyTimes() + } + + delAddr := delAddrs[0] + valAddr := valAddrs[0] + creationHeight := int64(0) + ubd := stakingtypes.NewUnbondingDelegation( + delAddr, + valAddr, + creationHeight, + time.Unix(0, 0).UTC(), + math.NewInt(5), + 0, + ) + + // set and retrieve a record + keeper.SetUnbondingDelegation(ctx, ubd) + resUnbond, found := keeper.GetUnbondingDelegation(ctx, delAddr, valAddr) + require.True(found) + require.Equal(ubd, resUnbond) + + initialEntries := ubd.Entries + require.Len(initialEntries, 1) + require.Equal(initialEntries[0].Balance, math.NewInt(5)) + require.Equal(initialEntries[0].UnbondingId, uint64(0)) // initial unbondingID + + // set unbonding delegation entry for existing creationHeight + // entries are expected to be merged + keeper.SetUnbondingDelegationEntry( + ctx, + delAddr, + valAddr, + creationHeight, + time.Unix(0, 0).UTC(), + math.NewInt(5), + ) + resUnbonding, found := keeper.GetUnbondingDelegation(ctx, delAddr, valAddr) + require.True(found) + require.Len(resUnbonding.Entries, 1) + require.NotEqual(initialEntries, resUnbonding.Entries) + require.Equal(creationHeight, resUnbonding.Entries[0].CreationHeight) + require.Equal(initialEntries[0].UnbondingId, resUnbonding.Entries[0].UnbondingId) // initial unbondingID remains unchanged + require.Equal(resUnbonding.Entries[0].Balance, math.NewInt(10)) // 5 from previous entry + 5 from merged entry + + // set unbonding delegation entry for newCreationHeight + // new entry is expected to be appended to the existing entries + newCreationHeight := int64(1) + keeper.SetUnbondingDelegationEntry( + ctx, + delAddr, + valAddr, + newCreationHeight, + time.Unix(1, 0).UTC(), + math.NewInt(10), + ) + resUnbonding, found = keeper.GetUnbondingDelegation(ctx, delAddr, valAddr) + require.True(found) + require.Len(resUnbonding.Entries, 2) + require.NotEqual(initialEntries, resUnbonding.Entries) + require.NotEqual(resUnbonding.Entries[0], resUnbonding.Entries[1]) + require.Equal(creationHeight, resUnbonding.Entries[0].CreationHeight) + require.Equal(newCreationHeight, resUnbonding.Entries[1].CreationHeight) + + // unbondingID is incremented on every call to SetUnbondingDelegationEntry + // unbondingID == 1 was skipped because the entry was merged with the existing entry with unbondingID == 0 + // unbondingID comes from a global counter -> gaps in unbondingIDs are OK as long as every unbondingID is unique + require.Equal(uint64(2), resUnbonding.Entries[1].UnbondingId) +} diff --git a/x/staking/types/delegation.go b/x/staking/types/delegation.go index 746d33a382..355ef50007 100644 --- a/x/staking/types/delegation.go +++ b/x/staking/types/delegation.go @@ -127,7 +127,7 @@ func NewUnbondingDelegation( } // AddEntry - append entry to the unbonding delegation -func (ubd *UnbondingDelegation) AddEntry(creationHeight int64, minTime time.Time, balance math.Int, unbondingID uint64) { +func (ubd *UnbondingDelegation) AddEntry(creationHeight int64, minTime time.Time, balance math.Int, unbondingID uint64) bool { // Check the entries exists with creation_height and complete_time entryIndex := -1 for index, ubdEntry := range ubd.Entries { @@ -144,11 +144,12 @@ func (ubd *UnbondingDelegation) AddEntry(creationHeight int64, minTime time.Time // update the entry ubd.Entries[entryIndex] = ubdEntry - } else { - // append the new unbond delegation entry - entry := NewUnbondingDelegationEntry(creationHeight, minTime, balance, unbondingID) - ubd.Entries = append(ubd.Entries, entry) + return false } + // append the new unbond delegation entry + entry := NewUnbondingDelegationEntry(creationHeight, minTime, balance, unbondingID) + ubd.Entries = append(ubd.Entries, entry) + return true } // RemoveEntry - remove entry at index i to the unbonding delegation