-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
R4R: Split Proposal Interface #3779
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #3779 +/- ##
==========================================
- Coverage 60.95% 60.8% -0.16%
==========================================
Files 192 192
Lines 14360 14350 -10
==========================================
- Hits 8753 8725 -28
- Misses 5033 5047 +14
- Partials 574 578 +4 |
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.
Looks great @mossid -- I left a few minor bits of feedback. Mostly around constructor use and panics.
Co-Authored-By: mossid <torecursedivine@gmail.com>
Co-Authored-By: mossid <torecursedivine@gmail.com>
…s/cosmos-sdk into joon/3570-split-proposal-interface
Co-Authored-By: mossid <torecursedivine@gmail.com>
Co-Authored-By: mossid <torecursedivine@gmail.com>
…s/cosmos-sdk into joon/3570-split-proposal-interface
@mossid this needs a pending log entry too. |
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.
Mostly, this looks OK to me, but I'm not sure I'm in agreement with the broader design choices outlined in #3565 - I think we should discuss with @sunnya97 - in particular, I don't know if we want all modules to be able to define proposal types, as opposed to having the params module export a write-capable keeper to the governance module.
Upon reflection I think I like this design choice. One note is that we will need multiple modules to be able to react to a particular proposal, and possibly in a predetermined order. |
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.
Nice design! few comments left within, but generally looks really good
x/gov/proposals.go
Outdated
|
||
String() string | ||
// ProposalContent is an interface that has title, description, and proposaltype for the Proposal |
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.
Let's update this comment - AFAICT the struct which fulfills ProposalContent
may also contain more detailed specifications as to how the proposal is to be executed (ex. the parameter set change amount) - we should explain this in this comment here
x/gov/test_common.go
Outdated
|
||
// checks if two proposals are equal | ||
func ProposalEqual(proposalA Proposal, proposalB Proposal) bool { | ||
if proposalA.ProposalID == proposalB.ProposalID && |
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.
instead of testing all the fields, we should simply marshal both proposals and test if the bytes are equal, simplified the code, makes it redundant against field additions. (also we're not so concerned about efficiency as this is used for testing)
Looks like |
Glad to re-review but needs a rebase. |
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.
ACK, see one comment
} | ||
|
||
// checks if two proposals are equal | ||
func ProposalEqual(proposalA Proposal, proposalB Proposal) bool { |
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 is OK for tests, bad for production (slow), let's note that in a comment
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.
ACK still stands 👍
Closes: #3570
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 entries in
PENDING.md
with issue #rereviewed
Files changed
in the github PR explorerFor Admin Use: