Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[OTE-788] Update revshare safety #2284

Merged
merged 18 commits into from
Sep 24, 2024
Merged

[OTE-788] Update revshare safety #2284

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
ad7a78d
update validate revshare safety
affanv14 Sep 18, 2024
193df7c
update comment
affanv14 Sep 18, 2024
af30e36
lint
affanv14 Sep 18, 2024
d29f906
updaet
affanv14 Sep 18, 2024
12d634e
update
affanv14 Sep 18, 2024
4c842a6
add test
affanv14 Sep 18, 2024
d1a58d0
fix
affanv14 Sep 19, 2024
ed5be06
changes
affanv14 Sep 20, 2024
5b50b2f
temp
affanv14 Sep 20, 2024
21bb82b
address error
affanv14 Sep 23, 2024
11f651d
Merge remote-tracking branch 'origin/main' into affan/revshare-safety
affanv14 Sep 23, 2024
6111a9e
address comments
affanv14 Sep 23, 2024
f8e2a77
nit
affanv14 Sep 23, 2024
caa4017
address comments
affanv14 Sep 24, 2024
434c965
change to greater than or equal to
affanv14 Sep 24, 2024
059bd8d
Merge remote-tracking branch 'origin/main' into affan/revshare-safety
affanv14 Sep 24, 2024
7e9ef41
update comment
affanv14 Sep 24, 2024
6a0051d
add todo
affanv14 Sep 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions protocol/app/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -970,6 +970,8 @@ func New(
)
feeTiersModule := feetiersmodule.NewAppModule(appCodec, app.FeeTiersKeeper)

app.AffiliatesKeeper.SetFeetiersKeeper(app.FeeTiersKeeper)

app.RevShareKeeper = *revsharemodulekeeper.NewKeeper(
appCodec,
keys[revsharemoduletypes.StoreKey],
Expand All @@ -983,6 +985,7 @@ func New(

// Set the revshare keeper in the affiliates keeper.
app.AffiliatesKeeper.SetRevShareKeeper(app.RevShareKeeper)
app.FeeTiersKeeper.SetRevShareKeeper(app.RevShareKeeper)

app.PricesKeeper = *pricesmodulekeeper.NewKeeper(
appCodec,
Expand Down
3 changes: 3 additions & 0 deletions protocol/testutil/keeper/clob.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,9 @@ func NewClobKeepersTestContextWithUninitializedMemStore(
cdc,
)
revShareKeeper, _, _ := createRevShareKeeper(stateStore, db, cdc, affiliatesKeeper, ks.FeeTiersKeeper)
ks.FeeTiersKeeper.SetRevShareKeeper(revShareKeeper)
affiliatesKeeper.SetRevShareKeeper(revShareKeeper)
affiliatesKeeper.SetFeetiersKeeper(ks.FeeTiersKeeper)
ks.MarketMapKeeper, _ = createMarketMapKeeper(stateStore, db, cdc)
ks.PricesKeeper, _, _, mockTimeProvider = createPricesKeeper(
stateStore,
Expand Down
5 changes: 5 additions & 0 deletions protocol/x/affiliates/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ type (
authorities map[string]struct{}
statsKeeper types.StatsKeeper
revShareKeeper types.RevShareKeeper
feetiersKeeper types.FeetiersKeeper
indexerEventManager indexer_manager.IndexerEventManager
}
)
Expand Down Expand Up @@ -255,6 +256,10 @@ func (k *Keeper) SetRevShareKeeper(revShareKeeper types.RevShareKeeper) {
k.revShareKeeper = revShareKeeper
}

func (k *Keeper) SetFeetiersKeeper(feetiersKeeper types.FeetiersKeeper) {
k.feetiersKeeper = feetiersKeeper
}

func (k Keeper) GetIndexerEventManager() indexer_manager.IndexerEventManager {
return k.indexerEventManager
}
Expand Down
4 changes: 3 additions & 1 deletion protocol/x/affiliates/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,11 @@ func (k msgServer) UpdateAffiliateTiers(ctx context.Context,
return nil, err
}
marketMapperRevShareParams := k.revShareKeeper.GetMarketMapperRevenueShareParams(sdkCtx)
lowestTakerFee := k.feetiersKeeper.GetAffiliateRefereeLowestTakerFee(sdkCtx)
lowestMakerFee := k.feetiersKeeper.GetLowestMakerFee(sdkCtx)

if !k.revShareKeeper.ValidateRevShareSafety(sdkCtx, msg.Tiers,
unconditionalRevShareConfig, marketMapperRevShareParams) {
unconditionalRevShareConfig, marketMapperRevShareParams, lowestTakerFee, lowestMakerFee) {
return nil, errorsmod.Wrapf(
types.ErrRevShareSafetyViolation,
"rev share safety violation",
Expand Down
7 changes: 7 additions & 0 deletions protocol/x/affiliates/types/expected_keepers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,5 +23,12 @@ type RevShareKeeper interface {
affiliateTiers AffiliateTiers,
unconditionalRevShareConfig revsharetypes.UnconditionalRevShareConfig,
marketMapperRevShareParams revsharetypes.MarketMapperRevenueShareParams,
lowestTakerFee int32,
lowestMakerFee int32,
) bool
}

type FeetiersKeeper interface {
GetAffiliateRefereeLowestTakerFee(ctx sdk.Context) int32
GetLowestMakerFee(ctx sdk.Context) int32
Comment on lines +15 to +17
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

Codebase Verification

Incomplete Removal of RevShareKeeper Detected

The search results indicate that RevShareKeeper is still present in multiple areas of the codebase, including interfaces, keeper implementations, and test files. This suggests that the replacement with FeetiersKeeper is not fully implemented.

Action Items:

  • Complete Refactoring: Ensure that all instances of RevShareKeeper are replaced with FeetiersKeeper across the entire codebase.
  • Update Dependencies: Verify that all modules and tests are updated to use FeetiersKeeper instead of RevShareKeeper.
  • Review Safety Checks: Confirm that any safety checks previously handled by RevShareKeeper are appropriately managed by the new FeetiersKeeper.
Analysis chain

New FeetiersKeeper interface introduced

The new FeetiersKeeper interface has been added, replacing the previous RevShareKeeper. This change aligns with the PR objective of updating revenue share safety.

A few points to consider:

  1. The new methods focus on retrieving the lowest fees, which is a shift from the previous revenue sharing approach.
  2. Ensure that all places where RevShareKeeper was used have been updated to use FeetiersKeeper instead.
  3. The removal of RevShareKeeper methods like ValidateRevShareSafety suggests that the safety checks might have been moved elsewhere or implemented differently.

To ensure that all usages of the old RevShareKeeper have been properly updated, please run the following script:

This script will help identify any places where the old RevShareKeeper might still be in use and confirm that FeetiersKeeper is being used appropriately.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining usages of RevShareKeeper in the codebase

# Search for RevShareKeeper usages
echo "Searching for RevShareKeeper usages:"
rg --type go "RevShareKeeper"

# Search for the old methods that were in RevShareKeeper
echo "Searching for old RevShareKeeper methods:"
rg --type go "GetUnconditionalRevShareConfigParams|GetMarketMapperRevenueShareParams|ValidateRevShareSafety"

# Search for new FeetiersKeeper usages
echo "Searching for FeetiersKeeper usages:"
rg --type go "FeetiersKeeper"

Length of output: 8519

}
21 changes: 15 additions & 6 deletions protocol/x/feetiers/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/dydxprotocol/v4-chain/protocol/lib"
"github.com/dydxprotocol/v4-chain/protocol/x/feetiers/types"
revsharetypes "github.com/dydxprotocol/v4-chain/protocol/x/revshare/types"
)

type (
Expand All @@ -21,6 +22,7 @@ type (
storeKey storetypes.StoreKey
authorities map[string]struct{}
affiliatesKeeper types.AffiliatesKeeper
revShareKeeper types.RevShareKeeper
}
)

Expand Down Expand Up @@ -132,15 +134,22 @@ func (k Keeper) GetLowestMakerFee(ctx sdk.Context) int32 {
return lowestMakerFee
}

func (k Keeper) GetHighestTakerFee(ctx sdk.Context) int32 {
// GetAffiliateRefxereeLowestTakerFee returns the lowest taker fee
// for volume under Max30dRefereeVolumeQuantums.
func (k Keeper) GetAffiliateRefereeLowestTakerFee(ctx sdk.Context) int32 {
feeParams := k.GetPerpetualFeeParams(ctx)

highestTakerFee := int32(math.MinInt32)
for _, tier := range feeParams.Tiers {
if tier.TakerFeePpm > highestTakerFee {
highestTakerFee = tier.TakerFeePpm
// assumes tiers are ordered by absolute volume requirement
if tier.AbsoluteVolumeRequirement < revsharetypes.Max30dRefereeVolumeQuantums {
return tier.TakerFeePpm
} else {
break
}
}

return highestTakerFee
return feeParams.Tiers[types.RefereeStartingFeeTier].TakerFeePpm
}

func (k *Keeper) SetRevShareKeeper(revShareKeeper types.RevShareKeeper) {
k.revShareKeeper = revShareKeeper
}
25 changes: 25 additions & 0 deletions protocol/x/feetiers/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,3 +346,28 @@ func TestGetMaxMakerRebate(t *testing.T) {
})
}
}

func TestGetAffiliateRefereeLowestTakerFee(t *testing.T) {
tests := map[string]struct {
expectedLowestTakerFee int32
feeTiers types.PerpetualFeeParams
}{
"tiers are ordered by absolute volume requirement": {
feeTiers: types.StandardParams(),
expectedLowestTakerFee: 350,
},
}

for name, tc := range tests {
t.Run(name, func(t *testing.T) {
tApp := testapp.NewTestAppBuilder(t).Build()
ctx := tApp.InitChain()
k := tApp.App.FeeTiersKeeper
err := k.SetPerpetualFeeParams(
ctx,
tc.feeTiers,
)
require.NoError(t, err)
})
}
}
37 changes: 37 additions & 0 deletions protocol/x/feetiers/keeper/params.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package keeper

import (
"math"

errorsmod "cosmossdk.io/errors"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/dydxprotocol/v4-chain/protocol/x/feetiers/types"
)
Expand All @@ -27,6 +30,40 @@ func (k Keeper) SetPerpetualFeeParams(
return err
}

lowestMakerFee := int32(math.MaxInt32)
lowestTakerFee := int32(math.MaxInt32)
for _, tier := range params.Tiers {
Copy link
Contributor

@teddyding teddyding Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of duplicating logic let's create GetLowestMaker/TakerFeeFromFeeTiers static methods which take in the fee tiers and call them here. The original GetLowestMaker/TakerFee can call this helper to maintain their signature

if tier.MakerFeePpm < lowestMakerFee {
lowestMakerFee = tier.MakerFeePpm
}
if tier.TakerFeePpm < lowestTakerFee {
lowestTakerFee = tier.TakerFeePpm
}
}
affiliateTiers, err := k.affiliatesKeeper.GetAllAffiliateTiers(ctx)
if err != nil {
return err
}

unconditionalRevShareConfig, err := k.revShareKeeper.GetUnconditionalRevShareConfigParams(ctx)
if err != nil {
return err
}

marketMapperRevShareParams := k.revShareKeeper.GetMarketMapperRevenueShareParams(ctx)
if err != nil {
return err
}

valid := k.revShareKeeper.ValidateRevShareSafety(ctx, affiliateTiers, unconditionalRevShareConfig,
marketMapperRevShareParams, lowestTakerFee, lowestMakerFee)
if !valid {
return errorsmod.Wrapf(
types.ErrRevShareSafetyViolation,
"rev share safety violation",
)
}

store := ctx.KVStore(k.storeKey)
b := k.cdc.MustMarshal(&params)
store.Set([]byte(types.PerpetualFeeParamsKey), b)
Expand Down
5 changes: 5 additions & 0 deletions protocol/x/feetiers/types/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,9 @@ var (
404,
"Authority is invalid",
)
ErrRevShareSafetyViolation = errorsmod.Register(
ModuleName,
405,
"Rev share safety violation",
)
)
25 changes: 22 additions & 3 deletions protocol/x/feetiers/types/expected_keepers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@ package types

import (
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/dydxprotocol/v4-chain/protocol/x/stats/types"
affiliatetypes "github.com/dydxprotocol/v4-chain/protocol/x/affiliates/types"
revsharetypes "github.com/dydxprotocol/v4-chain/protocol/x/revshare/types"
statstypes "github.com/dydxprotocol/v4-chain/protocol/x/stats/types"
)

// StatsKeeper defines the expected stats keeper
type StatsKeeper interface {
GetUserStats(ctx sdk.Context, address string) *types.UserStats
GetGlobalStats(ctx sdk.Context) *types.GlobalStats
GetUserStats(ctx sdk.Context, address string) *statstypes.UserStats
GetGlobalStats(ctx sdk.Context) *statstypes.GlobalStats
}

// VaultKeeper defines the expected vault keeper.
Expand All @@ -19,4 +21,21 @@ type VaultKeeper interface {
// AffiliatesKeeper defines the expected affiliates keeper.
type AffiliatesKeeper interface {
GetReferredBy(ctx sdk.Context, referee string) (string, bool)
GetAllAffiliateTiers(ctx sdk.Context) (affiliatetypes.AffiliateTiers, error)
}

// RevShareKeeper defines the expected revshare keeper.
type RevShareKeeper interface {
GetUnconditionalRevShareConfigParams(ctx sdk.Context) (revsharetypes.UnconditionalRevShareConfig, error)
GetMarketMapperRevenueShareParams(
ctx sdk.Context,
) revsharetypes.MarketMapperRevenueShareParams
ValidateRevShareSafety(
ctx sdk.Context,
affiliateTiers affiliatetypes.AffiliateTiers,
unconditionalRevShareConfig revsharetypes.UnconditionalRevShareConfig,
marketMapperRevShareParams revsharetypes.MarketMapperRevenueShareParams,
lowestTakerFee int32,
lowestMakerFee int32,
) bool
}
1 change: 1 addition & 0 deletions protocol/x/feetiers/types/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (

type FeeTiersKeeper interface {
GetLowestMakerFee(ctx sdk.Context) int32
GetAffiliateRefereeLowestTakerFee(ctx sdk.Context) int32
GetPerpetualFeePpm(ctx sdk.Context, address string, isTaker bool) int32
GetPerpetualFeeParams(
ctx sdk.Context,
Expand Down
11 changes: 10 additions & 1 deletion protocol/x/revshare/keeper/msg_set_marketmapper_revenue_share.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,16 @@ func (k msgServer) SetMarketMapperRevenueShare(
if err != nil {
return nil, err
}
if !k.ValidateRevShareSafety(ctx, affiliateTiers, unconditionalRevShareConfig, msg.Params) {
lowestTakerFeePpm := k.feetiersKeeper.GetAffiliateRefereeLowestTakerFee(ctx)
lowestMakerFeePpm := k.feetiersKeeper.GetLowestMakerFee(ctx)
if !k.ValidateRevShareSafety(
ctx,
affiliateTiers,
unconditionalRevShareConfig,
msg.Params,
lowestTakerFeePpm,
lowestMakerFeePpm,
) {
return nil, errorsmod.Wrapf(
types.ErrRevShareSafetyViolation,
"rev share safety violation",
Expand Down
11 changes: 10 additions & 1 deletion protocol/x/revshare/keeper/msg_update_unconditional_revshare_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,18 @@ func (k msgServer) UpdateUnconditionalRevShareConfig(
if err != nil {
return nil, err
}
lowestTakerFee := k.feetiersKeeper.GetAffiliateRefereeLowestTakerFee(ctx)
lowestMakerFee := k.feetiersKeeper.GetLowestMakerFee(ctx)

marketMapperRevShareParams := k.GetMarketMapperRevenueShareParams(ctx)
if !k.ValidateRevShareSafety(ctx, affiliateTiers, msg.Config, marketMapperRevShareParams) {
if !k.ValidateRevShareSafety(
ctx,
affiliateTiers,
msg.Config,
marketMapperRevShareParams,
lowestTakerFee,
lowestMakerFee,
) {
return nil, errorsmod.Wrapf(
types.ErrRevShareSafetyViolation,
"rev share safety violation",
Expand Down
12 changes: 6 additions & 6 deletions protocol/x/revshare/keeper/revshare.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,18 +124,18 @@ func (k Keeper) SetUnconditionalRevShareConfigParams(ctx sdk.Context, config typ
store.Set([]byte(types.UnconditionalRevShareConfigKey), unconditionalRevShareConfigBytes)
}

// ValidateRevShareSafety roughly checks if the total rev share is valid using the formula below
// highest_affiliate_taker_share * (highest_taker_fee_share_ppm /
// (lowest_maker_fee_share_ppm + highest_taker_fee_share_ppm))
// ValidateRevShareSafety roughly checks if the total rev share is valid using the formula below:
// highest_affiliate_taker_share * (lowest_taker_fee_ppm /
// (lowest_maker_ppm + lowest_taker_fee_ppm))
Copy link
Contributor

@teddyding teddyding Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document that we always expect lowest_maker_ppm to be negative

// + sum(unconditional_rev_shares) + market_mapper_rev_share < 100%
func (k Keeper) ValidateRevShareSafety(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since both totalUnconditionalRevSharePpm and totalMarketMapperRevSharePpm are post maker rebates and taker affiliate rev share, I think we just need:

  • totalUnconditionalRevSharePpm + totalMarketMapperRevSharePpm < 100
  • Highest_affiliate_rev_share[protocol constant of 50%] * lowest_taker_fee - lowest_maker_fee < lowest_taker_fee

Which is actually a lot simpler 😓 Lmk if this makes sense

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me. Made the change!

ctx sdk.Context,
affiliateTiers affiliatetypes.AffiliateTiers,
unconditionalRevShareConfig types.UnconditionalRevShareConfig,
marketMapperRevShareParams types.MarketMapperRevenueShareParams,
lowestTakerFeePpm int32,
lowestMakerFeePpm int32,
) bool {
highestTakerFeeSharePpm := k.feetiersKeeper.GetHighestTakerFee(ctx)
lowestMakerFeeSharePpm := k.feetiersKeeper.GetLowestMakerFee(ctx)
highestTierRevSharePpm := uint32(0)
if len(affiliateTiers.Tiers) > 0 {
highestTierRevSharePpm = affiliateTiers.Tiers[len(affiliateTiers.Tiers)-1].TakerFeeSharePpm
Expand All @@ -148,7 +148,7 @@ func (k Keeper) ValidateRevShareSafety(

totalRevSharePpm := totalUnconditionalRevSharePpm + totalMarketMapperRevSharePpm +
uint32(math.Ceil(float64(highestTierRevSharePpm)*(float64(
highestTakerFeeSharePpm)/float64(lowestMakerFeeSharePpm+highestTakerFeeSharePpm))))
lowestTakerFeePpm)/float64(lowestMakerFeePpm+lowestTakerFeePpm))))
return totalRevSharePpm < lib.OneMillion
}

Expand Down
Loading
Loading