-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Change User Supplied Param Change Value to json.RawMessage #4253
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4253 +/- ##
==========================================
- Coverage 58.91% 58.89% -0.03%
==========================================
Files 215 216 +1
Lines 14482 14499 +17
==========================================
+ Hits 8532 8539 +7
- Misses 5310 5320 +10
Partials 640 640 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK
It'd be nice to have test cases covering NewParamChangeJSON()
,ToParamChange()
, and ToParamChanges()
to keep up coverage.
I think this is worth a pending entry too.
Co-Authored-By: alexanderbez <[email protected]>
@alessio it's not worth a pending entry because this feature hasn't been released yet -- ie. the pending log entry would be redundant. I also added unit tests for the utils. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Addressed your feedback @alessio |
@@ -44,7 +44,7 @@ where proposal.json contains: | |||
{ | |||
"subspace": "staking", | |||
"key": "MaxValidators", | |||
"value": "105" | |||
"value": 105 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this break amino encoding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, in this case MaxValidators
is a uint16
which isn't string-encoded -- it's just an int.
I have tried of changing the the MaxValidators,SignedBlocksWindow in my own test network. The proposal was passed but the parameter is not changed. |
@vdsprakash I would double check your proposal and if the proposal actually passed or not. If it did not pass, did it fail or was it rejected? I've tested the param change proposal process on Gaia v2.0.2 which we'll use for the next major upgrade and it works just fine. Here is a screen recording where I change the two parameters you've mentioned. Here is the parameter change proposal content: {
"title": "Update MaxValidators and SignedBlocksWindow params",
"description": "Update MaxValidators and SignedBlocksWindow params",
"changes": [
{
"subspace": "staking",
"key": "MaxValidators",
"value": 200
},
{
"subspace": "slashing",
"key": "SignedBlocksWindow",
"value": "200"
}
],
"deposit": [
{
"denom": "stake",
"amount": "10000000"
}
]
} |
The values in
changes
in Parameter change proposals are now of typejson.RawMessage
so users no longer have to string-encode them leading to a better UX.closes #4237
Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (
docs/
)Added a relevant changelog entry:
clog add [section] [stanza] [message]
rereviewed
Files changed
in the github PR explorerFor Admin Use: