-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
feat: consensus module #12905
feat: consensus module #12905
Conversation
I'd call it |
@marbar3778 : @JeancarloBarrios might hop into this PR to fix some stuff in the meantime while you're out. |
|
* remove lint error for accountkeeper and bankkeeper cant be nil * regenerated protos with removing some unused imports * add legacy exported + keeper and module dep inject functions
|
||
consensusParams := new(tmproto.ConsensusParams) | ||
|
||
consensusParams.Version = &tmproto.VersionParams{AppVersion: 0} |
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.
@marbar3778 I'm not sure how to handle the version for now I left the same value that is in default params, but maybe when you see this we can discuss it
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.
this would be the app version set by the upgrade module I believe. We may need to get it from there or leave it blank for now
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.
I left an empty struct. However, I'm reading through x/upgrade to check if I should bring it from there
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.
Should I just pass the upgrade keeper as a dependency?
8a70bef
to
6f55f8b
Compare
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.
thanks for pushing this over the finish line. I can add the docs in the morning to wrap it up then merge .
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #12905 +/- ##
==========================================
- Coverage 53.98% 53.89% -0.10%
==========================================
Files 641 657 +16
Lines 55020 56650 +1630
==========================================
+ Hits 29704 30531 +827
- Misses 22934 23682 +748
- Partials 2382 2437 +55
|
I can wrap up the docs shortly |
added changelog and upgrading docs. We should merge and then we can follow up with a readme since this pr is already fairly large |
Description
Closes: #12652
Add consensus param module in line with deprecation of param module
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change