Control pallet major changes #89

vovacha · 2022-08-15T14:03:25Z

Refactoring reasons: SBP-authored issues (storage, error handling), common naming convention (state names, errors, events, etc.)

Control new features:

New extrinsic "spend_funds" Organisation Control #75
New extrinsic "update_org" Change Organisation #74
Allowed origins: root or prime if OrgType::Individual.
Updated signature for "create_org": some of the params are optional.
Validation related to org creation (Company and Hybrid not allowed).
Added migration (flush the old data).

Control refactoring:

Naming: Controller -> Prime.
Campaign id generation: hash of campaign object instead of hash of nonce (same approach for proposals).
Removed redundant storage, discussion -> SBP: Storage usage #53 (comment)
Removed redundant errors.
Removed unused and commented code.
Error handling changes due to SBP: Error Handling #54
Naming changes (states, event names, storage names, etc.)
Benchmarks, tests, mocks.

signal/src/lib.rs

2075 · 2022-08-15T15:18:51Z

flow/src/types.rs

-	Active = 1,
-	Paused = 2,
-	Finalizing = 3,
-	Reverting = 4,


what if you get an issue while reverting?

If you are asking from the perspective of having panic during on_initialize - the whole chain will stop working.
If the question is about error handling in general, there are no expected issues (unreserve balance operation looks reliable enough). Not sure that I got the question.

in case A there needs to be a way to recover. without that state recovering will be hard as you have to check all the balances, blocks and revert respecitvely.

Is something wrong with the changes in the current PR?
Is something wrong with the design in general?

Currently, CampaignFinalizationQueue holds information about what was scheduled and what was done. It doesn't differ too much from how it was implemented before. It's even easier to debug. No need to use indexes. Instead, there is a separate structure created only for queue handling.
@2075

flow/src/types.rs

control/src/lib.rs

vayesy · 2022-08-15T18:21:32Z

control/src/lib.rs

 			Ok(())
 		}

 		/// Enable Org
 		///
 		/// Enables an Org to be used and changes it's state to Active.
-		/// Root origin only.


Probably specify Root or prime only.?

in that case the question is: who would disable e.g. a vote? why and how?

root only exists now, later on there is not root

a prime might have bad intensions, but could/should be able to lock an org if he wants to "have a break"

usually this would be imposed by either a DAO internal council vote

in case of bad actors, disabling could be imposed by a Protocol Council Vote, like a circuit breaker.

therefore it must be root, e.g. 3/4 majority council ( tbd as per last discussion @5-mark re majorities and quorums)

@2075 I agree with these points.
However documentation should reflect current logic and not the one, which we would like to achieve in some future.
At the moment, voting is not yet implemented here. Once it will, we should update documentation accordingly.

The design idea, as I understand it, is next:

Control pallet interface allows direct changes for root or prime if OrgType::Individual.

Signal pallet interface allows indirect changes through governance mechanisms if OrgType::Dao - TBD.

@2075, does it make sense, or should we reconsider it?

And talking about the Control pallet scope (no governance involved), what's your suggestion here?
At the moment, the permissions model for Control pallet extrinsics is:

add_member - root or prime

remove_member - root or prime

enable_org - root or prime

disable_org - root or prime

spend_funds - root or prime

@2075, what should be changed here?

Also, ensure_root_or_prime idea was partially discussed here #75 for the spend_funds extrinsic, I've just extrapolated it for everything else.

cc: @5-mark

vayesy · 2022-08-15T18:23:00Z

control/src/lib.rs

 			Self::deposit_event(Event::OrgEnabled(org_id));
 			Ok(())
 		}

 		/// Disable Org
 		///
 		/// Disables an Org to be used and changes it's state to Inactive.
-		/// Root origin only.


Root or prime only.?

see response before

control/src/lib.rs

5-mark · 2022-08-17T16:17:14Z

@vovacha / @2075 / @vasylenko-yevhen

Findings on `updateOrg`

✅ only controller of the organisation can run the updateOrg Extrinsic
✅ updateOrg changes all fields correctly
✅ / ❓ account performing the addMember Extrinsic pays the membership fees, not the account which was added. (actually I believe only the account which performs the action should be added as member, but this is already a step forward)
❓ accessModel contains 3 values: Open, Prime & Voting. Does Prime mean "private & whitelist" and Voting means "private & apply"? If yes, also both (private, whitelist & apply) would/should be possible.

❌ I could change the prime to an account which is not member of the organisation.
❌ change organisation type is missing

Findings on `spendFunds`

✅ only controller of the organisation can run the updateOrg Extrinsic
✅ spending works through controller
❓ I could not test a scenario where funds are reserved. We dont have an extrinsic in currencies pallet to reserve funds like we have on balances pallet (eg see balances setBalance)

findings on validation

✅ validation on Company and Hybrid works. We need to follow up / keep in mind that certain actions (KYB) can change the org type to company or hybrid - will take care later of that.

2075 · 2022-08-17T20:53:02Z

good testing, should also drive the test cases in code.

✅ only controller of the organisation can run the updateOrg Extrinsic
-> shouldn't this invoke a voting? but for now prime is sufficient

❓ account performing the addMember Extrinsic pays the membership fees, not the account which was added. (actually I believe only the account which performs the action should be added as member, but this is already a step forward)
-> this was the intended design, a user adds itself and pays the fees, otherwise people will add random other people and drain their balances?

❓ accessModel contains 3 values: Open, Prime & Voting. Does Prime mean "private & whitelist" and Voting means "private & apply"? If yes, also both (private, whitelist & apply) would/should be possible.
-> need a matrix for that

❌ I could change the prime to an account which is not member of the organisation.
-> outch, should definitely not be possible and also only be executed by a prime vote I'd say

✅ only controller of the organisation can run the updateOrg Extrinsic
✅ spending works through controller
-> for now ok that prime is executing it

❓ I could not test a scenario where funds are reserved. We dont have an extrinsic in currencies pallet to reserve funds like we have on balances pallet (eg see balances setBalance)

findings on validation

✅ validation on Company and Hybrid works. We need to follow up / keep in mind that certain actions (KYB) can change the org type to company or hybrid - will take care later of that.
-> for the time being we should rely on native functions we have

vovacha · 2022-08-18T10:36:33Z

actually I believe only the account which performs the action should be added as member

I'll finalize this PR after the add/remove member discussion #85 (comment) has some outcomes.
As for the other issues - already been fixed.

2075 · 2022-08-18T11:17:07Z

actually I believe only the account which performs the action should be added as member

I'll finalize this PR after the add/remove member discussion #85 (comment) has some outcomes. As for the other issues - already been fixed.

what about the root / prime / voting discussion?
please either fix it now or create a backlog item

vovacha · 2022-08-18T11:21:16Z

what about the root / prime / voting discussion?

I've only created a discussion in the context of this PR, which is also about root and prime - #85 (comment)
As for the voting - sure, let's have a discussion. Fill free to create one.

vovacha · 2022-08-19T07:52:59Z

what about the root / prime / voting discussion? please either fix it now or create a backlog item

Backlog item created: #98

vovacha · 2022-08-19T07:58:08Z

I'm going to merge it.
add_member, remove_member logic will not be changed for the moment since there were no suggestions here #85 (comment).

2075 reviewed Aug 15, 2022

View reviewed changes

signal/src/lib.rs Outdated Show resolved Hide resolved

2075 reviewed Aug 15, 2022

View reviewed changes

signal/src/lib.rs Outdated Show resolved Hide resolved

2075 reviewed Aug 15, 2022

View reviewed changes

signal/src/lib.rs Outdated Show resolved Hide resolved

2075 reviewed Aug 15, 2022

View reviewed changes

flow/src/types.rs Show resolved Hide resolved

Major Control pallet changes

0c1e1e5

vovacha force-pushed the feature/control-changes branch from 93410a4 to 0c1e1e5 Compare August 15, 2022 17:07

vayesy suggested changes Aug 15, 2022

View reviewed changes

Tests fixes, changes after review

1a5b432

vovacha marked this pull request as ready for review August 16, 2022 12:22

vovacha requested review from 2075 and vayesy August 16, 2022 12:22

vayesy approved these changes Aug 16, 2022

View reviewed changes

Changes after review

ee8ddae

vovacha merged commit daf8686 into release-1.1.2 Aug 19, 2022

vovacha deleted the feature/control-changes branch September 13, 2022 11:04

vovacha added a commit that referenced this pull request Sep 13, 2022

Control pallet major changes (#89)

a7b3763

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Control pallet major changes #89

Control pallet major changes #89

vovacha commented Aug 15, 2022 •

edited

Loading

2075 Aug 15, 2022

vovacha Aug 15, 2022

2075 Aug 15, 2022

vovacha Aug 16, 2022

vayesy Aug 15, 2022

2075 Aug 15, 2022

vayesy Aug 16, 2022

vovacha Aug 16, 2022 •

edited

Loading

vayesy Aug 15, 2022

2075 Aug 15, 2022

5-mark commented Aug 17, 2022

2075 commented Aug 17, 2022

findings on validation

vovacha commented Aug 18, 2022 •

edited

Loading

2075 commented Aug 18, 2022 •

edited

Loading

vovacha commented Aug 18, 2022

vovacha commented Aug 19, 2022 •

edited

Loading

vovacha commented Aug 19, 2022

Control pallet major changes #89

Control pallet major changes #89

Conversation

vovacha commented Aug 15, 2022 • 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

vovacha Aug 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

5-mark commented Aug 17, 2022

Findings on updateOrg

Findings on spendFunds

findings on validation

2075 commented Aug 17, 2022

findings on validation

vovacha commented Aug 18, 2022 • edited Loading

2075 commented Aug 18, 2022 • edited Loading

vovacha commented Aug 18, 2022

vovacha commented Aug 19, 2022 • edited Loading

vovacha commented Aug 19, 2022

vovacha commented Aug 15, 2022 •

edited

Loading

vovacha Aug 16, 2022 •

edited

Loading

Findings on `updateOrg`

Findings on `spendFunds`

vovacha commented Aug 18, 2022 •

edited

Loading

2075 commented Aug 18, 2022 •

edited

Loading

vovacha commented Aug 19, 2022 •

edited

Loading