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.

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

[OTE-788] Update revshare safety #2284

Merged
merged 18 commits into from
Sep 24, 2024
46 changes: 25 additions & 21 deletions protocol/app/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -949,26 +949,43 @@ func New(
)
affiliatesModule := affiliatesmodule.NewAppModule(appCodec, app.AffiliatesKeeper)

app.MarketMapKeeper = *marketmapmodulekeeper.NewKeeper(
runtime.NewKVStoreService(keys[marketmapmoduletypes.StoreKey]),
appCodec,
authtypes.NewModuleAddress(govtypes.ModuleName),
)

marketmapModule := marketmapmodule.NewAppModule(appCodec, &app.MarketMapKeeper)

app.FeeTiersKeeper = feetiersmodulekeeper.NewKeeper(
appCodec,
app.StatsKeeper,
app.AffiliatesKeeper,
keys[feetiersmoduletypes.StoreKey],
// set the governance and delaymsg module accounts as the authority for conducting upgrades
[]string{
lib.GovModuleAddress.String(),
delaymsgmoduletypes.ModuleAddress.String(),
},
)
feeTiersModule := feetiersmodule.NewAppModule(appCodec, app.FeeTiersKeeper)

app.AffiliatesKeeper.SetFeetiersKeeper(app.FeeTiersKeeper)

app.RevShareKeeper = *revsharemodulekeeper.NewKeeper(
appCodec,
keys[revsharemoduletypes.StoreKey],
[]string{
lib.GovModuleAddress.String(),
},
app.AffiliatesKeeper,
*app.FeeTiersKeeper,
)
revShareModule := revsharemodule.NewAppModule(appCodec, app.RevShareKeeper)

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

app.MarketMapKeeper = *marketmapmodulekeeper.NewKeeper(
runtime.NewKVStoreService(keys[marketmapmoduletypes.StoreKey]),
appCodec,
authtypes.NewModuleAddress(govtypes.ModuleName),
)

marketmapModule := marketmapmodule.NewAppModule(appCodec, &app.MarketMapKeeper)
app.FeeTiersKeeper.SetRevShareKeeper(app.RevShareKeeper)

app.PricesKeeper = *pricesmodulekeeper.NewKeeper(
appCodec,
Expand Down Expand Up @@ -1041,19 +1058,6 @@ func New(
)
perpetualsModule := perpetualsmodule.NewAppModule(appCodec, app.PerpetualsKeeper)

app.FeeTiersKeeper = feetiersmodulekeeper.NewKeeper(
appCodec,
app.StatsKeeper,
app.AffiliatesKeeper,
keys[feetiersmoduletypes.StoreKey],
// set the governance and delaymsg module accounts as the authority for conducting upgrades
[]string{
lib.GovModuleAddress.String(),
delaymsgmoduletypes.ModuleAddress.String(),
},
)
feeTiersModule := feetiersmodule.NewAppModule(appCodec, app.FeeTiersKeeper)

app.VestKeeper = *vestmodulekeeper.NewKeeper(
appCodec,
keys[vestmoduletypes.StoreKey],
Expand Down
4 changes: 3 additions & 1 deletion protocol/testutil/keeper/assets.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,9 @@ func AssetsKeepers(
stakingKeeper,
)
affiliatesKeeper, _ := createAffiliatesKeeper(stateStore, db, cdc, statsKeeper, transientStoreKey, msgSenderEnabled)
revShareKeeper, _, _ := createRevShareKeeper(stateStore, db, cdc, affiliatesKeeper)
vaultKeeper, _ := createVaultKeeper(stateStore, db, cdc, transientStoreKey)
feetiersKeeper, _ := createFeeTiersKeeper(stateStore, statsKeeper, vaultKeeper, affiliatesKeeper, db, cdc)
revShareKeeper, _, _ := createRevShareKeeper(stateStore, db, cdc, affiliatesKeeper, feetiersKeeper)
marketMapKeeper, _ := createMarketMapKeeper(stateStore, db, cdc)
pricesKeeper, _, _, mockTimeProvider = createPricesKeeper(
stateStore,
Expand Down
33 changes: 18 additions & 15 deletions protocol/testutil/keeper/clob.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,24 @@ func NewClobKeepersTestContextWithUninitializedMemStore(
)
ks.AffiliatesKeeper, _ = createAffiliatesKeeper(stateStore, db, cdc, ks.StatsKeeper,
indexerEventsTransientStoreKey, true)
revShareKeeper, _, _ := createRevShareKeeper(stateStore, db, cdc, ks.AffiliatesKeeper)
ks.VaultKeeper, _ = createVaultKeeper(
stateStore,
db,
cdc,
indexerEventsTransientStoreKey,
)
ks.FeeTiersKeeper, _ = createFeeTiersKeeper(
stateStore,
ks.StatsKeeper,
ks.VaultKeeper,
ks.AffiliatesKeeper,
db,
cdc,
)
revShareKeeper, _, _ := createRevShareKeeper(stateStore, db, cdc, ks.AffiliatesKeeper, ks.FeeTiersKeeper)
ks.FeeTiersKeeper.SetRevShareKeeper(revShareKeeper)
ks.AffiliatesKeeper.SetRevShareKeeper(revShareKeeper)
ks.AffiliatesKeeper.SetFeetiersKeeper(ks.FeeTiersKeeper)
ks.MarketMapKeeper, _ = createMarketMapKeeper(stateStore, db, cdc)
ks.PricesKeeper, _, _, mockTimeProvider = createPricesKeeper(
stateStore,
Expand Down Expand Up @@ -138,20 +155,6 @@ func NewClobKeepersTestContextWithUninitializedMemStore(
true,
)
ks.BlockTimeKeeper, _ = createBlockTimeKeeper(stateStore, db, cdc)
ks.VaultKeeper, _ = createVaultKeeper(
stateStore,
db,
cdc,
indexerEventsTransientStoreKey,
)
ks.FeeTiersKeeper, _ = createFeeTiersKeeper(
stateStore,
ks.StatsKeeper,
ks.VaultKeeper,
ks.AffiliatesKeeper,
db,
cdc,
)
ks.RewardsKeeper, _ = createRewardsKeeper(
stateStore,
ks.AssetsKeeper,
Expand Down
30 changes: 15 additions & 15 deletions protocol/testutil/keeper/listing.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,21 @@ func ListingKeepers(
stakingKeeper,
)
affiliatesKeeper, _ := createAffiliatesKeeper(stateStore, db, cdc, statsKeeper, transientStoreKey, true)
revShareKeeper, _, _ := createRevShareKeeper(stateStore, db, cdc, affiliatesKeeper)
vaultKeeper, _ := createVaultKeeper(
stateStore,
db,
cdc,
transientStoreKey,
)
feeTiersKeeper, _ := createFeeTiersKeeper(
stateStore,
statsKeeper,
vaultKeeper,
affiliatesKeeper,
db,
cdc,
)
revShareKeeper, _, _ := createRevShareKeeper(stateStore, db, cdc, affiliatesKeeper, feeTiersKeeper)
marketMapKeeper, _ = createMarketMapKeeper(stateStore, db, cdc)
pricesKeeper, _, _, mockTimeProvider = createPricesKeeper(
stateStore,
Expand Down Expand Up @@ -102,20 +116,6 @@ func ListingKeepers(
)

blockTimeKeeper, _ := createBlockTimeKeeper(stateStore, db, cdc)
vaultKeeper, _ := createVaultKeeper(
stateStore,
db,
cdc,
transientStoreKey,
)
feeTiersKeeper, _ := createFeeTiersKeeper(
stateStore,
statsKeeper,
vaultKeeper,
affiliatesKeeper,
db,
cdc,
)
rewardsKeeper, _ := createRewardsKeeper(
stateStore,
assetsKeeper,
Expand Down
4 changes: 3 additions & 1 deletion protocol/testutil/keeper/perpetuals.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,9 @@ func PerpetualsKeepersWithClobHelpers(
stakingKeeper,
)
affiliatesKeeper, _ := createAffiliatesKeeper(stateStore, db, cdc, statsKeeper, transientStoreKey, true)
revShareKeeper, _, _ := createRevShareKeeper(stateStore, db, cdc, affiliatesKeeper)
vaultKeeper, _ := createVaultKeeper(stateStore, db, cdc, transientStoreKey)
feetiersKeeper, _ := createFeeTiersKeeper(stateStore, statsKeeper, vaultKeeper, affiliatesKeeper, db, cdc)
revShareKeeper, _, _ := createRevShareKeeper(stateStore, db, cdc, affiliatesKeeper, feetiersKeeper)
pc.MarketMapKeeper, _ = createMarketMapKeeper(stateStore, db, cdc)
pc.PricesKeeper, _, pc.IndexPriceCache, pc.MockTimeProvider = createPricesKeeper(
stateStore,
Expand Down
4 changes: 3 additions & 1 deletion protocol/testutil/keeper/prices.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,9 @@ func PricesKeepers(t testing.TB) (
stakingKeeper,
)
affiliatesKeeper, _ := createAffiliatesKeeper(stateStore, db, cdc, statsKeeper, transientStoreKey, true)
revShareKeeper, _, _ = createRevShareKeeper(stateStore, db, cdc, affiliatesKeeper)
vaultKeeper, _ := createVaultKeeper(stateStore, db, cdc, transientStoreKey)
feetiersKeeper, _ := createFeeTiersKeeper(stateStore, statsKeeper, vaultKeeper, affiliatesKeeper, db, cdc)
revShareKeeper, _, _ = createRevShareKeeper(stateStore, db, cdc, affiliatesKeeper, feetiersKeeper)
marketMapKeeper, _ = createMarketMapKeeper(stateStore, db, cdc)
// Define necessary keepers here for unit tests
keeper, storeKey, indexPriceCache, mockTimeProvider =
Expand Down
7 changes: 6 additions & 1 deletion protocol/testutil/keeper/revshare.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
"github.com/dydxprotocol/v4-chain/protocol/lib"
affiliateskeeper "github.com/dydxprotocol/v4-chain/protocol/x/affiliates/keeper"
feetierskeeper "github.com/dydxprotocol/v4-chain/protocol/x/feetiers/keeper"
"github.com/dydxprotocol/v4-chain/protocol/x/revshare/types"

storetypes "cosmossdk.io/store/types"
Expand Down Expand Up @@ -54,8 +55,10 @@ func RevShareKeepers(t testing.TB) (
stakingKeeper,
)
affiliatesKeeper, _ := createAffiliatesKeeper(stateStore, db, cdc, statsKeeper, transientStoreKey, true)
vaultKeeper, _ := createVaultKeeper(stateStore, db, cdc, transientStoreKey)
feetiersKeeper, _ := createFeeTiersKeeper(stateStore, statsKeeper, vaultKeeper, affiliatesKeeper, db, cdc)
keeper, storeKey, mockTimeProvider =
createRevShareKeeper(stateStore, db, cdc, affiliatesKeeper)
createRevShareKeeper(stateStore, db, cdc, affiliatesKeeper, feetiersKeeper)

return []GenesisInitializer{keeper}
},
Expand All @@ -69,6 +72,7 @@ func createRevShareKeeper(
db *dbm.MemDB,
cdc *codec.ProtoCodec,
affiliatesKeeper *affiliateskeeper.Keeper,
feetiersKeeper *feetierskeeper.Keeper,
) (
*keeper.Keeper,
storetypes.StoreKey,
Expand All @@ -83,6 +87,7 @@ func createRevShareKeeper(
lib.GovModuleAddress.String(),
},
*affiliatesKeeper,
*feetiersKeeper,
)

return k, storeKey, mockTimeProvider
Expand Down
24 changes: 12 additions & 12 deletions protocol/testutil/keeper/rewards.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,18 +66,6 @@ func RewardsKeepers(
stakingKeeper,
)
affiliatesKeeper, _ := createAffiliatesKeeper(stateStore, db, cdc, statsKeeper, transientStoreKey, true)
revShareKeeper, _, _ := createRevShareKeeper(stateStore, db, cdc, affiliatesKeeper)
marketMapKeeper, _ := createMarketMapKeeper(stateStore, db, cdc)
pricesKeeper, _, _, _ = createPricesKeeper(stateStore, db, cdc, transientStoreKey, revShareKeeper, marketMapKeeper)
// Mock time provider response for market creation.
assetsKeeper, _ = createAssetsKeeper(
stateStore,
db,
cdc,
pricesKeeper,
transientStoreKey,
true,
)
vaultKeeper, _ := createVaultKeeper(
stateStore,
db,
Expand All @@ -92,6 +80,18 @@ func RewardsKeepers(
db,
cdc,
)
revShareKeeper, _, _ := createRevShareKeeper(stateStore, db, cdc, affiliatesKeeper, feetiersKeeper)
marketMapKeeper, _ := createMarketMapKeeper(stateStore, db, cdc)
pricesKeeper, _, _, _ = createPricesKeeper(stateStore, db, cdc, transientStoreKey, revShareKeeper, marketMapKeeper)
// Mock time provider response for market creation.
assetsKeeper, _ = createAssetsKeeper(
stateStore,
db,
cdc,
pricesKeeper,
transientStoreKey,
true,
)
rewardsKeeper, storeKey = createRewardsKeeper(
stateStore,
assetsKeeper,
Expand Down
4 changes: 3 additions & 1 deletion protocol/testutil/keeper/sending.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,9 @@ func SendingKeepersWithSubaccountsKeeper(t testing.TB, saKeeper types.Subaccount
stakingKeeper,
)
affiliatesKeeper, _ := createAffiliatesKeeper(stateStore, db, cdc, statsKeeper, transientStoreKey, true)
revShareKeeper, _, _ := createRevShareKeeper(stateStore, db, cdc, affiliatesKeeper)
vaultKeeper, _ := createVaultKeeper(stateStore, db, cdc, transientStoreKey)
feetiersKeeper, _ := createFeeTiersKeeper(stateStore, statsKeeper, vaultKeeper, affiliatesKeeper, db, cdc)
revShareKeeper, _, _ := createRevShareKeeper(stateStore, db, cdc, affiliatesKeeper, feetiersKeeper)
marketMapKeeper, _ := createMarketMapKeeper(stateStore, db, cdc)
ks.PricesKeeper, _, _, mockTimeProvider = createPricesKeeper(
stateStore,
Expand Down
4 changes: 3 additions & 1 deletion protocol/testutil/keeper/subaccounts.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,9 @@ func SubaccountsKeepers(t testing.TB, msgSenderEnabled bool) (
stakingKeeper,
)
affiliatesKeeper, _ = createAffiliatesKeeper(stateStore, db, cdc, statsKeeper, transientStoreKey, true)
revShareKeeper, _, _ = createRevShareKeeper(stateStore, db, cdc, affiliatesKeeper)
vaultKeeper, _ := createVaultKeeper(stateStore, db, cdc, transientStoreKey)
feetiersKeeper, _ := createFeeTiersKeeper(stateStore, statsKeeper, vaultKeeper, affiliatesKeeper, db, cdc)
revShareKeeper, _, _ = createRevShareKeeper(stateStore, db, cdc, affiliatesKeeper, feetiersKeeper)
affiliatesKeeper.SetRevShareKeeper(revShareKeeper)
marketMapKeeper, _ := createMarketMapKeeper(stateStore, db, cdc)
pricesKeeper, _, _, mockTimeProvider = createPricesKeeper(
Expand Down
5 changes: 5 additions & 0 deletions protocol/x/affiliates/keeper/keeper.go
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 @@ -282,6 +283,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
42 changes: 2 additions & 40 deletions protocol/x/affiliates/keeper/msg_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"context"
"errors"

errorsmod "cosmossdk.io/errors"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/dydxprotocol/v4-chain/protocol/x/affiliates/types"
)
Expand Down Expand Up @@ -38,25 +37,8 @@ func (k msgServer) UpdateAffiliateTiers(ctx context.Context,
}

sdkCtx := sdk.UnwrapSDKContext(ctx)
unconditionalRevShareConfig, err := k.revShareKeeper.GetUnconditionalRevShareConfigParams(sdkCtx)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not needed anymore since we check only the max affiliate rev share cap

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need a reference to revShareKeeper on affiliateKeeper?

if err != nil {
return nil, err
}
marketMapperRevShareParams := k.revShareKeeper.GetMarketMapperRevenueShareParams(sdkCtx)
affiliateWhitelist, err := k.GetAffiliateWhitelist(sdkCtx)
if err != nil {
return nil, err
}

if !k.revShareKeeper.ValidateRevShareSafety(msg.Tiers,
unconditionalRevShareConfig, marketMapperRevShareParams, affiliateWhitelist) {
return nil, errorsmod.Wrapf(
types.ErrRevShareSafetyViolation,
"rev share safety violation",
)
}

err = k.Keeper.UpdateAffiliateTiers(sdkCtx, msg.Tiers)
err := k.Keeper.UpdateAffiliateTiers(sdkCtx, msg.Tiers)
if err != nil {
return nil, err
}
Expand All @@ -70,27 +52,7 @@ func (k msgServer) UpdateAffiliateWhitelist(ctx context.Context,
return nil, errors.New("invalid authority")
}

sdkCtx := sdk.UnwrapSDKContext(ctx)
unconditionalRevShareConfig, err := k.revShareKeeper.GetUnconditionalRevShareConfigParams(sdkCtx)
if err != nil {
return nil, err
}
marketMapperRevShareParams := k.revShareKeeper.GetMarketMapperRevenueShareParams(sdkCtx)

affiliateTiers, err := k.Keeper.GetAllAffiliateTiers(sdkCtx)
if err != nil {
return nil, err
}

if !k.revShareKeeper.ValidateRevShareSafety(affiliateTiers,
unconditionalRevShareConfig, marketMapperRevShareParams, msg.Whitelist) {
return nil, errorsmod.Wrapf(
types.ErrRevShareSafetyViolation,
"rev share safety violation",
)
}

err = k.Keeper.SetAffiliateWhitelist(sdk.UnwrapSDKContext(ctx), msg.Whitelist)
err := k.Keeper.SetAffiliateWhitelist(sdk.UnwrapSDKContext(ctx), msg.Whitelist)
if err != nil {
return nil, err
}
Expand Down
10 changes: 8 additions & 2 deletions protocol/x/affiliates/types/expected_keepers.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,15 @@ type RevShareKeeper interface {
ctx sdk.Context,
) revsharetypes.MarketMapperRevenueShareParams
ValidateRevShareSafety(
affiliateTiers AffiliateTiers,
ctx sdk.Context,
unconditionalRevShareConfig revsharetypes.UnconditionalRevShareConfig,
marketMapperRevShareParams revsharetypes.MarketMapperRevenueShareParams,
affiliateWhitelist AffiliateWhitelist,
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

}
Loading
Loading