Nd kavamint touchups #1422

nddeluca · 2022-12-08T20:05:06Z

draft for review -- will rebase into feat branch instead of merging

for genesis and params (and other store objects)

repace proto stringers and remove getters; keep sdk.Dec values strings in protos; increase validation of sdk.Dec values in params; increase coverage; add module account permission checks; ensure import and export of genesis does not change state

nddeluca · 2022-12-08T20:05:29Z

client/docs/config.json

@@ -216,6 +216,23 @@
        ]
      }
    },
+    {
+      "url": "./out/swagger/kava/kavamint/v1beta1/query.swagger.json",


This is required for the swagger to be generated

nddeluca · 2022-12-08T20:06:35Z

proto/kava/kavamint/v1beta1/kavamint.proto

 import "gogoproto/gogo.proto";

 option go_package = "github.com/kava-labs/kava/x/kavamint/types";

 // Params wraps the governance parameters for the kavamint module
 message Params {
+  option (gogoproto.goproto_stringer) = false;


We override the stringer since sdk.Dec doesn't work well with the built-in method

nddeluca · 2022-12-08T20:07:02Z

proto/kava/kavamint/v1beta1/kavamint.proto

  // yearly inflation of total token supply minted to the community pool.
-  bytes community_pool_inflation = 1 [
+  string community_pool_inflation = 1 [


sdk.Dec should be strings - cosmos/cosmos-sdk#10863

nddeluca · 2022-12-08T20:07:34Z

proto/kava/kavamint/v1beta1/query.proto

@@ -11,7 +12,7 @@ option go_package = "github.com/kava-labs/kava/x/kavamint/types";
 service Query {
  // Params queries the parameters of x/kavamint module.
  rpc Params(QueryParamsRequest) returns (QueryParamsResponse) {
-    option (google.api.http).get = "/kava/kavamint/v1beta1/parameters";
+    option (google.api.http).get = "/kava/kavamint/v1beta1/params";


standard is params, though not all modules have followed it

nddeluca · 2022-12-08T20:08:12Z

x/kavamint/abci.go

@@ -12,8 +12,8 @@ import (
 func BeginBlocker(ctx sdk.Context, k keeper.KeeperI) {
 	params := k.GetParams(ctx)
 	// determine seconds since last mint
-	previousBlockTime, found := k.GetPreviousBlockTime(ctx)
-	if !found || previousBlockTime.IsZero() {
+	previousBlockTime := k.GetPreviousBlockTime(ctx)


We just need to protect the zero case -- found is redundant since we have zero value

nddeluca · 2022-12-08T20:08:57Z

x/kavamint/genesis.go


-	if macc := ak.GetModuleAccount(ctx, types.ModuleName); macc == nil {
+	// get module account -- creates one with allowed permissions if it does not exist
+	macc := ak.GetModuleAccount(ctx, types.ModuleName)


This actually creates the module account, so the panic is just a double check incase that behavior changes

yup, originally had no check but @DracoLi suggested i add one.

is pulling the call out of the nil check just for clarity on the fact this code potentially does something rather than just checks if an account exists?

I wrote it both ways, but thought this was cleaner and easier to read without the permission check nested.

Could go either way on it, basically just increased the scope of macc to use below

nddeluca · 2022-12-08T20:09:17Z

x/kavamint/genesis.go

 		panic(fmt.Sprintf("%s module account has not been set", types.ModuleName))
 	}
+
+	// check module account has minter permissions
+	if !macc.HasPermission(authtypes.Minter) {


Good to check required permissions as well

nddeluca · 2022-12-08T20:10:08Z

x/kavamint/genesis.go

+
+	// set store state from genesis
+	keeper.SetParams(ctx, gs.Params)
+	keeper.SetPreviousBlockTime(ctx, gs.PreviousBlockTime)


Change in behavior here -- we allow the zero value to be set, which then gets populated on the first block. Genesis assumes to previous block

nddeluca · 2022-12-08T20:10:49Z

x/kavamint/genesis.go

-		previousBlockTime = ctx.BlockTime()
-	}
-	return types.NewGenesisState(params, previousBlockTime)
+	return types.NewGenesisState(keeper.GetParams(ctx), keeper.GetPreviousBlockTime(ctx))


No modifications to genesis on export.

We test that an imported genesis always results in the same exported genesis -- best not to make modifications outside of abci

nddeluca · 2022-12-08T20:11:28Z

x/kavamint/keeper/keeper.go

 	if err := blockTime.UnmarshalBinary(b); err != nil {
 		panic(err)
 	}
-	return blockTime, true
+
+	return
 }

 // SetPreviousBlockTime set the time of the previous block
 func (k Keeper) SetPreviousBlockTime(ctx sdk.Context, blockTime time.Time) {


We delete the key if the previous block time is blank

nddeluca · 2022-12-08T20:12:45Z

x/kavamint/types/genesis.go

 	time "time"
 )

+var (
+	// DefaultPreviousBlockTime represents a time that is unset -- no previous block has occured
+	DefaultPreviousBlockTime = time.Time{}


change of default to assume no previous block and no accumulation on the first height after initgenesis

@pirtleshell we can set a previous block time explicity in the upgrade handler if we want

nddeluca · 2022-12-08T20:13:21Z

x/kavamint/types/genesis.go

-		return fmt.Errorf("previous block time not set")
-	}
-	return nil
+	return gs.Params.Validate()


Unset is a valid case for new chains since there are no previous blocks

nddeluca · 2022-12-08T20:13:56Z

x/kavamint/types/genesis_test.go

 			}
 		})
 	}
 }
+
+func TestGenesis_JSONEncoding(t *testing.T) {


These json tests help serve as examples and prevent regressions in protobuf encoding/tags

nddeluca · 2022-12-08T20:15:22Z

x/kavamint/types/params.go

-	// TODO consider lowering max rate. when it's this large the precision is very bad.
-	MaxMintingRate = sdk.NewDecWithPrec(1765, 1)
+	// MaxMintingRate returns the per second rate equivalent to 10,000% per year
+	MaxMintingRate = sdk.MustNewDecFromStr("0.000000146028999310")


Reduced the max per second rate significantly -- we should never need a value higher than this. The previous max was too high imo, which if a proposal had a typo could result in a lot of coins minted before another proposal could correct it

Fixed to be the yearly value -- I got confused with the per second rates used in other modules

nddeluca · 2022-12-08T20:15:50Z

x/kavamint/types/params.go

+// String implements fmt.Stringer
+func (p Params) String() string {
+	return fmt.Sprintf(`Params:
+	CommunityPoolInflation: %s


Just a helpful change -- protobuf Stringer() doesn't show sdk.Dec with a decimal place

nddeluca · 2022-12-08T20:16:21Z

x/kavamint/types/params.go

-		return fmt.Errorf("rate out of bounds. the max allowed rate is %s", MaxMintingRate)
+// validateRate ensures rate is properly initialized (non-nil), not negative, and not greater than the max rate
+func validateRate(rate sdk.Dec) error {
+	if rate.IsNil() || rate.IsNegative() || rate.GT(MaxMintingRate) {


It's important to check that sdk.Dec values are not nill as they will panic if somehow set

nddeluca · 2022-12-08T20:27:34Z

x/kavamint/keeper/querier.go

@@ -1,48 +0,0 @@
-package keeper
-


Removed -- the legacy querier paths are not needed for new modules

pirtleshell · 2022-12-08T21:04:27Z

x/kavamint/abci_test.go

@@ -44,7 +42,7 @@ func (suite *abciTestSuite) CheckCommunityPoolBalance(ctx sdk.Context, expectedA
 }

 func TestGRPCQueryTestSuite(t *testing.T) {
-	suite.Run(t, new(abciTestSuite))
+	//suite.Run(t, new(abciTestSuite))


reminder to re-enable this test

pirtleshell · 2022-12-08T21:06:29Z

x/kavamint/genesis.go


-	if macc := ak.GetModuleAccount(ctx, types.ModuleName); macc == nil {
+	// get module account -- creates one with allowed permissions if it does not exist
+	macc := ak.GetModuleAccount(ctx, types.ModuleName)


yup, originally had no check but @DracoLi suggested i add one.

is pulling the call out of the nil check just for clarity on the fact this code potentially does something rather than just checks if an account exists?

pirtleshell · 2022-12-08T21:59:30Z

x/kavamint/keeper/keeper.go

 }

 // SetPreviousBlockTime set the time of the previous block
 func (k Keeper) SetPreviousBlockTime(ctx sdk.Context, blockTime time.Time) {
 	store := prefix.NewStore(ctx.KVStore(k.storeKey), types.PreviousBlockTimeKey)
+
+	if blockTime.IsZero() {
+		store.Delete(types.PreviousBlockTimeKey)


🧠 nice, this makes sense.

pirtleshell · 2022-12-08T22:03:22Z

x/kavamint/types/genesis_test.go

 		},
 		{
-			"invalid - no time set",
+			"vali - no time set",


Suggested change

"vali - no time set",

"valid - no time set",

pirtleshell · 2022-12-08T22:04:30Z

x/kavamint/types/keys.go

 )

 // PreviousBlockTimeKey is the store key for the previous block time
-var PreviousBlockTimeKey = []byte{0x00}
+var PreviousBlockTimeKey = []byte{0x01}


start w/ 1 to prevent confusion with nil value?

hmm I think I fat fingered a key here, didn't mean to modify it

nddeluca added 5 commits December 8, 2022 20:03

use string type for sdk.Dec proto fields; disable generation of getters

1b676dc

for genesis and params (and other store objects)

follow sdk conventions for params endpoint naming

8a50a1b

add kavamint to swagger; update swagger & proto docs

7712ba3

remove legacy querier; touch up module to match others

a3cab01

nddeluca commented Dec 8, 2022

View reviewed changes

fix scaling of rate values -- should be yearly rates

3053443

pirtleshell reviewed Dec 8, 2022

View reviewed changes

nddeluca merged commit 3053443 into feat/community-pool-enhancements Dec 8, 2022

nddeluca deleted the nd-kavamint-touchups branch December 8, 2022 22:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nd kavamint touchups #1422

Nd kavamint touchups #1422

nddeluca commented Dec 8, 2022

nddeluca Dec 8, 2022

nddeluca Dec 8, 2022

nddeluca Dec 8, 2022

nddeluca Dec 8, 2022

nddeluca Dec 8, 2022

nddeluca Dec 8, 2022

pirtleshell Dec 8, 2022

nddeluca Dec 8, 2022

nddeluca Dec 8, 2022

nddeluca Dec 8, 2022

nddeluca Dec 8, 2022

nddeluca Dec 8, 2022 •

edited

Loading

nddeluca Dec 8, 2022

nddeluca Dec 8, 2022

nddeluca Dec 8, 2022

nddeluca Dec 8, 2022

nddeluca Dec 8, 2022 •

edited

Loading

nddeluca Dec 8, 2022

nddeluca Dec 8, 2022

nddeluca Dec 8, 2022

nddeluca Dec 8, 2022

pirtleshell Dec 8, 2022

pirtleshell Dec 8, 2022

pirtleshell Dec 8, 2022

pirtleshell Dec 8, 2022

pirtleshell Dec 8, 2022

nddeluca Dec 8, 2022

Nd kavamint touchups #1422

Nd kavamint touchups #1422

Conversation

nddeluca commented Dec 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nddeluca Dec 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nddeluca Dec 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nddeluca Dec 8, 2022 •

edited

Loading

nddeluca Dec 8, 2022 •

edited

Loading