Merge PR #2491: Paramstore Followup

PENDING.md

@@ -70,6 +70,7 @@ BREAKING CHANGES
     * [x/staking] \#2236 more distribution hooks for distribution
     * [x/stake] \#2394 Split up UpdateValidator into distinct state transitions applied only in EndBlock
     * [x/stake] \#2412 Added an unbonding validator queue to EndBlock to automatically update validator.Status when finished Unbonding
+    * [x/params] Global Paramstore refactored
 
 * Tendermint
   * Update tendermint version from v0.23.0 to v0.25.0, notable changes

docs/spec/params/README.md

@@ -0,0 +1,20 @@
+# Params module specification
+
+## Abstract
+
+Package params provides a globally available parameter store.
+
+There are two main types, Keeper and Subspace. Subspace is an isolated namespace for a
+paramstore, where keys are prefixed by preconfigured spacename. Keeper has a
+permission to access all existing spaces.
+
+Subspace can be used by the individual keepers, who needs a private parameter store
+that the other keeper cannot modify. Keeper can be used by the Governance keeper,
+who need to modify any parameter in case of the proposal passes.
+
+The following contents explains how to use params module for master and user modules.
+
+## Contents
+
+1. [Keeper](keeper.md)
+1. [Subspace](subspace.md)

docs/spec/params/keeper.md

@@ -0,0 +1,17 @@
+# Keeper
+
+In the app initialization stage, `Keeper.Subspace(Paramspace)` is passed to the user modules, and the subspaces are stored in `Keeper.spaces`. Later it can be retrieved with `Keeper.GetSubspace`, so the keepers holding `Keeper` can access to any subspace. For example, Gov module can take `Keeper` as its argument and modify parameter of any subspace when a `ParameterChangeProposal` is accepted.  
+
+```go
+type MasterKeeper struct {
+	pk params.Keeper
+}
+
+func (k MasterKeeper) SetParam(ctx sdk.Context, space string, key string, param interface{}) {
+	space, ok := k.ps.GetSubspace(space)
+	if !ok {
+		return
+	}
+	space.Set(ctx, key, param)
+}
+```

docs/spec/params/subspace.md

@@ -0,0 +1,76 @@
+# Subspace
+
+## Basic Usage
+
+First, declare parameter space and parameter keys for the module. Then include params.Subspace in the keeper. Since we prefix the keys with the spacename, it is recommended to use the same name with the module's.
+
+```go
+const (
+	DefaultParamspace = "mymodule"
+)
+
+const (
+	KeyParameter1 = "myparameter1"
+	KeyParameter2 = "myparameter2"
+)
+
+type Keeper struct {
+	cdc *wire.Codec
+	key sdk.StoreKey
+
+	ps params.Subspace
+}
+```
+
+Pass a params.Subspace to NewKeeper with DefaultParamSubspace (or another)
+
+```go
+app.myKeeper = mymodule.NewKeeper(cdc, key, app.paramStore.SubStore(mymodule.DefaultParamspace))
+```
+
+`NewKeeper` should register a `TypeTable`, which defines a map from parameter keys from types.
+
+```go
+func NewKeeper(cdc *codec.Codec, key sdk.StoreKey, space params.Subspace) Keeper {
+    return Keeper {
+        cdc: cdc,
+        key: key,
+        ps: space.WithTypeTable(ParamTypeTable()),
+    }
+}
+```
+
+Now we can access to the paramstore using Paramstore Keys
+
+```go
+var param MyStruct
+k.ps.Get(KeyParameter1, &param)
+k.ps.Set(KeyParameter2, param)
+```
+
+# Genesis Usage
+
+Declare a struct for parameters and make it implement params.ParamSet. It will then be able to be passed to SetParamSet.
+
+```go
+type MyParams struct {
+	Parameter1 uint64
+	Parameter2 string
+}
+
+// Implements params.ParamSet
+// KeyValuePairs must return the list of (ParamKey, PointerToTheField)
+func (p *MyParams) KeyValuePairs() params.KeyValuePairs {
+	return params.KeyFieldPairs {
+		{KeyParameter1, &p.Parameter1},
+		{KeyParameter2, &p.Parameter2},
+	}
+}
+
+func InitGenesis(ctx sdk.Context, k Keeper, data GenesisState) {
+	k.ps.SetParamSet(ctx, &data.params)
+}
+```
+
+The method is pointer receiver because there could be a case that we read from the store and set the result to the struct.
+

store/prefixstore_test.go

@@ -119,10 +119,6 @@ func TestPrefixStoreIterate(t *testing.T) {
 }
 
 func incFirstByte(bz []byte) {
-	if bz[0] == byte(255) {
-		bz[0] = byte(0)
-		return
-	}
 	bz[0]++
 }
 

x/params/doc.go

@@ -3,18 +3,18 @@ package params
 /*
 Package params provides a globally available parameter store.
 
-There are two main types, Keeper and Space. Space is an isolated namespace for a
+There are two main types, Keeper and Subspace. Subspace is an isolated namespace for a
 paramstore, where keys are prefixed by preconfigured spacename. Keeper has a
-permission to access all existing spaces and create new space.
+permission to access all existing spaces.
 
-Space can be used by the individual keepers, who needs a private parameter store
-that the other keeper are not able to modify. Keeper can be used by the Governance
-keeper, who need to modify any parameter in case of the proposal passes.
+Subspace can be used by the individual keepers, who needs a private parameter store
+that the other keeper cannot modify. Keeper can be used by the Governance keeper,
+who need to modify any parameter in case of the proposal passes.
 
 Basic Usage:
 
 First, declare parameter space and parameter keys for the module. Then include
-params.Store in the keeper. Since we prefix the keys with the spacename, it is
+params.Subspace in the keeper. Since we prefix the keys with the spacename, it is
 recommended to use the same name with the module's.
 
 	const (
@@ -33,53 +33,56 @@ recommended to use the same name with the module's.
 		ps params.Subspace
 	}
 
-Pass a params.Store to NewKeeper with DefaultParamSpace (or another)
+Pass a params.Subspace to NewKeeper with DefaultParamspace (or another)
 
 	app.myKeeper = mymodule.NewKeeper(app.paramStore.SubStore(mymodule.DefaultParamspace))
 
 Now we can access to the paramstore using Paramstore Keys
 
+	var param MyStruct
 	k.ps.Get(KeyParameter1, &param)
 	k.ps.Set(KeyParameter2, param)
 
 Genesis Usage:
 
-Declare a struct for parameters and make it implement ParamStruct. It will then
-be able to be passed to SetFromParamStruct.
+Declare a struct for parameters and make it implement params.ParamSet. It will then
+be able to be passed to SetParamSet.
 
 	type MyParams struct {
 		Parameter1 uint64
 		Parameter2 string
 	}
 
-	func (p *MyParams) KeyFieldPairs() params.KeyFieldPairs {
+	// Implements params.ParamSet
+	// KeyValuePairs must return the list of (ParamKey, PointerToTheField)
+	func (p *MyParams) KeyValuePairs() params.KeyValuePairs {
 		return params.KeyFieldPairs {
 			{KeyParameter1, &p.Parameter1},
 			{KeyParameter2, &p.Parameter2},
 		}
 	}
 
 	func InitGenesis(ctx sdk.Context, k Keeper, data GenesisState) {
-		k.ps.SetFromParamStruct(ctx, &data.params)
+		k.ps.SetParamSet(ctx, &data.params)
 	}
 
 The method is pointer receiver because there could be a case that we read from
 the store and set the result to the struct.
 
-Master Permission Usage:
+Master Keeper Usage:
 
 Keepers that require master permission to the paramstore, such as gov, can take
-params.Keeper itself to access all substores(using GetSubstore)
+params.Keeper itself to access all subspace(using GetSubspace)
 
 	type MasterKeeper struct {
-		ps params.Store
+		pk params.Keeper
 	}
 
 	func (k MasterKeeper) SetParam(ctx sdk.Context, space string, key string, param interface{}) {
-		store, ok := k.ps.GetSubstore(space)
+		space, ok := k.pk.GetSubspace(space)
 		if !ok {
 			return
 		}
-		store.Set(ctx, key, param)
+		space.Set(ctx, key, param)
 	}
 */

x/params/keeper_test.go

@@ -65,45 +65,82 @@ func TestKeeper(t *testing.T) {
 		[]byte("extra2"), string(""),
 	)
 
+	cdc := codec.New()
 	skey := sdk.NewKVStoreKey("test")
 	tkey := sdk.NewTransientStoreKey("transient_test")
 	ctx := defaultContext(skey, tkey)
-	store := NewKeeper(codec.New(), skey, tkey).Subspace("test").WithTypeTable(table)
+	keeper := NewKeeper(cdc, skey, tkey)
+	space := keeper.Subspace("test").WithTypeTable(table)
+	store := ctx.KVStore(skey).Prefix([]byte("test/"))
 
+	// Set params
 	for i, kv := range kvs {
-		require.NotPanics(t, func() { store.Set(ctx, []byte(kv.key), kv.param) }, "store.Set panics, tc #%d", i)
+		require.NotPanics(t, func() { space.Set(ctx, []byte(kv.key), kv.param) }, "space.Set panics, tc #%d", i)
 	}
 
+	// Test space.Get
 	for i, kv := range kvs {
 		var param int64
-		require.NotPanics(t, func() { store.Get(ctx, []byte(kv.key), &param) }, "store.Get panics, tc #%d", i)
+		require.NotPanics(t, func() { space.Get(ctx, []byte(kv.key), &param) }, "space.Get panics, tc #%d", i)
 		require.Equal(t, kv.param, param, "stored param not equal, tc #%d", i)
 	}
 
-	cdc := codec.New()
+	// Test space.GetRaw
 	for i, kv := range kvs {
 		var param int64
-		bz := store.GetRaw(ctx, []byte(kv.key))
+		bz := space.GetRaw(ctx, []byte(kv.key))
 		err := cdc.UnmarshalJSON(bz, &param)
 		require.Nil(t, err, "err is not nil, tc #%d", i)
 		require.Equal(t, kv.param, param, "stored param not equal, tc #%d", i)
 	}
 
+	// Test store.Get equals space.Get
+	for i, kv := range kvs {
+		var param int64
+		bz := store.Get([]byte(kv.key))
+		require.NotNil(t, bz, "KVStore.Get returns nil, tc #%d", i)
+		err := cdc.UnmarshalJSON(bz, &param)
+		require.NoError(t, err, "UnmarshalJSON returns error, tc #%d", i)
+		require.Equal(t, kv.param, param, "stored param not equal, tc #%d", i)
+	}
+
+	// Test invalid space.Get
 	for i, kv := range kvs {
 		var param bool
-		require.Panics(t, func() { store.Get(ctx, []byte(kv.key), &param) }, "invalid store.Get not panics, tc #%d", i)
+		require.Panics(t, func() { space.Get(ctx, []byte(kv.key), &param) }, "invalid space.Get not panics, tc #%d", i)
+	}
+
+	// Test invalid space.Set
+	for i, kv := range kvs {
+		require.Panics(t, func() { space.Set(ctx, []byte(kv.key), true) }, "invalid space.Set not panics, tc #%d", i)
 	}
 
+	// Test GetSubspace
 	for i, kv := range kvs {
-		require.Panics(t, func() { store.Set(ctx, []byte(kv.key), true) }, "invalid store.Set not panics, tc #%d", i)
+		var gparam, param int64
+		gspace, ok := keeper.GetSubspace("test")
+		require.True(t, ok, "cannot retrieve subspace, tc #%d", i)
+
+		require.NotPanics(t, func() { gspace.Get(ctx, []byte(kv.key), &gparam) })
+		require.NotPanics(t, func() { space.Get(ctx, []byte(kv.key), &param) })
+		require.Equal(t, gparam, param, "GetSubspace().Get not equal with space.Get, tc #%d", i)
+
+		require.NotPanics(t, func() { gspace.Set(ctx, []byte(kv.key), int64(i)) })
+		require.NotPanics(t, func() { space.Get(ctx, []byte(kv.key), &param) })
+		require.Equal(t, int64(i), param, "GetSubspace().Set not equal with space.Get, tc #%d", i)
 	}
 }
 
-func TestGet(t *testing.T) {
+func indirect(ptr interface{}) interface{} {
+	return reflect.ValueOf(ptr).Elem().Interface()
+}
+
+func TestSubspace(t *testing.T) {
+	cdc := createTestCodec()
 	key := sdk.NewKVStoreKey("test")
 	tkey := sdk.NewTransientStoreKey("transient_test")
 	ctx := defaultContext(key, tkey)
-	keeper := NewKeeper(createTestCodec(), key, tkey)
+	keeper := NewKeeper(cdc, key, tkey)
 
 	kvs := []struct {
 		key   string
@@ -140,29 +177,41 @@ func TestGet(t *testing.T) {
 		[]byte("struct"), s{},
 	)
 
-	store := keeper.Subspace("test").WithTypeTable(table)
+	store := ctx.KVStore(key).Prefix([]byte("test/"))
+	space := keeper.Subspace("test").WithTypeTable(table)
 
+	// Test space.Set, space.Modified
 	for i, kv := range kvs {
-		require.False(t, store.Modified(ctx, []byte(kv.key)), "store.Modified returns true before setting, tc #%d", i)
-		require.NotPanics(t, func() { store.Set(ctx, []byte(kv.key), kv.param) }, "store.Set panics, tc #%d", i)
-		require.True(t, store.Modified(ctx, []byte(kv.key)), "store.Modified returns false after setting, tc #%d", i)
+		require.False(t, space.Modified(ctx, []byte(kv.key)), "space.Modified returns true before setting, tc #%d", i)
+		require.NotPanics(t, func() { space.Set(ctx, []byte(kv.key), kv.param) }, "space.Set panics, tc #%d", i)
+		require.True(t, space.Modified(ctx, []byte(kv.key)), "space.Modified returns false after setting, tc #%d", i)
 	}
 
+	// Test space.Get, space.GetIfExists
 	for i, kv := range kvs {
-		require.NotPanics(t, func() { store.GetIfExists(ctx, []byte("invalid"), kv.ptr) }, "store.GetIfExists panics when no value exists, tc #%d", i)
-		require.Equal(t, kv.zero, reflect.ValueOf(kv.ptr).Elem().Interface(), "store.GetIfExists unmarshalls when no value exists, tc #%d", i)
-		require.Panics(t, func() { store.Get(ctx, []byte("invalid"), kv.ptr) }, "invalid store.Get not panics when no value exists, tc #%d", i)
-		require.Equal(t, kv.zero, reflect.ValueOf(kv.ptr).Elem().Interface(), "invalid store.Get unmarshalls when no value exists, tc #%d", i)
+		require.NotPanics(t, func() { space.GetIfExists(ctx, []byte("invalid"), kv.ptr) }, "space.GetIfExists panics when no value exists, tc #%d", i)
+		require.Equal(t, kv.zero, indirect(kv.ptr), "space.GetIfExists unmarshalls when no value exists, tc #%d", i)
+		require.Panics(t, func() { space.Get(ctx, []byte("invalid"), kv.ptr) }, "invalid space.Get not panics when no value exists, tc #%d", i)
+		require.Equal(t, kv.zero, indirect(kv.ptr), "invalid space.Get unmarshalls when no value exists, tc #%d", i)
+
+		require.NotPanics(t, func() { space.GetIfExists(ctx, []byte(kv.key), kv.ptr) }, "space.GetIfExists panics, tc #%d", i)
+		require.Equal(t, kv.param, indirect(kv.ptr), "stored param not equal, tc #%d", i)
+		require.NotPanics(t, func() { space.Get(ctx, []byte(kv.key), kv.ptr) }, "space.Get panics, tc #%d", i)
+		require.Equal(t, kv.param, indirect(kv.ptr), "stored param not equal, tc #%d", i)
 
-		require.NotPanics(t, func() { store.GetIfExists(ctx, []byte(kv.key), kv.ptr) }, "store.GetIfExists panics, tc #%d", i)
-		require.Equal(t, kv.param, reflect.ValueOf(kv.ptr).Elem().Interface(), "stored param not equal, tc #%d", i)
-		require.NotPanics(t, func() { store.Get(ctx, []byte(kv.key), kv.ptr) }, "store.Get panics, tc #%d", i)
-		require.Equal(t, kv.param, reflect.ValueOf(kv.ptr).Elem().Interface(), "stored param not equal, tc #%d", i)
+		require.Panics(t, func() { space.Get(ctx, []byte("invalid"), kv.ptr) }, "invalid space.Get not panics when no value exists, tc #%d", i)
+		require.Equal(t, kv.param, indirect(kv.ptr), "invalid space.Get unmarshalls when no value existt, tc #%d", i)
 
-		require.Panics(t, func() { store.Get(ctx, []byte("invalid"), kv.ptr) }, "invalid store.Get not panics when no value exists, tc #%d", i)
-		require.Equal(t, kv.param, reflect.ValueOf(kv.ptr).Elem().Interface(), "invalid store.Get unmarshalls when no value existt, tc #%d", i)
+		require.Panics(t, func() { space.Get(ctx, []byte(kv.key), nil) }, "invalid space.Get not panics when the pointer is nil, tc #%d", i)
+		require.Panics(t, func() { space.Get(ctx, []byte(kv.key), new(invalid)) }, "invalid space.Get not panics when the pointer is different type, tc #%d", i)
+	}
 
-		require.Panics(t, func() { store.Get(ctx, []byte(kv.key), nil) }, "invalid store.Get not panics when the pointer is nil, tc #%d", i)
-		require.Panics(t, func() { store.Get(ctx, []byte(kv.key), new(invalid)) }, "invalid store.Get not panics when the pointer is different type, tc #%d", i)
+	// Test store.Get equals space.Get
+	for i, kv := range kvs {
+		bz := store.Get([]byte(kv.key))
+		require.NotNil(t, bz, "store.Get() returns nil, tc #%d", i)
+		err := cdc.UnmarshalJSON(bz, kv.ptr)
+		require.NoError(t, err, "cdc.UnmarshalJSON() returns error, tc #%d", i)
+		require.Equal(t, kv.param, indirect(kv.ptr), "stored param not equal, tc #%d", i)
 	}
 }