Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Governance: Voters put money where mouth is #1183

Merged
merged 8 commits into from
Dec 10, 2018
Merged

Conversation

gavofyork
Copy link
Member

@gavofyork gavofyork commented Dec 2, 2018

  • Public referendums are only enacted after a delay (council referendums don't necessarily need a delay)
  • Successful voters have their funds locked up until enactment.

Closes #94


Self::voters_for(index).into_iter()
.filter(|a| (Self::vote_of((index, a.clone())).unwrap_or(false)/*defensive only: all items come from `voters`; for an item to be in `voters` there must be a vote registered; qed*/ == approved))
.for_each(|a| <Bondage<T>>::mutate(a, |b| if *b < now + delay { *b = now + delay }));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should it be *b <= now so it'll also apply the delay to accounts who's funds became liquid at the current block (now) as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's mathematically equivalent.

@@ -156,7 +180,12 @@ decl_storage! {
/// The next referendum index that should be tallied.
pub NextTally get(next_tally) build(|_| 0 as ReferendumIndex): ReferendumIndex;
/// Information concerning any given referendum.
pub ReferendumInfoOf get(referendum_info): map ReferendumIndex => Option<(T::BlockNumber, T::Proposal, VoteThreshold)>;
pub ReferendumInfoOf get(referendum_info): map ReferendumIndex => Option<(ReferendumInfo<T::BlockNumber, T::Proposal>)>;
Copy link
Contributor

@ltfschoen ltfschoen Dec 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want the return value also include the vote threshold and the delay block number? i.e. Option<(ReferendumInfo<T::BlockNumber, T::Proposal, VoteThreshold, T::BlockNumber>)>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ReferendumInfo does includes both already.

type SessionKey: Parameter + Default + MaybeSerializeDebugButNotDeserialize;

/// The session key type used by authorities.
#[cfg(feature = "std")]
Copy link
Contributor

@ltfschoen ltfschoen Dec 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are the changes in this file related to to this PR? they appear to be causing tests to fail

@ltfschoen
Copy link
Contributor

i've created PR #1186 into this branch since some existing tests now require the public_delay to be provided as the 4th argument in order to pass

};
// If all council members voted yes, then it's strongly unconversial; we require a negative
// super-majority at referendum in order to defeat it.
Copy link
Contributor

@ltfschoen ltfschoen Dec 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(nitpick) typo uncontroversial

assert_ok!(Democracy::internal_start_referendum(proposal.clone(), VoteThreshold::SuperMajorityApprove), 0);
assert_eq!(Democracy::active_referendums(), vec![(0, 4, proposal, VoteThreshold::SuperMajorityApprove)]);
assert_ok!(Democracy::internal_start_referendum(proposal.clone(), VoteThreshold::SuperMajorityApprove, 0), 0);
assert_eq!(Democracy::active_referendums(), vec![(0, 4, proposal, VoteThreshold::SuperMajorityApprove, 0)]);
Copy link
Contributor

@ltfschoen ltfschoen Dec 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would there be any benefit to using Zero::zero() for each of these instead of 0 (as was done in e1853be)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not within tests, where the type is concrete. Zero::zero() is needed when the type is abstract.

@@ -581,7 +581,7 @@ mod tests {
fn simple_passing_should_work() {
with_externalities(&mut new_test_ext(), || {
System::set_block_number(1);
let r = Democracy::inject_referendum(1, set_balance_proposal(2), VoteThreshold::SuperMajorityApprove).unwrap();
let r = Democracy::inject_referendum(1, set_balance_proposal(2), VoteThreshold::SuperMajorityApprove, 0).unwrap();
Copy link
Contributor

@ltfschoen ltfschoen Dec 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as per previous comment, any benefit in using Zero::zero() instead of 0?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not within tests, where the type is concrete.

@gavofyork gavofyork added the A0-please_review Pull request needs code review. label Dec 10, 2018
@gavofyork gavofyork requested a review from bkchr December 10, 2018 08:31
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just one missing issue for a TODO.

Ok(())
}
}

// TODO: These should be wired-in into the runtime.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please create an issue and link it in the comment.

@gavofyork gavofyork merged commit 18d818b into master Dec 10, 2018
@gavofyork gavofyork deleted the gav-money-where-mouth-is branch December 10, 2018 16:29
MTDK1 pushed a commit to bdevux/substrate that referenced this pull request Apr 12, 2019
* Referendums only gett enacted after a delay; successful voters must
lock funds up until enactment.

* Build fixes.

* Configurable council enact delay, fix test builds.

* Fix spelling

* Remove TODO
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants