-
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
ADR 037 Implementation: Governance Split Votes #7802
ADR 037 Implementation: Governance Split Votes #7802
Conversation
… gov_split_vote
Governance split vote
Codecov Report
@@ Coverage Diff @@
## master #7802 +/- ##
==========================================
+ Coverage 61.39% 61.43% +0.03%
==========================================
Files 656 656
Lines 37320 37526 +206
==========================================
+ Hits 22914 23055 +141
- Misses 12005 12063 +58
- Partials 2401 2408 +7
|
251d8b1
to
138ae17
Compare
c38f84a
to
56ed27a
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.
Overall LGTM!
changelog entry needs to be added, since this is state breaking.
Also, a migration script too, but probably as a follow-up PR.
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.
The proto-breaking change seems a bit worrying to me, but I don't really have an idea why it's showing as breaking (related: #7802 (comment))
It seems to not like the fact that we tried to reserve the name "option" when it was already used in past for field 3. But according to Protobuf docs, names and fields need to be reserved separately. I think this is more a mistake on the proto-breaking check logic that should handle this better. |
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, can we avoid adding new legacy REST endpoints?
x/gov/client/rest/tx.go
Outdated
@@ -22,6 +22,7 @@ func registerTxHandlers(clientCtx client.Context, r *mux.Router, phs []ProposalR | |||
r.HandleFunc("/gov/proposals", newPostProposalHandlerFn(clientCtx)).Methods("POST") | |||
r.HandleFunc(fmt.Sprintf("/gov/proposals/{%s}/deposits", RestProposalID), newDepositHandlerFn(clientCtx)).Methods("POST") | |||
r.HandleFunc(fmt.Sprintf("/gov/proposals/{%s}/votes", RestProposalID), newVoteHandlerFn(clientCtx)).Methods("POST") | |||
r.HandleFunc(fmt.Sprintf("/gov/proposals/{%s}/weightedvotes", RestProposalID), newWeightedVoteHandlerFn(clientCtx)).Methods("POST") |
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.
legacy REST is being deprecated, let's not add new legacy REST endpoints
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.
Ok
Co-authored-by: Anil Kumar Kammari <anil@vitwit.com>
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
Co-authored-by: Marko <markobaricevic3778@gmail.com>
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.
Description
This PR implements the ADR proposed in #7733
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerCodecov Report
in the comment section below once CI passes