Introduce treasury and document #646

gavofyork · 2018-09-02T09:10:45Z

Rename Executable to OnFinalise to reflect meaning.
Remove unused storage item.
Introduce OnMinted trait.
Introduce new skeleton treasury module.

- Remove HasPublicAux - Rename Concrete -> Runtime

dvdplm

A good read, I learned plenty. Some typos and style question marks.

dvdplm · 2018-09-04T05:23:16Z

demo/runtime/src/lib.rs

@@ -65,8 +65,8 @@ pub use runtime_primitives::BuildStorage;
 // Workaround for https://github.com/rust-lang/rust/issues/26925 . Remove when sorted.
 #[derive(Clone, Copy, PartialEq, Eq)]
 #[cfg_attr(feature = "std", derive(Debug, Serialize, Deserialize))]
-/// Concrete runtime type used to parameterize the various modules.
-pub struct Concrete;
+/// Runtime runtime type used to parameterize the various modules.


Maybe /// Runtime is the concrete parametrization of the various runtime modules.?

bikeshed: I would suggest ConcreteRuntime as the name – this type is sort of the apex type on top of a pile of code containing "runtime"; might be a worth giving it a clearly distinct name.

I like short, bold and unequivocal naming. The struct that is created is best described as the Runtime as all runtime logic hangs from it. ConcreteRuntime suggests there's an AbstractRuntime trait somewhere, which there isn't.

dvdplm · 2018-09-04T05:32:09Z

substrate/runtime/contract/src/lib.rs

-impl<T: Trait> Executable for Module<T> {
-	fn execute() {
+impl<T: Trait> OnFinalise<T::BlockNumber> for Module<T> {
+	fn on_finalise(_n: T::BlockNumber) {


Update the docs above to //! Call ['Module::on_finalise'] at the end of the block…

feel free to make a PR, but that's out of scope for this one.

dvdplm · 2018-09-04T05:33:07Z

substrate/runtime/example/src/lib.rs

@@ -0,0 +1,385 @@
+// Copyright 2017 Parity Technologies (UK) Ltd.


out of scope - this is blanket legal boilerplate and duplicated across the codebase and as such should be handled in its own PR for a blanket upgrade.

dvdplm · 2018-09-04T05:35:30Z

substrate/runtime/example/src/lib.rs

+use runtime_support::{StorageValue, dispatch::Result};
+
+/// Our module's configuration trait. All our types and consts go in here. If the
+/// module is dependent on specfiic other modules, then their configuration traits


typo: specfiic

dvdplm · 2018-09-04T05:36:36Z

substrate/runtime/example/src/lib.rs

+}
+
+// The module declaration. This states the entry points that we handle. The
+// macro looks after the marshalling of arguments and dispatch.


s/looks after/takes care of/

dvdplm · 2018-09-04T06:25:23Z

substrate/runtime/treasury/src/lib.rs

@@ -0,0 +1,555 @@
+// Copyright 2017 Parity Technologies (UK) Ltd.


s/2017/2018/

dvdplm · 2018-09-04T06:25:58Z

substrate/runtime/treasury/src/lib.rs

+use balances::OnMinted;
+
+/// Our module's configuration trait. All our types and consts go in here. If the
+/// module is dependent on specfiic other modules, then their configuration traits


s/specfiic/specific/

dvdplm · 2018-09-04T06:26:44Z

substrate/runtime/treasury/src/lib.rs

+
+	#[cfg_attr(feature = "std", derive(Serialize, Deserialize))]
+	pub enum Call where aux: T::PublicAux {
+		// Put forward a suggestion for spending. A bond of 


This looks weird. Rephrase?

dvdplm · 2018-09-04T06:31:51Z

substrate/runtime/treasury/src/lib.rs

+		// proposal gets these back. A rejected proposal doesn't.
+		ProposalBond get(proposal_bond): required Permill;
+
+		// Proportion of funds that should be bonded in order to place a proposal. An accepted


Is this really a proportion? If so, shouldn't it be a Permill?

dvdplm · 2018-09-04T06:32:59Z

substrate/runtime/treasury/src/lib.rs

+		// Proposals that have been made.
+		Proposals get(proposals): map [ ProposalIndex => Proposal<T::AccountId, T::Balance> ];
+
+		// Proposals that have been made.


s/have been made/have been approved/?

Grumbles addressed

pepyakin · 2018-09-04T10:46:56Z

substrate/runtime/treasury/src/lib.rs

+	#[cfg_attr(feature = "std", derive(Serialize, Deserialize))]
+	pub enum PrivCall {
+		// A priviledged call; in this case it resets our dummy value to something new.
+		fn set_pot(new_pot: T::Balance) -> Result = 0;


Can we update the comment here?

pepyakin · 2018-09-04T10:47:14Z

substrate/runtime/treasury/src/lib.rs

+		fn configure(proposal_bond: Permill, proposal_bond_minimum: T::Balance, spend_period: T::BlockNumber, burn: Permill) -> Result = 1;
+
+		// A priviledged call; in this case it resets our dummy value to something new.
+		fn reject_proposal(proposal_id: ProposalIndex) -> Result = 2;


Can we update the comment here?

pepyakin · 2018-09-04T10:47:23Z

substrate/runtime/treasury/src/lib.rs

+		fn reject_proposal(proposal_id: ProposalIndex) -> Result = 2;
+
+		// A priviledged call; in this case it resets our dummy value to something new.
+		fn approve_proposal(proposal_id: ProposalIndex) -> Result = 3;


Can we update the comment here?

pepyakin · 2018-09-04T10:52:01Z

substrate/runtime/primitives/src/traits.rs

+/// for your module when the block is ending.
+pub trait OnFinalise<BlockNumber> {
+	/// The block is being finalised. Implement to have something happen.
+	fn on_finalise(_n: BlockNumber) {}


It's very cool for this trait be renamed to OnFinalise (I even had this in my latent todo for the next "random fixes" series PR)!

However, I'm not sure if we actually want to introduce the block number here. Why is it here?

it's available in the caller and it's useful in many of the callees.

True, but i worry a bit that making a slight improvement for two modules we might get a sligh loss in tests: e.g. if we want to test on_finalise in some module and that module doesn't care about block number then we will still have to supply it.

can always have a helper that calls Module::on_finalise(System::block_number())

pepyakin · 2018-09-04T10:53:26Z

polkadot/parachain/test-chains/basic_add/Cargo.lock

@@ -0,0 +1,224 @@
+[[package]]


Hm, I think this is not supposed to be checked in, is it?

i think so - it's an entirely separate project

pepyakin · 2018-09-04T11:37:32Z

substrate/runtime/treasury/src/lib.rs

+		spend_period: T::BlockNumber,
+		burn: Permill
+	) -> Result {
+		<ProposalBond<T>>::put(proposal_bond);


This is subtle: we must make sure that this isn't called if we already have any proposals, right? Does this worth a check and/or a comment?

Good catch. Needs further work but will just make it safe for now.

gavofyork · 2018-09-04T15:15:54Z

@pepyakin Grumbles addressed

* master: (22 commits) Introduce treasury and document (#646) Off-the-table staking preference (#656) Implement function `json_metadata` in `decl_module!` (#654) Fix warnings in networking (#652) Add a reputation system (#645) Check for pruned block state (#648) Contract runtime polishing (#601) WIP on chain heap (#639) Events to track extrinsic success (#640) Install llvm-tools-preview component (#643) fix wasm executor compile error (#631) random fixes (#638) Empty becomes (), reflecting convention (#637) Allow to build_upon skipped entries, but don't walk back (#635) Separate out staking module into balances and payment (#629) Update .gitlab-ci.yml (#633) Do not attempt to rustup if in CI. This is taken care of by the base (#621) Avoid need for ident strings in storage (#624) rename to panic_handler as panic_implementation is deprecated in nightly (#626) 5 random fixes (#2) (#623) ...

…and-rlpcodec * master: Upgrade to libp2p master (#660) Include function comments into modules `json_metadata` (#657) Replace old headers with CHT in light clients (#512) Fix build Introduce treasury and document (#646) Off-the-table staking preference (#656) Implement function `json_metadata` in `decl_module!` (#654) Fix warnings in networking (#652) Add a reputation system (#645) Check for pruned block state (#648)

Introduce treasury and document

8fe8c54

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

gavofyork added 11 commits September 2, 2018 10:13

Revert bad changes

a132ba5

More reversions

f194a42

Add example crate

3b94238

- Remove HasPublicAux - Rename Concrete -> Runtime

Actually commit stuff

d641b7b

Changes

721eafc

Propagate block number in finalise.

b072089

Fix and build example

7d7ec13

Fixes.

34b9838

Fix compilation for treasury.

70c65ca

Fix the treasury test

fc4f0a3

Tests

99e0f26

dvdplm previously requested changes Sep 4, 2018

View reviewed changes

gavofyork added 3 commits September 4, 2018 08:52

Fix.

10a4c45

Fix tests

026ad90

Fix a few grumbles

87b537a

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 Sep 4, 2018

Fixes

60fced6

gavofyork requested review from arkpar and pepyakin September 4, 2018 09:58

pepyakin reviewed Sep 4, 2018

View reviewed changes

Fix grumbles

d46e8e1

pepyakin approved these changes Sep 4, 2018

View reviewed changes

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

gavofyork merged commit 2b41b77 into master Sep 4, 2018

gavofyork deleted the gav-treasury branch September 4, 2018 15:29

pepyakin mentioned this pull request Sep 4, 2018

Off-the-table staking preference #656

Merged

lamafab pushed a commit to lamafab/substrate that referenced this pull request Jun 16, 2020

Change README to reflect master branch for Kusama CC3 (paritytech#646)

0526413

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce treasury and document #646

Introduce treasury and document #646

gavofyork commented Sep 2, 2018 •

edited

Loading

dvdplm left a comment

dvdplm Sep 4, 2018

gavofyork Sep 4, 2018

dvdplm Sep 4, 2018

gavofyork Sep 4, 2018

dvdplm Sep 4, 2018

gavofyork Sep 4, 2018

dvdplm Sep 4, 2018

dvdplm Sep 4, 2018

dvdplm Sep 4, 2018

dvdplm Sep 4, 2018

dvdplm Sep 4, 2018

dvdplm Sep 4, 2018

dvdplm Sep 4, 2018

pepyakin Sep 4, 2018

pepyakin Sep 4, 2018

pepyakin Sep 4, 2018

pepyakin Sep 4, 2018

gavofyork Sep 4, 2018

pepyakin Sep 4, 2018 •

edited

Loading

gavofyork Sep 4, 2018

pepyakin Sep 4, 2018 •

edited

Loading

gavofyork Sep 4, 2018

pepyakin Sep 4, 2018

gavofyork Sep 4, 2018

gavofyork commented Sep 4, 2018

		@@ -0,0 +1,385 @@
		// Copyright 2017 Parity Technologies (UK) Ltd.

		@@ -0,0 +1,555 @@
		// Copyright 2017 Parity Technologies (UK) Ltd.

Introduce treasury and document #646

Introduce treasury and document #646

Conversation

gavofyork commented Sep 2, 2018 • edited Loading

dvdplm left a 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

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 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepyakin Sep 4, 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

gavofyork commented Sep 4, 2018

gavofyork commented Sep 2, 2018 •

edited

Loading

pepyakin Sep 4, 2018 •

edited

Loading

pepyakin Sep 4, 2018 •

edited

Loading