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-617] Integrate PML and Megavault #2331

Merged
merged 13 commits into from
Sep 25, 2024
Merged

[TRA-617] Integrate PML and Megavault #2331

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
25d99e0
Add TransferToVault keeper method
shrenujb Sep 23, 2024
a03943b
Merge branch 'main' into tra617
shrenujb Sep 23, 2024
c6bb787
change name
shrenujb Sep 23, 2024
3c6074a
fix merge
shrenujb Sep 23, 2024
d40ab11
Integrate PML and Megavault
shrenujb Sep 24, 2024
84f1ebb
indexer gen
shrenujb Sep 24, 2024
e1d0d6c
fix lint and pregenesis json
shrenujb Sep 24, 2024
a2bcb7b
fix tests
shrenujb Sep 24, 2024
68f7397
fix new vault deposit amount
shrenujb Sep 25, 2024
2e94210
address comments and fix tests
shrenujb Sep 25, 2024
f82e1a6
fix some tests
shrenujb Sep 25, 2024
ab9aebd
change func name
shrenujb Sep 25, 2024
9aa85a3
Merge branch 'main' into tra617
shrenujb Sep 25, 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
19 changes: 18 additions & 1 deletion indexer/packages/v4-protos/src/codegen/dydxprotocol/listing/genesis.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { ListingVaultDepositParams, ListingVaultDepositParamsSDKType } from "./params";
import * as _m0 from "protobufjs/minimal";
import { DeepPartial } from "../../helpers";
/** GenesisState defines `x/listing`'s genesis state. */
Expand All @@ -8,6 +9,9 @@ export interface GenesisState {
* listed
*/
hardCapForMarkets: number;
/** listing_vault_deposit_params is the params for PML megavault deposits */

listingVaultDepositParams?: ListingVaultDepositParams;
}
/** GenesisState defines `x/listing`'s genesis state. */

Expand All @@ -17,11 +21,15 @@ export interface GenesisStateSDKType {
* listed
*/
hard_cap_for_markets: number;
/** listing_vault_deposit_params is the params for PML megavault deposits */

listing_vault_deposit_params?: ListingVaultDepositParamsSDKType;
}

function createBaseGenesisState(): GenesisState {
return {
hardCapForMarkets: 0
hardCapForMarkets: 0,
listingVaultDepositParams: undefined
};
}

Expand All @@ -31,6 +39,10 @@ export const GenesisState = {
writer.uint32(8).uint32(message.hardCapForMarkets);
}

if (message.listingVaultDepositParams !== undefined) {
ListingVaultDepositParams.encode(message.listingVaultDepositParams, writer.uint32(18).fork()).ldelim();
}

return writer;
},

Expand All @@ -47,6 +59,10 @@ export const GenesisState = {
message.hardCapForMarkets = reader.uint32();
break;

case 2:
message.listingVaultDepositParams = ListingVaultDepositParams.decode(reader, reader.uint32());
break;

default:
reader.skipType(tag & 7);
break;
Expand All @@ -59,6 +75,7 @@ export const GenesisState = {
fromPartial(object: DeepPartial<GenesisState>): GenesisState {
const message = createBaseGenesisState();
message.hardCapForMarkets = object.hardCapForMarkets ?? 0;
message.listingVaultDepositParams = object.listingVaultDepositParams !== undefined && object.listingVaultDepositParams !== null ? ListingVaultDepositParams.fromPartial(object.listingVaultDepositParams) : undefined;
return message;
}

Expand Down
7 changes: 7 additions & 0 deletions proto/dydxprotocol/listing/genesis.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,18 @@
syntax = "proto3";
package dydxprotocol.listing;

import "gogoproto/gogo.proto";
import "dydxprotocol/listing/params.proto";

option go_package = "github.com/dydxprotocol/v4-chain/protocol/x/listing/types";

// GenesisState defines `x/listing`'s genesis state.
message GenesisState {
// hard_cap_for_markets is the hard cap for the number of markets that can be
// listed
uint32 hard_cap_for_markets = 1;

// listing_vault_deposit_params is the params for PML megavault deposits
ListingVaultDepositParams listing_vault_deposit_params = 2
[ (gogoproto.nullable) = false ];
}
2 changes: 2 additions & 0 deletions protocol/app/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1206,6 +1206,7 @@ func New(
app.ClobKeeper,
&app.MarketMapKeeper,
app.PerpetualsKeeper,
app.VaultKeeper,
)
listingModule := listingmodule.NewAppModule(
appCodec,
Expand All @@ -1214,6 +1215,7 @@ func New(
app.ClobKeeper,
&app.MarketMapKeeper,
app.PerpetualsKeeper,
app.VaultKeeper,
)

// Initialize authenticators
Expand Down
7 changes: 6 additions & 1 deletion protocol/app/testdata/default_genesis_state.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,12 @@
}
},
"listing": {
"hard_cap_for_markets": 0
"hard_cap_for_markets": 500,
"listing_vault_deposit_params": {
"new_vault_deposit_amount": "10000000000",
"main_vault_deposit_amount": "0",
"num_blocks_to_lock_shares": 2592000
}
},
"params": null,
"perpetuals": {
Expand Down
7 changes: 6 additions & 1 deletion protocol/scripts/genesis/sample_pregenesis.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -869,7 +869,12 @@
}
},
"listing": {
"hard_cap_for_markets": 0
"hard_cap_for_markets": 500,
"listing_vault_deposit_params": {
"main_vault_deposit_amount": "0",
"new_vault_deposit_amount": "10000000000",
"num_blocks_to_lock_shares": 2592000
}
},
"marketmap": {
"last_updated": "0",
Expand Down
8 changes: 8 additions & 0 deletions protocol/testing/genesis.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2258,6 +2258,14 @@ function edit_genesis() {
# ICA Controller Params
update_ica_controller_params

# Listing
# Set hard cap for markets
dasel put -t int -f "$GENESIS" ".app_state.listing.hard_cap_for_markets" -v '500'
# Set default listing vault deposit params
dasel put -t string -f "$GENESIS" ".app_state.listing.listing_vault_deposit_params.new_vault_deposit_amount" -v "10000000000" # 10_000 USDC
dasel put -t string -f "$GENESIS" ".app_state.listing.listing_vault_deposit_params.main_vault_deposit_amount" -v "0" # 0 USDC
dasel put -t int -f "$GENESIS" ".app_state.listing.listing_vault_deposit_params.num_blocks_to_lock_shares" -v '2592000' # 30 days

# Vaults
# Set default quoting params.
dasel put -t int -f "$GENESIS" ".app_state.vault.default_quoting_params.spread_min_ppm" -v '3000'
Expand Down
7 changes: 6 additions & 1 deletion protocol/testutil/constants/genesis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -873,7 +873,12 @@ const GenesisState = `{
}
},
"listing": {
"hard_cap_for_markets": 0
"hard_cap_for_markets": 500,
"listing_vault_deposit_params": {
"new_vault_deposit_amount": "10000000000",
"main_vault_deposit_amount": "0",
"num_blocks_to_lock_shares": 2592000
}
},
"perpetuals": {
"liquidity_tiers": [
Expand Down
4 changes: 4 additions & 0 deletions protocol/testutil/keeper/listing.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
clobkeeper "github.com/dydxprotocol/v4-chain/protocol/x/clob/keeper"
perpetualskeeper "github.com/dydxprotocol/v4-chain/protocol/x/perpetuals/keeper"
priceskeeper "github.com/dydxprotocol/v4-chain/protocol/x/prices/keeper"
vaultkeeper "github.com/dydxprotocol/v4-chain/protocol/x/vault/keeper"
marketmapkeeper "github.com/skip-mev/slinky/x/marketmap/keeper"
"github.com/stretchr/testify/mock"

Expand Down Expand Up @@ -166,6 +167,7 @@ func ListingKeepers(
perpetualsKeeper,
clobKeeper,
marketMapKeeper,
vaultKeeper,
)

return []GenesisInitializer{keeper}
Expand All @@ -183,6 +185,7 @@ func createListingKeeper(
perpetualsKeeper *perpetualskeeper.Keeper,
clobKeeper *clobkeeper.Keeper,
marketMapKeeper *marketmapkeeper.Keeper,
vaultkeeper *vaultkeeper.Keeper,
) (
*keeper.Keeper,
storetypes.StoreKey,
Expand All @@ -202,6 +205,7 @@ func createListingKeeper(
clobKeeper,
marketMapKeeper,
perpetualsKeeper,
vaultkeeper,
)

return k, storeKey, mockTimeProvider
Expand Down
22 changes: 6 additions & 16 deletions protocol/x/clob/keeper/clob_pair.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,9 @@ func (k Keeper) CreatePerpetualClobPair(
return clobPair, err
}

// Write the `ClobPair` to state.
k.SetClobPair(ctx, clobPair)

err := k.CreateClobPair(ctx, clobPair)
if err != nil {
return clobPair, err
Expand Down Expand Up @@ -165,19 +168,6 @@ func (k Keeper) createOrderbook(ctx sdk.Context, clobPair types.ClobPair) {
// CreateClobPair creates a new `ClobPair` in the store and creates the corresponding orderbook in the memclob.
shrenujb marked this conversation as resolved.
Show resolved Hide resolved
// This function returns an error if a value for the ClobPair's id already exists in state.
func (k Keeper) CreateClobPair(ctx sdk.Context, clobPair types.ClobPair) error {
// Validate the given clob pair id is not already in use.
if _, exists := k.GetClobPair(ctx, clobPair.GetClobPairId()); exists {
shrenujb marked this conversation as resolved.
Show resolved Hide resolved
panic(
fmt.Sprintf(
"ClobPair with id %+v already exists in state",
clobPair.GetClobPairId(),
),
)
}

// Write the `ClobPair` to state.
k.setClobPair(ctx, clobPair)
shrenujb marked this conversation as resolved.
Show resolved Hide resolved

// Create the corresponding orderbook in the memclob.
k.createOrderbook(ctx, clobPair)

Expand Down Expand Up @@ -217,8 +207,8 @@ func (k Keeper) CreateClobPair(ctx sdk.Context, clobPair types.ClobPair) error {
return nil
}

// setClobPair sets a specific `ClobPair` in the store from its index.
func (k Keeper) setClobPair(ctx sdk.Context, clobPair types.ClobPair) {
// SetClobPair sets a specific `ClobPair` in the store from its index.
func (k Keeper) SetClobPair(ctx sdk.Context, clobPair types.ClobPair) {
store := k.getClobPairStore(ctx)
b := k.cdc.MustMarshal(&clobPair)
store.Set(clobPairKey(clobPair.GetClobPairId()), b)
Expand Down Expand Up @@ -567,7 +557,7 @@ func (k Keeper) UpdateClobPair(
return err
}

k.setClobPair(ctx, clobPair)
k.SetClobPair(ctx, clobPair)

// Send UpdateClobPair to indexer.
k.GetIndexerEventManager().AddTxnEvent(
Expand Down
4 changes: 4 additions & 0 deletions protocol/x/listing/genesis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ func InitGenesis(ctx sdk.Context, k keeper.Keeper, genState types.GenesisState)
if err := k.SetMarketsHardCap(ctx, genState.HardCapForMarkets); err != nil {
panic(err)
}

if err := k.SetListingVaultDepositParams(ctx, genState.ListingVaultDepositParams); err != nil {
panic(err)
}
Comment on lines +17 to +19
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

ExportGenesis Function Missing ListingVaultDepositParams

The ExportGenesis function does not include ListingVaultDepositParams, which prevents the export of the newly set parameters.

Action Required:

  • Update the ExportGenesis function to include ListingVaultDepositParams in the returned genesis state.

Example update:

func ExportGenesis(ctx sdk.Context, k keeper.Keeper) *types.GenesisState {
	genesis := types.DefaultGenesis()
	genesis.HardCapForMarkets = k.GetMarketsHardCap(ctx)
	genesis.ListingVaultDepositParams = k.GetListingVaultDepositParams(ctx)
	return genesis
}
Analysis chain

LGTM with suggestions for improvement

The addition of SetListingVaultDepositParams aligns well with the PR objectives of integrating Megavault into PML. The error handling is consistent with the existing pattern in the function.

Suggestions for improvement:

  1. Consider adding a comment explaining the purpose of setting the listing vault deposit parameters.
  2. Update the ExportGenesis function to include the new ListingVaultDepositParams in the returned genesis state.

Consider updating the ExportGenesis function as follows:

func ExportGenesis(ctx sdk.Context, k keeper.Keeper) *types.GenesisState {
    genesis := types.DefaultGenesis()
    genesis.HardCapForMarkets = k.GetMarketsHardCap(ctx)
    genesis.ListingVaultDepositParams = k.GetListingVaultDepositParams(ctx)
    return genesis
}

To ensure the correctness and completeness of the implementation, please run the following verification:

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify the implementation of ListingVaultDepositParams and related functions

# Check if ListingVaultDepositParams is defined in GenesisState
echo "Checking GenesisState definition:"
rg --type go "type GenesisState struct" -A 20 protocol/x/listing/types

# Verify the implementation of SetListingVaultDepositParams in the keeper
echo "\nChecking SetListingVaultDepositParams implementation:"
rg --type go "func \(k Keeper\) SetListingVaultDepositParams" -A 10 protocol/x/listing/keeper

# Check for the existence of GetListingVaultDepositParams in the keeper
echo "\nChecking for GetListingVaultDepositParams:"
rg --type go "func \(k Keeper\) GetListingVaultDepositParams" -A 10 protocol/x/listing/keeper

Length of output: 3965

Script:

#!/bin/bash
# Verify that ExportGenesis includes ListingVaultDepositParams

echo "Checking ExportGenesis function implementation:"
rg --type go "func ExportGenesis" -A 15 protocol/x/listing/genesis.go

Length of output: 304

}

// ExportGenesis returns the module's exported genesis.
Expand Down
3 changes: 3 additions & 0 deletions protocol/x/listing/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ type (
ClobKeeper types.ClobKeeper
MarketMapKeeper types.MarketMapKeeper
PerpetualsKeeper types.PerpetualsKeeper
VaultKeeper types.VaultKeeper
}
)

Expand All @@ -31,6 +32,7 @@ func NewKeeper(
clobKeeper types.ClobKeeper,
marketMapKeeper types.MarketMapKeeper,
perpetualsKeeper types.PerpetualsKeeper,
vaultKeeper types.VaultKeeper,
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Additional NewKeeper Instances Missing vaultKeeper Parameter

The verification revealed that only the protocol/x/listing/keeper/keeper.go file has been updated to include the vaultKeeper parameter in the NewKeeper function. Other modules still invoke NewKeeper without this parameter, which may lead to inconsistencies or runtime issues.

Modules to Address:

  • protocol/x/vest/keeper/keeper.go
  • protocol/x/clob/keeper/keeper.go
  • protocol/x/vault/keeper/keeper.go
  • protocol/x/subaccounts/keeper/keeper.go
  • ... (and others as listed in the script output)
Analysis chain

LGTM. Verify NewKeeper usage across the codebase.

The changes to the NewKeeper function are consistent with the addition of the VaultKeeper field to the Keeper struct. The new parameter allows for proper dependency injection, which is good for modularity and testability.

To ensure that all calls to NewKeeper have been updated to include the vaultKeeper parameter, run the following script:

Please review the output to confirm that all NewKeeper calls have been updated correctly.

Also applies to: 45-45

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all calls to NewKeeper include the vaultKeeper parameter

echo "Checking NewKeeper function calls:"
rg --type go -e 'NewKeeper\s*\(' -A 10

Length of output: 47020

) *Keeper {
return &Keeper{
cdc: cdc,
Expand All @@ -40,6 +42,7 @@ func NewKeeper(
ClobKeeper: clobKeeper,
MarketMapKeeper: marketMapKeeper,
PerpetualsKeeper: perpetualsKeeper,
VaultKeeper: vaultKeeper,
}
}

Expand Down
73 changes: 73 additions & 0 deletions protocol/x/listing/keeper/listing.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@ package keeper

import (
"math"
"math/big"

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

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

"github.com/dydxprotocol/v4-chain/protocol/lib"

Expand Down Expand Up @@ -101,6 +106,8 @@ func (k Keeper) CreateClobPair(
return 0, err
}

k.ClobKeeper.SetClobPair(ctx, clobPair)

// Only create the clob pair if we are in deliver tx mode. This is to prevent populating
// in memory data structures in the CLOB during simulation mode.
if lib.IsDeliverTxMode(ctx) {
Expand Down Expand Up @@ -187,3 +194,69 @@ func (k Keeper) GetListingVaultDepositParams(
k.cdc.MustUnmarshal(b, &vaultDepositParams)
return vaultDepositParams
}

// Function to deposit to the megavault for a new PML market
// This function deposits money to the megavault, transfers the new vault
// deposit amount to the new market vault and locks the shares for the deposit
func (k Keeper) DepositToMegavaultforPML(
ctx sdk.Context,
fromSubaccount satypes.SubaccountId,
clobPairId uint32,
) error {
// Get the listing vault deposit params
vaultDepositParams := k.GetListingVaultDepositParams(ctx)

Comment on lines +206 to +208
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle potential errors when retrieving vault deposit parameters

The function retrieves the vault deposit parameters without handling the case where they might be unset or invalid. If GetListingVaultDepositParams fails to retrieve valid parameters, it could lead to a panic or unintended behavior.

Consider adding error handling to ensure that vaultDepositParams are valid:

vaultDepositParams := k.GetListingVaultDepositParams(ctx)
if vaultDepositParams == nil {
    return errors.New("vault deposit parameters are not set")
}

Or handle the case appropriately based on how GetListingVaultDepositParams behaves when parameters are missing.

// Deposit to the megavault
totalDepositAmount := new(big.Int).Add(
vaultDepositParams.NewVaultDepositAmount.BigInt(),
vaultDepositParams.MainVaultDepositAmount.BigInt(),
)
shrenujb marked this conversation as resolved.
Show resolved Hide resolved
mintedShares, err := k.VaultKeeper.DepositToMegavault(
ctx,
fromSubaccount,
totalDepositAmount,
)
if err != nil {
return err
}

vaultId := vaulttypes.VaultId{
Type: vaulttypes.VaultType_VAULT_TYPE_CLOB,
Number: clobPairId,
}

// Transfer the new vault deposit amount to the new market vault
err = k.VaultKeeper.AllocateToVault(
ctx,
vaultId,
vaultDepositParams.NewVaultDepositAmount.BigInt(),
)
if err != nil {
return err
}

// Lock the shares for the new vault deposit amount
err = k.VaultKeeper.LockShares(
ctx,
fromSubaccount.Owner,
vaulttypes.BigIntToNumShares(mintedShares),
uint32(ctx.BlockHeight())+vaultDepositParams.NumBlocksToLockShares,
)
Comment on lines +243 to +244
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Potential integer overflow when converting block height to uint32

Casting ctx.BlockHeight() from int64 to uint32 may cause an integer overflow if the block height exceeds the maximum value of a uint32. This could lead to incorrect share lock expiration times.

Consider using uint64 for the lock expiration calculation to prevent overflow:

-        uint32(ctx.BlockHeight()) + vaultDepositParams.NumBlocksToLockShares,
+        uint64(ctx.BlockHeight()) + uint64(vaultDepositParams.NumBlocksToLockShares),

And ensure that the LockShares method accepts a uint64 parameter for the lock expiration block height.

Committable suggestion was skipped due to low confidence.

if err != nil {
return err
}

// Activate vault to quoting status
err = k.VaultKeeper.SetVaultParams(
shrenujb marked this conversation as resolved.
Show resolved Hide resolved
ctx,
vaultId,
vaulttypes.VaultParams{
Status: vaulttypes.VaultStatus_VAULT_STATUS_QUOTING,
},
)
if err != nil {
return err
}

return nil
}
Loading
Loading