diff --git a/PENDING.md b/PENDING.md index 6ef5c3717a..654e342f8c 100644 --- a/PENDING.md +++ b/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 diff --git a/docs/spec/params/README.md b/docs/spec/params/README.md new file mode 100644 index 0000000000..d9c8b9221f --- /dev/null +++ b/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) diff --git a/docs/spec/params/keeper.md b/docs/spec/params/keeper.md new file mode 100644 index 0000000000..fe376911ed --- /dev/null +++ b/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) +} +``` diff --git a/docs/spec/params/subspace.md b/docs/spec/params/subspace.md new file mode 100644 index 0000000000..3781778982 --- /dev/null +++ b/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, ¶m) +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. + diff --git a/store/prefixstore_test.go b/store/prefixstore_test.go index 8445991b6e..4e555292c3 100644 --- a/store/prefixstore_test.go +++ b/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]++ } diff --git a/x/params/doc.go b/x/params/doc.go index 06d6620b20..bee45e79ec 100644 --- a/x/params/doc.go +++ b/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,26 +33,29 @@ 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, ¶m) 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}, @@ -60,26 +63,26 @@ be able to be passed to SetFromParamStruct. } 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) } */ diff --git a/x/params/keeper_test.go b/x/params/keeper_test.go index 640661a245..585db3c410 100644 --- a/x/params/keeper_test.go +++ b/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), ¶m) }, "store.Get panics, tc #%d", i) + require.NotPanics(t, func() { space.Get(ctx, []byte(kv.key), ¶m) }, "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, ¶m) 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 bool - require.Panics(t, func() { store.Get(ctx, []byte(kv.key), ¶m) }, "invalid store.Get not panics, tc #%d", i) + var param int64 + bz := store.Get([]byte(kv.key)) + require.NotNil(t, bz, "KVStore.Get returns nil, tc #%d", i) + err := cdc.UnmarshalJSON(bz, ¶m) + 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 { - require.Panics(t, func() { store.Set(ctx, []byte(kv.key), true) }, "invalid store.Set not panics, tc #%d", i) + var param bool + require.Panics(t, func() { space.Get(ctx, []byte(kv.key), ¶m) }, "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 { + 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), ¶m) }) + 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), ¶m) }) + 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() { 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.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.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("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(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) + 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) + } + + // 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) } } diff --git a/x/params/subspace/subspace.go b/x/params/subspace/subspace.go index 7c9ca5fdaa..fe5889a908 100644 --- a/x/params/subspace/subspace.go +++ b/x/params/subspace/subspace.go @@ -7,10 +7,6 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" ) -// Additional capicity to be allocated for Subspace.name -// So we don't have to allocate extra space each time appending to the key -const extraKeyCap = 20 - // Individual parameter store for each keeper // Transient store persists for a block, so we use it for // recording whether the parameter has been changed or not @@ -30,32 +26,35 @@ func NewSubspace(cdc *codec.Codec, key sdk.StoreKey, tkey sdk.StoreKey, name str cdc: cdc, key: key, tkey: tkey, + name: []byte(name), + table: TypeTable{ + m: make(map[string]reflect.Type), + }, } - namebz := []byte(name) - res.name = make([]byte, len(namebz), len(namebz)+extraKeyCap) - copy(res.name, namebz) return } // WithTypeTable initializes TypeTable and returns modified Subspace -func (s Subspace) WithTypeTable(table TypeTable) (res Subspace) { - if table == nil { +func (s Subspace) WithTypeTable(table TypeTable) Subspace { + if table.m == nil { panic("SetTypeTable() called with nil TypeTable") } - if s.table != nil { - panic("SetTypeTable() called on initialized Subspace") + if len(s.table.m) != 0 { + panic("SetTypeTable() called on already initialized Subspace") } - res = Subspace{ - cdc: s.cdc, - key: s.key, - tkey: s.tkey, - name: s.name, - table: table, + for k, v := range table.m { + s.table.m[k] = v } - return + // Allocate additional capicity for Subspace.name + // So we don't have to allocate extra space each time appending to the key + name := s.name + s.name = make([]byte, len(name), len(name)+table.maxKeyLength()) + copy(s.name, name) + + return s } // Returns a KVStore identical with ctx.KVStore(s.key).Prefix() @@ -118,7 +117,7 @@ func (s Subspace) Modified(ctx sdk.Context, key []byte) bool { func (s Subspace) Set(ctx sdk.Context, key []byte, param interface{}) { store := s.kvStore(ctx) - ty, ok := s.table[string(key)] + ty, ok := s.table.m[string(key)] if !ok { panic("Parameter not registered") } diff --git a/x/params/subspace/table.go b/x/params/subspace/table.go index 363d0243d2..19e41da090 100644 --- a/x/params/subspace/table.go +++ b/x/params/subspace/table.go @@ -5,7 +5,9 @@ import ( ) // TypeTable subspaces appropriate type for each parameter key -type TypeTable map[string]reflect.Type +type TypeTable struct { + m map[string]reflect.Type +} // Constructs new table func NewTypeTable(keytypes ...interface{}) (res TypeTable) { @@ -13,7 +15,9 @@ func NewTypeTable(keytypes ...interface{}) (res TypeTable) { panic("odd number arguments in NewTypeTypeTable") } - res = make(map[string]reflect.Type) + res = TypeTable{ + m: make(map[string]reflect.Type), + } for i := 0; i < len(keytypes); i += 2 { res = res.RegisterType(keytypes[i].([]byte), keytypes[i+1]) @@ -22,10 +26,27 @@ func NewTypeTable(keytypes ...interface{}) (res TypeTable) { return } +func isAlphaNumeric(key []byte) bool { + for _, b := range key { + if !((48 <= b && b <= 57) || // numeric + (65 <= b && b <= 90) || // upper case + (97 <= b && b <= 122)) { // lower case + return false + } + } + return true +} + // Register single key-type pair func (t TypeTable) RegisterType(key []byte, ty interface{}) TypeTable { + if len(key) == 0 { + panic("cannot register empty key") + } + if !isAlphaNumeric(key) { + panic("non alphanumeric parameter key") + } keystr := string(key) - if _, ok := t[keystr]; ok { + if _, ok := t.m[keystr]; ok { panic("duplicate parameter key") } @@ -36,7 +57,7 @@ func (t TypeTable) RegisterType(key []byte, ty interface{}) TypeTable { rty = rty.Elem() } - t[keystr] = rty + t.m[keystr] = rty return t } @@ -48,3 +69,13 @@ func (t TypeTable) RegisterParamSet(ps ParamSet) TypeTable { } return t } + +func (t TypeTable) maxKeyLength() (res int) { + for k := range t.m { + l := len(k) + if l > res { + res = l + } + } + return +} diff --git a/x/params/subspace/table_test.go b/x/params/subspace/table_test.go new file mode 100644 index 0000000000..8f9142e1ec --- /dev/null +++ b/x/params/subspace/table_test.go @@ -0,0 +1,35 @@ +package subspace + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +type testparams struct { + i int64 + b bool +} + +func (tp *testparams) KeyValuePairs() KeyValuePairs { + return KeyValuePairs{ + {[]byte("i"), &tp.i}, + {[]byte("b"), &tp.b}, + } +} + +func TestTypeTable(t *testing.T) { + table := NewTypeTable() + + require.Panics(t, func() { table.RegisterType([]byte(""), nil) }) + require.Panics(t, func() { table.RegisterType([]byte("!@#$%"), nil) }) + require.Panics(t, func() { table.RegisterType([]byte("hello,"), nil) }) + require.Panics(t, func() { table.RegisterType([]byte("hello"), nil) }) + + require.NotPanics(t, func() { table.RegisterType([]byte("hello"), bool(false)) }) + require.NotPanics(t, func() { table.RegisterType([]byte("world"), int64(0)) }) + require.Panics(t, func() { table.RegisterType([]byte("hello"), bool(false)) }) + + require.NotPanics(t, func() { table.RegisterParamSet(&testparams{}) }) + require.Panics(t, func() { table.RegisterParamSet(&testparams{}) }) +}