From 06b9c0db9029dbb85a2cd078e2900fb35d23cf7e Mon Sep 17 00:00:00 2001 From: javiersuweijie Date: Wed, 20 Mar 2024 14:23:55 +0800 Subject: [PATCH 1/3] fix: increase rounding sensitivity --- x/alliance/keeper/delegation.go | 2 +- x/alliance/keeper/tests/unbonding_test.go | 92 +++++++++++++++++++++++ x/alliance/types/asset.go | 4 +- x/alliance/types/params.go | 3 + 4 files changed, 98 insertions(+), 3 deletions(-) diff --git a/x/alliance/keeper/delegation.go b/x/alliance/keeper/delegation.go index a563ace7..d9d937f6 100644 --- a/x/alliance/keeper/delegation.go +++ b/x/alliance/keeper/delegation.go @@ -383,7 +383,7 @@ func (k Keeper) ValidateDelegatedAmount(delegation types.Delegation, coin sdk.Co // Account for rounding in which shares for a full withdraw is slightly more or less than the number of shares recorded // Withdraw all in that case // 1e6 of margin should be enough to handle realistic rounding issues caused by using the fix-point math. - if delegation.Shares.Sub(delegationSharesToUpdate).Abs().LT(sdk.NewDecWithPrec(1, 6)) { + if delegation.Shares.Sub(delegationSharesToUpdate).Abs().LT(types.Rounder) { return delegation.Shares, nil } diff --git a/x/alliance/keeper/tests/unbonding_test.go b/x/alliance/keeper/tests/unbonding_test.go index 32904c41..e848289c 100644 --- a/x/alliance/keeper/tests/unbonding_test.go +++ b/x/alliance/keeper/tests/unbonding_test.go @@ -103,3 +103,95 @@ func TestUnbondingMethods(t *testing.T) { unbondings, ) } + +func TestUnbondingMethodsLargeNumbers(t *testing.T) { + // Setup the context with an alliance asset + app, ctx := createTestContext(t) + startTime := time.Now() + ctx = ctx.WithBlockTime(startTime).WithBlockHeight(1) + allianceAsset := types.AllianceAsset{ + Denom: AllianceDenom, + RewardWeight: math.LegacyMustNewDecFromStr("0.025"), + TakeRate: math.LegacyNewDec(0), + TotalTokens: math.ZeroInt(), + TotalValidatorShares: math.LegacyZeroDec(), + RewardStartTime: startTime, + RewardChangeRate: math.LegacyNewDec(0), + RewardChangeInterval: time.Duration(0), + LastRewardChangeTime: startTime, + RewardWeightRange: types.RewardWeightRange{Min: math.LegacyNewDec(0), Max: math.LegacyNewDec(1)}, + IsInitialized: true, + } + app.AllianceKeeper.SetAsset(ctx, allianceAsset) + + // Query staking module unbonding time to assert later on + unbondingTime := app.StakingKeeper.UnbondingTime(ctx) + + // Get the native delegations to have a validator address where to delegate + delegations := app.StakingKeeper.GetAllDelegations(ctx) + valAddr, err := sdk.ValAddressFromBech32(delegations[0].ValidatorAddress) + require.NoError(t, err) + + // Get a delegator address with funds + delAddrs := test_helpers.AddTestAddrsIncremental(app, ctx, 2, sdk.NewCoins(sdk.NewCoin(AllianceDenom, math.NewInt(1000_000_000_000_000)))) + delAddr := delAddrs[0] + delAddr1 := delAddrs[1] + + // Get an alliance validator + val, err := app.AllianceKeeper.GetAllianceValidator(ctx, valAddr) + require.NoError(t, err) + + // Delegate the alliance asset with both accounts + res, err := app.AllianceKeeper.Delegate(ctx, delAddr, val, sdk.NewCoin(AllianceDenom, math.NewInt(830697941465481))) + require.Nil(t, err) + require.Equal(t, math.LegacyNewDec(830697941465481), *res) + res2, err := app.AllianceKeeper.Delegate(ctx, delAddr1, val, sdk.NewCoin(AllianceDenom, math.NewInt(975933204219431))) + require.Nil(t, err) + require.Equal(t, math.LegacyNewDec(975933204219431), *res2) + + // Undelegate the alliance assets with both accounts + undelRes, err := app.AllianceKeeper.Undelegate(ctx, delAddr, val, sdk.NewCoin(AllianceDenom, math.NewInt(564360383558874))) + require.Nil(t, err) + require.Equal(t, ctx.BlockHeader().Time.Add(unbondingTime), *undelRes) + undelRes2, err := app.AllianceKeeper.Undelegate(ctx, delAddr1, val, sdk.NewCoin(AllianceDenom, math.NewInt(384108763572096))) + require.Nil(t, err) + require.Equal(t, ctx.BlockHeader().Time.Add(unbondingTime), *undelRes2) + + // Validate that both user delegations executed the unbonding process + unbondings, err := app.AllianceKeeper.GetUnbondingsByDelegator(ctx, delAddr) + require.NoError(t, err) + require.Equal(t, + []types.UnbondingDelegation{{ + ValidatorAddress: valAddr.String(), + Amount: math.NewInt(564360383558874), + CompletionTime: ctx.BlockHeader().Time.Add(unbondingTime), + Denom: AllianceDenom, + }}, + unbondings, + ) + + // Validate that both user delegations executed the unbonding process + unbondings, err = app.AllianceKeeper.GetUnbondingsByDenomAndDelegator(ctx, AllianceDenom, delAddr1) + require.NoError(t, err) + require.Equal(t, + []types.UnbondingDelegation{{ + ValidatorAddress: valAddr.String(), + Amount: math.NewInt(384108763572096), + CompletionTime: ctx.BlockHeader().Time.Add(unbondingTime), + Denom: AllianceDenom, + }}, + unbondings, + ) + + unbondings, err = app.AllianceKeeper.GetUnbondings(ctx, AllianceDenom, delAddr1, valAddr) + require.NoError(t, err) + require.Equal(t, + []types.UnbondingDelegation{{ + ValidatorAddress: valAddr.String(), + Amount: math.NewInt(384108763572096), + CompletionTime: ctx.BlockHeader().Time.Add(unbondingTime), + Denom: AllianceDenom, + }}, + unbondings, + ) +} diff --git a/x/alliance/types/asset.go b/x/alliance/types/asset.go index a8fe8f3b..c2f405cc 100644 --- a/x/alliance/types/asset.go +++ b/x/alliance/types/asset.go @@ -47,7 +47,7 @@ func GetDelegationTokens(del Delegation, val AllianceValidator, asset AllianceAs // We add a small epsilon before rounding down to make sure cases like // 9.999999 get round to 10 - delTokens = delTokens.Add(sdk.NewDecWithPrec(1, 6)) + delTokens = delTokens.Add(Rounder) return sdk.NewCoin(asset.Denom, delTokens.TruncateInt()) } @@ -58,7 +58,7 @@ func GetDelegationTokensWithShares(delegatorShares sdk.Dec, val AllianceValidato // We add a small epsilon before rounding down to make sure cases like // 9.999999 get round to 10 - delTokens = delTokens.Add(sdk.NewDecWithPrec(1, 6)) + delTokens = delTokens.Add(Rounder) return sdk.NewCoin(asset.Denom, delTokens.TruncateInt()) } diff --git a/x/alliance/types/params.go b/x/alliance/types/params.go index c9f74de6..44d90d8f 100644 --- a/x/alliance/types/params.go +++ b/x/alliance/types/params.go @@ -1,6 +1,7 @@ package types import ( + "cosmossdk.io/math" "fmt" "time" @@ -10,6 +11,8 @@ import ( ) var ( + // Rounder is used to round up small errors due to fix point math + Rounder = math.LegacyNewDecWithPrec(1, 2) RewardDelayTime = []byte("RewardDelayTime") TakeRateClaimInterval = []byte("TakeRateClaimInterval") LastTakeRateClaimTime = []byte("LastTakeRateClaimTime") From 111381b78c286fa2a911ba13adbba800dfb1604d Mon Sep 17 00:00:00 2001 From: javiersuweijie Date: Wed, 20 Mar 2024 22:25:56 +0800 Subject: [PATCH 2/3] fix: rewards should be the same across validators --- x/alliance/keeper/reward.go | 11 +- x/alliance/keeper/tests/reward_test.go | 145 ++++++++++++++++++++++++- 2 files changed, 150 insertions(+), 6 deletions(-) diff --git a/x/alliance/keeper/reward.go b/x/alliance/keeper/reward.go index 486228a3..4e65c56b 100644 --- a/x/alliance/keeper/reward.go +++ b/x/alliance/keeper/reward.go @@ -149,20 +149,23 @@ func (k Keeper) AddAssetsToRewardPool(ctx sdk.Context, from sdk.AccAddress, val } alliances := k.GetAllAssets(ctx) - // Get total reward weight to normalize weights - totalRewardWeight := sdk.NewDec(0) + totalStakedRewardWeight := sdk.NewDec(0) + // Map is used here only as a lookup table so it does not change the order of the results therefore it is consensus safe + assetStakedRewardWeights := make(map[string]sdk.Dec) for _, asset := range alliances { if shouldSkipRewardsToAsset(ctx, *asset, val) { continue } - totalRewardWeight = totalRewardWeight.Add(asset.RewardWeight) + stakedRewardWeight := asset.RewardWeight.Mul(val.TotalTokensWithAsset(*asset)).QuoInt(asset.TotalTokens) + assetStakedRewardWeights[asset.Denom] = stakedRewardWeight + totalStakedRewardWeight = totalStakedRewardWeight.Add(stakedRewardWeight) } for _, asset := range alliances { if shouldSkipRewardsToAsset(ctx, *asset, val) { continue } - normalizedWeight := asset.RewardWeight.Quo(totalRewardWeight) + normalizedWeight := assetStakedRewardWeights[asset.Denom].Quo(totalStakedRewardWeight) for _, c := range coins { rewardHistory, found := rewardHistories.GetIndexByDenom(c.Denom, asset.Denom) totalTokens := val.TotalTokensWithAsset(*asset) diff --git a/x/alliance/keeper/tests/reward_test.go b/x/alliance/keeper/tests/reward_test.go index e656c385..378ed1a0 100644 --- a/x/alliance/keeper/tests/reward_test.go +++ b/x/alliance/keeper/tests/reward_test.go @@ -1288,11 +1288,11 @@ func TestMigratedRewards(t *testing.T) { rewards1, err := app.AllianceKeeper.ClaimDelegationRewards(ctx, user1, val1, AllianceDenom) require.NoError(t, err) - require.Equal(t, sdk.NewInt(1_000_000+2_000_000), rewards1.AmountOf(bondDenom)) + require.Equal(t, sdk.NewInt(1_000_000+4285714), rewards1.AmountOf(bondDenom)) rewards2, err := app.AllianceKeeper.ClaimDelegationRewards(ctx, user2, val1, AllianceDenomTwo) require.NoError(t, err) - require.Equal(t, sdk.NewInt(4_000_000+8_000_000), rewards2.AmountOf(bondDenom)) + require.Equal(t, sdk.NewInt(4_000_000+5714285), rewards2.AmountOf(bondDenom)) rewards3, err := app.AllianceKeeper.ClaimDelegationRewards(ctx, user1, val2, AllianceDenomTwo) require.NoError(t, err) @@ -1319,3 +1319,144 @@ func TestMigratedRewards(t *testing.T) { require.NoError(t, err) require.Equal(t, sdk.NewInt(0), rewards4.AmountOf(bondDenom)) } + +func TestClaimRewardsWithDifferentValidators(t *testing.T) { + var err error + app, ctx := createTestContext(t) + app.AllianceKeeper.InitGenesis(ctx, &types.GenesisState{ + Params: types.DefaultParams(), + Assets: []types.AllianceAsset{ + types.NewAllianceAsset(AllianceDenom, sdk.NewDec(2), sdk.NewDec(0), sdk.NewDec(5), sdk.NewDec(0), ctx.BlockTime()), + types.NewAllianceAsset(AllianceDenomTwo, sdk.NewDec(2), sdk.NewDec(0), sdk.NewDec(5), sdk.NewDec(0), ctx.BlockTime()), + }, + }) + + // Set tax and rewards to be zero for easier calculation + distParams := app.DistrKeeper.GetParams(ctx) + distParams.CommunityTax = sdk.ZeroDec() + + err = app.DistrKeeper.SetParams(ctx, distParams) + require.NoError(t, err) + + // Accounts + // mintPoolAddr := app.AccountKeeper.GetModuleAddress(minttypes.ModuleName) + // rewardsPoolAddr := app.AccountKeeper.GetModuleAddress(types.RewardsPoolName) + delegations := app.StakingKeeper.GetAllDelegations(ctx) + valAddr0, err := sdk.ValAddressFromBech32(delegations[0].ValidatorAddress) + require.NoError(t, err) + val0, found := app.StakingKeeper.GetValidator(ctx, valAddr0) + require.True(t, found) + + addrs := test_helpers.AddTestAddrsIncremental(app, ctx, 4, sdk.NewCoins( + sdk.NewCoin(AllianceDenom, sdk.NewInt(10_00_000_000)), + sdk.NewCoin(AllianceDenomTwo, sdk.NewInt(10_00_000_000)), + )) + pks := test_helpers.CreateTestPubKeys(2) + + // Creating two validators: 1 with 0% commission, 1 with 100% commission + valAddr1 := sdk.ValAddress(addrs[0]) + _val1 := teststaking.NewValidator(t, valAddr1, pks[0]) + _val1.Commission = stakingtypes.Commission{ + CommissionRates: stakingtypes.CommissionRates{ + Rate: sdk.NewDec(0), + MaxRate: sdk.NewDec(0), + MaxChangeRate: sdk.NewDec(0), + }, + UpdateTime: time.Now(), + } + test_helpers.RegisterNewValidator(t, app, ctx, _val1) + + valAddr2 := sdk.ValAddress(addrs[1]) + _val2 := teststaking.NewValidator(t, valAddr2, pks[1]) + _val2.Commission = stakingtypes.Commission{ + CommissionRates: stakingtypes.CommissionRates{ + Rate: sdk.NewDec(0), + MaxRate: sdk.NewDec(0), + MaxChangeRate: sdk.NewDec(0), + }, + UpdateTime: time.Now(), + } + test_helpers.RegisterNewValidator(t, app, ctx, _val2) + + val1, _ := app.AllianceKeeper.GetAllianceValidator(ctx, valAddr1) + val2, _ := app.AllianceKeeper.GetAllianceValidator(ctx, valAddr2) + + user1 := addrs[2] + user2 := addrs[3] + + // Mint tokens + err = app.BankKeeper.MintCoins(ctx, minttypes.ModuleName, sdk.NewCoins(sdk.NewCoin("stake", sdk.NewInt(4000_000)))) + require.NoError(t, err) + + // New delegations from user 1 + _, err = app.AllianceKeeper.Delegate(ctx, user1, val1, sdk.NewCoin(AllianceDenom, sdk.NewInt(1_000_000))) + require.NoError(t, err) + _, err = app.AllianceKeeper.Delegate(ctx, user1, val2, sdk.NewCoin(AllianceDenom, sdk.NewInt(1_000_000))) + require.NoError(t, err) + + // New delegation from user 2 + _, err = app.AllianceKeeper.Delegate(ctx, user2, val1, sdk.NewCoin(AllianceDenom, sdk.NewInt(2_000_000))) + require.NoError(t, err) + _, err = app.AllianceKeeper.Delegate(ctx, user2, val2, sdk.NewCoin(AllianceDenom, sdk.NewInt(1_000_000))) + require.NoError(t, err) + _, err = app.AllianceKeeper.Delegate(ctx, user2, val1, sdk.NewCoin(AllianceDenomTwo, sdk.NewInt(5_000_000))) + require.NoError(t, err) + _, err = app.AllianceKeeper.Delegate(ctx, user2, val2, sdk.NewCoin(AllianceDenomTwo, sdk.NewInt(5_000_000))) + require.NoError(t, err) + assets := app.AllianceKeeper.GetAllAssets(ctx) + err = app.AllianceKeeper.RebalanceBondTokenWeights(ctx, assets) + require.NoError(t, err) + + // Total AllianceDenom staked = 5_000_000 + // Total AllianceDenomTwo staked = 10_000_000 + // Total rewards to vals = 4_000_000 + // Normalized reward weight = 0.4 + // AllianceDenom rewards per token = 4_000_000 * 0.4 / 5_000_000 = 0.32 + // AllianceDenomTwo rewards per token = 4_000_000 * 0.4 / 10_000_000 = 0.16 + + // Transfer to rewards to fee pool to be distributed + err = app.BankKeeper.SendCoinsFromModuleToModule(ctx, minttypes.ModuleName, authtypes.FeeCollectorName, sdk.NewCoins(sdk.NewCoin("stake", sdk.NewInt(4_000_000)))) + require.NoError(t, err) + + ctx = ctx.WithBlockHeight(ctx.BlockHeight() + 1) + // Distribute in the next begin block + // At the next begin block, tokens will be distributed from the fee pool + cons0, _ := val0.GetConsAddr() + cons1, _ := val1.GetConsAddr() + cons2, _ := val2.GetConsAddr() + var votingPower int64 = 100 + app.DistrKeeper.AllocateTokens(ctx, votingPower, []abcitypes.VoteInfo{ + { + Validator: abcitypes.Validator{ + Address: cons0, + Power: 20, + }, + SignedLastBlock: true, + }, + { + Validator: abcitypes.Validator{ + Address: cons1, + Power: 44, + }, + SignedLastBlock: true, + }, + { + Validator: abcitypes.Validator{ + Address: cons2, + Power: 36, + }, + SignedLastBlock: true, + }, + }) + + // User 2 claims rewards + // 0.16 * 5_000_000 = 800_000 + coins, err := app.AllianceKeeper.ClaimDelegationRewards(ctx, user2, val1, AllianceDenomTwo) + require.NoError(t, err) + require.Equal(t, sdk.NewCoins(sdk.NewCoin("stake", sdk.NewInt(800_000))), coins) + + // 0.16 * 5_000_000 = 800_000 + coins, err = app.AllianceKeeper.ClaimDelegationRewards(ctx, user2, val2, AllianceDenomTwo) + require.NoError(t, err) + require.Equal(t, sdk.NewCoins(sdk.NewCoin("stake", sdk.NewInt(800_000))), coins) +} From cbff7be0ad3792747bc4302d77582a2014c57d36 Mon Sep 17 00:00:00 2001 From: javiersuweijie Date: Wed, 20 Mar 2024 22:47:23 +0800 Subject: [PATCH 3/3] fix: lint --- x/alliance/keeper/tests/delegation_test.go | 1 + x/alliance/keeper/tests/unbonding_test.go | 1 + x/alliance/tests/e2e/delegate_undelegate_test.go | 1 + x/alliance/types/params.go | 3 ++- 4 files changed, 5 insertions(+), 1 deletion(-) diff --git a/x/alliance/keeper/tests/delegation_test.go b/x/alliance/keeper/tests/delegation_test.go index 6bfa38e6..1117bbb8 100644 --- a/x/alliance/keeper/tests/delegation_test.go +++ b/x/alliance/keeper/tests/delegation_test.go @@ -5,6 +5,7 @@ import ( "time" "cosmossdk.io/math" + test_helpers "github.com/terra-money/alliance/app" "github.com/terra-money/alliance/x/alliance" "github.com/terra-money/alliance/x/alliance/keeper" diff --git a/x/alliance/keeper/tests/unbonding_test.go b/x/alliance/keeper/tests/unbonding_test.go index e848289c..e5585fda 100644 --- a/x/alliance/keeper/tests/unbonding_test.go +++ b/x/alliance/keeper/tests/unbonding_test.go @@ -8,6 +8,7 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" "github.com/stretchr/testify/require" + test_helpers "github.com/terra-money/alliance/app" "github.com/terra-money/alliance/x/alliance/types" ) diff --git a/x/alliance/tests/e2e/delegate_undelegate_test.go b/x/alliance/tests/e2e/delegate_undelegate_test.go index 6eca657a..e6a1fae6 100644 --- a/x/alliance/tests/e2e/delegate_undelegate_test.go +++ b/x/alliance/tests/e2e/delegate_undelegate_test.go @@ -5,6 +5,7 @@ import ( "time" "cosmossdk.io/math" + "github.com/terra-money/alliance/x/alliance" "github.com/terra-money/alliance/x/alliance/keeper" diff --git a/x/alliance/types/params.go b/x/alliance/types/params.go index 44d90d8f..51c77588 100644 --- a/x/alliance/types/params.go +++ b/x/alliance/types/params.go @@ -1,10 +1,11 @@ package types import ( - "cosmossdk.io/math" "fmt" "time" + "cosmossdk.io/math" + "golang.org/x/exp/slices" paramtypes "github.com/cosmos/cosmos-sdk/x/params/types"