fix: call AfterUnbondingInitiated for new unbonding entries only (#16043

)

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.

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
 }
 

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)
+}

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