Council motions for approving treasury proposals #694

gavofyork · 2018-09-08T13:27:48Z

Closes #680

Repot Permill and add constructors
Integrate treasury into demo runtime
Introduce EnsureOrigin trait for allowing parameterised origin-guards
Introduce system implementations of EnsureOrigin
Introduce council Members origin and an according implementation of EnsureOrigin
Introduce parameterisable origin guard for treasury
Introduce council_motions module that can dispatch from council origin
Integrate council_motions into demo runtime.
Tests.

gavofyork · 2018-09-08T13:31:48Z

substrate/runtime/primitives/src/lib.rs

+#[derive(Encode, Decode, Default, Copy, Clone, PartialEq, Eq)]
+pub struct Permill(u32);
+
+// TODO: impl Mul<Permill> for N where N: As<usize>


Still todo; not introduced with this PR.

pepyakin · 2018-09-10T08:35:39Z

demo/cli/src/lib.rs

@@ -211,6 +211,12 @@ pub fn run<I, T>(args: I) -> error::Result<()> where
 		timestamp: Some(TimestampConfig {
 			period: 5,					// 5 second block time.
 		}),
+		treasury:  Some(TreasuryConfig {


Double space

pepyakin · 2018-09-10T09:02:14Z

substrate/runtime/council/src/motions.rs

+
+		<Proposals<T>>::mutate(|proposals| proposals.push(proposal_hash));
+		<ProposalOf<T>>::insert(proposal_hash, *proposal);
+		<Voting<T>>::insert(proposal_hash, (threshold, vec![who.clone()], vec![]));


What happens if threshold is less than 2? Does it makes sense at all?

1 is technically fine (just means "one council member").

hm, but the condition for the approval is:

let approved = yes_votes >= threshold;

then if threshold=1 then such a proposal should be accepted on creation, right?

pepyakin · 2018-09-10T09:07:17Z

substrate/runtime/council/src/motions.rs

+				return Err("duplicate vote ignored")
+			}
+			if let Some(pos) = position_no {
+				voting.2.remove(pos);


can we use swap_remove here and for the disapprove case?

pepyakin · 2018-09-10T09:08:37Z

substrate/runtime/council/src/motions.rs

+		}
+
+		let yes_votes = voting.1.len() as u32;
+		let no_votes = voting.2.len() as u32;


voting.1 and voting.2 seems a little obscure to me here and above, consider introducing a separate structure.

an extra structure is fiddly to get right when you need to have it generic over an associated type of the configuration trait thanks to serde and compiler bugs.

pepyakin · 2018-09-10T09:14:41Z

substrate/runtime/council/src/motions.rs

+				return Err("duplicate vote ignored")
+			}
+			if let Some(pos) = position_yes {
+				voting.1.remove(pos);


Why would one want to change his mind?

they might be female.

ducks :-)

heh, but i still wonder if it's strictly necessary.

I'm a bit worried that it might be susceptible to be gamable somehow...

can't see how...

pepyakin · 2018-09-10T09:23:33Z

substrate/runtime/council/src/motions.rs

+		Self::deposit_event(RawEvent::Voted(who, proposal, approve, yes_votes, no_votes));
+
+		let threshold = voting.0;
+		let potential_votes = <Council<T>>::active_council().len() as u32;


Do we care about changing the structure of the council?

As far as I can see, it's possible, but we don't check for that and the code seems to depend on this.

Could you expand on that?
no_votes cannot ever be greater than potential_votes, right?

pepyakin · 2018-09-10T09:28:35Z

demo/cli/src/lib.rs

+		treasury:  Some(TreasuryConfig {
+			proposal_bond: Permill::from_percent(5),
+			proposal_bond_minimum: 1_000_000,
+			spend_period: 12 * 60 * 24,


hm, this looks like a timestamp or something, but it's actually should be a BlockNumber

it should not be a block number per se but rather a number of blocks for which the type is BlockNumber. in this case, we assume 12 blocks per minute (1 every 5 seconds), multiply by 60 and 24 to get approximately one day.

ha, that's clear now!

pepyakin · 2018-09-10T09:30:28Z

substrate/runtime/council/src/motions.rs

+
+		ensure!(Self::is_councillor(&who), "voter not on council");
+
+		let mut voting = Self::voting(&proposal);


Consider there are two extrinsics that vote for the same proposal. The first one will lead to execution of proposal. What would happen with the second one?

panic, and indeed, that should just be a bail.

pepyakin · 2018-09-10T10:26:38Z

substrate/runtime/council/src/motions.rs

+
+		let proposal_hash = T::Hashing::hash_of(&proposal);
+
+		ensure!(!<ProposalOf<T>>::exists(proposal_hash), "duplicate proposals not allowed");


note that there is a (slight) chance of reusing votes.

Let's say that we have proposal P, 4 active councillors (c0..c3) and the following extrinsics were published:

propose(c0, 2, P)

vote(c1, H(P))

vote(c2, H(P))

vote(c3, H(P))

Now it seems that the 3rd extrinsic will panic, but let's assume that it's fixed by no-oping.

Some rogue councillor c0 can issue propose(c0, 2, P) extrinsic that has a slight chance to be applied before votes from c2 and c3 and thus use these votes for approving the second proposal. This would be more effective, if c0 also act as front-running validator.

i guess i can introduce a proposal index to ensure they're unique.

gavofyork · 2018-09-10T12:40:08Z

@pepyakin ^^^

bkchr · 2018-09-10T12:40:55Z

substrate/primitives/src/u32_trait.rs

+	/// The actual value represented by the impl'ing type.
+	const VALUE: u32;
+}
+/// Type representingn the value 0 for the `Value` trait.


Nitpick: representingn spelling mistake that repeats for all defintions.

pepyakin · 2018-09-10T13:29:29Z

substrate/runtime/council/src/motions.rs

+	type Event: From<Event<Self>> + Into<<Self as system::Trait>::Event>;
+}
+
+/// Origin for the system module. 


Can we update this comment?

gavofyork · 2018-09-10T14:01:38Z

@pepyakin ^^^

gavofyork · 2018-09-10T14:01:46Z

@bkchr ^^^

* Add some tests Signed-off-by: koushiro <[email protected]> * Add test Signed-off-by: koushiro <[email protected]> * Cargo fmt Signed-off-by: koushiro <[email protected]> * Replace assert with assert_ok and assert_noop Signed-off-by: koushiro <[email protected]> * Cargo fmt Signed-off-by: koushiro <[email protected]>

* error: `RpcError` with custom client error Signed-off-by: Alexandru Vasile <[email protected]> * error: Add `SubscriptionDropped` and panic on param errors Signed-off-by: Alexandru Vasile <[email protected]> * Update subxt/src/rpc/rpc_client_t.rs Co-authored-by: James Wilson <[email protected]> * Apply rustfmt Signed-off-by: Alexandru Vasile <[email protected]> Signed-off-by: Alexandru Vasile <[email protected]> Co-authored-by: James Wilson <[email protected]>

Treasury in runtime, generic approve/reject

b1591b9

gavofyork assigned pepyakin, bkchr and arkpar Sep 8, 2018

gavofyork requested review from bkchr, pepyakin and arkpar September 8, 2018 13:28

gavofyork unassigned pepyakin, bkchr and arkpar Sep 8, 2018

gavofyork added the A0-please_review Pull request needs code review. label Sep 8, 2018

gavofyork commented Sep 8, 2018

View reviewed changes

gavofyork added 3 commits September 9, 2018 13:23

Add logic for council origin

df7da51

Merge remote-tracking branch 'origin/master' into gav-council-origin

58e713d

Add tests.

ac746c8

gavofyork changed the title ~~Treasury in runtime, generic approve/reject~~ Council motions for approving treasury proposals Sep 9, 2018

Configurable number of members in EnsureMembers

865113d

Merge remote-tracking branch 'origin/master' into gav-council-origin

11d77cd

pepyakin reviewed Sep 10, 2018

View reviewed changes

Fix grumbles

22d8f92

bkchr reviewed Sep 10, 2018

View reviewed changes

gavofyork added 2 commits September 10, 2018 14:51

Merge remote-tracking branch 'origin/master' into gav-council-origin

398738b

Fix spelling

a6babc4

pepyakin reviewed Sep 10, 2018

View reviewed changes

Comment

43ba86c

pepyakin approved these changes Sep 10, 2018

View reviewed changes

pepyakin added A8-looksgood and removed A0-please_review Pull request needs code review. labels Sep 10, 2018

gavofyork merged commit d307019 into master Sep 10, 2018

gavofyork deleted the gav-council-origin branch September 10, 2018 14:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Council motions for approving treasury proposals #694

Council motions for approving treasury proposals #694

gavofyork commented Sep 8, 2018 •

edited

Loading

gavofyork Sep 8, 2018

pepyakin Sep 10, 2018

pepyakin Sep 10, 2018

gavofyork Sep 10, 2018

pepyakin Sep 10, 2018

gavofyork Sep 10, 2018

pepyakin Sep 10, 2018

pepyakin Sep 10, 2018

gavofyork Sep 10, 2018

pepyakin Sep 10, 2018

gavofyork Sep 10, 2018

pepyakin Sep 10, 2018

gavofyork Sep 10, 2018

pepyakin Sep 10, 2018

gavofyork Sep 10, 2018

pepyakin Sep 10, 2018

pepyakin Sep 10, 2018 •

edited

Loading

gavofyork Sep 10, 2018 •

edited

Loading

pepyakin Sep 10, 2018

pepyakin Sep 10, 2018

gavofyork Sep 10, 2018

pepyakin Sep 10, 2018

gavofyork Sep 10, 2018

gavofyork commented Sep 10, 2018

bkchr Sep 10, 2018

pepyakin Sep 10, 2018

gavofyork commented Sep 10, 2018

gavofyork commented Sep 10, 2018


		ensure!(Self::is_councillor(&who), "voter not on council");

		let mut voting = Self::voting(&proposal);


		let proposal_hash = T::Hashing::hash_of(&proposal);

		ensure!(!<ProposalOf<T>>::exists(proposal_hash), "duplicate proposals not allowed");

Council motions for approving treasury proposals #694

Council motions for approving treasury proposals #694

Conversation

gavofyork commented Sep 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepyakin Sep 10, 2018 • edited Loading

Choose a reason for hiding this comment

gavofyork Sep 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gavofyork commented Sep 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gavofyork commented Sep 10, 2018

gavofyork commented Sep 10, 2018

gavofyork commented Sep 8, 2018 •

edited

Loading

pepyakin Sep 10, 2018 •

edited

Loading

gavofyork Sep 10, 2018 •

edited

Loading