Multisig Permissions Extension

[area]

This PR implements a 'multisig permissions' extension. The underlying intention here is to make it so that powerful permissions (e.g. root) don't have to be under the control of a single EOA. So an action that the multisig should take can be suggested, and then if enough people (with the 'right' permissions) sign off on it, then it can be executed.

Specification

Should mirror the colony-level ('core' being the nomenclature I've adopted for those) permissions exactly - so being able to award permissions in specific domains, as well as being able to leverage permissions in child domains of the domains where the permissions have been explicitly awarded.

In terms of the 'threshold' for execution, the ideal functionality was some fraction of people with the relevant permissions. However, I couldn't establish a way to realistically do this, and so went with an 'acceptable' threshold for 'votes' on an action before it could be executed. If a reviewer comes up with a way to implement fraction-based rather than absolute-based approvals, I'd be interested to hear it.

Permissions work as such:

• If a fixed threshold for the relevant domain is set, that number is used
• Otherwise, if a fixed global threshold is set, that number is used
• Otherwise, n/2 + 1 approvals are required, where n is the number of people who explicitly have the relevant permission in the domain in question. People with the permission inherited can approve, but do not count towards the calculation of the threshold.

Less prose-like requirements:

• Removal of a permission doesn't remove approvals already made, and you can remove your approvals even if the permission that originally allowed you to make an approval is removed.
• Anyone can execute an action if it has enough approvals.
• Overapprovals are allowed
• To suggest an action, you require the permissions at least one of the permissions that would be needed to approve it, and you automatically approve it on creation.
• Multiple transactions can be suggested in a single action (either natively, or via multicall), but they must all require the same permissions.
• If an action fails on execution, the transaction reverts if it's been less than a week since it was eligible for execution. Otherwise, the action fails, though the transaction succeeds overall, and the motion will not be able to be retried.
• Core permissions can award multisig permissions they could award core versions of, and vice-versa (if the threshold is reached when going via multisig)

Implementation notes

There is some arguably surprising behaviour around what happens if the threshold is changed and motions have already had approvals made, but I've tried to make it as straightforward as possible; if an approval/unapproval causes a motion to cross the threshold, the timestamp is updated. If the timestamp is 0, but we're able to execute, the call of execute will update the timestamp to now, and do nothing else. A second call to execute will actually execute the motion. I'm very open to suggestions here, as I'm not totally happy with where I've landed.

Where multiple permissions are required for a function call (currently the case for OneTxPayment, and nothing else), each permission has a separate threshold, and both must be met for execution to be allowed.

[arrenv]

If an action fails on execution, the transaction reverts if it's been less than a week since it was eligible for execution. Otherwise, the transaction succeeds and the motion will not be able to be retried.

Should this be "Otherwise, the transaction fails and the motion will not be able to be retried."?

[arrenv]

There is some arguably surprising behaviour around what happens if the threshold is changed and motions have already had approvals made, but I've tried to make it as straightforward as possible; if an approval/unapproval causes a motion to cross the threshold, the timestamp is updated. If the timestamp is 0, but we're able to execute, the call of execute will update the timestamp to now, and do nothing else. A second call to execute will actually execute the motion. I'm very open to suggestions here, as I'm not totally happy with where I've landed.

What I understand from this is that we have these scenarios:

• Motion HAS passed the threshold and CAN be finalized, a new EOA is awarded the same permissions and now the threshold changes and now the motion CANNOT be finalized until another approval is given.
• Motion HAS NOT passed the threshold, an EOA has their permissions revoked and now the threshold changes and now the motion CAN be finalized.

However, you have a timestamp to account for this, but that requires an additional transaction each time? Can they be bundled?

Am I right in this understanding?

[area]

If an action fails on execution, the transaction reverts if it's been less than a week since it was eligible for execution. Otherwise, the transaction succeeds and the motion will not be able to be retried.

Should this be "Otherwise, the transaction fails and the motion will not be able to be retried."?

It should be "Otherwise, the action fails, though the transaction succeeds overall, and the motion will not be able to be retried".

[area]

What I understand from this is that we have these scenarios:

* Motion HAS passed the threshold and CAN be finalized, a new EOA is awarded the same permissions and now the threshold changes and now the motion CANNOT be finalized until another approval is given.

* Motion HAS NOT passed the threshold, an EOA has their permissions revoked and now the threshold changes and now the motion CAN be finalized.

However, you have a timestamp to account for this, but that requires an additional transaction each time? Can they be bundled?

Am I right in this understanding?

First off, the number of addresses with a given permission is independent of the threshold, which is just a fixed number. If that many people or more have approved something, it can be called.

In the first scenario, let's say 2 people have approved a motion (which was sufficient for execution, and set the timestamp) and the threshold is now 3. Attempts to execute will fail (as we do not meet the threshold). If a third approves, the timestamp will be updated, and the motion will have a week as expected before failed executions are allowed. No additional transactions are required.

In the second scenario, let's say 2 people had approved a motion, with a threshold of 3. The threshold is now moved to 2, and so the motion can be executed, but the timestamp hasn't been set. The first call of execute will set the timestamp and do nothing else. A second call of execute will actually execute the motion (or try to, if it fails). These can be bundled if the motion is going to execute successfully, but if the motion fails and the transaction reverts, if you have bundled them then the timestamp will also not be set, and so the 'clock' for allowing a failing execution will not have started.

I'll also note that in normal activity (i.e. the threshold not being changed), only one execute call is ever required.

[arrenv]

First off, the number of addresses with a given permission is independent of the threshold, which is just a fixed number. If that many people or more have approved something, it can be called.

This is on me, as we spoke about this but I misunderstood. I had understood our two options were a configurable fixed threshold which was difficult and the other was a broad approach using the same formula as exiting recovery numRequired = totalAuthorized / 2 + 1. Although, I distinctly remember you saying it needed to be fixed and I seemed to have perceived that as meaning the opposite. So, now I have questions:

• How is this set? Is it per Colony or Colony Network wide?
• Is it configurable on extension enabling?
• Is it changeable after it has been set?
• Is it configurable per permission type and per team?
• If it is set to a value higher then the number of permission holders, does that mean Motions can never be approved?

In the second scenario, let's say 2 people had approved a motion, with a threshold of 3. The threshold is now moved to 2, and so the motion can be executed, but the timestamp hasn't been set. The first call of execute will set the timestamp and do nothing else. A second call of execute will actually execute the motion (or try to, if it fails). These can be bundled if the motion is going to execute successfully, but if the motion fails and the transaction reverts, if you have bundled them then the timestamp will also not be set, and so the 'clock' for allowing a failing execution will not have started.

So, basically by bundling them you can get around the 1 week rule, in a sense. So, they need to be two transactions so that the rule stays in place.

EDIT:

These were discussed on a call.

How is this set? Is it per Colony or Colony Network wide?

• This is set per Colony.

Is it configurable on extension enabling?

• You configure it on initialization.

Is it changeable after it has been set?

• You can change it after initialization using Root permissions.

Is it configurable per permission type and per team?

• Not configurable per type or per team

If it is set to a value higher then the number of permission holders, does that mean Motions can never be approved?

• This is correct. However, if this is an issue in child domain, anyone with parent team permissions would also be permission holders.

UPDATES:
Based on this, it has been decided to modify to allow the following options:

• Single fixed Colony wide threshold, which is changeable by the root permission.
• Relative value dependent on the number of explicit permission holders of the team, not inherited
• A fixed threshold configurable per team.

[kronosapiens]

Initial skim. My first reaction is that I don't particularly like so much of the permissions logic being simply duplicated here, but will need to think more about the intended functionality before making an argument in any direction.

[area]

Relative value dependent on the number of explicit permission holders of the team, not inherited

Just noting this explicitly/precisely for posterity

[kronosapiens]

Two high-level thoughts:

• There are a lot of comments here. I'm not sure if you intend to leave them there or if they're artifacts of your thinking through the design as you went, but either way I don't think they need to stay in the final version

• There's some code overlap between this and Update VotingReputation for Multicall #1135. We've discussed introducing "mixins" containing reusable functions and inheriting from them. Would you want try doing that here? I'm thinking getActionDomainSkillId and extractCallData specifically.

[kronosapiens]

Reiterating that I'd like to see a mixin architecture for some of the functionality used in the multisig, since a lot of those functions are pulled from the voting contracts.

[area]

I'm not sure I've ever had someone say 'there are too many comments'. They certainly originated from when I was thinking through the implementation, but what's your reasoning here for removing them? Do you not see them as helpful?

[kronosapiens]

@area ultimately the comments read to me like they were written to describe the logical flow of the contract to help you implement, rather than comments designed to add useful clarifying information. Now that the implementation is done, I think the code can stand on its own. Redundant comments create additional complexity when parsing a contract, and so I would remove them.

Example:

          if (_approved) {
            // They are now approving it