From b2bfe05170c4dcdf364f796000b6fb94dcdeed61 Mon Sep 17 00:00:00 2001 From: Ruaridh Date: Mon, 26 Jul 2021 21:07:18 +0100 Subject: [PATCH] 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 | 7 - x/incentive/keeper/rewards_swap.go | 12 +- x/incentive/keeper/rewards_swap_sync_test.go | 153 +++++++++++++++++++ x/incentive/keeper/unit_test.go | 17 ++- 4 files changed, 173 insertions(+), 16 deletions(-) diff --git a/x/incentive/keeper/rewards_delegator.go b/x/incentive/keeper/rewards_delegator.go index c712ed4b..5a0bed6d 100644 --- a/x/incentive/keeper/rewards_delegator.go +++ b/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 { diff --git a/x/incentive/keeper/rewards_swap.go b/x/incentive/keeper/rewards_swap.go index 9e25ac46..c2e527ff 100644 --- a/x/incentive/keeper/rewards_swap.go +++ b/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 } diff --git a/x/incentive/keeper/rewards_swap_sync_test.go b/x/incentive/keeper/rewards_swap_sync_test.go index 02a97416..ebb47c0f 100644 --- a/x/incentive/keeper/rewards_swap_sync_test.go +++ b/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) +} + diff --git a/x/incentive/keeper/unit_test.go b/x/incentive/keeper/unit_test.go index 32d707d4..d21cc473 100644 --- a/x/incentive/keeper/unit_test.go +++ b/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. - return sdk.Int{}, false + shares, found := k.depositShares[poolID][depositor.String()] + return shares, found } // fakeHardKeeper is a stub hard keeper.