Merge PR #4403: Paramchange proposal skips omitempty fields

Author: mossid

.pending/improvements/sdk/4403-parameter-chang

@@ -0,0 +1,2 @@
+#4403 Allow for parameter change proposals to supply only desired fields to be updated
+in objects instead of the entire object (only applies to values that are objects).

x/gov/keeper.go

@@ -11,26 +11,12 @@ import (
 	"github.com/tendermint/tendermint/libs/log"
 )
 
-// Parameter store key
 var (
-	ParamStoreKeyDepositParams = []byte("depositparams")
-	ParamStoreKeyVotingParams  = []byte("votingparams")
-	ParamStoreKeyTallyParams   = []byte("tallyparams")
-
 	// TODO: Find another way to implement this without using accounts, or find a cleaner way to implement it using accounts.
 	DepositedCoinsAccAddr     = sdk.AccAddress(crypto.AddressHash([]byte("govDepositedCoins")))
 	BurnedDepositCoinsAccAddr = sdk.AccAddress(crypto.AddressHash([]byte("govBurnedDepositCoins")))
 )
 
-// Key declaration for parameters
-func ParamKeyTable() params.KeyTable {
-	return params.NewKeyTable(
-		ParamStoreKeyDepositParams, DepositParams{},
-		ParamStoreKeyVotingParams, VotingParams{},
-		ParamStoreKeyTallyParams, TallyParams{},
-	)
-}
-
 // Governance Keeper
 type Keeper struct {
 	// The reference to the Param Keeper to get and set Global Params

x/gov/params.go

@@ -5,12 +5,29 @@ import (
 	"time"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
+	"github.com/cosmos/cosmos-sdk/x/params"
 )
 
+// Parameter store key
+var (
+	ParamStoreKeyDepositParams = []byte("depositparams")
+	ParamStoreKeyVotingParams  = []byte("votingparams")
+	ParamStoreKeyTallyParams   = []byte("tallyparams")
+)
+
+// Key declaration for parameters
+func ParamKeyTable() params.KeyTable {
+	return params.NewKeyTable(
+		ParamStoreKeyDepositParams, DepositParams{},
+		ParamStoreKeyVotingParams, VotingParams{},
+		ParamStoreKeyTallyParams, TallyParams{},
+	)
+}
+
 // Param around deposits for governance
 type DepositParams struct {
-	MinDeposit       sdk.Coins     `json:"min_deposit"`        //  Minimum deposit for a proposal to enter voting period.
-	MaxDepositPeriod time.Duration `json:"max_deposit_period"` //  Maximum period for Atom holders to deposit on a proposal. Initial value: 2 months
+	MinDeposit       sdk.Coins     `json:"min_deposit,omitempty"`        //  Minimum deposit for a proposal to enter voting period.
+	MaxDepositPeriod time.Duration `json:"max_deposit_period,omitempty"` //  Maximum period for Atom holders to deposit on a proposal. Initial value: 2 months
 }
 
 // NewDepositParams creates a new DepositParams object
@@ -34,9 +51,9 @@ func (dp DepositParams) Equal(dp2 DepositParams) bool {
 
 // Param around Tallying votes in governance
 type TallyParams struct {
-	Quorum    sdk.Dec `json:"quorum"`    //  Minimum percentage of total stake needed to vote for a result to be considered valid
-	Threshold sdk.Dec `json:"threshold"` //  Minimum proportion of Yes votes for proposal to pass. Initial value: 0.5
-	Veto      sdk.Dec `json:"veto"`      //  Minimum value of Veto votes to Total votes ratio for proposal to be vetoed. Initial value: 1/3
+	Quorum    sdk.Dec `json:"quorum,omitempty"`    //  Minimum percentage of total stake needed to vote for a result to be considered valid
+	Threshold sdk.Dec `json:"threshold,omitempty"` //  Minimum proportion of Yes votes for proposal to pass. Initial value: 0.5
+	Veto      sdk.Dec `json:"veto,omitempty"`      //  Minimum value of Veto votes to Total votes ratio for proposal to be vetoed. Initial value: 1/3
 }
 
 // NewTallyParams creates a new TallyParams object
@@ -58,7 +75,7 @@ func (tp TallyParams) String() string {
 
 // Param around Voting in governance
 type VotingParams struct {
-	VotingPeriod time.Duration `json:"voting_period"` //  Length of the voting period.
+	VotingPeriod time.Duration `json:"voting_period,omitempty"` //  Length of the voting period.
 }
 
 // NewVotingParams creates a new VotingParams object

x/gov/test_common.go

@@ -40,9 +40,11 @@ func getMockApp(t *testing.T, numGenAccs int, genState GenesisState, genAccs []a
 	tKeyStaking := sdk.NewTransientStoreKey(staking.TStoreKey)
 	keyGov := sdk.NewKVStoreKey(StoreKey)
 
-	rtr := NewRouter().AddRoute(RouterKey, ProposalHandler)
-
 	pk := mApp.ParamsKeeper
+
+	rtr := NewRouter().
+		AddRoute(RouterKey, ProposalHandler)
+
 	ck := bank.NewBaseKeeper(mApp.AccountKeeper, mApp.ParamsKeeper.Subspace(bank.DefaultParamspace), bank.DefaultCodespace)
 	sk := staking.NewKeeper(mApp.Cdc, keyStaking, tKeyStaking, ck, pk.Subspace(staking.DefaultParamspace), staking.DefaultCodespace)
 	keeper := NewKeeper(mApp.Cdc, keyGov, pk, pk.Subspace("testgov"), ck, sk, DefaultCodespace, rtr)

x/params/alias.go

@@ -37,6 +37,7 @@ var (
 	ErrEmptyValue              = types.ErrEmptyValue
 	NewParameterChangeProposal = types.NewParameterChangeProposal
 	NewParamChange             = types.NewParamChange
+	NewParamChangeWithSubkey   = types.NewParamChangeWithSubkey
 	ValidateChanges            = types.ValidateChanges
 )
 

x/params/client/cli/tx.go

@@ -26,15 +26,16 @@ func GetCmdSubmitProposal(cdc *codec.Codec) *cobra.Command {
 		Short: "Submit a parameter change proposal",
 		Long: strings.TrimSpace(
 			fmt.Sprintf(`Submit a parameter proposal along with an initial deposit.
-The proposal details must be supplied via a JSON file.
+The proposal details must be supplied via a JSON file. For values that contains
+objects, only non-empty fields will be updated.
 
 IMPORTANT: Currently parameter changes are evaluated but not validated, so it is
 very important that any "value" change is valid (ie. correct type and within bounds)
 for its respective parameter, eg. "MaxValidators" should be an integer and not a decimal.
 
 Proper vetting of a parameter change proposal should prevent this from happening
 (no deposits should occur during the governance process), but it should be noted
-regardless. 
+regardless.
 
 Example:
 $ %s tx gov submit-proposal param-change <path/to/proposal.json> --from=<key_or_address>

x/params/client/utils/utils.go

@@ -51,7 +51,7 @@ func NewParamChangeJSON(subspace, key, subkey string, value json.RawMessage) Par
 
 // ToParamChange converts a ParamChangeJSON object to ParamChange.
 func (pcj ParamChangeJSON) ToParamChange() params.ParamChange {
-	return params.NewParamChange(pcj.Subspace, pcj.Key, pcj.Subkey, string(pcj.Value))
+	return params.NewParamChangeWithSubkey(pcj.Subspace, pcj.Key, pcj.Subkey, string(pcj.Value))
 }
 
 // ToParamChanges converts a slice of ParamChangeJSON objects to a slice of

x/params/keeper_test.go

@@ -6,40 +6,10 @@ import (
 
 	"github.com/stretchr/testify/require"
 
-	abci "github.com/tendermint/tendermint/abci/types"
-	dbm "github.com/tendermint/tendermint/libs/db"
-	"github.com/tendermint/tendermint/libs/log"
-
-	"github.com/cosmos/cosmos-sdk/codec"
-	"github.com/cosmos/cosmos-sdk/store"
 	"github.com/cosmos/cosmos-sdk/store/prefix"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 )
 
-func defaultContext(key sdk.StoreKey, tkey sdk.StoreKey) sdk.Context {
-	db := dbm.NewMemDB()
-	cms := store.NewCommitMultiStore(db)
-	cms.MountStoreWithDB(key, sdk.StoreTypeIAVL, db)
-	cms.MountStoreWithDB(tkey, sdk.StoreTypeTransient, db)
-	cms.LoadLatestVersion()
-	ctx := sdk.NewContext(cms, abci.Header{}, false, log.NewNopLogger())
-	return ctx
-}
-
-type invalid struct{}
-
-type s struct {
-	I int
-}
-
-func createTestCodec() *codec.Codec {
-	cdc := codec.New()
-	sdk.RegisterCodec(cdc)
-	cdc.RegisterConcrete(s{}, "test/s", nil)
-	cdc.RegisterConcrete(invalid{}, "test/invalid", nil)
-	return cdc
-}
-
 func TestKeeper(t *testing.T) {
 	kvs := []struct {
 		key   string
@@ -66,11 +36,8 @@ func TestKeeper(t *testing.T) {
 		[]byte("extra2"), string(""),
 	)
 
-	cdc := codec.New()
-	skey := sdk.NewKVStoreKey("test")
-	tkey := sdk.NewTransientStoreKey("transient_test")
-	ctx := defaultContext(skey, tkey)
-	keeper := NewKeeper(cdc, skey, tkey, DefaultCodespace)
+	cdc, ctx, skey, _, keeper := testComponents()
+
 	store := prefix.NewStore(ctx.KVStore(skey), []byte("test/"))
 	space := keeper.Subspace("test").WithKeyTable(table)
 
@@ -137,11 +104,7 @@ func indirect(ptr interface{}) interface{} {
 }
 
 func TestSubspace(t *testing.T) {
-	cdc := createTestCodec()
-	key := sdk.NewKVStoreKey("test")
-	tkey := sdk.NewTransientStoreKey("transient_test")
-	ctx := defaultContext(key, tkey)
-	keeper := NewKeeper(cdc, key, tkey, DefaultCodespace)
+	cdc, ctx, key, _, keeper := testComponents()
 
 	kvs := []struct {
 		key   string
@@ -216,3 +179,34 @@ func TestSubspace(t *testing.T) {
 		require.Equal(t, kv.param, indirect(kv.ptr), "stored param not equal, tc #%d", i)
 	}
 }
+
+type paramJSON struct {
+	Param1 int64  `json:"param1,omitempty"`
+	Param2 string `json:"param2,omitempty"`
+}
+
+func TestJSONUpdate(t *testing.T) {
+	_, ctx, _, _, keeper := testComponents()
+
+	key := []byte("key")
+
+	space := keeper.Subspace("test").WithKeyTable(NewKeyTable(key, paramJSON{}))
+
+	var param paramJSON
+
+	space.Update(ctx, key, []byte(`{"param1": "10241024"}`))
+	space.Get(ctx, key, &param)
+	require.Equal(t, paramJSON{10241024, ""}, param)
+
+	space.Update(ctx, key, []byte(`{"param2": "helloworld"}`))
+	space.Get(ctx, key, &param)
+	require.Equal(t, paramJSON{10241024, "helloworld"}, param)
+
+	space.Update(ctx, key, []byte(`{"param1": "20482048"}`))
+	space.Get(ctx, key, &param)
+	require.Equal(t, paramJSON{20482048, "helloworld"}, param)
+
+	space.Update(ctx, key, []byte(`{"param1": "40964096", "param2": "goodbyeworld"}`))
+	space.Get(ctx, key, &param)
+	require.Equal(t, paramJSON{40964096, "goodbyeworld"}, param)
+}

x/params/proposal_handler.go

@@ -32,12 +32,13 @@ func handleParameterChangeProposal(ctx sdk.Context, k Keeper, p ParameterChangeP
 			k.Logger(ctx).Info(
 				fmt.Sprintf("setting new parameter; key: %s, value: %s", c.Key, c.Value),
 			)
-			err = ss.SetRaw(ctx, []byte(c.Key), []byte(c.Value))
+
+			err = ss.Update(ctx, []byte(c.Key), []byte(c.Value))
 		} else {
 			k.Logger(ctx).Info(
 				fmt.Sprintf("setting new parameter; key: %s, subkey: %s, value: %s", c.Key, c.Subspace, c.Value),
 			)
-			err = ss.SetRawWithSubkey(ctx, []byte(c.Key), []byte(c.Subkey), []byte(c.Value))
+			err = ss.UpdateWithSubkey(ctx, []byte(c.Key), []byte(c.Subkey), []byte(c.Value))
 		}
 
 		if err != nil {

x/params/proposal_handler_test.go

@@ -27,16 +27,24 @@ var (
 	_ subspace.ParamSet = (*testParams)(nil)
 
 	keyMaxValidators = "MaxValidators"
+	keySlashingRate  = "SlashingRate"
 	testSubspace     = "TestSubspace"
 )
 
+type testParamsSlashingRate struct {
+	DoubleSign uint16 `json:"double_sign,omitempty"`
+	Downtime   uint16 `json:"downtime,omitempty"`
+}
+
 type testParams struct {
-	MaxValidators uint16 `json:"max_validators"` // maximum number of validators (max uint16 = 65535)
+	MaxValidators uint16                 `json:"max_validators"` // maximum number of validators (max uint16 = 65535)
+	SlashingRate  testParamsSlashingRate `json:"slashing_rate"`
 }
 
 func (tp *testParams) ParamSetPairs() subspace.ParamSetPairs {
 	return subspace.ParamSetPairs{
 		{[]byte(keyMaxValidators), &tp.MaxValidators},
+		{[]byte(keySlashingRate), &tp.SlashingRate},
 	}
 }
 
@@ -76,7 +84,7 @@ func TestProposalHandlerPassed(t *testing.T) {
 		params.NewKeyTable().RegisterParamSet(&testParams{}),
 	)
 
-	tp := testProposal(params.NewParamChange(testSubspace, keyMaxValidators, "", "1"))
+	tp := testProposal(params.NewParamChange(testSubspace, keyMaxValidators, "1"))
 	hdlr := params.NewParamChangeProposalHandler(input.keeper)
 	require.NoError(t, hdlr(input.ctx, tp))
 
@@ -91,9 +99,31 @@ func TestProposalHandlerFailed(t *testing.T) {
 		params.NewKeyTable().RegisterParamSet(&testParams{}),
 	)
 
-	tp := testProposal(params.NewParamChange(testSubspace, keyMaxValidators, "", "invalidType"))
+	tp := testProposal(params.NewParamChange(testSubspace, keyMaxValidators, "invalidType"))
 	hdlr := params.NewParamChangeProposalHandler(input.keeper)
 	require.Error(t, hdlr(input.ctx, tp))
 
 	require.False(t, ss.Has(input.ctx, []byte(keyMaxValidators)))
 }
+
+func TestProposalHandlerUpdateOmitempty(t *testing.T) {
+	input := newTestInput(t)
+	ss := input.keeper.Subspace(testSubspace).WithKeyTable(
+		params.NewKeyTable().RegisterParamSet(&testParams{}),
+	)
+
+	hdlr := params.NewParamChangeProposalHandler(input.keeper)
+	var param testParamsSlashingRate
+
+	tp := testProposal(params.NewParamChange(testSubspace, keySlashingRate, `{"downtime": 7}`))
+	require.NoError(t, hdlr(input.ctx, tp))
+
+	ss.Get(input.ctx, []byte(keySlashingRate), &param)
+	require.Equal(t, testParamsSlashingRate{0, 7}, param)
+
+	tp = testProposal(params.NewParamChange(testSubspace, keySlashingRate, `{"double_sign": 10}`))
+	require.NoError(t, hdlr(input.ctx, tp))
+
+	ss.Get(input.ctx, []byte(keySlashingRate), &param)
+	require.Equal(t, testParamsSlashingRate{10, 7}, param)
+}