Skip to content
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: Governance Proposal Refactor #2613

Closed

Conversation

mossid
Copy link
Contributor

@mossid mossid commented Oct 26, 2018

  • 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 explorer

Split from: #2571

Common fields that can be used for different implementation of Proposal are extracted into ProposalBase.


For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@mossid mossid requested a review from sunnya97 October 26, 2018 19:04
@mossid mossid changed the title R4R: Governance Proposal Refactor WIP: Governance Proposal Refactor Oct 26, 2018
@mossid mossid added the wip label Oct 26, 2018
@sunnya97
Copy link
Member

@mossid

ProposalQueue has been changed to InfoQueue so we don't have to unmarshal the whole proposal each time(consider the case where a TextProposal has a very long description)

Check out #2638. I think that is a cleaner solution to this issue.

Unlike ProposalInfo where it gets pruned after the VotingPeriod, Proposal is immutable after its creation, so the user can always query the Proposal.

Why do we want to remove the ProposalInfo after the voting period? I think there is valuable information in there that should continue to be queriable for posterity. The obvious one being Status (was the proposal accepted or rejected?)! But also the other three fields are useful as well.

Enact is called when the proposal passes. It is useful when the proposal needs to trigger an event automatically.

I like this. We should just add this to the Proposal interface as it already is.

@codecov
Copy link

codecov bot commented Nov 1, 2018

Codecov Report

Merging #2613 into sunny/gov-proposals-queue-iterator will increase coverage by 0.12%.
The diff coverage is 66.66%.

@@                          Coverage Diff                           @@
##           sunny/gov-proposals-queue-iterator    #2613      +/-   ##
======================================================================
+ Coverage                                58.7%   58.82%   +0.12%     
======================================================================
  Files                                     152      152              
  Lines                                    9430     9434       +4     
======================================================================
+ Hits                                     5536     5550      +14     
+ Misses                                   3527     3515      -12     
- Partials                                  367      369       +2

@mossid mossid changed the base branch from joon/paramstore-subkey to develop November 1, 2018 16:20
@mossid mossid changed the title WIP: Governance Proposal Refactor R4R: Governance Proposal Refactor Nov 1, 2018
@alexanderbez
Copy link
Contributor

@sunnya97 is this more or less a dup of #2638? Can we maybe adapt concepts from one or the other and close one?

@mossid mossid changed the base branch from develop to sunny/gov-proposals-queue-iterator November 5, 2018 20:30
@sunnya97 sunnya97 force-pushed the sunny/gov-proposals-queue-iterator branch from eddccde to 46f62d5 Compare November 5, 2018 23:23
@cwgoes
Copy link
Contributor

cwgoes commented Dec 12, 2018

@mossid What's the status of this PR? Are these changes still desirable? If so, can we rebase it and I'll review - otherwise, let's close it.

@mossid mossid closed this Dec 14, 2018
@mossid mossid deleted the joon/governance-refactor branch December 14, 2018 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants