feature: new multisig version #19

clauBv23 · 2024-08-21T19:29:16Z

Description

Add condition for create proposal permission.
Define a target that will work as executor instead of DAO
Update the proposal id to be a hash of the proposal's properties.

A couple of things remaining: ???

Checklist:

I have selected the correct base branch.
I have performed a self-review of my own code.
I have commented my code, particularly in hard-to-understand areas.
I have made corresponding changes to the documentation.
My changes generate no new warnings.
Any dependent changes have been merged and published in downstream modules.
I created tasks to update dependent repositories (OSx, Plugins)
I ran all tests with success and extended them if necessary.
I have updated the CHANGELOG.md file in the root folder.

nivida

We also could generally add FuncDocs to the Solidity contracts.. but this maybe should be done within another PR to keep a narrow scope of changes here :-)

nivida · 2024-08-26T12:27:36Z

packages/contracts/src/Multisig.sol


        // Create the proposal
        Proposal storage proposal_ = proposals[proposalId];

+        // Multisig doesn't allow `minApprovals` settings to be less than 0.


Suggested change

// Multisig doesn't allow `minApprovals` settings to be less than 0.

// Multisig doesn't allow `minApprovals` settings to be 0.

nivida · 2024-08-26T12:28:12Z

packages/contracts/src/Multisig.sol


        // Create the proposal
        Proposal storage proposal_ = proposals[proposalId];

+        // Multisig doesn't allow `minApprovals` settings to be less than 0.
+        // If it is, that means proposal hasn't been created yet.


Suggested change

// If it is, that means proposal hasn't been created yet.

// If it is, that means proposal has been created yet.

The comment or the error type is wrong :-)

nivida · 2024-08-26T12:29:29Z

packages/contracts/src/Multisig.sol

+        TargetConfig memory currentTarget = getTargetConfig();
+
+        if (currentTarget.target == address(0)) {
+            proposal_.target = address(dao());
+            proposal_.operation = Operation.Call;


We probably could have this check within getTargetConfig instead of duplicating it in all plugins.. wdyt? :-)

Good idea, one question though. How would consumer(in this case, Multisig) would figure out if target is 0 or not in case it needs to know this for some reason ? if we return dao when target is 0 inside getTargetConfig, the scenario I explained will not be possible. wdyt ?

nivida · 2024-08-26T12:30:59Z

packages/contracts/src/Multisig.sol

@@ -382,19 +433,29 @@ contract Multisig is
        return isListed(_account);
    }

+    function hashProposal(


A more declarative name would be getProposalHash().. just an idea 🤷

... and I think we don't need named return values here.. we also can just directly return it wdyt? :-)

done in 4ef810f

Where do you think is a better idea to have getProposalHash function - in IMultisig interface or IProposal interface ?

nivida · 2024-08-26T12:34:52Z

packages/contracts/src/Multisig.sol

-            _proposalId,
+        _execute(
+            proposal_.target,
+            bytes32(_proposalId),


What was the reason again to have proposalID stored as uint256 instead of bytes32?
I think we could save us two type conversions here.. 🤔

backwards compatibility. The current multisig/tokenvoting return uint256 and if we go with bytes32, it's not backwards compatible anymore.

clauBv23 · 2024-08-30T14:06:59Z

packages/contracts/src/Multisig.sol

+        address target; // added in v1.3.0
+        Operation operation; // added in v1.3.0


Suggested change

address target; // added in v1.3.0

Operation operation; // added in v1.3.0

address target; // added in v1.3

Operation operation; // added in v1.3

clauBv23 · 2024-08-30T14:55:29Z

packages/contracts/src/Multisig.sol

            this.getProposal.selector;

    /// @notice The ID of the permission required to call the `addAddresses` and `removeAddresses` functions.
    bytes32 public constant UPDATE_MULTISIG_SETTINGS_PERMISSION_ID =
        keccak256("UPDATE_MULTISIG_SETTINGS_PERMISSION");

+    /// @notice The ID of the permission required to call the `addAddresses` and `removeAddresses` functions.


Suggested change

/// @notice The ID of the permission required to call the `addAddresses` and `removeAddresses` functions.

/// @notice The ID of the permission required to call the `createProposal` function.

clauBv23 · 2024-09-13T14:10:22Z

packages/contracts/src/Multisig.sol

+        if (_fromBuild < 3) {
+            TargetConfig memory targetConfig = abi.decode(_initData, (TargetConfig));
+            _setTargetConfig(targetConfig);
+        }


Should we consider reverting when initializing from an invalid build, similar to how it's done in OSX?

For this specific case, I don't see reasoning why we should revert. If target config is not set for some reason on the upgrade, it is still not end of the world.

clauBv23 · 2024-09-13T14:11:13Z

packages/contracts/src/Multisig.sol

+    /// @param _fromBuild The build version number of the previous implementation contract this upgrade is transitioning from.
+    /// @param _initData The initialization data to be passed to via `upgradeToAndCall` (see [ERC-1967](https://docs.openzeppelin.com/contracts/4.x/api/proxy#ERC1967Upgrade)).
+    function initializeFrom(uint16 _fromBuild, bytes calldata _initData) external reinitializer(2) {
+        if (_fromBuild < 3) {


I noticed that in OSX, they are checking the entire version. However, it seems like this may not be necessary for plugins, since upgrading between different releases (e.g., from 1.5 to 2.3) isn’t allowed as far as I understand.

I'd like to hear your thoughts on this.

Yes, We don't use semantic versioning for plugins and can not be upgraded between different releases. So all good.

clauBv23 · 2024-09-13T14:45:08Z

packages/contracts/hardhat.config.ts

@@ -158,6 +158,7 @@ const config: HardhatUserConfig = {
  solidity: {
    version: '0.8.17',
    settings: {
+      viaIR: true,


the solidity-coverage that the project is using has problems with viaIR: true, had to update to latest version to be able to run hh coverage

clauBv23 · 2024-09-13T15:24:02Z

packages/contracts/src/MultisigSetup.sol

+        permissions[1] = PermissionLib.MultiTargetPermission(
+            PermissionLib.Operation.Revoke,
+            _payload.plugin,
+            _dao,
+            PermissionLib.NO_CONDITION,
+            UPGRADE_PLUGIN_PERMISSION_ID
+        );


from v1.3 the UPGRADE_PLUGIN_PERMISSION_ID is not being granted to the DAO, so it doesn’t need to be revoked when uninstalling.
It is already being revoked when upgrading from a previous version

clauBv23 · 2024-09-20T13:29:14Z

packages/contracts/src/Multisig.sol

@@ -170,7 +181,8 @@ contract Multisig is
        _setTargetConfig(_targetConfig);
    }

-    /// @notice Reinitializes the TokenVoting after an upgrade from a previous protocol version.
+    /// @notice Reinitializes the TokenVoting after an upgrade from a previous protocol version. For each reinitialization step, use the `_fromBuild` version to decide which internal functions to call for reinitialization.
+    /// @dev WARNING: The contract should only be upgradeable through PSP to ensure that _fromBuild is not incorrectly passed, and that the appropriate permissions for the upgrade are properly configured.


I would add a description of what could happen if called with an incorrect _fromBuild.
Ex,
Calling it with incorrect _fromBuild could initialize already initialized values, be cautious.

novaknole added 2 commits August 20, 2024 17:51

fix multisig/multisigsetup tests for new implementation

ccb7dee

fix more tests

2ca8113

nivida reviewed Aug 26, 2024

View reviewed changes

novaknole added 3 commits August 27, 2024 11:02

proposal hash return value

4ef810f

add create proposal id

43ef91e

remove .only

a8c7376

clauBv23 commented Aug 30, 2024

View reviewed changes

initializeFrom and erc165 changes for upgrades

6a2c080

clauBv23 commented Sep 13, 2024

View reviewed changes

novaknole added 2 commits September 19, 2024 13:06

modifier added for safety and target config improved

065802a

add comment

6b6fa6f

clauBv23 commented Sep 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: new multisig version #19

feature: new multisig version #19

clauBv23 commented Aug 21, 2024 •

edited by jira bot

Loading

nivida left a comment

nivida Aug 26, 2024

nivida Aug 26, 2024

nivida Aug 26, 2024

novaknole Aug 27, 2024

nivida Aug 26, 2024

novaknole Aug 27, 2024

novaknole Aug 27, 2024

nivida Aug 26, 2024

novaknole Aug 27, 2024

clauBv23 Aug 30, 2024

clauBv23 Aug 30, 2024

clauBv23 Sep 13, 2024

novaknole Sep 21, 2024

clauBv23 Sep 13, 2024

novaknole Sep 21, 2024

clauBv23 Sep 13, 2024 •

edited

Loading

clauBv23 Sep 13, 2024

clauBv23 Sep 20, 2024

	// Multisig doesn't allow `minApprovals` settings to be less than 0.
	// Multisig doesn't allow `minApprovals` settings to be 0.

	// If it is, that means proposal hasn't been created yet.
	// If it is, that means proposal has been created yet.

		address target; // added in v1.3.0
		Operation operation; // added in v1.3.0

	/// @notice The ID of the permission required to call the `addAddresses` and `removeAddresses` functions.
	/// @notice The ID of the permission required to call the `createProposal` function.

feature: new multisig version #19

Are you sure you want to change the base?

feature: new multisig version #19

Conversation

clauBv23 commented Aug 21, 2024 • edited by jira bot Loading

Description

Checklist:

nivida 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

clauBv23 Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clauBv23 commented Aug 21, 2024 •

edited by jira bot

Loading

clauBv23 Sep 13, 2024 •

edited

Loading