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

CLI should show outcome of parameter change proposals #4339

Closed
mossid opened this issue May 14, 2019 · 8 comments
Closed

CLI should show outcome of parameter change proposals #4339

mossid opened this issue May 14, 2019 · 8 comments

Comments

@mossid
Copy link
Contributor

mossid commented May 14, 2019

After #4247 is implemented, the parameter change proposal content will not completely override the existing parameter, potentially set different value from the proposers/voters' intention.

The CLI should show the expected parameter that will be set after the proposal passes, depending on the current parameter set, by locally simulating the parameter change proposal handler.

  1. Separate the parameter overwriting logic from the parameter change proposal handler, and make it public

  2. Make the CLI call this function with (current_parameter, provided_update) before submitting the transaction, and prints the expected updated parameter

@alexanderbez
Copy link
Contributor

alexanderbez commented May 14, 2019

I don't follow @mossid. The proposal is already simulated when the msg is executed. In addition, the parameter(s) to be set are the ones exactly set in the proposal. What does this accomplish for us?

Also, I imagine most, if not all, param change proposals will go through some form of social discourse (eg. forum post) initially where the exact proposal will be crafted in draft form and agreed upon by some majority before the proposal is actually made, at which we know exactly what is being changed.

@mossid mossid added the T: UX label May 14, 2019
@mossid
Copy link
Contributor Author

mossid commented May 14, 2019

One specific example of the possible cases is excluding zero values on value. The golang json marshaller will omit omitempty fields if the values are set to zero value. So if the proposer does not use a handwritten json value, but instead a parameter value marshaled from a golang struct, which contains a zero value as its field, the marshaler will omit that and the parameter will not be changed on that field. So it is not for the failure of the logic itself(as it is tested on the msg execution as you said), but for the human mistake that can happen if there is no visible outcome displayed.

I'm also pretty sure that the social discussion can catch this sort of human errors, but having another safety layer will be not bad.

@alexanderbez
Copy link
Contributor

alexanderbez commented May 14, 2019

I don't quite follow the proposal or if it's worth the technical overhead, but if you believe the implementation to be minimal and trivial to implement, I'd be happy to review a PR 👍

@mossid
Copy link
Contributor Author

mossid commented May 14, 2019

In addition, the parameter(s) to be set are the ones exactly set in the proposal. What does this accomplish for us?

I think this is the part I missed detailed explaination; it won't be, after #4247 is implemented. The proposal will only contain the fields that will be updated, not the whole parameter, so the parameters to be set are different from the ones set in the proposal.

@alexanderbez
Copy link
Contributor

Correct, after #4247 is complete, the param change proposal only includes what's being changed so it should be self-evident. Am I missing something?

@fedekunze
Copy link
Collaborator

are there any actionable items to do here then?

@alexanderbez
Copy link
Contributor

I don't think there is anything to do here. Are you opposed to this being closed @mossid?

@mossid
Copy link
Contributor Author

mossid commented Jul 19, 2019

Nope, okay to be closed 👍 @fedekunze @alexanderbez

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

No branches or pull requests

3 participants