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

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

Merged
merged 10 commits into from
Jun 27, 2024
1 change: 1 addition & 0 deletions protocol/app/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -1029,6 +1029,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
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
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
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
26 changes: 19 additions & 7 deletions protocol/testutil/keeper/subaccounts.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package keeper
import (
"testing"

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

"github.com/cosmos/gogoproto/proto"

dbm "github.com/cosmos/cosmos-db"
Expand All @@ -27,10 +29,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 @@ -39,6 +38,7 @@ func SubaccountsKeepers(
bankKeeper *bankkeeper.BaseKeeper,
assetsKeeper *asskeeper.Keeper,
blocktimeKeeper *blocktimekeeper.Keeper,
revShareKeeper *revsharekeeper.Keeper,
storeKey storetypes.StoreKey,
) {
var mockTimeProvider *mocks.TimeProvider
Expand All @@ -50,7 +50,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 @@ -74,17 +74,27 @@ 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 @@ -95,6 +105,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 @@ -113,6 +124,7 @@ func createSubaccountsKeeper(
bk,
pk,
btk,
rsk,
mockIndexerEventsManager,
)

Expand Down
8 changes: 6 additions & 2 deletions protocol/x/clob/keeper/process_single_match.go
Original file line number Diff line number Diff line change
Expand Up @@ -427,13 +427,17 @@ 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.
// Distribute the fee amount from subacounts module to fee collector and rev share accounts
bigTotalFeeQuoteQuantums := new(big.Int).Add(bigTakerFeeQuoteQuantums, bigMakerFeeQuoteQuantums)
if err := k.subaccountsKeeper.TransferFeesToFeeCollectorModule(
if err := k.subaccountsKeeper.DistributeFees(
ctx,
assettypes.AssetUsdc.Id,
bigTotalFeeQuoteQuantums,
Expand Down
7 changes: 6 additions & 1 deletion protocol/x/clob/types/expected_keepers.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@ type SubaccountsKeeper interface {
perpetualId uint32,
blockHeight uint32,
) error
TransferFeesToFeeCollectorModule(ctx sdk.Context, assetId uint32, amount *big.Int, perpetualId uint32) error
TransferInsuranceFundPayments(
ctx sdk.Context,
amount *big.Int,
Expand All @@ -73,6 +72,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
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
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
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,
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -40,5 +40,5 @@ func TestQueryMarketMapperRevShareDetailsFailure(t *testing.T) {
MarketId: 42,
},
)
require.ErrorContains(t, err, "MarketMapperRevShareDetails not found for marketId: 42")
require.ErrorIs(t, err, types.ErrMarketMapperRevShareDetailsNotFound)
}
31 changes: 28 additions & 3 deletions protocol/x/revshare/keeper/revshare.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
package keeper

import (
"fmt"

"cosmossdk.io/store/prefix"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/dydxprotocol/v4-chain/protocol/lib"
Expand Down Expand Up @@ -58,7 +56,7 @@ func (k Keeper) GetMarketMapperRevShareDetails(
store := prefix.NewStore(ctx.KVStore(k.storeKey), []byte(types.MarketMapperRevSharePrefix))
b := store.Get(lib.Uint32ToKey(marketId))
if b == nil {
return params, fmt.Errorf("MarketMapperRevShareDetails not found for marketId: %d", marketId)
return params, types.ErrMarketMapperRevShareDetailsNotFound
}
k.cdc.MustUnmarshal(b, &params)
return params, nil
Expand All @@ -76,3 +74,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
}

94 changes: 93 additions & 1 deletion protocol/x/revshare/keeper/revshare_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ package keeper_test

import (
"testing"
"time"

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

"github.com/dydxprotocol/v4-chain/protocol/testutil/constants"

Expand Down Expand Up @@ -43,7 +46,7 @@ func TestGetMarketMapperRevShareDetailsFailure(t *testing.T) {

// Get the rev share details for non-existent market
_, err := k.GetMarketMapperRevShareDetails(ctx, 42)
require.ErrorContains(t, err, "MarketMapperRevShareDetails not found for marketId: 42")
require.ErrorIs(t, err, types.ErrMarketMapperRevShareDetailsNotFound)
}

func TestCreateNewMarketRevShare(t *testing.T) {
Expand Down Expand Up @@ -75,3 +78,92 @@ func TestCreateNewMarketRevShare(t *testing.T) {
expectedExpirationTs := ctx.BlockTime().Unix() + 240*24*60*60
require.Equal(t, details.ExpirationTs, uint64(expectedExpirationTs))
}

func TestGetMarketMapperRevenueShareForMarket(t *testing.T) {
tests := map[string]struct {
revShareParams types.MarketMapperRevenueShareParams
marketId uint32
expirationDelta int64
setRevShareDetails bool

// expected
expectedMarketMapperAddr sdk.AccAddress
expectedRevenueSharePpm uint32
expectedErr error
}{
"valid market": {
revShareParams: types.MarketMapperRevenueShareParams{
Address: constants.AliceAccAddress.String(),
RevenueSharePpm: 100_000, // 10%
ValidDays: 0,
},
marketId: 42,
expirationDelta: 10,
setRevShareDetails: true,

expectedMarketMapperAddr: constants.AliceAccAddress,
expectedRevenueSharePpm: 100_000,
},
"invalid market": {
revShareParams: types.MarketMapperRevenueShareParams{
Address: constants.AliceAccAddress.String(),
RevenueSharePpm: 100_000, // 10%
ValidDays: 0,
},
marketId: 42,
setRevShareDetails: false,

expectedErr: types.ErrMarketMapperRevShareDetailsNotFound,
},
// TODO (TRA-455): investigate why tApp blocktime doesn't translate to ctx.BlockTime()
//"expired market rev share": {
// revShareParams: types.MarketMapperRevenueShareParams{
// Address: constants.AliceAccAddress.String(),
// RevenueSharePpm: 100_000, // 10%
// ValidDays: 0,
// },
// marketId: 42,
// expirationDelta: -10,
// setRevShareDetails: true,
//
// expectedMarketMapperAddr: nil,
// expectedRevenueSharePpm: 0,
//},
}

for name, tc := range tests {
t.Run(
name, func(t *testing.T) {
tApp := testapp.NewTestAppBuilder(t).Build()
ctx := tApp.InitChain()
k := tApp.App.RevShareKeeper
tApp.AdvanceToBlock(
2, testapp.AdvanceToBlockOptions{BlockTime: time.Now()},
)

// Set base rev share params
err := k.SetMarketMapperRevenueShareParams(ctx, tc.revShareParams)
require.NoError(t, err)

// Set market rev share details
if tc.setRevShareDetails {
k.SetMarketMapperRevShareDetails(
ctx, tc.marketId, types.MarketMapperRevShareDetails{
ExpirationTs: uint64(ctx.BlockTime().Unix() + tc.expirationDelta),
},
)
}

// Get the revenue share for the market
marketMapperAddr, revenueSharePpm, err := k.GetMarketMapperRevenueShareForMarket(ctx, tc.marketId)
if tc.expectedErr != nil {
require.ErrorIs(t, err, tc.expectedErr)
} else {
require.NoError(t, err)
require.Equal(t, tc.expectedMarketMapperAddr, marketMapperAddr)
require.Equal(t, tc.expectedRevenueSharePpm, revenueSharePpm)
}
},
)
}
}
6 changes: 6 additions & 0 deletions protocol/x/revshare/types/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,10 @@ var (
2,
"invalid revenue share ppm",
)

ErrMarketMapperRevShareDetailsNotFound = errorsmod.Register(
ModuleName,
3,
"MarketMapperRevShareDetails not found for marketId",
)
)
Loading
Loading