Address PR comments

dydxprotocol · Jul 23, 2024 · 3e106c3 · 3e106c3
1 parent 952dfdc
commit 3e106c3
Show file tree

Hide file tree

Showing 12 changed files with 126 additions and 364 deletions.
diff --git a/proto/dydxprotocol/accountplus/accountplus.proto b/proto/dydxprotocol/accountplus/accountplus.proto
@@ -1,12 +1,15 @@
 syntax = "proto3";
 package dydxprotocol.accountplus;
 
+import "gogoproto/gogo.proto";
+
 option go_package = "github.com/dydxprotocol/v4-chain/protocol/x/accountplus/types";
 
 // Account State
 message AccountState {
   string address = 1;
-  TimestampNonceDetails timestamp_nonce_details = 2;
+  TimestampNonceDetails timestamp_nonce_details = 2
+   [ (gogoproto.nullable) = false ];
 }
 
 // Timestamp nonce details

diff --git a/proto/dydxprotocol/accountplus/genesis.proto b/proto/dydxprotocol/accountplus/genesis.proto
@@ -1,9 +1,11 @@
 syntax = "proto3";
 package dydxprotocol.accountplus;
 
+import "gogoproto/gogo.proto";
 import "dydxprotocol/accountplus/accountplus.proto";
 
 option go_package = "github.com/dydxprotocol/v4-chain/protocol/x/accountplus/types";
 
 // Module genesis state
-message GenesisState { repeated AccountState accounts = 1; }
+message GenesisState { repeated AccountState accounts = 1 
+    [ (gogoproto.nullable) = false ]; }
diff --git a/protocol/x/accountplus/genesis.go b/protocol/x/accountplus/genesis.go
@@ -14,7 +14,11 @@ func InitGenesis(ctx sdk.Context, k keeper.Keeper, data types.GenesisState) {
 }
 
 func ExportGenesis(ctx sdk.Context, k keeper.Keeper) *types.GenesisState {
+	accounts, err := k.GetAllAccountStates(ctx)
+	if err != nil {
+		panic(err)
+	}
 	return &types.GenesisState{
-		Accounts: k.GetAllAccountStates(ctx),
+		Accounts: accounts,
 	}
 }
diff --git a/protocol/x/accountplus/genesis_test.go b/protocol/x/accountplus/genesis_test.go
@@ -4,12 +4,9 @@ import (
 	"math"
 	"testing"
 
-	sdk "github.com/cosmos/cosmos-sdk/types"
 	testapp "github.com/dydxprotocol/v4-chain/protocol/testutil/app"
 	"github.com/dydxprotocol/v4-chain/protocol/testutil/constants"
 	"github.com/dydxprotocol/v4-chain/protocol/x/accountplus"
-	"github.com/dydxprotocol/v4-chain/protocol/x/accountplus/keeper"
-	"github.com/dydxprotocol/v4-chain/protocol/x/accountplus/testutils"
 	"github.com/dydxprotocol/v4-chain/protocol/x/accountplus/types"
 	"github.com/stretchr/testify/require"
 )
@@ -20,53 +17,53 @@ func TestImportExportGenesis(t *testing.T) {
 		genesisState *types.GenesisState
 		// The order of this list may not match the order in GenesisState. We want our tests to be deterministic so
 		// order of expectedAccountStates was manually set based on test debug. This ordering should only be changed if
-		// additional accounts added to genesisState. If a feature breaks the existing ordering, should look into ßwhy.
-		expectedAccountStates []*types.AccountState
+		// additional accounts added to genesisState. If a feature breaks the existing ordering, should look into why.
+		expectedAccountStates []types.AccountState
 	}{
 		"non-empty genesis": {
 			genesisState: &types.GenesisState{
-				Accounts: []*types.AccountState{
+				Accounts: []types.AccountState{
 					{
 						Address: constants.AliceAccAddress.String(),
-						TimestampNonceDetails: &types.TimestampNonceDetails{
+						TimestampNonceDetails: types.TimestampNonceDetails{
 							TimestampNonces: []uint64{baseTsNonce + 1, baseTsNonce + 2, baseTsNonce + 3},
 							MaxEjectedNonce: baseTsNonce,
 						},
 					},
 					{
 						Address: constants.BobAccAddress.String(),
-						TimestampNonceDetails: &types.TimestampNonceDetails{
+						TimestampNonceDetails: types.TimestampNonceDetails{
 							TimestampNonces: []uint64{baseTsNonce + 5, baseTsNonce + 6, baseTsNonce + 7},
 							MaxEjectedNonce: baseTsNonce + 1,
 						},
 					},
 					{
 						Address: constants.CarlAccAddress.String(),
-						TimestampNonceDetails: &types.TimestampNonceDetails{
+						TimestampNonceDetails: types.TimestampNonceDetails{
 							TimestampNonces: []uint64{baseTsNonce + 5, baseTsNonce + 6, baseTsNonce + 7},
 							MaxEjectedNonce: baseTsNonce + 1,
 						},
 					},
 				},
 			},
-			expectedAccountStates: []*types.AccountState{
+			expectedAccountStates: []types.AccountState{
 				{
 					Address: constants.AliceAccAddress.String(),
-					TimestampNonceDetails: &types.TimestampNonceDetails{
+					TimestampNonceDetails: types.TimestampNonceDetails{
 						TimestampNonces: []uint64{baseTsNonce + 1, baseTsNonce + 2, baseTsNonce + 3},
 						MaxEjectedNonce: baseTsNonce,
 					},
 				},
 				{
 					Address: constants.CarlAccAddress.String(),
-					TimestampNonceDetails: &types.TimestampNonceDetails{
+					TimestampNonceDetails: types.TimestampNonceDetails{
 						TimestampNonces: []uint64{baseTsNonce + 5, baseTsNonce + 6, baseTsNonce + 7},
 						MaxEjectedNonce: baseTsNonce + 1,
 					},
 				},
 				{
 					Address: constants.BobAccAddress.String(),
-					TimestampNonceDetails: &types.TimestampNonceDetails{
+					TimestampNonceDetails: types.TimestampNonceDetails{
 						TimestampNonces: []uint64{baseTsNonce + 5, baseTsNonce + 6, baseTsNonce + 7},
 						MaxEjectedNonce: baseTsNonce + 1,
 					},
@@ -75,9 +72,9 @@ func TestImportExportGenesis(t *testing.T) {
 		},
 		"empty genesis": {
 			genesisState: &types.GenesisState{
-				Accounts: []*types.AccountState{},
+				Accounts: []types.AccountState{},
 			},
-			expectedAccountStates: []*types.AccountState{},
+			expectedAccountStates: []types.AccountState{},
 		},
 	}
 
@@ -87,38 +84,24 @@ func TestImportExportGenesis(t *testing.T) {
 			ctx := tApp.InitChain()
 			k := tApp.App.AccountPlusKeeper
 
-			// Initialize genesis state
 			accountplus.InitGenesis(ctx, k, *tc.genesisState)
 
-			// Check that keeper state is correct
-			compareKeeperWithGenesisState(t, ctx, &k, tc.expectedAccountStates)
+			// Check that keeper accounts states are correct
+			actualAccountStates, _ := k.GetAllAccountStates(ctx)
+			require.Equal(
+				t,
+				tc.expectedAccountStates,
+				actualAccountStates,
+				"Keeper account states do not match Genesis account states",
+			)
 
-			// Export the genesis state
 			exportedGenesis := accountplus.ExportGenesis(ctx, k)
 
-			// Ensure the exported state matches the expected state
+			// Check that the exported state matches the expected state
 			expectedGenesis := &types.GenesisState{
 				Accounts: tc.expectedAccountStates,
 			}
-			requireGenesisStatesEqual(t, exportedGenesis, expectedGenesis)
+			require.Equal(t, *exportedGenesis, *expectedGenesis)
 		})
 	}
 }
-
-func compareKeeperWithGenesisState(
-	t *testing.T,
-	ctx sdk.Context,
-	k *keeper.Keeper,
-	expectedAccountStates []*types.AccountState,
-) {
-	// Compare states. Order matters.
-	isEqual := testutils.CompareAccountStateLists(k.GetAllAccountStates(ctx), expectedAccountStates)
-
-	require.True(t, isEqual, "Keeper account states does not match Genesis account states")
-}
-
-func requireGenesisStatesEqual(t *testing.T, actualGenesisState, expectedGenesisState *types.GenesisState) {
-	isEqual := testutils.CompareAccountStateLists(actualGenesisState.GetAccounts(), expectedGenesisState.GetAccounts())
-
-	require.True(t, isEqual, "GenesisState mismatch")
-}
diff --git a/protocol/x/accountplus/keeper/keeper.go b/protocol/x/accountplus/keeper/keeper.go
@@ -11,6 +11,16 @@ import (
 	"github.com/dydxprotocol/v4-chain/protocol/x/accountplus/types"
 )
 
+func DefaultAccountState(address sdk.AccAddress) types.AccountState {
+	return types.AccountState{
+		Address: address.String(),
+		TimestampNonceDetails: types.TimestampNonceDetails{
+			MaxEjectedNonce: 0,
+			TimestampNonces: []uint64{},
+		},
+	}
+}
+
 type Keeper struct {
 	cdc      codec.BinaryCodec
 	storeKey storetypes.StoreKey
@@ -32,77 +42,78 @@ func (k Keeper) InitializeForGenesis(ctx sdk.Context) {
 }
 
 // Get all account details pairs in store
-func (k Keeper) GetAllAccountStates(ctx sdk.Context) []*types.AccountState {
+func (k Keeper) GetAllAccountStates(ctx sdk.Context) ([]types.AccountState, error) {
 	store := ctx.KVStore(k.storeKey)
 
 	iterator := storetypes.KVStorePrefixIterator(store, nil)
 	defer iterator.Close()
 
-	var accounts []*types.AccountState
+	accounts := []types.AccountState{}
 	for ; iterator.Valid(); iterator.Next() {
-		var account types.AccountState
-		k.cdc.MustUnmarshal(iterator.Value(), &account)
-		accounts = append(accounts, &account)
+		accountState, found := k.GetAccountState(ctx, iterator.Key())
+		if !found {
+			return accounts, errors.New("Could not get account state for address: " + sdk.AccAddress(iterator.Key()).String())
+		}
+		accounts = append(accounts, accountState)
 	}
 
-	return accounts
+	return accounts, nil
 }
 
 // Set genesis state
 func (k Keeper) SetGenesisState(ctx sdk.Context, data types.GenesisState) error {
-	store := ctx.KVStore(k.storeKey)
-
 	for _, account := range data.Accounts {
 		address, err := sdk.AccAddressFromBech32(account.Address)
 		if err != nil {
 			return err
 		}
-		k.setAccountState(store, address, *account)
+		k.setAccountState(ctx, address, account)
 	}
 
 	return nil
 }
 
-func (k Keeper) InitializeAccount(ctx sdk.Context, address sdk.AccAddress) (types.AccountState, error) {
-	store := ctx.KVStore(k.storeKey)
-
-	if _, found := k.getAccountState(store, address); found {
-		return types.AccountState{}, errors.New(
+func (k Keeper) InitializeAccount(ctx sdk.Context, address sdk.AccAddress) error {
+	if _, found := k.GetAccountState(ctx, address); found {
+		return errors.New(
 			"Cannot initialize AccountState for address with existing AccountState, address: " + address.String(),
 		)
 	}
 
-	initialAccountState := types.AccountState{
-		Address:               address.String(),
-		TimestampNonceDetails: DeepCopyTimestampNonceDetails(InitialTimestampNonceDetails),
-	}
-
-	k.setAccountState(store, address, initialAccountState)
+	k.setAccountState(ctx, address, DefaultAccountState(address))
 
-	return initialAccountState, nil
+	return nil
 }
 
 // Get the AccountState from KVStore for a given account address
-func (k Keeper) getAccountState(
-	store storetypes.KVStore,
+func (k Keeper) GetAccountState(
+	ctx sdk.Context,
 	address sdk.AccAddress,
 ) (types.AccountState, bool) {
+	store := ctx.KVStore(k.storeKey)
 	bz := store.Get(address.Bytes())
 	if bz == nil {
 		return types.AccountState{}, false
 	}
 
 	var accountState types.AccountState
 	k.cdc.MustUnmarshal(bz, &accountState)
+
+	// By default empty slices are Unmarshed into nil
+	if accountState.TimestampNonceDetails.TimestampNonces == nil {
+		accountState.TimestampNonceDetails.TimestampNonces = make([]uint64, 0)
+	}
+
 	return accountState, true
 }
 
 // Set the AccountState into KVStore for a given account address
 func (k Keeper) setAccountState(
-	store storetypes.KVStore,
+	ctx sdk.Context,
 	address sdk.AccAddress,
 	accountState types.AccountState,
 ) {
+	store := ctx.KVStore(k.storeKey)
 	bz := k.cdc.MustMarshal(&accountState)
 	store.Set(address.Bytes(), bz)
 }
diff --git a/protocol/x/accountplus/keeper/keeper_test.go b/protocol/x/accountplus/keeper/keeper_test.go
@@ -10,24 +10,23 @@ import (
 	"github.com/dydxprotocol/v4-chain/protocol/testutil/constants"
 	"github.com/dydxprotocol/v4-chain/protocol/x/accountplus"
 	"github.com/dydxprotocol/v4-chain/protocol/x/accountplus/keeper"
-	"github.com/dydxprotocol/v4-chain/protocol/x/accountplus/testutils"
 	"github.com/dydxprotocol/v4-chain/protocol/x/accountplus/types"
 )
 
 func TestInitializeAccount(t *testing.T) {
 	baseTsNonce := uint64(math.Pow(2, 40))
 	genesisState := &types.GenesisState{
-		Accounts: []*types.AccountState{
+		Accounts: []types.AccountState{
 			{
 				Address: constants.AliceAccAddress.String(),
-				TimestampNonceDetails: &types.TimestampNonceDetails{
+				TimestampNonceDetails: types.TimestampNonceDetails{
 					TimestampNonces: []uint64{baseTsNonce + 1, baseTsNonce + 2, baseTsNonce + 3},
 					MaxEjectedNonce: baseTsNonce,
 				},
 			},
 			{
 				Address: constants.BobAccAddress.String(),
-				TimestampNonceDetails: &types.TimestampNonceDetails{
+				TimestampNonceDetails: types.TimestampNonceDetails{
 					TimestampNonces: []uint64{baseTsNonce + 5, baseTsNonce + 6, baseTsNonce + 7},
 					MaxEjectedNonce: baseTsNonce + 1,
 				},
@@ -38,23 +37,21 @@ func TestInitializeAccount(t *testing.T) {
 	t.Run("Cannot initialize existing account", func(t *testing.T) {
 		ctx, k, _, _ := keepertest.TimestampNonceKeepers(t)
 		accountplus.InitGenesis(ctx, *k, *genesisState)
-		_, err := k.InitializeAccount(ctx, constants.AliceAccAddress)
+		err := k.InitializeAccount(ctx, constants.AliceAccAddress)
 		require.NotNil(t, err, "Account should not be able to be initialized if already exists")
 	})
 
 	t.Run("Can initialize new account", func(t *testing.T) {
 		ctx, k, _, _ := keepertest.TimestampNonceKeepers(t)
 		accountplus.InitGenesis(ctx, *k, *genesisState)
 
-		expectedAccount := types.AccountState{
-			Address:               constants.CarlAccAddress.String(),
-			TimestampNonceDetails: keeper.DeepCopyTimestampNonceDetails(keeper.InitialTimestampNonceDetails),
-		}
+		expectedAccount := keeper.DefaultAccountState(constants.CarlAccAddress)
 
-		account, err := k.InitializeAccount(ctx, constants.CarlAccAddress)
+		err := k.InitializeAccount(ctx, constants.CarlAccAddress)
 		require.Nil(t, err, "Should be able to initialize account if it did not exist")
 
-		isAccountEqual := testutils.CompareAccountStates(&account, &expectedAccount)
-		require.True(t, isAccountEqual, "Initialized account does not have correct initial state")
+		actualAccount, found := k.GetAccountState(ctx, constants.CarlAccAddress)
+		require.True(t, found, "Could not find account")
+		require.Equal(t, actualAccount, expectedAccount)
 	})
 }
diff --git a/protocol/x/accountplus/keeper/timestampnonce.go b/protocol/x/accountplus/keeper/timestampnonce.go
@@ -1,26 +1,3 @@
 package keeper
 
-import (
-	"github.com/dydxprotocol/v4-chain/protocol/x/accountplus/types"
-)
-
-var InitialTimestampNonceDetails = &types.TimestampNonceDetails{
-	MaxEjectedNonce: 0,
-	TimestampNonces: []uint64{},
-}
-
-func DeepCopyTimestampNonceDetails(details *types.TimestampNonceDetails) *types.TimestampNonceDetails {
-	if details == nil {
-		return nil
-	}
-
-	copyDetails := &types.TimestampNonceDetails{
-		MaxEjectedNonce: details.MaxEjectedNonce,
-		TimestampNonces: make([]uint64, len(details.TimestampNonces)),
-	}
-
-	// Copy the slice elements
-	copy(copyDetails.TimestampNonces, details.TimestampNonces)
-
-	return copyDetails
-}
+func Placeholder() {}