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

[TRA-417] Allocate market mapper revshare for appropriate markets #1773

Merged
merged 10 commits into from
Jun 27, 2024
Merged

[TRA-417] Allocate market mapper revshare for appropriate markets #1773

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e490166
wip
shrenujb Jun 24, 2024
19fbea3
Merge branch 'main' into tra417
shrenujb Jun 24, 2024
0292c2b
Allocate market mapper revshare on every trade
shrenujb Jun 25, 2024
2fbcffa
remove unused code
shrenujb Jun 25, 2024
3fa5de8
fix
shrenujb Jun 25, 2024
bbd93c9
add test case for negative fees
shrenujb Jun 26, 2024
020b8b6
Merge branch 'main' into tra417
shrenujb Jun 26, 2024
6c0d0dc
address comments and add tests
shrenujb Jun 27, 2024
49942c3
address nits and fix tests
shrenujb Jun 27, 2024
d6b1e71
add comment and log for rev share err case
shrenujb Jun 27, 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
1 change: 1 addition & 0 deletions protocol/app/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1030,6 +1030,7 @@ func New(
app.BankKeeper,
app.PerpetualsKeeper,
app.BlockTimeKeeper,
app.RevShareKeeper,
app.IndexerEventManager,
)
subaccountsModule := subaccountsmodule.NewAppModule(
Expand Down
1 change: 1 addition & 0 deletions protocol/testutil/keeper/clob.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ func NewClobKeepersTestContextWithUninitializedMemStore(
bankKeeper,
ks.PerpetualsKeeper,
ks.BlockTimeKeeper,
revShareKeeper,
indexerEventsTransientStoreKey,
true,
)
Expand Down
9 changes: 9 additions & 0 deletions protocol/testutil/keeper/prices.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import (
"fmt"
"testing"

revsharetypes "github.com/dydxprotocol/v4-chain/protocol/x/revshare/types"

"github.com/cosmos/gogoproto/proto"

storetypes "cosmossdk.io/store/types"
Expand Down Expand Up @@ -115,6 +117,13 @@ func CreateTestMarkets(t testing.TB, ctx sdk.Context, k *keeper.Keeper) {
},
})
require.NoError(t, err)

// update all markets to not have revenue share
k.RevShareKeeper.SetMarketMapperRevShareDetails(
ctx,
uint32(i),
revsharetypes.MarketMapperRevShareDetails{ExpirationTs: 0},
)
}
}

Expand Down
1 change: 1 addition & 0 deletions protocol/testutil/keeper/sending.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ func SendingKeepersWithSubaccountsKeeper(t testing.TB, saKeeper types.Subaccount
ks.BankKeeper,
ks.PerpetualsKeeper,
blockTimeKeeper,
revShareKeeper,
transientStoreKey,
true,
)
Expand Down
17 changes: 10 additions & 7 deletions protocol/testutil/keeper/subaccounts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import (
"math/big"
"testing"

revsharekeeper "github.com/dydxprotocol/v4-chain/protocol/x/revshare/keeper"

"github.com/cosmos/gogoproto/proto"

dbm "github.com/cosmos/cosmos-db"
Expand All @@ -30,10 +32,7 @@ import (
"github.com/dydxprotocol/v4-chain/protocol/x/subaccounts/types"
)

func SubaccountsKeepers(
t testing.TB,
msgSenderEnabled bool,
) (
func SubaccountsKeepers(t testing.TB, msgSenderEnabled bool) (
ctx sdk.Context,
keeper *keeper.Keeper,
pricesKeeper *priceskeeper.Keeper,
Expand All @@ -42,6 +41,7 @@ func SubaccountsKeepers(
bankKeeper *bankkeeper.BaseKeeper,
assetsKeeper *asskeeper.Keeper,
blocktimeKeeper *blocktimekeeper.Keeper,
revShareKeeper *revsharekeeper.Keeper,
storeKey storetypes.StoreKey,
) {
var mockTimeProvider *mocks.TimeProvider
Expand All @@ -53,7 +53,7 @@ func SubaccountsKeepers(
transientStoreKey storetypes.StoreKey,
) []GenesisInitializer {
// Define necessary keepers here for unit tests
revShareKeeper, _, _ := createRevShareKeeper(stateStore, db, cdc)
revShareKeeper, _, _ = createRevShareKeeper(stateStore, db, cdc)
pricesKeeper, _, _, mockTimeProvider = createPricesKeeper(
stateStore,
db,
Expand All @@ -77,17 +77,18 @@ func SubaccountsKeepers(
bankKeeper,
perpetualsKeeper,
blocktimeKeeper,
revShareKeeper,
transientStoreKey,
msgSenderEnabled,
)

return []GenesisInitializer{pricesKeeper, perpetualsKeeper, assetsKeeper, keeper}
return []GenesisInitializer{pricesKeeper, perpetualsKeeper, assetsKeeper, revShareKeeper, keeper}
})

// Mock time provider response for market creation.
mockTimeProvider.On("Now").Return(constants.TimeT)

return ctx, keeper, pricesKeeper, perpetualsKeeper, accountKeeper, bankKeeper, assetsKeeper, blocktimeKeeper, storeKey
return ctx, keeper, pricesKeeper, perpetualsKeeper, accountKeeper, bankKeeper, assetsKeeper, blocktimeKeeper, revShareKeeper, storeKey
}

func createSubaccountsKeeper(
Expand All @@ -98,6 +99,7 @@ func createSubaccountsKeeper(
bk types.BankKeeper,
pk *perpskeeper.Keeper,
btk *blocktimekeeper.Keeper,
rsk *revsharekeeper.Keeper,
transientStoreKey storetypes.StoreKey,
msgSenderEnabled bool,
) (*keeper.Keeper, storetypes.StoreKey) {
Expand All @@ -116,6 +118,7 @@ func createSubaccountsKeeper(
bk,
pk,
btk,
rsk,
mockIndexerEventsManager,
)

Expand Down
22 changes: 21 additions & 1 deletion protocol/x/clob/keeper/process_single_match.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,13 +426,33 @@ func (k Keeper) persistMatchedOrders(
)
}

// TODO: get perpetual from perpetualId once and pass it to the functions that need the full
// perpetual object. This will reduce the number of times we need to get the perpetual from the
// keeper.

if err := k.subaccountsKeeper.TransferInsuranceFundPayments(ctx, insuranceFundDelta, perpetualId); err != nil {
return takerUpdateResult, makerUpdateResult, err
}

// Transfer the fee amount from subacounts module to fee collector module account.
bigTotalFeeQuoteQuantums := new(big.Int).Add(bigTakerFeeQuoteQuantums, bigMakerFeeQuoteQuantums)
if err := k.subaccountsKeeper.TransferFeesToFeeCollectorModule(
//if err := k.subaccountsKeeper.TransferFeesToFeeCollectorModule(
// ctx,
// assettypes.AssetUsdc.Id,
// bigTotalFeeQuoteQuantums,
// perpetualId,
//); err != nil {
// return takerUpdateResult, makerUpdateResult, errorsmod.Wrapf(
// types.ErrSubaccountFeeTransferFailed,
// "persistMatchedOrders: subaccounts (%v, %v) updated, but fee transfer (bigFeeQuoteQuantums: %v)"+
// " to fee-collector failed. Err: %v",
// matchWithOrders.MakerOrder.GetSubaccountId(),
// matchWithOrders.TakerOrder.GetSubaccountId(),
// bigTotalFeeQuoteQuantums,
// err,
// )
//}
if err := k.subaccountsKeeper.DistributeFees(
ctx,
assettypes.AssetUsdc.Id,
bigTotalFeeQuoteQuantums,
Expand Down
6 changes: 6 additions & 0 deletions protocol/x/clob/types/expected_keepers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,12 @@ type SubaccountsKeeper interface {
ctx sdk.Context,
perpetualId uint32,
) (sdk.AccAddress, error)
DistributeFees(
ctx sdk.Context,
assetId uint32,
quantums *big.Int,
perpetualId uint32,
) error
}

type AssetsKeeper interface {
Expand Down
4 changes: 2 additions & 2 deletions protocol/x/prices/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ type (
authorities map[string]struct{}
currencyPairIDCache *CurrencyPairIDCache
currencyPairIdCacheInitialized *atomic.Bool
revShareKeeper types.RevShareKeeper
RevShareKeeper types.RevShareKeeper
}
)

Expand All @@ -52,7 +52,7 @@ func NewKeeper(
authorities: lib.UniqueSliceToSet(authorities),
currencyPairIDCache: NewCurrencyPairIDCache(),
currencyPairIdCacheInitialized: &atomic.Bool{}, // Initialized to false
revShareKeeper: revShareKeeper,
RevShareKeeper: revShareKeeper,
}
}

Expand Down
2 changes: 1 addition & 1 deletion protocol/x/prices/keeper/market.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ func (k Keeper) CreateMarket(
metrics.SetMarketPairForTelemetry(marketParam.Id, marketParam.Pair)

// create a new market rev share
k.revShareKeeper.CreateNewMarketRevShare(ctx, marketParam.Id)
k.RevShareKeeper.CreateNewMarketRevShare(ctx, marketParam.Id)

return marketParam, nil
}
Expand Down
7 changes: 7 additions & 0 deletions protocol/x/prices/types/expected_keepers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package types
import (
"context"

"github.com/dydxprotocol/v4-chain/protocol/x/revshare/types"

sdk "github.com/cosmos/cosmos-sdk/types"
)

Expand All @@ -19,4 +21,9 @@ type BankKeeper interface {
// RevShareKeeper defines the expected revshare keeper used for simulations.
type RevShareKeeper interface {
CreateNewMarketRevShare(ctx sdk.Context, marketId uint32)
SetMarketMapperRevShareDetails(
ctx sdk.Context,
marketId uint32,
params types.MarketMapperRevShareDetails,
)
}
27 changes: 27 additions & 0 deletions protocol/x/revshare/keeper/revshare.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,3 +76,30 @@ func (k Keeper) CreateNewMarketRevShare(ctx sdk.Context, marketId uint32) {
}
k.SetMarketMapperRevShareDetails(ctx, marketId, details)
}

func (k Keeper) GetMarketMapperRevenueShareForMarket(ctx sdk.Context, marketId uint32) (
shrenujb marked this conversation as resolved.
Show resolved Hide resolved
address sdk.AccAddress,
revenueSharePpm uint32,
err error,
) {
// get the revenue share details for the market
revShareDetails, err := k.GetMarketMapperRevShareDetails(ctx, marketId)
if err != nil {
return nil, 0, err
}

// check if the rev share details are expired
if revShareDetails.ExpirationTs < uint64(ctx.BlockTime().Unix()) {
return nil, 0, nil
}

// Get revenue share params
revShareParams := k.GetMarketMapperRevenueShareParams(ctx)

revShareAddr, err := sdk.AccAddressFromBech32(revShareParams.Address)
if err != nil {
return nil, 0, err
}

return revShareAddr, revShareParams.RevenueSharePpm, nil
}
Comment on lines +78 to +103
Copy link
Contributor

Choose a reason for hiding this comment

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

New method to retrieve market mapper revenue share

The method GetMarketMapperRevenueShareForMarket is well implemented. It fetches the revenue share details and checks for expiration. However, it returns nil instead of an empty sdk.AccAddress when an error occurs, which could lead to potential issues if not handled properly by the caller.

-		return nil, 0, err
+		return sdk.AccAddress{}, 0, err
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (k Keeper) GetMarketMapperRevenueShareForMarket(ctx sdk.Context, marketId uint32) (
address sdk.AccAddress,
revenueSharePpm uint32,
err error,
) {
// get the revenue share details for the market
revShareDetails, err := k.GetMarketMapperRevShareDetails(ctx, marketId)
if err != nil {
return nil, 0, err
}
// check if the rev share details are expired
if revShareDetails.ExpirationTs < uint64(ctx.BlockTime().Unix()) {
return nil, 0, nil
}
// Get revenue share params
revShareParams := k.GetMarketMapperRevenueShareParams(ctx)
revShareAddr, err := sdk.AccAddressFromBech32(revShareParams.Address)
if err != nil {
return nil, 0, err
}
return revShareAddr, revShareParams.RevenueSharePpm, nil
}
func (k Keeper) GetMarketMapperRevenueShareForMarket(ctx sdk.Context, marketId uint32) (
address sdk.AccAddress,
revenueSharePpm uint32,
err error,
) {
// get the revenue share details for the market
revShareDetails, err := k.GetMarketMapperRevShareDetails(ctx, marketId)
if err != nil {
return sdk.AccAddress{}, 0, err
}
// check if the rev share details are expired
if revShareDetails.ExpirationTs < uint64(ctx.BlockTime().Unix()) {
return sdk.AccAddress{}, 0, nil
}
// Get revenue share params
revShareParams := k.GetMarketMapperRevenueShareParams(ctx)
revShareAddr, err := sdk.AccAddressFromBech32(revShareParams.Address)
if err != nil {
return sdk.AccAddress{}, 0, err
}
return revShareAddr, revShareParams.RevenueSharePpm, nil
}

2 changes: 1 addition & 1 deletion protocol/x/subaccounts/genesis_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func TestGenesis(t *testing.T) {
},
}

ctx, k, _, _, _, _, _, _, _ := keepertest.SubaccountsKeepers(t, true)
ctx, k, _, _, _, _, _, _, _, _ := keepertest.SubaccountsKeepers(t, true)
subaccounts.InitGenesis(ctx, *k, genesisState)
assertSubaccountUpdateEventsInIndexerBlock(t, k, ctx, 2)
got := subaccounts.ExportGenesis(ctx, *k)
Expand Down
2 changes: 1 addition & 1 deletion protocol/x/subaccounts/keeper/grpc_query_collateral_pool_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ func TestQueryCollateralPoolAddress(t *testing.T) {
},
} {
t.Run(testName, func(t *testing.T) {
ctx, keeper, pricesKeeper, perpetualsKeeper, _, _, _, _, _ := keepertest.SubaccountsKeepers(t, true)
ctx, keeper, pricesKeeper, perpetualsKeeper, _, _, _, _, _, _ := keepertest.SubaccountsKeepers(t, true)
keepertest.CreateTestMarkets(t, ctx, pricesKeeper)
keepertest.CreateTestLiquidityTiers(t, ctx, perpetualsKeeper)
keepertest.CreateTestPerpetuals(t, ctx, perpetualsKeeper)
Expand Down
4 changes: 2 additions & 2 deletions protocol/x/subaccounts/keeper/grpc_query_subaccount_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import (
var _ = strconv.IntSize

func TestSubaccountQuerySingle(t *testing.T) {
ctx, keeper, _, _, _, _, _, _, _ := keepertest.SubaccountsKeepers(t, true)
ctx, keeper, _, _, _, _, _, _, _, _ := keepertest.SubaccountsKeepers(t, true)
msgs := createNSubaccount(keeper, ctx, 2, big.NewInt(1_000))
for _, tc := range []struct {
desc string
Expand Down Expand Up @@ -90,7 +90,7 @@ func TestSubaccountQuerySingle(t *testing.T) {
}

func TestSubaccountQueryPaginated(t *testing.T) {
ctx, keeper, _, _, _, _, _, _, _ := keepertest.SubaccountsKeepers(t, true)
ctx, keeper, _, _, _, _, _, _, _, _ := keepertest.SubaccountsKeepers(t, true)
msgs := createNSubaccount(keeper, ctx, 5, big.NewInt(1_000))

request := func(next []byte, offset, limit uint64, total bool) *types.QueryAllSubaccountRequest {
Expand Down
5 changes: 4 additions & 1 deletion protocol/x/subaccounts/keeper/grpc_query_withdrawal_and_transfers_blocked_info_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,10 @@ func TestQueryWithdrawalAndTransfersBlockedInfo(t *testing.T) {
},
} {
t.Run(testName, func(t *testing.T) {
ctx, keeper, pricesKeeper, perpetualsKeeper, _, _, _, blocktimeKeeper, _ := keepertest.SubaccountsKeepers(t, true)
ctx, keeper, pricesKeeper, perpetualsKeeper, _, _, _, blocktimeKeeper, _, _ := keepertest.SubaccountsKeepers(
t,
true,
)
keepertest.CreateTestMarkets(t, ctx, pricesKeeper)
keepertest.CreateTestLiquidityTiers(t, ctx, perpetualsKeeper)
keepertest.CreateTestPerpetuals(t, ctx, perpetualsKeeper)
Expand Down
3 changes: 3 additions & 0 deletions protocol/x/subaccounts/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ type (
bankKeeper types.BankKeeper
perpetualsKeeper types.PerpetualsKeeper
blocktimeKeeper types.BlocktimeKeeper
revShareKeeper types.RevShareKeeper
indexerEventManager indexer_manager.IndexerEventManager
}
)
Expand All @@ -30,6 +31,7 @@ func NewKeeper(
bankKeeper types.BankKeeper,
perpetualsKeeper types.PerpetualsKeeper,
blocktimeKeeper types.BlocktimeKeeper,
revShareKeeper types.RevShareKeeper,
indexerEventManager indexer_manager.IndexerEventManager,
) *Keeper {
return &Keeper{
Expand All @@ -39,6 +41,7 @@ func NewKeeper(
bankKeeper: bankKeeper,
perpetualsKeeper: perpetualsKeeper,
blocktimeKeeper: blocktimeKeeper,
revShareKeeper: revShareKeeper,
indexerEventManager: indexerEventManager,
}
}
Expand Down
7 changes: 5 additions & 2 deletions protocol/x/subaccounts/keeper/negative_tnc_subaccount_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,10 @@ func TestGetSetNegativeTncSubaccountSeenAtBlock(t *testing.T) {
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
// Setup keeper state and test parameters.
ctx, subaccountsKeeper, pricesKeeper, perpetualsKeeper, _, _, _, _, _ := keepertest.SubaccountsKeepers(t, true)
ctx, subaccountsKeeper, pricesKeeper, perpetualsKeeper, _, _, _, _, _, _ := keepertest.SubaccountsKeepers(
t,
true,
)
keepertest.CreateTestMarkets(t, ctx, pricesKeeper)
keepertest.CreateTestLiquidityTiers(t, ctx, perpetualsKeeper)
keepertest.CreateTestPerpetuals(t, ctx, perpetualsKeeper)
Expand All @@ -247,7 +250,7 @@ func TestGetSetNegativeTncSubaccountSeenAtBlock(t *testing.T) {

func TestGetSetNegativeTncSubaccountSeenAtBlock_PanicsOnDecreasingBlock(t *testing.T) {
// Setup keeper state and test parameters.
ctx, subaccountsKeeper, pricesKeeper, perpetualsKeeper, _, _, _, _, _ := keepertest.SubaccountsKeepers(t, true)
ctx, subaccountsKeeper, pricesKeeper, perpetualsKeeper, _, _, _, _, _, _ := keepertest.SubaccountsKeepers(t, true)
keepertest.CreateTestMarkets(t, ctx, pricesKeeper)
keepertest.CreateTestLiquidityTiers(t, ctx, perpetualsKeeper)
keepertest.CreateTestPerpetuals(t, ctx, perpetualsKeeper)
Expand Down
6 changes: 3 additions & 3 deletions protocol/x/subaccounts/keeper/safety_heap_state_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (

func TestGetSetSubaccountAtIndex(t *testing.T) {
// Setup keeper state and test parameters.
ctx, subaccountsKeeper, _, _, _, _, _, _, _ := keepertest.SubaccountsKeepers(t, false)
ctx, subaccountsKeeper, _, _, _, _, _, _, _, _ := keepertest.SubaccountsKeepers(t, false)

// Write a couple of subaccounts to the store.
store := subaccountsKeeper.GetSafetyHeapStore(ctx, 0, types.Long)
Expand Down Expand Up @@ -60,7 +60,7 @@ func TestGetSetSubaccountAtIndex(t *testing.T) {

func TestGetSetSubaccountHeapIndex(t *testing.T) {
// Setup keeper state and test parameters.
ctx, subaccountsKeeper, _, _, _, _, _, _, _ := keepertest.SubaccountsKeepers(t, false)
ctx, subaccountsKeeper, _, _, _, _, _, _, _, _ := keepertest.SubaccountsKeepers(t, false)

// Write a couple of subaccounts to the store.
store := subaccountsKeeper.GetSafetyHeapStore(ctx, 0, types.Long)
Expand Down Expand Up @@ -109,7 +109,7 @@ func TestGetSetSubaccountHeapIndex(t *testing.T) {

func TestGetSetSafetyHeapLength(t *testing.T) {
// Setup keeper state and test parameters.
ctx, subaccountsKeeper, _, _, _, _, _, _, _ := keepertest.SubaccountsKeepers(t, false)
ctx, subaccountsKeeper, _, _, _, _, _, _, _, _ := keepertest.SubaccountsKeepers(t, false)

// Write a couple of subaccounts to the store.
store := subaccountsKeeper.GetSafetyHeapStore(ctx, 0, types.Long)
Expand Down
Loading
Loading