Modularised dispatch #95

gavofyork · 2018-03-13T11:35:26Z

#91 and #92 and more.

Not yet complete but 75% there.

rphmeier · 2018-03-15T20:01:36Z

demo/runtime/src/dispatch.rs

+ = $id:expr ;
+ )*
+ ) => {
+ pub mod $mod_name {


could put all the warnings to silence on here

what's the best way of doing that? is there a #[allow(*)]?

i mean the warnings can be blocked at a module level so they don't need to be silenced per struct.

isn't it better to keep them focussed? suppressing at module level might let accidental errors get through.

rphmeier · 2018-03-15T20:02:47Z

demo/runtime/src/dispatch.rs

+ fn from_u8(value: u8) -> Option<Id> {
+ use self::*;
+ let functions = [
+ $( Id::$fn_name ),*


why not generate a match statement?

fixed (not intentional - just was adapting existing code)

rphmeier · 2018-03-18T23:07:25Z

demo/runtime/src/runtime/system.rs

@@ -42,12 +44,17 @@ pub fn block_hash(number: BlockNumber) -> Hash {
 storage::get_or_default(&number.to_keyed_vec(BLOCK_HASH_AT))
 }

-pub mod privileged {
- use super::*;
+pub struct PrivPass;


also this could be a little harder to create PrivPass.x() doesn't guard anything that x() itself doesn't.

rphmeier · 2018-03-18T23:08:47Z

demo/runtime/src/runtime/session.rs

@@ -22,6 +22,8 @@ use codec::KeyedVec;
 use runtime_support::{storage, StorageVec};
 use demo_primitives::{AccountId, SessionKey, BlockNumber};
 use runtime::{system, staking, consensus};
+use runtime::system::PrivPass;
+use runtime::staking::PublicPass;


these should really reside in the same module where all passes are defined

they now both reside in the only module that is able to actually create them.

rphmeier · 2018-03-18T23:11:27Z

demo/runtime/src/runtime/system.rs

 };

+ println!("TX: {}", HexDisplay::from(&tx.transaction.encode()));


stray println

rphmeier · 2018-03-18T23:14:06Z

demo/runtime/src/runtime/democracy.rs

@@ -151,72 +184,92 @@ pub fn next_free_ref_index() -> ReferendumIndex {
 storage::get_or_default(REFERENDUM_COUNT)
 }

-pub mod public {
- use super::*;
+pub type BoxedProposal = Box<Proposal>;


rphmeier · 2018-03-18T23:15:32Z

demo/runtime/src/runtime/democracy.rs

- use super::*;
+impl_dispatch! {
+ pub mod privileged;
+ fn start_referendum(proposal: Box<Proposal>, vote_threshold: VoteThreshold) = 0;


why does this accept a Box?

because it's inside impl_dispatch so it needs to be pass-by-value (since the same types will be used in the enum item prototypes).

just to clarify: there can be a referendum to dispatch any kind of private-only call from any module? (which we've renamed to Proposal within this module -- I found that confusing)

correct, though it's not "private-only", it's "privileged-only".

rphmeier · 2018-03-18T23:15:37Z

demo/runtime/src/runtime/democracy.rs


+impl_dispatch! {
+ pub mod public;
+ fn propose(proposal: Box<Proposal>, value: Balance) = 0;


rphmeier · 2018-03-18T23:18:06Z

demo/runtime/src/dispatch.rs

+ pub enum Call {
+ $(
+ #[allow(non_camel_case_types)]
+ $camelcase ( $crate::runtime::$sub_name::$path::Call )


this is a little non-general. If we want this to be reusable for other runtimes we shouldn't make any assumptions about module structure.

rphmeier · 2018-03-18T23:18:15Z

demo/runtime/src/dispatch.rs

+ }
+
+ impl $crate::dispatch::Slicable for Call {
+ fn decode<I: $crate::dispatch::Input>(input: &mut I) -> Option<Self> {


likewise here

rphmeier · 2018-03-18T23:18:32Z

demo/runtime/src/dispatch.rs

+ }
+
+ fn encode(&self) -> Vec<u8> {
+ let mut v = $crate::dispatch::Vec::new();


rphmeier · 2018-03-18T23:18:45Z

demo/runtime/src/dispatch.rs

- council_vote::privileged::set_voting_period(a),
+use runtime::{staking, system};
+pub use rstd::prelude::Vec;
+pub use codec::{Slicable, Input, NonTrivialSlicable};


re-exports in a random module worry me.

it's for the macro.

i understand that, but it's still bad style -- re-exports should at the very least be #[doc(hidden)] and preferably in the crate root (ideally, the macro is in its own crate and we can reuse it)

rphmeier · 2018-03-18T23:24:56Z

demo/runtime/src/lib.rs

@@ -33,48 +36,12 @@ extern crate demo_primitives;

 extern crate integer_sqrt;

+#[macro_use] pub mod dispatch;


the macros from dispatch aren't used outside of it as far as I can see

impl_dispatch is.

rphmeier · 2018-03-19T02:16:35Z

demo/runtime/src/runtime/democracy.rs

 use runtime::staking::{PublicPass, Balance};

+/// A token for privileged dispatch. Can only be created in this module.
+pub struct PrivPass;


This struct can be created anywhere. It would need a private field to prevent creation in other modules.

arkpar · 2018-03-19T02:24:47Z

demo/executor/src/lib.rs

@@ -188,15 +194,15 @@ mod tests {
 Executor::new().call(&mut t, COMPACT_CODE, "execute_block", &block1().0).unwrap();

 runtime_io::with_externalities(&mut t, || {
- assert_eq!(balance(&Alice), 42);
+ assert_eq!(balance(&Alice), 41);


Just wondering why have these changed?

there's a transaction fee now.

rphmeier · 2018-03-19T02:28:18Z

demo/runtime/src/dispatch.rs

+pub use codec::{Slicable, Input, NonTrivialSlicable};
+
+#[macro_export]
+macro_rules! impl_dispatch {


wasm successful pending has not remove {:?} debug trait

…depth-requirement Increase confirmation depth requirement to 100 blocks

Completely rework dispatch mechanism into something modular.

a83059a

Not yet complete but 75% there.

gavofyork added the A3-in_progress Pull request is in progress. No review needed at this stage. label Mar 13, 2018

gavofyork changed the title ~~Gav dispatch~~ Modularised dispatch Mar 13, 2018

gavofyork added 7 commits March 13, 2018 12:46

Council vote tests.

82a178d

Fix tests.

1d28d9a

whitespace.

69c88c3

Fix demo runtime tests.

b12a708

Merge branch 'gav-demo' into gav-dispatch

e48e86d

Fix up tests.

8e3cc51

Merge branch 'gav-demo' into gav-dispatch

53e2fdf

gavofyork added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Mar 14, 2018

gavofyork added 3 commits March 14, 2018 12:59

Merge branch 'master' into gav-dispatch

27ecd6f

Remove dead code.

5eca74a

Merge branch 'master' into gav-dispatch

7cece3b

rphmeier reviewed Mar 15, 2018

View reviewed changes

Use match for Id

faa0a44

rphmeier reviewed Mar 18, 2018

View reviewed changes

demo/runtime/src/runtime/system.rs Outdated

};

println!("TX: {}", HexDisplay::from(&tx.transaction.encode()));

Copy link

Contributor

rphmeier Mar 18, 2018

Choose a reason for hiding this comment

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

stray println

rphmeier reviewed Mar 18, 2018

View reviewed changes

demo/runtime/src/dispatch.rs

}

fn encode(&self) -> Vec<u8> {

let mut v = $crate::dispatch::Vec::new();

Copy link

Contributor

rphmeier Mar 18, 2018

Choose a reason for hiding this comment

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

and here

rphmeier reviewed Mar 18, 2018

View reviewed changes

gavofyork added 2 commits March 19, 2018 10:02

Make PrivPass better protected.

7912de1

Address other grumbles.

aac3899

rphmeier reviewed Mar 19, 2018

View reviewed changes

gavofyork added 2 commits March 19, 2018 10:21

Give PrivPass a private member.

26519c6

Testing PrivPass.

9f32ea7

arkpar reviewed Mar 19, 2018

View reviewed changes

rphmeier reviewed Mar 19, 2018

View reviewed changes

Add docs.

8966951

rphmeier added A8-looksgood and removed A0-please_review Pull request needs code review. labels Mar 19, 2018

rphmeier approved these changes Mar 19, 2018

View reviewed changes

gavofyork merged commit 7443c0a into master Mar 19, 2018

gavofyork deleted the gav-dispatch branch March 19, 2018 02:51

JoshOrndorff added a commit to moonbeam-foundation/substrate that referenced this pull request Apr 21, 2021

Fix some wrong indentation (paritytech#95)

de37114

liuchengxu pushed a commit to chainx-org/substrate that referenced this pull request Aug 23, 2021

refactor cxrml dir (paritytech#95)

85759df

wasm successful pending has not remove {:?} debug trait

liuchengxu pushed a commit to autonomys/substrate that referenced this pull request Jun 3, 2022

Merge pull request paritytech#95 from subspace/increase-confirmation-…

d0129e6

…depth-requirement Increase confirmation depth requirement to 100 blocks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modularised dispatch #95

Modularised dispatch #95

gavofyork commented Mar 13, 2018 •

edited

Loading

rphmeier Mar 15, 2018

gavofyork Mar 16, 2018

rphmeier Mar 16, 2018

gavofyork Mar 16, 2018

rphmeier Mar 15, 2018

gavofyork Mar 16, 2018

rphmeier Mar 18, 2018

rphmeier Mar 18, 2018

rphmeier Mar 18, 2018 •

edited

Loading

gavofyork Mar 19, 2018

rphmeier Mar 18, 2018

rphmeier Mar 18, 2018

rphmeier Mar 18, 2018

gavofyork Mar 19, 2018

rphmeier Mar 19, 2018 •

edited

Loading

gavofyork Mar 19, 2018

rphmeier Mar 18, 2018

gavofyork Mar 19, 2018

rphmeier Mar 18, 2018

rphmeier Mar 18, 2018

rphmeier Mar 18, 2018

rphmeier Mar 18, 2018

gavofyork Mar 19, 2018

rphmeier Mar 19, 2018 •

edited

Loading

rphmeier Mar 18, 2018

gavofyork Mar 19, 2018

rphmeier Mar 19, 2018

gavofyork Mar 19, 2018

arkpar Mar 19, 2018

gavofyork Mar 19, 2018

rphmeier Mar 19, 2018

		};

		println!("TX: {}", HexDisplay::from(&tx.transaction.encode()));

		@@ -33,48 +36,12 @@ extern crate demo_primitives;

		extern crate integer_sqrt;

		#[macro_use] pub mod dispatch;

Modularised dispatch #95

Modularised dispatch #95

Conversation

gavofyork commented Mar 13, 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

rphmeier Mar 18, 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

rphmeier Mar 19, 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

rphmeier Mar 19, 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

gavofyork commented Mar 13, 2018 •

edited

Loading

rphmeier Mar 18, 2018 •

edited

Loading

rphmeier Mar 19, 2018 •

edited

Loading

rphmeier Mar 19, 2018 •

edited

Loading