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-116] add x/vault total shares store #1179

Merged
merged 3 commits into from
Mar 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
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
16 changes: 16 additions & 0 deletions protocol/testutil/constants/vault.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package constants

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

var (
Vault_Clob_0 = vaulttypes.VaultId{
Type: vaulttypes.VaultType_VAULT_TYPE_CLOB,
Number: 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this field named Number instead of like Id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think Id is a bit confusing as VaultId already has Id in the name. For example, see subaccountId

}
Vault_Clob_1 = vaulttypes.VaultId{
Type: vaulttypes.VaultType_VAULT_TYPE_CLOB,
Number: 1,
}
)
41 changes: 41 additions & 0 deletions protocol/x/vault/keeper/shares.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package keeper

import (
"cosmossdk.io/store/prefix"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/dydxprotocol/v4-chain/protocol/dtypes"
"github.com/dydxprotocol/v4-chain/protocol/x/vault/types"
)

// GetTotalShares gets TotalShares for a vault.
func (k Keeper) GetTotalShares(
ctx sdk.Context,
vaultId types.VaultId,
) (val types.NumShares, exists bool) {
store := prefix.NewStore(ctx.KVStore(k.storeKey), []byte(types.TotalSharesKeyPrefix))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is finding the existing keyvalue store that is associated with this keeper? Why do we use this instead of just storing this KV store directly in the keeper in x/vault/keeper.go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We usually create a separate store (within KVStore) for each "functionality"


b := store.Get(vaultId.ToStateKey())
if b == nil {
return val, false
Copy link
Contributor

Choose a reason for hiding this comment

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

how is val being returned here without being initialized? Is val being in the return of this function also initialize it? Does that also mean exists is initialized?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's correct. See here for more details.

}

k.cdc.MustUnmarshal(b, &val)
return val, true
}

// SetTotalShares sets TotalShares for a vault. Returns error if `totalShares` is negative.
func (k Keeper) SetTotalShares(
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if instead of having SetTotalShares, we should have incrementTotalShares, decrementTotalShares, or resetTotalShares(setting total shares to 0). It seems like we will never set totalShares to an arbitrary number, so SetTotalShares would require additional arithmetic.

nit: Should we panic on totalShares being a negative number?

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 we can build these increment decrement on top of Get and Set for sure.

As for verifying whether totalShares is non-negative, perhaps we can add that later when building out MsgDepositToVault?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

chatted with Vincent offline about this check on negative. will change this to return error if negative

ctx sdk.Context,
vaultId types.VaultId,
totalShares types.NumShares,
) error {
if totalShares.NumShares.Cmp(dtypes.NewInt(0)) == -1 {
return types.ErrNegativeShares
}

b := k.cdc.MustMarshal(&totalShares)
totalSharesStore := prefix.NewStore(ctx.KVStore(k.storeKey), []byte(types.TotalSharesKeyPrefix))
totalSharesStore.Set(vaultId.ToStateKey(), b)

return nil
}
67 changes: 67 additions & 0 deletions protocol/x/vault/keeper/shares_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
package keeper_test

import (
"testing"

"github.com/dydxprotocol/v4-chain/protocol/dtypes"
testapp "github.com/dydxprotocol/v4-chain/protocol/testutil/app"
"github.com/dydxprotocol/v4-chain/protocol/testutil/constants"
"github.com/dydxprotocol/v4-chain/protocol/x/vault/types"
"github.com/stretchr/testify/require"
)

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

// Get total shares for a non-existing vault.
_, exists := k.GetTotalShares(ctx, constants.Vault_Clob_0)
require.Equal(t, false, exists)

// Set total shares for a vault and then get.
err := k.SetTotalShares(ctx, constants.Vault_Clob_0, types.NumShares{
NumShares: dtypes.NewInt(7),
})
require.NoError(t, err)
numShares, exists := k.GetTotalShares(ctx, constants.Vault_Clob_0)
require.Equal(t, true, exists)
require.Equal(t, dtypes.NewInt(7), numShares.NumShares)

// Set total shares for another vault and then get.
err = k.SetTotalShares(ctx, constants.Vault_Clob_1, types.NumShares{
NumShares: dtypes.NewInt(456),
})
require.NoError(t, err)
numShares, exists = k.GetTotalShares(ctx, constants.Vault_Clob_1)
require.Equal(t, true, exists)
require.Equal(t, dtypes.NewInt(456), numShares.NumShares)

// Set total shares for second vault to 0.
err = k.SetTotalShares(ctx, constants.Vault_Clob_1, types.NumShares{
NumShares: dtypes.NewInt(0),
})
require.NoError(t, err)
numShares, exists = k.GetTotalShares(ctx, constants.Vault_Clob_1)
require.Equal(t, true, exists)
require.Equal(t, dtypes.NewInt(0), numShares.NumShares)

// Set total shares for the first vault again and then get.
err = k.SetTotalShares(ctx, constants.Vault_Clob_0, types.NumShares{
NumShares: dtypes.NewInt(123),
})
require.NoError(t, err)
numShares, exists = k.GetTotalShares(ctx, constants.Vault_Clob_0)
require.Equal(t, true, exists)
require.Equal(t, dtypes.NewInt(123), numShares.NumShares)

// Set total shares for the first vault to a negative value.
// Should get error and total shares should remain unchanged.
err = k.SetTotalShares(ctx, constants.Vault_Clob_0, types.NumShares{
NumShares: dtypes.NewInt(-1),
})
require.Equal(t, types.ErrNegativeShares, err)
numShares, exists = k.GetTotalShares(ctx, constants.Vault_Clob_0)
require.Equal(t, true, exists)
require.Equal(t, dtypes.NewInt(123), numShares.NumShares)
}
13 changes: 13 additions & 0 deletions protocol/x/vault/types/errors.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package types

// DONTCOVER

import errorsmod "cosmossdk.io/errors"

var (
ErrNegativeShares = errorsmod.Register(
ModuleName,
1,
"Shares are negative",
)
)
12 changes: 9 additions & 3 deletions protocol/x/vault/types/keys.go
Original file line number Diff line number Diff line change
@@ -1,10 +1,16 @@
package types

// Module name and store keys
// Module name and store keys.
const (
// ModuleName defines the module name
// ModuleName defines the module name.
ModuleName = "vault"

// StoreKey defines the primary module store key
// StoreKey defines the primary module store key.
StoreKey = ModuleName
)

// State.
const (
// TotalSharesKeyPrefix is the prefix to retrieve all TotalShares.
TotalSharesKeyPrefix = "TotalShares:"
)
17 changes: 17 additions & 0 deletions protocol/x/vault/types/keys_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package types_test

import (
"testing"

"github.com/dydxprotocol/v4-chain/protocol/x/vault/types"
"github.com/stretchr/testify/require"
)

func TestModuleKeys(t *testing.T) {
require.Equal(t, "vault", types.ModuleName)
require.Equal(t, "vault", types.StoreKey)
}

func TestStateKeys(t *testing.T) {
require.Equal(t, "TotalShares:", types.TotalSharesKeyPrefix)
}
9 changes: 9 additions & 0 deletions protocol/x/vault/types/vault_id.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package types

func (id *VaultId) ToStateKey() []byte {
b, err := id.Marshal()
if err != nil {
panic(err)
}
return b
tqin7 marked this conversation as resolved.
Show resolved Hide resolved
}
16 changes: 16 additions & 0 deletions protocol/x/vault/types/vault_id_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package types_test

import (
"testing"

"github.com/dydxprotocol/v4-chain/protocol/testutil/constants"
"github.com/stretchr/testify/require"
)

func TestToStateKey(t *testing.T) {
b, _ := constants.Vault_Clob_0.Marshal()
require.Equal(t, b, constants.Vault_Clob_0.ToStateKey())

b, _ = constants.Vault_Clob_1.Marshal()
require.Equal(t, b, constants.Vault_Clob_1.ToStateKey())
}
Loading