Merge pull request #481 from osmosis-labs/mattverse/code-clean

mattverse · web-flow · commit ea6bda76c515 · 2021-10-07T14:50:17.000+09:00
Clean Code
diff --git a/app/app.go b/app/app.go
@@ -6,6 +6,17 @@ import (
 	"os"
 	"path/filepath"
 
+	"github.com/gorilla/mux"
+	"github.com/rakyll/statik/fs"
+	"github.com/spf13/cast"
+
+	abci "github.com/tendermint/tendermint/abci/types"
+	tmjson "github.com/tendermint/tendermint/libs/json"
+	"github.com/tendermint/tendermint/libs/log"
+	tmos "github.com/tendermint/tendermint/libs/os"
+	tmproto "github.com/tendermint/tendermint/proto/tendermint/types"
+	dbm "github.com/tendermint/tm-db"
+
 	ibcclient "github.com/cosmos/cosmos-sdk/x/ibc/core/02-client"
 	paramproposal "github.com/cosmos/cosmos-sdk/x/params/types/proposal"
 
@@ -72,7 +83,7 @@ import (
 	upgradeclient "github.com/cosmos/cosmos-sdk/x/upgrade/client"
 	upgradekeeper "github.com/cosmos/cosmos-sdk/x/upgrade/keeper"
 	upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
-	"github.com/gorilla/mux"
+
 	appparams "github.com/osmosis-labs/osmosis/app/params"
 	_ "github.com/osmosis-labs/osmosis/client/docs/statik"
 	"github.com/osmosis-labs/osmosis/x/claim"
@@ -97,14 +108,6 @@ import (
 	poolincentivesclient "github.com/osmosis-labs/osmosis/x/pool-incentives/client"
 	poolincentiveskeeper "github.com/osmosis-labs/osmosis/x/pool-incentives/keeper"
 	poolincentivestypes "github.com/osmosis-labs/osmosis/x/pool-incentives/types"
-	"github.com/rakyll/statik/fs"
-	"github.com/spf13/cast"
-	abci "github.com/tendermint/tendermint/abci/types"
-	tmjson "github.com/tendermint/tendermint/libs/json"
-	"github.com/tendermint/tendermint/libs/log"
-	tmos "github.com/tendermint/tendermint/libs/os"
-	tmproto "github.com/tendermint/tendermint/proto/tendermint/types"
-	dbm "github.com/tendermint/tm-db"
 )
 
 const appName = "OsmosisApp"
diff --git a/simapp/state.go b/simapp/state.go
@@ -2,8 +2,8 @@ package simapp
 
 import (
 	"encoding/json"
-	"fmt"
 	"io/ioutil"
+	"log"
 	"math/rand"
 	"time"
 
@@ -94,7 +94,7 @@ func AppStateRandomizedFn(
 		numInitiallyBonded = numAccs
 	}
 
-	fmt.Printf(
+	log.Printf(
 		`Selected randomly generated parameters for simulated genesis:
 {
   stake_per_account: "%d",
diff --git a/x/claim/genesis.go b/x/claim/genesis.go
@@ -12,11 +12,10 @@ import (
 // InitGenesis initializes the capability module's state from a provided genesis
 // state.
 func InitGenesis(ctx sdk.Context, k keeper.Keeper, genState types.GenesisState) {
-	// TODO: Can we ensure that the module account created is equal everytime?
-	k.CreateModuleAccount(ctx, genState.ModuleAccountBalance)
-
+	// If its the chain genesis, set the airdrop start time to be now, and setup the needed module accounts.
 	if genState.Params.AirdropStartTime.Equal(time.Time{}) {
 		genState.Params.AirdropStartTime = ctx.BlockTime()
+		k.CreateModuleAccount(ctx, genState.ModuleAccountBalance)
 	}
 
 	if err := k.SetParams(ctx, genState.Params); err != nil {
@@ -29,7 +28,10 @@ func InitGenesis(ctx sdk.Context, k keeper.Keeper, genState types.GenesisState)
 
 // ExportGenesis returns the capability module's exported genesis.
 func ExportGenesis(ctx sdk.Context, k keeper.Keeper) *types.GenesisState {
-	params, _ := k.GetParams(ctx)
+	params, err := k.GetParams(ctx)
+	if err != nil {
+		panic(err)
+	}
 	genesis := types.DefaultGenesis()
 	genesis.ModuleAccountBalance = k.GetModuleAccountBalance(ctx)
 	genesis.Params = params
diff --git a/x/claim/genesis_test.go b/x/claim/genesis_test.go
@@ -20,9 +20,9 @@ var testGenesis = types.GenesisState{
 	ModuleAccountBalance: sdk.NewInt64Coin(types.DefaultClaimDenom, 750000000),
 	Params: types.Params{
 		AirdropStartTime:   now,
-		DurationUntilDecay: types.DefaultDurationUntilDecay, // 2 month
-		DurationOfDecay:    types.DefaultDurationOfDecay,    // 4 months
-		ClaimDenom:         types.DefaultClaimDenom,         // uosmo
+		DurationUntilDecay: types.DefaultDurationOfDecay,
+		DurationOfDecay:    types.DefaultDurationOfDecay,
+		ClaimDenom:         types.DefaultClaimDenom, // uosmo
 	},
 	ClaimRecords: []types.ClaimRecord{
 		{
@@ -44,6 +44,7 @@ func TestClaimInitGenesis(t *testing.T) {
 	ctx = ctx.WithBlockTime(now.Add(time.Second))
 	genesis := testGenesis
 	claim.InitGenesis(ctx, *app.ClaimKeeper, genesis)
+	app.ClaimKeeper.CreateModuleAccount(ctx, sdk.NewInt64Coin(types.DefaultClaimDenom, 750000000))
 
 	coin := app.ClaimKeeper.GetModuleAccountBalance(ctx)
 	require.Equal(t, coin.String(), genesis.ModuleAccountBalance.String())
@@ -62,6 +63,7 @@ func TestClaimExportGenesis(t *testing.T) {
 	ctx = ctx.WithBlockTime(now.Add(time.Second))
 	genesis := testGenesis
 	claim.InitGenesis(ctx, *app.ClaimKeeper, genesis)
+	app.ClaimKeeper.CreateModuleAccount(ctx, sdk.NewInt64Coin(types.DefaultClaimDenom, 750000000))
 
 	claimRecord, err := app.ClaimKeeper.GetClaimRecord(ctx, acc2)
 	require.NoError(t, err)
diff --git a/x/claim/keeper/claim.go b/x/claim/keeper/claim.go
@@ -16,7 +16,10 @@ func (k Keeper) GetModuleAccountAddress(ctx sdk.Context) sdk.AccAddress {
 // GetModuleAccountBalance gets the airdrop coin balance of module account
 func (k Keeper) GetModuleAccountBalance(ctx sdk.Context) sdk.Coin {
 	moduleAccAddr := k.GetModuleAccountAddress(ctx)
-	params, _ := k.GetParams(ctx)
+	params, err := k.GetParams(ctx)
+	if err != nil {
+		panic(err)
+	}
 	return k.bankKeeper.GetBalance(ctx, moduleAccAddr, params.ClaimDenom)
 }
 
diff --git a/x/claim/types/genesis.go b/x/claim/types/genesis.go
@@ -2,6 +2,7 @@ package types
 
 import (
 	"encoding/json"
+	"fmt"
 	"time"
 
 	"github.com/cosmos/cosmos-sdk/codec"
@@ -51,5 +52,10 @@ func (gs GenesisState) Validate() error {
 	if !totalClaimable.IsEqual(sdk.NewCoins(gs.ModuleAccountBalance)) {
 		return ErrIncorrectModuleAccountBalance
 	}
+
+	if gs.Params.ClaimDenom != gs.ModuleAccountBalance.Denom {
+		return fmt.Errorf("denom for module and claim does not match")
+	}
+
 	return nil
 }
diff --git a/x/gamm/genesis.go b/x/gamm/genesis.go
@@ -55,7 +55,7 @@ func ExportGenesis(ctx sdk.Context, k keeper.Keeper) *types.GenesisState {
 		poolAnys = append(poolAnys, any)
 	}
 	return &types.GenesisState{
-		NextPoolNumber: k.GetNextPoolNumber(ctx),
+		NextPoolNumber: k.GetNextPoolNumberAndIncrement(ctx),
 		Pools:          poolAnys,
 		Params:         k.GetParams(ctx),
 	}
diff --git a/x/gamm/genesis_test.go b/x/gamm/genesis_test.go
@@ -48,7 +48,7 @@ func TestGammInitGenesis(t *testing.T) {
 		},
 	}, app.AppCodec())
 
-	require.Equal(t, app.GAMMKeeper.GetNextPoolNumber(ctx), uint64(2))
+	require.Equal(t, app.GAMMKeeper.GetNextPoolNumberAndIncrement(ctx), uint64(2))
 	poolStored, err := app.GAMMKeeper.GetPool(ctx, 1)
 	require.NoError(t, err)
 	require.Equal(t, pool.GetId(), poolStored.GetId())
diff --git a/x/gamm/keeper/grpc_query.go b/x/gamm/keeper/grpc_query.go
@@ -119,7 +119,7 @@ func (k Keeper) NumPools(
 	sdkCtx := sdk.UnwrapSDKContext(ctx)
 
 	return &types.QueryNumPoolsResponse{
-		NumPools: k.GetNextPoolNumber(sdkCtx) - 1,
+		NumPools: k.GetNextPoolNumberAndIncrement(sdkCtx) - 1,
 	}, nil
 }
 
diff --git a/x/gamm/keeper/pool.go b/x/gamm/keeper/pool.go
@@ -82,7 +82,7 @@ func (k Keeper) SetPool(ctx sdk.Context, pool types.PoolI) error {
 // newPool is an internal function that creates a new Pool object with the provided
 // parameters, initial assets, and future governor.
 func (k Keeper) newPool(ctx sdk.Context, poolParams types.PoolParams, assets []types.PoolAsset, futureGovernor string) (types.PoolI, error) {
-	poolId := k.GetNextPoolNumber(ctx)
+	poolId := k.GetNextPoolNumberAndIncrement(ctx)
 
 	pool, err := types.NewPool(poolId, poolParams, assets, futureGovernor, ctx.BlockTime())
 	if err != nil {
@@ -118,15 +118,14 @@ func (k Keeper) SetNextPoolNumber(ctx sdk.Context, poolNumber uint64) {
 	store.Set(types.KeyNextGlobalPoolNumber, bz)
 }
 
-// GetNextPoolNumber returns the next pool number
-func (k Keeper) GetNextPoolNumber(ctx sdk.Context) uint64 {
+// GetNextPoolNumberAndIncrement returns the next pool number, and increments the corresponding state entry
+func (k Keeper) GetNextPoolNumberAndIncrement(ctx sdk.Context) uint64 {
 	var poolNumber uint64
 	store := ctx.KVStore(k.storeKey)
 
 	bz := store.Get(types.KeyNextGlobalPoolNumber)
 	if bz == nil {
-		// initialize the pool id numbers
-		poolNumber = 1
+		panic(fmt.Errorf("pool has not been initialized -- Should have been done in InitGenesis"))
 	} else {
 		val := gogotypes.UInt64Value{}
 
diff --git a/x/gamm/keeper/pool_service.go b/x/gamm/keeper/pool_service.go
@@ -46,7 +46,7 @@ func (k Keeper) CreatePool(
 		coins = append(coins, asset.Token)
 	}
 	if coins == nil {
-		panic("No pool assets were in added in the pool. Two should've been added per validateBasic")
+		return 0, types.ErrTooFewPoolAssets
 	}
 
 	coins = coins.Sort()
diff --git a/x/gamm/types/genesis.pb.go b/x/gamm/types/genesis.pb.go
diff --git a/x/incentives/keeper/store.go b/x/incentives/keeper/store.go
@@ -67,10 +67,9 @@ func (k Keeper) addGaugeRefByKey(ctx sdk.Context, key []byte, gaugeID uint64) er
 
 // deleteGaugeRefByKey removes gauge ID from an array associated to provided key
 func (k Keeper) deleteGaugeRefByKey(ctx sdk.Context, key []byte, gaugeID uint64) error {
-	var index = -1
 	store := ctx.KVStore(k.storeKey)
 	gaugeIDs := k.getGaugeRefs(ctx, key)
-	gaugeIDs, index = removeValue(gaugeIDs, gaugeID)
+	gaugeIDs, index := removeValue(gaugeIDs, gaugeID)
 	if index < 0 {
 		return fmt.Errorf("specific gauge with ID %d not found", gaugeID)
 	}
diff --git a/x/incentives/types/keys.go b/x/incentives/types/keys.go
@@ -43,12 +43,6 @@ var (
 	// KeyIndexSeparator defines key for merging bytes
 	KeyIndexSeparator = []byte{0x07}
 
-	// KeyCurrentEpoch defines key for storing current epoch
-	KeyCurrentEpoch = []byte{0x08}
-
-	// KeyEpochBeginBlock defines key for storing begin block of current epoch
-	KeyEpochBeginBlock = []byte{0x09}
-
 	// LockableDurationsKey defines key for storing valid durations for giving incentives
 	LockableDurationsKey = []byte("lockable_durations")
 )
diff --git a/x/lockup/client/cli/tx.go b/x/lockup/client/cli/tx.go
@@ -36,7 +36,7 @@ func GetTxCmd() *cobra.Command {
 // NewLockTokensCmd lock tokens into lockup pool from user's account
 func NewLockTokensCmd() *cobra.Command {
 	cmd := &cobra.Command{
-		Use:   "lock-tokens",
+		Use:   "lock-tokens [tokens]",
 		Short: "lock tokens into lockup pool from user account",
 		Args:  cobra.ExactArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
@@ -106,7 +106,7 @@ func NewBeginUnlockingCmd() *cobra.Command {
 // NewBeginUnlockByIDCmd unlock individual period lock by ID
 func NewBeginUnlockByIDCmd() *cobra.Command {
 	cmd := &cobra.Command{
-		Use:   "begin-unlock-by-id",
+		Use:   "begin-unlock-by-id [id]",
 		Short: "begin unlock individual period lock by ID",
 		Args:  cobra.ExactArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
diff --git a/x/mint/keeper/keeper.go b/x/mint/keeper/keeper.go
@@ -206,7 +206,8 @@ func (k Keeper) DistributeMintedCoin(ctx sdk.Context, mintedCoin sdk.Coin) error
 		}
 	}
 
-	communityPoolCoins := sdk.NewCoins(k.GetProportions(ctx, mintedCoin, proportions.CommunityPool))
+	// subtract from original provision to ensure no coins left over after the allocations
+	communityPoolCoins := sdk.NewCoins(mintedCoin).Sub(stakingIncentivesCoins).Sub(poolIncentivesCoins).Sub(devRewardCoins)
 	err = k.distrKeeper.FundCommunityPool(ctx, communityPoolCoins, k.accountKeeper.GetModuleAddress(types.ModuleName))
 	if err != nil {
 		return err
diff --git a/x/pool-incentives/genesis.go b/x/pool-incentives/genesis.go
@@ -1,20 +1,26 @@
-package keeper
+package pool_incentives
 
 import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 
+	"github.com/osmosis-labs/osmosis/x/pool-incentives/keeper"
 	"github.com/osmosis-labs/osmosis/x/pool-incentives/types"
 )
 
-func (k Keeper) InitGenesis(ctx sdk.Context, genState *types.GenesisState) {
+func InitGenesis(ctx sdk.Context, k keeper.Keeper, genState *types.GenesisState) {
 	k.SetParams(ctx, genState.Params)
 	k.SetLockableDurations(ctx, genState.LockableDurations)
-	if genState.DistrInfo != nil {
+	if genState.DistrInfo == nil {
+		k.SetDistrInfo(ctx, types.DistrInfo{
+			TotalWeight: sdk.NewInt(0),
+			Records:     nil,
+		})
+	} else {
 		k.SetDistrInfo(ctx, *genState.DistrInfo)
 	}
 }
 
-func (k Keeper) ExportGenesis(ctx sdk.Context) *types.GenesisState {
+func ExportGenesis(ctx sdk.Context, k keeper.Keeper) *types.GenesisState {
 	distrInfo := k.GetDistrInfo(ctx)
 
 	return &types.GenesisState{
diff --git a/x/pool-incentives/genesis_test.go b/x/pool-incentives/genesis_test.go
@@ -9,6 +9,7 @@ import (
 	pool_incentives "github.com/osmosis-labs/osmosis/x/pool-incentives"
 	"github.com/osmosis-labs/osmosis/x/pool-incentives/types"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	tmproto "github.com/tendermint/tendermint/proto/tendermint/types"
 )
 
@@ -43,7 +44,7 @@ func TestMarshalUnmarshalGenesis(t *testing.T) {
 	am := pool_incentives.NewAppModule(appCodec, app.PoolIncentivesKeeper)
 
 	genesis := testGenesis
-	app.PoolIncentivesKeeper.InitGenesis(ctx, &genesis)
+	pool_incentives.InitGenesis(ctx, app.PoolIncentivesKeeper, &genesis)
 
 	genesisExported := am.ExportGenesis(ctx, appCodec)
 	assert.NotPanics(t, func() {
@@ -54,3 +55,43 @@ func TestMarshalUnmarshalGenesis(t *testing.T) {
 		am.InitGenesis(ctx, appCodec, genesisExported)
 	})
 }
+
+func TestInitGenesis(t *testing.T) {
+	app := simapp.Setup(false)
+	ctx := app.BaseApp.NewContext(false, tmproto.Header{})
+	ctx = ctx.WithBlockTime(now.Add(time.Second))
+	genesis := testGenesis
+	pool_incentives.InitGenesis(ctx, app.PoolIncentivesKeeper, &genesis)
+
+	params := app.PoolIncentivesKeeper.GetParams(ctx)
+	require.Equal(t, params, genesis.Params)
+
+	durations := app.PoolIncentivesKeeper.GetLockableDurations(ctx)
+	require.Equal(t, durations, genesis.LockableDurations)
+
+	distrInfo := app.PoolIncentivesKeeper.GetDistrInfo(ctx)
+	require.Equal(t, distrInfo, *genesis.DistrInfo)
+}
+
+func TestExportGenesis(t *testing.T) {
+	app := simapp.Setup(false)
+	ctx := app.BaseApp.NewContext(false, tmproto.Header{})
+	ctx = ctx.WithBlockTime(now.Add(time.Second))
+	genesis := testGenesis
+	pool_incentives.InitGenesis(ctx, app.PoolIncentivesKeeper, &genesis)
+
+	durations := []time.Duration{
+		time.Second,
+		time.Minute,
+		time.Hour,
+		time.Hour * 5,
+	}
+	app.PoolIncentivesKeeper.SetLockableDurations(ctx, durations)
+	savedDurations := app.PoolIncentivesKeeper.GetLockableDurations(ctx)
+	require.Equal(t, savedDurations, durations)
+
+	genesisExported := pool_incentives.ExportGenesis(ctx, app.PoolIncentivesKeeper)
+	require.Equal(t, genesisExported.Params, genesis.Params)
+	require.Equal(t, genesisExported.LockableDurations, durations)
+	require.Equal(t, genesisExported.DistrInfo, genesis.DistrInfo)
+}
diff --git a/x/pool-incentives/keeper/distr.go b/x/pool-incentives/keeper/distr.go
@@ -71,17 +71,6 @@ func (k Keeper) GetDistrInfo(ctx sdk.Context) types.DistrInfo {
 	store := ctx.KVStore(k.storeKey)
 	bz := store.Get(types.DistrInfoKey)
 
-	if len(bz) == 0 {
-		distrInfo := types.DistrInfo{
-			TotalWeight: sdk.NewInt(0),
-			Records:     nil,
-		}
-		bz = k.cdc.MustMarshalBinaryBare(&distrInfo)
-
-		store.Set(types.DistrInfoKey, bz)
-		return distrInfo
-	}
-
 	distrInfo := types.DistrInfo{}
 	k.cdc.MustUnmarshalBinaryBare(bz, &distrInfo)
 
diff --git a/x/pool-incentives/keeper/genesis_test.go b/x/pool-incentives/keeper/genesis_test.go
diff --git a/x/pool-incentives/module.go b/x/pool-incentives/module.go
diff --git a/x/pool-incentives/types/genesis.go b/x/pool-incentives/types/genesis.go
diff --git a/x/pool-incentives/types/params.go b/x/pool-incentives/types/params.go

Original file line number	Diff line number	Diff line change
`@@ -55,7 +55,7 @@ func ExportGenesis(ctx sdk.Context, k keeper.Keeper) *types.GenesisState {`
`55`	`55`	`poolAnys = append(poolAnys, any)`
`56`	`56`	`}`
`57`	`57`	`return &types.GenesisState{`
`58`		`- NextPoolNumber: k.GetNextPoolNumber(ctx),`
	`58`	`+ NextPoolNumber: k.GetNextPoolNumberAndIncrement(ctx),`
`59`	`59`	`Pools: poolAnys,`
`60`	`60`	`Params: k.GetParams(ctx),`
`61`	`61`	`}`
Original file line number	Diff line number	Diff line change
`@@ -119,7 +119,7 @@ func (k Keeper) NumPools(`
`119`	`119`	`sdkCtx := sdk.UnwrapSDKContext(ctx)`
`120`	`120`
`121`	`121`	`return &types.QueryNumPoolsResponse{`
`122`		`- NumPools: k.GetNextPoolNumber(sdkCtx) - 1,`
	`122`	`+ NumPools: k.GetNextPoolNumberAndIncrement(sdkCtx) - 1,`
`123`	`123`	`}, nil`
`124`	`124`	`}`
`125`	`125`