- 
                Notifications
    You must be signed in to change notification settings 
- Fork 273
Problem: rpc can't query old parameter format after migration #807
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
base: main
Are you sure you want to change the base?
Conversation
| Codecov Report
 
 @@            Coverage Diff             @@
##             main     #807      +/-   ##
==========================================
+ Coverage   34.26%   34.40%   +0.14%     
==========================================
  Files          30       30              
  Lines        1582     1587       +5     
==========================================
+ Hits          542      546       +4     
- Misses        983      984       +1     
  Partials       57       57              
 | 
| the issue is that we cannot query params after migration for older blocks? | 
| yes, the issue is that we can query params for blocks after the migration, but if we switched context to older blocks, we can not get params because params are not stored in  | 
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, maybe you could add a test in the python integration tests "test_upgrade.py" (just query params with older ctx after the upgrade)
| 
 good idea, I'll add it | 
| But in this way we have to stick with  | 
| yes, given we support querying params for old blocks for the params migration, maybe we should consider making other code changes in the future compatible with the old blocks data? | 
| assert cli.query_params(0) == { | ||
| "cronos_admin": "crc12luku6uxehhak02py4rcz65zu0swh7wjsrw0pp", | ||
| "enable_auto_deployment": True, | ||
| "ibc_cro_denom": "ibc/6411AE2ADA1E73DB59DB151" | ||
| "A8988F9B7D5E7E233D8414DB6817F8F1A01611F86", | ||
| "ibc_timeout": "86400000000000", | ||
| } | ||
|  | ||
| assert cli.query_params(target_height-5) == { | ||
| "cronos_admin": "crc12luku6uxehhak02py4rcz65zu0swh7wjsrw0pp", | ||
| "enable_auto_deployment": True, | ||
| "ibc_cro_denom": "ibc/6411AE2ADA1E73DB59DB151" | ||
| "A8988F9B7D5E7E233D8414DB6817F8F1A01611F86", | ||
| "ibc_timeout": "86400000000000", | ||
| } | ||
|  | 
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.
| assert cli.query_params(0) == { | |
| "cronos_admin": "crc12luku6uxehhak02py4rcz65zu0swh7wjsrw0pp", | |
| "enable_auto_deployment": True, | |
| "ibc_cro_denom": "ibc/6411AE2ADA1E73DB59DB151" | |
| "A8988F9B7D5E7E233D8414DB6817F8F1A01611F86", | |
| "ibc_timeout": "86400000000000", | |
| } | |
| assert cli.query_params(target_height-5) == { | |
| "cronos_admin": "crc12luku6uxehhak02py4rcz65zu0swh7wjsrw0pp", | |
| "enable_auto_deployment": True, | |
| "ibc_cro_denom": "ibc/6411AE2ADA1E73DB59DB151" | |
| "A8988F9B7D5E7E233D8414DB6817F8F1A01611F86", | |
| "ibc_timeout": "86400000000000", | |
| } | |
| expected_param = { | |
| "cronos_admin": "crc12luku6uxehhak02py4rcz65zu0swh7wjsrw0pp", | |
| "enable_auto_deployment": True, | |
| "ibc_cro_denom": "ibc/6411AE2ADA1E73DB59DB151" | |
| "A8988F9B7D5E7E233D8414DB6817F8F1A01611F86", | |
| "ibc_timeout": "86400000000000", | |
| } | |
| assert cli.query_params(0) == expected_param | |
| assert cli.query_params(target_height-5) == expected_param | 
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
close: #806
By querying params from
x/paramsif we get nil fromx/cronosstore, we could return correct params even if the context is in old blocks before the params migratiionPR Checklist:
make)make test)go fmt)golangci-lint run)go list -json -m all | nancy sleuth)Thank you for your code, it's appreciated! :)