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-115] decommission vaults at the beginning of a block #1264

Merged
merged 4 commits into from
Mar 28, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
7 changes: 7 additions & 0 deletions protocol/x/vault/abci.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,13 @@ import (
"github.com/dydxprotocol/v4-chain/protocol/x/vault/keeper"
)

func BeginBlocker(
ctx sdk.Context,
keeper *keeper.Keeper,
) {
keeper.DecommissionVaults(ctx)
}

func EndBlocker(
ctx sdk.Context,
keeper *keeper.Keeper,
Expand Down
83 changes: 83 additions & 0 deletions protocol/x/vault/keeper/vault.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
package keeper

import (
"math/big"

"cosmossdk.io/store/prefix"
storetypes "cosmossdk.io/store/types"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/dydxprotocol/v4-chain/protocol/lib/log"
satypes "github.com/dydxprotocol/v4-chain/protocol/x/subaccounts/types"
"github.com/dydxprotocol/v4-chain/protocol/x/vault/types"
)

// GetVaultEquity returns the equity of a vault (in quote quantums).
func (k Keeper) GetVaultEquity(
ctx sdk.Context,
vaultId types.VaultId,
) (*big.Int, error) {
netCollateral, _, _, err := k.subaccountsKeeper.GetNetCollateralAndMarginRequirements(
ctx,
satypes.Update{
SubaccountId: *vaultId.ToSubaccountId(),
},
)
if err != nil {
return nil, err
}
return netCollateral, nil
}

// DecommissionVaults decommissions all vaults with positive shares and non-positive equity.
func (k Keeper) DecommissionVaults(
Copy link
Contributor

@Christopher-Li Christopher-Li Mar 28, 2024

Choose a reason for hiding this comment

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

nit: can we clarify this function name? It sounds like DecomissionVaults would decomission all of the vaults, maybe DecommissionInvalidVaults

ctx sdk.Context,
) {
// Iterate through all vaults.
totalSharesIterator := k.getTotalSharesIterator(ctx)
defer totalSharesIterator.Close()
for ; totalSharesIterator.Valid(); totalSharesIterator.Next() {
var totalShares types.NumShares
k.cdc.MustUnmarshal(totalSharesIterator.Value(), &totalShares)

// Skip if TotalShares is non-positive.
totalSharesRat, err := totalShares.ToBigRat()
if err != nil || totalSharesRat.Sign() <= 0 {
continue
}
Comment on lines +42 to +46
Copy link
Contributor

Choose a reason for hiding this comment

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

is it positive for totalShares to be negative? How could that happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

negative shouldn't happen, but just added this check here to be future-proof

Copy link
Contributor

Choose a reason for hiding this comment

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

Curious, will this have to be updated when withdrawal logic is added? It's possible for a vault to have 0 shares and 0 equity in that case right? Non-blocking as that isn't part of Phase 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah 0 shares is possible. will think about that


// Get vault equity.
vaultId, err := types.GetVaultIdFromStateKey(totalSharesIterator.Key())
if err != nil {
log.ErrorLogWithError(ctx, "Failed to get vault ID from state key", err)
continue
}
equity, err := k.GetVaultEquity(ctx, *vaultId)
if err != nil {
log.ErrorLogWithError(ctx, "Failed to get vault equity", err)
continue
}

// Decommission vault if equity is non-positive.
if equity.Sign() <= 0 {
k.DecommissionVault(ctx, *vaultId)
}
}
}

// DecommissionVault decommissions a vault by deleting its tota shares and owner shares.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// DecommissionVault decommissions a vault by deleting its tota shares and owner shares.
// DecommissionVault decommissions a vault by deleting its total shares and owner shares.

func (k Keeper) DecommissionVault(
Copy link
Contributor

Choose a reason for hiding this comment

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

iiuc, if a user attempts to deposit in this vault before it's decommissioned, it will be recreated in the same block its decommissioned and a new vault will be created right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep! correct

Copy link
Contributor

@vincentwschau vincentwschau Mar 28, 2024

Choose a reason for hiding this comment

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

Is that the case? If a user deposits to the vault before begin block (when decomissioning occurs), then it would have happened in the previous block. What re-creates it in the new block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

deposit tx is processed after begin block. so if a vault is ready to be decommissioned, begin block would decommission it and then when deposit tx is processed, it will be created again.

if a user deposits before begin block, then that deposit tx is processed in the previous block

ctx sdk.Context,
vaultId types.VaultId,
) {
// Delete TotalShares of the vault.
totalSharesStore := prefix.NewStore(ctx.KVStore(k.storeKey), []byte(types.TotalSharesKeyPrefix))
totalSharesStore.Delete(vaultId.ToStateKey())

// Delete all OwnerShares of the vault.
ownerSharesStore := k.getVaultOwnerSharesStore(ctx, vaultId)
ownerSharesIterator := storetypes.KVStorePrefixIterator(ownerSharesStore, []byte{})
defer ownerSharesIterator.Close()
for ; ownerSharesIterator.Valid(); ownerSharesIterator.Next() {
ownerSharesStore.Delete(ownerSharesIterator.Key())
}
}
26 changes: 0 additions & 26 deletions protocol/x/vault/keeper/vault_info.go

This file was deleted.

135 changes: 135 additions & 0 deletions protocol/x/vault/keeper/vault_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
package keeper_test
Copy link
Contributor

Choose a reason for hiding this comment

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

non-blocking nit: Both tests here can be broken up into separate test cases w/ descriptions which would be more helpful for someone running the test if anything fails.


import (
"math/big"
"testing"

"github.com/cometbft/cometbft/types"
"github.com/dydxprotocol/v4-chain/protocol/dtypes"
testapp "github.com/dydxprotocol/v4-chain/protocol/testutil/app"
"github.com/dydxprotocol/v4-chain/protocol/testutil/constants"
assettypes "github.com/dydxprotocol/v4-chain/protocol/x/assets/types"
satypes "github.com/dydxprotocol/v4-chain/protocol/x/subaccounts/types"
vaulttypes "github.com/dydxprotocol/v4-chain/protocol/x/vault/types"
"github.com/stretchr/testify/require"
)

func TestDecomissionVaults(t *testing.T) {
vault0 := constants.Vault_Clob_0
vault1 := constants.Vault_Clob_1
// Initialize vault 0 with positive equity.
tApp := testapp.NewTestAppBuilder(t).WithGenesisDocFn(func() (genesis types.GenesisDoc) {
genesis = testapp.DefaultGenesis()
testapp.UpdateGenesisDocWithAppStateForModule(
&genesis,
func(genesisState *satypes.GenesisState) {
genesisState.Subaccounts = []satypes.Subaccount{
{
Id: vault0.ToSubaccountId(),
AssetPositions: []*satypes.AssetPosition{
{
AssetId: assettypes.AssetUsdc.Id,
Quantums: dtypes.NewInt(1),
},
},
},
}
},
)
return genesis
}).Build()
ctx := tApp.InitChain()
k := tApp.App.VaultKeeper

// Set total shares and owner shares for both vaults.
shares := vaulttypes.BigRatToNumShares(
big.NewRat(7, 1),
)
err := k.SetTotalShares(
ctx,
vault0,
shares,
)
require.NoError(t, err)
err = k.SetOwnerShares(
ctx,
vault0,
constants.Alice_Num0.Owner,
shares,
)
require.NoError(t, err)
err = k.SetTotalShares(
ctx,
vault1,
shares,
)
require.NoError(t, err)
err = k.SetOwnerShares(
ctx,
vault1,
constants.Bob_Num0.Owner,
shares,
)
require.NoError(t, err)

// Decomission all vaults.
k.DecommissionVaults(ctx)

// Check that total shares and owner shares are not deleted for vault 0.
got, exists := k.GetTotalShares(ctx, vault0)
require.Equal(t, true, exists)
require.Equal(t, shares, got)
got, exists = k.GetOwnerShares(ctx, vault0, constants.Alice_Num0.Owner)
require.Equal(t, true, exists)
require.Equal(t, shares, got)
// Check that total shares and owner shares are deleted for vault 1.
_, exists = k.GetTotalShares(ctx, vault1)
require.Equal(t, false, exists)
_, exists = k.GetOwnerShares(ctx, vault1, constants.Bob_Num0.Owner)
require.Equal(t, false, exists)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: shouldn't this be multiple separate test cases?

}

func TestDecomissionVault(t *testing.T) {
tApp := testapp.NewTestAppBuilder(t).Build()
ctx := tApp.InitChain()
k := tApp.App.VaultKeeper

// Decomission a non-existent vault.
k.DecommissionVault(ctx, constants.Vault_Clob_0)
Copy link
Contributor

Choose a reason for hiding this comment

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

what are you testing with this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

making sure that the function doesn't panic on non-existent vaults


// Set total shares and owner shares for two owners of a vault.
shares := vaulttypes.BigRatToNumShares(
big.NewRat(7, 1),
)
err := k.SetTotalShares(
ctx,
constants.Vault_Clob_0,
shares,
)
require.NoError(t, err)
err = k.SetOwnerShares(
ctx,
constants.Vault_Clob_0,
constants.Alice_Num0.Owner,
shares,
)
require.NoError(t, err)
err = k.SetOwnerShares(
ctx,
constants.Vault_Clob_0,
constants.Bob_Num0.Owner,
shares,
)
require.NoError(t, err)

// Decomission above vault.
k.DecommissionVault(ctx, constants.Vault_Clob_0)

// Check that total shares and owner shares are deleted.
_, exists := k.GetTotalShares(ctx, constants.Vault_Clob_0)
require.Equal(t, false, exists)
_, exists = k.GetOwnerShares(ctx, constants.Vault_Clob_0, constants.Alice_Num0.Owner)
require.Equal(t, false, exists)
_, exists = k.GetOwnerShares(ctx, constants.Vault_Clob_0, constants.Bob_Num0.Owner)
require.Equal(t, false, exists)
}
11 changes: 11 additions & 0 deletions protocol/x/vault/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ var (
_ module.HasGenesisBasics = AppModuleBasic{}

_ appmodule.AppModule = AppModule{}
_ appmodule.HasBeginBlocker = AppModule{}
_ appmodule.HasEndBlocker = AppModule{}
_ module.HasConsensusVersion = AppModule{}
_ module.HasGenesis = AppModule{}
Expand Down Expand Up @@ -147,6 +148,16 @@ func (am AppModule) ExportGenesis(ctx sdk.Context, cdc codec.JSONCodec) json.Raw
// be set to 1.
func (AppModule) ConsensusVersion() uint64 { return 1 }

// BeginBlock executes all ABCI BeginBlock logic respective to the vault module.
func (am AppModule) BeginBlock(ctx context.Context) error {
defer telemetry.ModuleMeasureSince(am.Name(), time.Now(), telemetry.MetricKeyBeginBlocker)
BeginBlocker(
lib.UnwrapSDKContext(ctx, types.ModuleName),
&am.keeper,
)
return nil
}

// EndBlock executes all ABCI EndBlock logic respective to the vault module.
func (am AppModule) EndBlock(ctx context.Context) error {
defer telemetry.ModuleMeasureSince(am.Name(), time.Now(), telemetry.MetricKeyEndBlocker)
Expand Down
Loading