Proposals v2 iteration2 #152

shamil-gadelshin · 2020-02-11T13:51:46Z

Copied PR to the development branch: original PR and commit history can be found here -
mnaamani#2

- add iteration 2 codex and engine modules

- delete codex/Cargo.lock - delete engine/Cargo.lock - update gitignore

bedeho

Execution model: The model seems very complex, and its not clear how exactly to naturally use it for different types. I already see that the proposal_code is not per say meant to be literal WASM code fragments, and it would be very impractical to expect that sort of heavy setup and handling in a codex module. I think the much simpler approach of passing a dispatchable, like is done in democracy, is extremely simple and clean. The codex would simply hold the dispatchables it wanted to run, and guard with root origin check.
We did not actually properly define what the side-effect of the text proposal should be. I think we should not confuse the proposal body, with the main proposition itself, as the body will possibly include a broader rationale, links to referecnes etc. The actual text proposal being voted on should be a standalone parameter I think. The side-effect would be that the proposal text is stored as in some state in the codex engine. In the future perhaps it should be stored somewhere else, but for now we may as well put it there, although I do like the idea of the codex being stateless.
As I point out later in the review, actors should not be identified with accounts, but with some configurable role identifier, at least in the engine. In the runtime, we probably want ot use the membership id for the proposer type, and the council member id for the voter type.
Just to confirm, is it the case that the codex crate is only consuming anything in the proposals engine crate via crate based dependency management? There is no use of relative paths of modules in the directory tree?

modules/proposals/engine/src/lib.rs

shamil-gadelshin · 2020-02-12T14:14:25Z

1. Execution model

I'm not sure it is possible. There are no dispatchable extrinsic before runtime compilation. In democracy, proposal associated type defined as

pub trait Trait: system::Trait + Sized {
	type Proposal: Parameter + Dispatchable<Origin=Self::Origin>;
...
}

and instantiated variant is

impl democracy::Trait for Runtime {
	type Proposal = Call;
...
}

Where Call is 'outer call' for the runtime:
https://substrate.dev/docs/en/runtime/types/call-enum

And you cannot have serialized extrinsic call inside of the neighbor extrinsic. I tried to do that in my first iteration. And this exactly the reason why I suggest to create proposal parameters on the frontend - to be able to pack extrinsic call together with parameters and create a proposal like 'democracy' does.

Current implementation reintroduces such approach with full control over the code.

Not fully understand about WASM. The code can be compiled with no_std and works in runtime.

2. Text proposal

For now, it is no more than the model of the actual proposals.
Do we have exact requirements for it?

From your comments:

add another 'text' parameter
store the text of the proposal in the codex storage

3. Actors and accounts

I can introduce a separate entity not connected with account_id, but we still need an account_id to place a stake. Current stakes module depends on Currency which is bound to the AccountId. And what should we do when account_id changes when we have an ongoing proposal?

4. Codex and engine crates

Not sure I understood you properly:

engine and codex are in separate crates
engine crate is independent (only substrate dependencies to the moment)
codex meant to be dependent on joystream crates
dependency path can be relative or git-based or any other
for now, I assume the 'path variant'

- introduce ProposalId type - change BTreeSet to the linked_map

- change VoteExistsByProposalByAccount to proposal_id first key - clear cache on proposal finalization

- rename and refactor tally() method in functional style - introduce internal type FinalizedProposalData

shamil-gadelshin · 2020-02-14T14:24:43Z

On account change during active proposal:
#154

shamil-gadelshin · 2020-02-14T14:25:34Z

I will implement max active proposals as a new feature during the current iteration.

shamil-gadelshin · 2020-02-14T14:47:54Z

On 'voter' and 'proposer': for now, I added associated types ProposerId and VoterId and removed account_id dependency. The exact relation between ProposerId and ProposerOrigin (VoterId and VoterOrigin) I will elaborate after Rome - when I connect council and membership modules to the codex.

bedeho

I see that you opt to have mutable variables in a few places. I think we should try to minimize this.
Something struck me as I was reviewing this about how we would do slashing: I recall we agreed that there should be a separate quorum and threshold for this action. If this is the case, then we must wait the full voting period before trying to decide whether to accept or slash a proposal, otherwise the engine is sensitive to the accidental order in which people may choose to vote. Please check my logic here.

modules/proposals/engine/src/lib.rs

bedeho · 2020-02-14T18:41:12Z

modules/proposals/engine/src/types.rs

+    pub tally_results: Option<TallyResult<BlockNumber>>,
+
+    /// Votes for the proposal
+    pub votes: Vec<Vote<VoterId>>,


This is OK, but its starts to become a performance issue if the engine is to be used with a large number of voters, as you will have to load/write back all current votes each time anyone votes. I did not mean for this field to be lifted out of storage, but in practice I think it will be OK, at least for us.

Make this a BTreeMap, using the VoterId as key, then we get greater semantic clarity, and runtime guarantees of no double voting, and checks for past voting in O(nlogn) time, rather than O(n).

Drop VoterId property from Vote, it is redundant, and opens up room for bugs, where a vote has different id value than key that maps to it.

--

Updated

Above I assumed that votes had replaced VoteExistsByProposalByVoter, which actually was not needed, but I see now that it has not. Now it is very unclear to me what the purpose is? it seems related to the computation in update_tally_results, which also appears needlessly complicated. I suspect I am missing something, so let me outline what I would have thought was the obvious approach.

Proposal has fields for counting abstentions, approvals and rejections. This is fine, because it is always well defined to have these in life cycle of a proposal. Initial values are set to 0.

Whenever a vote is submitted, increment the relevant counter. If quorum is reached, then we can update the state suitably.

In on_finalized check all active proposals for whether they may have expired in this block, in which case, update the state suitablly

Is this not sufficient?

bedeho · 2020-02-14T18:46:00Z

modules/proposals/engine/src/types.rs

    pub status: ProposalStatus,
+
+    /// Tally result for the proposal
+    pub tally_results: Option<TallyResult<BlockNumber>>,


Why is this an optional?

For all states of a proposal, it is well defined to have cumulative counter values set, this type choice gives the impression that there is some set of states

Even the TallyResult type seems to have little value at this point.

modules/proposals/engine/src/lib.rs

Migrating from TallyResults and vote calculation in the end of block to vote calculation on the vote() extrinsic call.

Migrating out from TallyResults

- refactor map-filter-map pattern to the filter_map - add comments

bedeho

Looks good!

Update Repo Cover Images

shamil-gadelshin added 2 commits February 11, 2020 16:33

Add proposals modules

60eab08

- add iteration 2 codex and engine modules

Delete modules *.lock files

979b8a4

- delete codex/Cargo.lock - delete engine/Cargo.lock - update gitignore

bedeho self-requested a review February 11, 2020 14:01

bedeho requested changes Feb 11, 2020

View reviewed changes

shamil-gadelshin added 9 commits February 13, 2020 16:40

Fix review comments 1

a56b4c5

- introduce ProposalId type - change BTreeSet to the linked_map

Fix tests and update codex module

2495f3c

Embed tally results into the proposal object

a77f8e2

Embed votes into the proposal struct

29c63ff

Change VoteExistsByProposalByAccount cache

a856fd2

- change VoteExistsByProposalByAccount to proposal_id first key - clear cache on proposal finalization

Refactor tally() method

484ae58

- rename and refactor tally() method in functional style - introduce internal type FinalizedProposalData

Codex module build fix

1cea688

Introduce proposer_id instead of account_id

f5857b3

Introduce VoterId type instead of AccountId for voting

3b79fae

Add TODO: add maximum allowed active proposals

cc81780

bedeho requested changes Feb 14, 2020

View reviewed changes

shamil-gadelshin added 7 commits February 17, 2020 12:13

Remove redundant check on voting

def6faf

Add VotingResults type

753fff5

Migrating from TallyResults and vote calculation in the end of block to vote calculation on the vote() extrinsic call.

Remove votes from the Proposal

c11fd84

Introduce define_proposal_decision_status()

bbabc67

Migrating out from TallyResults

Remove TallyResults and Vote types

b2278bf

Add comments and refactor

f350622

- refactor map-filter-map pattern to the filter_map - add comments

Apply cargo fmt

f2e78a3

bedeho approved these changes Feb 17, 2020

View reviewed changes

bedeho merged commit 6b447b6 into Joystream:proposals_v2 Feb 17, 2020

mnaamani pushed a commit to mnaamani/joystream that referenced this pull request Apr 16, 2020

Merge pull request Joystream#152 from blrhc/update-repo-covers

3bb8545

Update Repo Cover Images

shamil-gadelshin deleted the proposals_v2_iteration2_review branch June 25, 2020 14:39

Proposals v2 iteration2 #152

Proposals v2 iteration2 #152

Uh oh!

Conversation

shamil-gadelshin commented Feb 11, 2020

Uh oh!

bedeho left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shamil-gadelshin commented Feb 12, 2020

1. Execution model

2. Text proposal

3. Actors and accounts

4. Codex and engine crates

Uh oh!

shamil-gadelshin commented Feb 14, 2020

Uh oh!

shamil-gadelshin commented Feb 14, 2020

Uh oh!

shamil-gadelshin commented Feb 14, 2020

Uh oh!

bedeho left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bedeho Feb 14, 2020

Choose a reason for hiding this comment

Updated

Uh oh!

bedeho Feb 14, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bedeho left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants