Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Paramchange proposal should only contain updated fields #4247

Closed
mossid opened this issue May 1, 2019 · 3 comments · Fixed by #4403
Closed

Paramchange proposal should only contain updated fields #4247

mossid opened this issue May 1, 2019 · 3 comments · Fixed by #4403
Assignees
Milestone

Comments

@mossid
Copy link
Contributor

mossid commented May 1, 2019

For most of the parameter types, we need to duplicate the existing fields to not make it empty. For example in the gov(https://github.com/cosmos/cosmos-sdk/blob/master/x/gov/params.go),

// 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 propotion 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
}

And say the current param is TallyParams{Quorum: q, Threshold: t, Veto: v}. If a proposal wants to update only the Veto field, it has to submit a proposal contaning TallyParams{Quorum: q, Threshold: t, Veto: v_updated} as its value, not only the Veto field.

To solve this, add omitempty to all the parameter fields. This will make the decoder not override the existing value if the unmarshalling json text does not contain the corresponding value. So the proposer only has to write the fields that are going to be updated. In the example above, now the proposer can submit TallyFields{Veto: v_updated}.

The paramchange logic also should have to be updated, not just setting the provided parameter, but first unmarshalling it to the copy of the existing parameter, and then setting it.

Depending on: tendermint/go-amino#262

@fedekunze
Copy link
Collaborator

cc: @okwme this could be useful for the EditMetadata on the NFT implementation

@alexanderbez
Copy link
Contributor

Blocked on #4342

@alexanderbez
Copy link
Contributor

For reference, this is only applicable in the context of governance parameters. No other module uses structured parameters like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants