Proposals system. Version 2. Iteration6

[shamil-gadelshin]

Features

• monorepo integration
• introduced ActorOriginValidator in the common module for future integration with the membership and council modules
• proposal system authentication system (membership integration) in the engine and discussion modules (ActorOriginValidator implementation - MembershipOriginValidator)
• council integration for voting: ActorOriginValidator and VotersParameters implementations with tests - CouncilManager
• created tests for MembershipValidator and CouncilManager
• returned stake after the unstaking (StakingEventsHandler integration)
• removed proposals v1
• runtime integration and implementing all handlers required (council handler, total voters handler)
• change proposals execution model
• move all proposals modules from string error constants to decl_error! approach
• jammed 'srml_staking_reward_curve::build!' warning
• StakingEventsHandler integration with the ContentWorkingGroupStakingEventHandler
• set_countil() is public now for testing purposes (it's guarded by ensure_root()))

Related issues

• Map Substrate origin parameter to the MemberId and CouncillorId in the proposals module #174
• Fix use of array into iter #179
• Possible AccountId change during active proposal #154

Iteration 5 features (was skipped)

• spam gating: prevent the same author from writing more than N posts in a row.
• change EnsureOrigin to the ActorOriginValidator model (was rewritten during the 6th iteration )
• create events for the discussion system

[bedeho]

• This review goes beyond the diff in the PR, because it was frankly really hard to just follow that, given how long ago I looked at this, and also because prior reviews did not go that in depth on different extrinsics, as we were just getting started.

• In general, types with postfix "data" indicate a sloppy name or bad type, unless they literally represent unstructured data without any internal meaning. There is at least StakeData and FinalizationData in this iteration. For the former, I think it should not exist (as you will see in the review), and for the latter, see next section.

• Question: Is there any reason to check origin on pure Module methods? doesnet seem to make sense? the calling code will perform the origin check. Perhaps this is why we have the akward double origin thing... its not needed?

Engine

• When a council changed, I think we agreed to reset all the current votes and reset the finalization deadline. That is currently not implemented, right?

• execute_proposal appears to have a bug, as it unconditionally removes from PendingExecutionProposalIds, even if the first guard rejects the proposal. Moreover, the routine does not check that the proposal actually exists, yet it does check that it has the right state. This seems a bit inconsistent. I think that if its going to make an assumption without checking, it needs not check for any even weaker assumptions. Moreover, its also a bit akward that it is rechecking whether the given proposal is finalized, despite the caller already having done this check in computation get_approved_proposal_with_expired_grace_period_ids, that should be avoidable in the same code path by picking the right representations. Have a look, it may not be worth changing so much, but I wanted to draw your attention to it.

• For FinalizationData

  • I think there is a crisper name could be FinalizedProposalStatus
  • The field finalization_error should reflect that this is specifically the serialized error associated with unstaking. When reading it I first thought this is what held the error related to execution as well, but that is indeed separate, so name should be sharpened. Also, something needs to be done to very clearly signal that this is no normal error, but indeed the broken invariant error. At the very least the comment must say so, but perhaps also the name, e.g. encoded_unstaking_error_due_to_broken_runtime. Its a bit long, but the length helps to grab the attention to how distinct this field is.
  • Lets drop finalized_at. Its purely archival, and so query node should hold it, and it's not even clear that we will, in the end, hold the entire proposal itself after it is executed.

• Did we not decide that, for the time being, we will not store broken invariant errors in storage, simply for the benefit of archiving error messages. We would use either console logging or something else to capture them? Unfortunately, we should have written down what we decided. At any rate, this sort of storing of errors seems to do little if anything of increasing safety or blocking further state corruption, which are the strong reasons for handling broken runtimes in some way.

• The only reason we have ActiveProposalIds is so that more effectively iterate only the proposals that are active when trying to finalize. This also why we need to limit how many can be active at once.

  This is as intended, but I want to explore an alternative. If you think its an improvement, lets make an issue out of it and have someone work on it at a later time:

  We drop ActiveProposalIds and the limit on the number of active proposals, as well as any iteration in on_finalized. Instead, a new map finalize_at_block: T::BlockNumber => T::ProposalId is introduced, and a mapping is introduced whenever a proposal is created. This map is checked every on_finalized, but there is no iteration, you just check for the specific block you are on. There is an implicit limit that only one proposal can be finalized per block, and that is enforced upon proposal creation.

  It seems this is both safer, as there is no enumeration and iteration, and we drop one parameter value, and we have no limit on the number of simultaneous proposals in flight.

  What do you think?

  The same would apply to PendingExecutionProposalIds.

• Seems like this storage map should be dropped, and instead just hold the value as a field in the struct Proposal.

  pub ProposalCode get(fn proposal_codes): map T::ProposalId => Vec<u8>;

Codex

Looks great!

Discussion system

Looks great!

[bedeho]

This state variable is basically set if there is staking, and unset when there is finalization in finalize_proposal. This is a rational policy. However, the representation is not 100% safe: we still allow the representation of illegal states, even though we need not. In particular stake_data = Some(x) when status = ProposalStatus::Finalized(y).

• This particular principle of using the type system fully is something I think we cannot compromise, so I think we must change this.
• An alternative "workaround" is to never clear stake_data, then the representation is fully safe. However, I think this approach does not make sense, as we should in general not use state just for archival purposes. The query system will hold all such information if it's needed. For even things like FinalizationData::finalized_at need not exist for the same reason.

[shamil-gadelshin]

Yes, we were going to decide whether to remove stake information from the system on proposal finalization.

[bedeho]

• ProposalObject is not a great name. I think the full workaround is to just have Proposal accept T, rather than unbundled set of types. The general syntatic overhead is nicer. Personally I have mixed both approaches, but in the end I think just taking T is easier on the eyes.

Here is a convention about it from a while back #36 (comment).

[shamil-gadelshin]

Agree, will change to 'ProposalOf' similar with BalanceOf

[bedeho]

Let's not require full membership and governance module configuration trait Trait, just to get constraints for Ids. This means any user of the engine must involve these traits. Let just use normal integer id constraints.

[shamil-gadelshin]

Governance module dependency is needed to create CouncilManager. We can move it to the council module, but then the governance module will be dependent upon the membership module.

[bedeho]

The constraint here captures some runtime dispatchable + a set of parameters for used in a call to it. Is that orrect? If so, perhaps we can find a better name than this, as it alludes to raw code being involved, which threw me a bit off when reading the implementation. How about DispatchableCall ? Likewise when you look at create_proposal rather than saying proposal_code: Vec<u8>, which really makes it seem like perhaps this is raw byte code. This could be encoded_dispatchable_call: Vec<u8>, etc. for other places.

[shamil-gadelshin]

Ok.

[shamil-gadelshin]

When a council changed, I think we agreed to reset all the current votes and reset the finalization deadline. That is currently not implemented, right?

We didn't discuss it yet. I will add it to my backlog.

[shamil-gadelshin]

We need finalized_at not only for archival purposes, but we calculate our grace_period using its value.

Also, we moved finalized_at inside proposal status during the previous PRs.

[shamil-gadelshin]

This map is checked every on_finalized, but there is no iteration, you just check for the specific block you are on. There is an implicit limit that only one proposal can be finalized per block, and that is enforced upon proposal creation.

Proposal veto and cancellation cannot be predicted

[shamil-gadelshin]

Seems like this storage map should be dropped, and instead just hold the value as a field in the struct Proposal.

pub ProposalCode get(fn proposal_codes): map T::ProposalId => Vec;

We have a rule with the exact opposite:
#36 (comment)

And the rule, in general, makes sense, because, for example, WASM code can be 'heavy'. And proposal object is encoded and decoded multiple times during its lifetime.

[shamil-gadelshin]

Work to be done using this issue

[bedeho]

Great job!