Fix bug in syncing swap rewards (#977)

* fix bug in GetSyncedSwapClaim
It did not sync rewards for pools that had recently had rewards added

* remove dead code

x/incentive/keeper/rewards_delegator.go

@@ -154,13 +154,6 @@ func (k Keeper) GetTotalDelegated(ctx sdk.Context, delegator sdk.AccAddress, val
 	return totalDelegated
 }
 
-// ZeroDelegatorClaim zeroes out the claim object's rewards and returns the updated claim object
-func (k Keeper) ZeroDelegatorClaim(ctx sdk.Context, claim types.DelegatorClaim) types.DelegatorClaim {
-	claim.Reward = sdk.NewCoins()
-	k.SetDelegatorClaim(ctx, claim)
-	return claim
-}
-
 // SimulateDelegatorSynchronization calculates a user's outstanding delegator rewards by simulating reward synchronization
 func (k Keeper) SimulateDelegatorSynchronization(ctx sdk.Context, claim types.DelegatorClaim) types.DelegatorClaim {
 	for _, ri := range claim.RewardIndexes {

x/incentive/keeper/rewards_swap.go

@@ -74,7 +74,7 @@ func (k Keeper) SynchronizeSwapReward(ctx sdk.Context, poolID string, owner sdk.
 	k.SetSwapClaim(ctx, claim)
 }
 
-// synchronizeSwapReward updates the reward in a swap claim for one pool.
+// synchronizeSwapReward updates the reward and indexes in a swap claim for one pool.
 func (k *Keeper) synchronizeSwapReward(ctx sdk.Context, claim types.SwapClaim, poolID string, owner sdk.AccAddress, shares sdk.Int) types.SwapClaim {
 	globalRewardIndexes, found := k.GetSwapRewardIndexes(ctx, poolID)
 	if !found {
@@ -108,21 +108,23 @@ func (k *Keeper) synchronizeSwapReward(ctx sdk.Context, claim types.SwapClaim, p
 	return claim
 }
 
-// GetSynchronizedSwapClaim fetches a swap claim from the store and syncs rewards for all pools.
+// GetSynchronizedSwapClaim fetches a swap claim from the store and syncs rewards for all rewarded pools.
 func (k Keeper) GetSynchronizedSwapClaim(ctx sdk.Context, owner sdk.AccAddress) (types.SwapClaim, bool) {
 	claim, found := k.GetSwapClaim(ctx, owner)
 	if !found {
 		return types.SwapClaim{}, false
 	}
-	for _, indexes := range claim.RewardIndexes { // TODO shouldn't this loop through global indexes, in case some have been recently added?
-		poolID := indexes.CollateralType
 
+	k.IterateSwapRewardIndexes(ctx, func(poolID string, _ types.RewardIndexes) bool {
 		shares, found := k.swapKeeper.GetDepositorSharesAmount(ctx, owner, poolID)
 		if !found {
 			shares = sdk.ZeroInt()
 		}
 
 		claim = k.synchronizeSwapReward(ctx, claim, poolID, owner, shares)
-	}
+
+		return false
+	})
+
 	return claim, true
 }

x/incentive/keeper/rewards_swap_sync_test.go

@@ -315,3 +315,156 @@ func (suite *SynchronizeSwapRewardTests) TestClaimUpdatedWhenGlobalIndexesIncrea
 	// reward is unchanged
 	suite.Equal(claim.Reward, syncedClaim.Reward)
 }
+
+func (suite *SynchronizeSwapRewardTests) TestGetSyncedClaim_ClaimUnchangedWhenNoGlobalIndexes() {
+	poolID_1 := "btcb:usdx"
+	owner := arbitraryAddress()
+
+	swapKeeper := newFakeSwapKeeper().
+		addDeposit(poolID_1, owner, i(1e9))
+	suite.keeper = suite.NewKeeper(&fakeParamSubspace{}, nil, nil, nil, nil, nil, swapKeeper)
+
+	claim := types.SwapClaim{
+		BaseMultiClaim: types.BaseMultiClaim{
+			Owner:  owner,
+			Reward: nil,
+		},
+		RewardIndexes: types.MultiRewardIndexes{
+			{
+				CollateralType: poolID_1,
+				RewardIndexes:  nil, // this state only happens because Init stores empty indexes
+			},
+		},
+	}
+	suite.storeSwapClaim(claim)
+
+	// no global indexes for any pool
+
+	syncedClaim, f := suite.keeper.GetSynchronizedSwapClaim(suite.ctx, claim.Owner)
+	suite.True(f)
+
+	// indexes are unchanged
+	suite.Equal(claim.RewardIndexes, syncedClaim.RewardIndexes)
+	// reward is unchanged
+	suite.Equal(claim.Reward, syncedClaim.Reward)
+}
+
+func (suite *SynchronizeSwapRewardTests) TestGetSyncedClaim_ClaimUpdatedWhenMissingIndexAndHasNoSourceShares() {
+	poolID_1 := "btcb:usdx"
+	poolID_2 := "ukava:usdx"
+	owner := arbitraryAddress()
+
+	// owner has no shares in any pool
+	suite.keeper = suite.NewKeeper(&fakeParamSubspace{}, nil, nil, nil, nil, nil, newFakeSwapKeeper())
+
+	claim := types.SwapClaim{
+		BaseMultiClaim: types.BaseMultiClaim{
+			Owner:  owner,
+			Reward: arbitraryCoins(),
+		},
+		RewardIndexes: types.MultiRewardIndexes{
+			{
+				CollateralType: poolID_1,
+				RewardIndexes: types.RewardIndexes{
+					{
+						CollateralType: "rewarddenom1",
+						RewardFactor:   d("1000.001"),
+					},
+				},
+			},
+		},
+	}
+	suite.storeSwapClaim(claim)
+
+	globalIndexes := types.MultiRewardIndexes{
+		{
+			CollateralType: poolID_1,
+			RewardIndexes: types.RewardIndexes{
+				{
+					CollateralType: "rewarddenom1",
+					RewardFactor:   d("2000.002"),
+				},
+			},
+		},
+		{
+			CollateralType: poolID_2,
+			RewardIndexes: types.RewardIndexes{
+				{
+					CollateralType: "rewarddenom2",
+					RewardFactor:   d("2000.002"),
+				},
+			},
+		},
+	}
+	suite.storeGlobalSwapIndexes(globalIndexes)
+
+	syncedClaim, f := suite.keeper.GetSynchronizedSwapClaim(suite.ctx, claim.Owner)
+	suite.True(f)
+
+	// indexes updated from global
+	suite.Equal(globalIndexes, syncedClaim.RewardIndexes)
+	// reward is unchanged
+	suite.Equal(claim.Reward, syncedClaim.Reward)
+}
+
+func (suite *SynchronizeSwapRewardTests) TestGetSyncedClaim_ClaimUpdatedWhenMissingIndexButHasSourceShares() {
+	poolID_1 := "btcb:usdx"
+	poolID_2 := "ukava:usdx"
+	owner := arbitraryAddress()
+
+	swapKeeper := newFakeSwapKeeper().
+		addDeposit(poolID_1, owner, i(1e9)).
+		addDeposit(poolID_2, owner, i(1e9))
+	suite.keeper = suite.NewKeeper(&fakeParamSubspace{}, nil, nil, nil, nil, nil, swapKeeper)
+
+	claim := types.SwapClaim{
+		BaseMultiClaim: types.BaseMultiClaim{
+			Owner:  owner,
+			Reward: arbitraryCoins(),
+		},
+		RewardIndexes: types.MultiRewardIndexes{
+			{
+				CollateralType: poolID_1,
+				RewardIndexes: types.RewardIndexes{
+					{
+						CollateralType: "rewarddenom1",
+						RewardFactor:   d("1000.001"),
+					},
+				},
+			},
+		},
+	}
+	suite.storeSwapClaim(claim)
+
+	globalIndexes := types.MultiRewardIndexes{
+		{
+			CollateralType: poolID_1,
+			RewardIndexes: types.RewardIndexes{
+				{
+					CollateralType: "rewarddenom1",
+					RewardFactor:   d("2000.002"),
+				},
+			},
+		},
+		{
+			CollateralType: poolID_2,
+			RewardIndexes: types.RewardIndexes{
+				{
+					CollateralType: "rewarddenom2",
+					RewardFactor:   d("2000.002"),
+				},
+			},
+		},
+	}
+	suite.storeGlobalSwapIndexes(globalIndexes)
+
+	syncedClaim, f := suite.keeper.GetSynchronizedSwapClaim(suite.ctx, claim.Owner)
+	suite.True(f)
+
+	// indexes updated from global
+	suite.Equal(globalIndexes, syncedClaim.RewardIndexes)
+	// reward is incremented
+	expectedReward := cs(c("rewarddenom1", 1_000_001_000_000), c("rewarddenom2", 2_000_002_000_000))
+	suite.Equal(claim.Reward.Add(expectedReward...), syncedClaim.Reward)
+}
+

x/incentive/keeper/unit_test.go

@@ -121,27 +121,36 @@ func (subspace *fakeParamSubspace) WithKeyTable(params.KeyTable) params.Subspace
 // fakeSwapKeeper is a stub swap keeper.
 // It can be used to return values to the incentive keeper without having to initialize a full swap keeper.
 type fakeSwapKeeper struct {
-	poolShares map[string]sdk.Int
+	poolShares    map[string]sdk.Int
+	depositShares map[string](map[string]sdk.Int)
 }
 
 var _ types.SwapKeeper = newFakeSwapKeeper()
 
 func newFakeSwapKeeper() *fakeSwapKeeper {
 	return &fakeSwapKeeper{
-		poolShares: map[string]sdk.Int{},
+		poolShares:    map[string]sdk.Int{},
+		depositShares: map[string](map[string]sdk.Int){},
 	}
 }
 func (k *fakeSwapKeeper) addPool(id string, shares sdk.Int) *fakeSwapKeeper {
 	k.poolShares[id] = shares
 	return k
 }
+func (k *fakeSwapKeeper) addDeposit(poolID string, depositor sdk.AccAddress, shares sdk.Int) *fakeSwapKeeper {
+	if k.depositShares[poolID] == nil {
+		k.depositShares[poolID] = map[string]sdk.Int{}
+	}
+	k.depositShares[poolID][depositor.String()] = shares
+	return k
+}
 func (k *fakeSwapKeeper) GetPoolShares(_ sdk.Context, poolID string) (sdk.Int, bool) {
 	shares, ok := k.poolShares[poolID]
 	return shares, ok
 }
 func (k *fakeSwapKeeper) GetDepositorSharesAmount(_ sdk.Context, depositor sdk.AccAddress, poolID string) (sdk.Int, bool) {
-	// This is just to implement the swap keeper interface.
+	shares, found := k.depositShares[poolID][depositor.String()]
-	return sdk.Int{}, false
+	return shares, found
 }
 
 // fakeHardKeeper is a stub hard keeper.