Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: new multisig version #19

Merged
merged 67 commits into from
Nov 5, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
67 commits
Select commit Hold shift + click to select a range
ccb7dee
fix multisig/multisigsetup tests for new implementation
novaknole Aug 20, 2024
2ca8113
fix more tests
novaknole Aug 21, 2024
516a532
feat: add unit test for initialize from function
clauBv23 Aug 21, 2024
c710d76
ci: remove comment
clauBv23 Aug 21, 2024
b2996b6
feat: add check to ensure the plugin is being reinitialized when upgr…
clauBv23 Aug 21, 2024
8bfe5d0
fix test
clauBv23 Aug 21, 2024
b80d626
ci: remove comment
clauBv23 Aug 21, 2024
1b24da1
feat: add reinitialization when upgrading plugin and check it on the …
clauBv23 Aug 21, 2024
2b679ed
ci: remove comment
clauBv23 Aug 21, 2024
a50a5c8
ci: add comment
clauBv23 Aug 21, 2024
d4e80c0
ci: update comment
clauBv23 Aug 21, 2024
3300d84
fix lint
clauBv23 Aug 21, 2024
4ef810f
proposal hash return value
novaknole Aug 27, 2024
43ef91e
add create proposal id
novaknole Aug 27, 2024
a8c7376
remove .only
novaknole Aug 27, 2024
ae7046c
Merge branch 'feature/multisig-improvements' into feat/add-tests
clauBv23 Aug 30, 2024
6a2c080
initializeFrom and erc165 changes for upgrades
novaknole Sep 10, 2024
e3cf955
Merge branch 'feature/multisig-improvements' into feat/add-tests
clauBv23 Sep 13, 2024
31ebac1
ci: define latest version as constant
clauBv23 Sep 13, 2024
f7ec5fc
feat: fix test for initialize from function
clauBv23 Sep 13, 2024
6f84d88
fix: upgradeability tests
clauBv23 Sep 13, 2024
065802a
modifier added for safety and target config improved
novaknole Sep 19, 2024
6b6fa6f
add comment
novaknole Sep 19, 2024
c87ff68
bytes param added
novaknole Sep 19, 2024
11364d5
rename param function:
novaknole Sep 19, 2024
33910d9
Merge branch 'feature/multisig-improvements' into feat/add-tests
novaknole Sep 23, 2024
a10d40b
Merge pull request #18 from aragon/feat/add-tests
novaknole Sep 23, 2024
9164a7c
Update packages/contracts/src/Multisig.sol
novaknole Sep 23, 2024
28d65f7
Update packages/contracts/src/Multisig.sol
novaknole Sep 25, 2024
c262bad
Update packages/contracts/src/Multisig.sol
novaknole Sep 25, 2024
27c09ee
add test for delegatecall
novaknole Sep 26, 2024
0eaa19d
silence warnings
novaknole Sep 26, 2024
451c556
executor interface
novaknole Sep 26, 2024
7e86090
action as free level
novaknole Sep 27, 2024
abf3435
Merge pull request #23 from aragon/feature/executors
novaknole Sep 27, 2024
bc751dd
Merge pull request #21 from aragon/feature/bytes-param-add
novaknole Sep 27, 2024
dfe1336
remove unnecessary revoke
novaknole Sep 27, 2024
b1fdf94
better upgrade tests
novaknole Sep 28, 2024
4d68a56
callinitialization from osx-commons
novaknole Sep 28, 2024
056dbfa
fix tohex
novaknole Sep 29, 2024
71de210
add metadata support
novaknole Oct 7, 2024
4a00b14
metadata extension
novaknole Oct 8, 2024
832eafd
Update packages/contracts/src/MultisigSetup.sol
novaknole Oct 8, 2024
d095f55
Update packages/contracts/src/Multisig.sol
novaknole Oct 8, 2024
032ea3c
Update packages/contracts/src/Multisig.sol
novaknole Oct 8, 2024
8e66bfa
Update packages/contracts/src/Multisig.sol
novaknole Oct 8, 2024
fc1381d
permission added
novaknole Oct 8, 2024
732bf42
rename
novaknole Oct 8, 2024
eded00b
Merge pull request #24 from aragon/feature/metadata
novaknole Oct 8, 2024
3cea0c5
metadata rename
novaknole Oct 9, 2024
2141a95
fix contracts and test (#25)
novaknole Oct 14, 2024
da23965
feat: add auth to execute() and update Setup & testing
Rekard0 Oct 31, 2024
343b352
add test for recording vote with tryExecution when caller don't have …
Rekard0 Oct 31, 2024
7b948f2
comment
novaknole Nov 1, 2024
005aacb
remove .only
novaknole Nov 1, 2024
c674484
add hassucceeded
novaknole Nov 4, 2024
c8a4f3c
comments in tests
novaknole Nov 4, 2024
682b074
reverts
novaknole Nov 4, 2024
76ee964
remove .only
novaknole Nov 4, 2024
8d3268a
Merge pull request #26 from aragon/feature/hasSucceeded
novaknole Nov 4, 2024
a51622d
docgen
novaknole Nov 5, 2024
5b5fe64
new line comment
novaknole Nov 5, 2024
752fff1
original comment
novaknole Nov 5, 2024
ceee8fd
multi line comment
novaknole Nov 5, 2024
42948ff
comments
novaknole Nov 5, 2024
9115d42
add docgen
novaknole Nov 5, 2024
2818638
reorder comment
novaknole Nov 5, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/contracts/hardhat.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ const config: HardhatUserConfig = {
solidity: {
version: '0.8.17',
settings: {
viaIR: true,
novaknole marked this conversation as resolved.
Show resolved Hide resolved
metadata: {
// Not including the metadata hash
// https://github.com/paulrberg/hardhat-template/issues/31
Expand Down
32 changes: 32 additions & 0 deletions packages/contracts/src/ListedCheckCondition.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// SPDX-License-Identifier: AGPL-3.0-or-later

pragma solidity ^0.8.8;

import {Multisig} from "./Multisig.sol";

import {PermissionCondition} from "@aragon/osx-commons-contracts/src/permission/condition/PermissionCondition.sol";

contract ListedCheckCondition is PermissionCondition {
Multisig private immutable multisig;

Check warning on line 10 in packages/contracts/src/ListedCheckCondition.sol

View workflow job for this annotation

GitHub Actions / checks

Immutable variables name are set to be in capitalized SNAKE_CASE

Check warning on line 10 in packages/contracts/src/ListedCheckCondition.sol

View workflow job for this annotation

GitHub Actions / formatting-linting / checks

Immutable variables name are set to be in capitalized SNAKE_CASE

Check warning on line 10 in packages/contracts/src/ListedCheckCondition.sol

View workflow job for this annotation

GitHub Actions / formatting-linting / checks

Immutable variables name are set to be in capitalized SNAKE_CASE

constructor(address _multisig) {
multisig = Multisig(_multisig);
}

function isGranted(
address _where,
address _who,
bytes32 _permissionId,
bytes calldata _data
) public view override returns (bool) {
(_where, _data, _permissionId);

(bool onlyListed, ) = multisig.multisigSettings();

if (onlyListed && !multisig.isListed(_who)) {
return false;
}

return true;
}
}
121 changes: 98 additions & 23 deletions packages/contracts/src/Multisig.sol
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import {Addresslist} from "@aragon/osx-commons-contracts/src/plugin/extensions/governance/Addresslist.sol";
import {ProposalUpgradeable} from "@aragon/osx-commons-contracts/src/plugin/extensions/proposal/ProposalUpgradeable.sol";
import {PluginUUPSUpgradeable} from "@aragon/osx-commons-contracts/src/plugin/PluginUUPSUpgradeable.sol";
import {IProposal} from "@aragon/osx-commons-contracts/src/plugin/extensions/proposal/IProposal.sol";
import {IDAO} from "@aragon/osx-commons-contracts/src/dao/IDAO.sol";

import {IMultisig} from "./IMultisig.sol";
Expand Down Expand Up @@ -45,6 +46,8 @@
mapping(address => bool) approvers;
IDAO.Action[] actions;
uint256 allowFailureMap;
address target; // added in v1.3.0
Operation operation; // added in v1.3.0
novaknole marked this conversation as resolved.
Show resolved Hide resolved
}

/// @notice A container for the proposal parameters.
Expand All @@ -69,15 +72,21 @@

/// @notice The [ERC-165](https://eips.ethereum.org/EIPS/eip-165) interface ID of the contract.
bytes4 internal constant MULTISIG_INTERFACE_ID =
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this the old interface, I would make it clear by the name.
The new interface is checked via the interface ID

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not the old one, but in the new code, the IProposal createProposal function was added.

Note that this createProposal function is not needed on the current interface id because it belong to IProposal

this.initialize.selector ^
this.updateMultisigSettings.selector ^
this.createProposal.selector ^
this.updateMultisigSettings.selector ^
bytes4(
keccak256(
"createProposal(bytes,(address,uint256,bytes)[],uint256,bool,bool,uint64,uint64)"
)
) ^
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.
novaknole marked this conversation as resolved.
Show resolved Hide resolved
bytes32 public constant CREATE_PROPOSAL_PERMISSION_ID = keccak256("CREATE_PROPOSAL_PERMISSION");

/// @notice A mapping between proposal IDs and proposal information.
// solhint-disable-next-line named-parameters-mapping
mapping(uint256 => Proposal) internal proposals;
Expand Down Expand Up @@ -117,6 +126,10 @@
/// @param actual The actual value.
error AddresslistLengthOutOfBounds(uint16 limit, uint256 actual);

/// @notice Thrown if the proposal with same actions and metadata already exists.
/// @param proposalId The id of the proposal.
error ProposalAlreadyExists(uint256 proposalId);

/// @notice Thrown if a date is out of bounds.
/// @param limit The limit value.
/// @param actual The actual value.
Expand All @@ -140,8 +153,9 @@
function initialize(
IDAO _dao,
address[] calldata _members,
MultisigSettings calldata _multisigSettings
) external initializer {
MultisigSettings calldata _multisigSettings,
TargetConfig calldata _targetConfig
) external reinitializer(2) {
__PluginUUPSUpgradeable_init(_dao);

if (_members.length > type(uint16).max) {
Expand All @@ -152,6 +166,18 @@
emit MembersAdded({members: _members});

_updateMultisigSettings(_multisigSettings);

_setTargetConfig(_targetConfig);
}

/// @notice Reinitializes the TokenVoting after an upgrade from a previous protocol version.
/// @param _fromBuild The build version number of the previous implementation contract this upgrade is transitioning from.

Check failure on line 174 in packages/contracts/src/Multisig.sol

View workflow job for this annotation

GitHub Actions / checks

Line length must be no more than 120 but current length is 126

Check failure on line 174 in packages/contracts/src/Multisig.sol

View workflow job for this annotation

GitHub Actions / formatting-linting / checks

Line length must be no more than 120 but current length is 126

Check failure on line 174 in packages/contracts/src/Multisig.sol

View workflow job for this annotation

GitHub Actions / formatting-linting / checks

Line length must be no more than 120 but current length is 126
/// @param _initData The initialization data to be passed to via `upgradeToAndCall` (see [ERC-1967](https://docs.openzeppelin.com/contracts/4.x/api/proxy#ERC1967Upgrade)).

Check failure on line 175 in packages/contracts/src/Multisig.sol

View workflow job for this annotation

GitHub Actions / checks

Line length must be no more than 120 but current length is 175

Check failure on line 175 in packages/contracts/src/Multisig.sol

View workflow job for this annotation

GitHub Actions / formatting-linting / checks

Line length must be no more than 120 but current length is 175

Check failure on line 175 in packages/contracts/src/Multisig.sol

View workflow job for this annotation

GitHub Actions / formatting-linting / checks

Line length must be no more than 120 but current length is 175
function initializeFrom(uint16 _fromBuild, bytes calldata _initData) external reinitializer(2) {
if (_fromBuild < 3) {
clauBv23 marked this conversation as resolved.
Show resolved Hide resolved
TargetConfig memory targetConfig = abi.decode(_initData, (TargetConfig));
_setTargetConfig(targetConfig);
}
clauBv23 marked this conversation as resolved.
Show resolved Hide resolved
}

/// @notice Checks if this or the parent contract supports an interface by its ID.
Expand Down Expand Up @@ -236,11 +262,7 @@
bool _tryExecution,
uint64 _startDate,
uint64 _endDate
) external returns (uint256 proposalId) {
if (multisigSettings.onlyListed && !isListed(_msgSender())) {
revert ProposalCreationForbidden(_msgSender());
}

) public auth(CREATE_PROPOSAL_PERMISSION_ID) returns (uint256 proposalId) {
uint64 snapshotBlock;
unchecked {
// The snapshot block must be mined already to protect the transaction against backrunning transactions
Expand All @@ -264,23 +286,32 @@
revert DateOutOfBounds({limit: _startDate, actual: _endDate});
}

proposalId = _createProposal({
_creator: _msgSender(),
_metadata: _metadata,
_startDate: _startDate,
_endDate: _endDate,
_actions: _actions,
_allowFailureMap: _allowFailureMap
});
proposalId = createProposalId(_actions, _metadata);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if I need to submit a recurring proposal each week with identical actions and description?

  • If I wanted to automate the resubmission from a contract of my own, I wouldn't be able to
  • I would be forced to append garbage bytes to the metadata URI and corrupt the UI trying to fetch it

Recommendation:

  • Add the startDate as a salt parameter
  • Consider having random and unique proposal ID's unless there's a major reason for strong determinism

Copy link
Author

@clauBv23 clauBv23 Oct 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


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

// Multisig doesn't allow `minApprovals` settings to be less than 0.
novaknole marked this conversation as resolved.
Show resolved Hide resolved
// If it is, that means proposal hasn't been created yet.
novaknole marked this conversation as resolved.
Show resolved Hide resolved
if (proposal_.parameters.minApprovals != 0) {
revert ProposalAlreadyExists(proposalId);
}

proposal_.parameters.snapshotBlock = snapshotBlock;
proposal_.parameters.startDate = _startDate;
proposal_.parameters.endDate = _endDate;
proposal_.parameters.minApprovals = multisigSettings.minApprovals;

TargetConfig memory currentTarget = getTargetConfig();

if (currentTarget.target == address(0)) {
proposal_.target = address(dao());
proposal_.operation = Operation.Call;
novaknole marked this conversation as resolved.
Show resolved Hide resolved
} else {
proposal_.target = currentTarget.target;
proposal_.operation = currentTarget.operation;
}

// Reduce costs
if (_allowFailureMap != 0) {
proposal_.allowFailureMap = _allowFailureMap;
Expand All @@ -296,6 +327,26 @@
if (_approveProposal) {
approve(proposalId, _tryExecution);
}

emit ProposalCreated(
proposalId,
_msgSender(),
_startDate,
_endDate,
_metadata,
_actions,
_allowFailureMap
);
}

function createProposal(
bytes calldata _metadata,
IDAO.Action[] calldata _actions,
uint64 _startDate,
uint64 _endDate
) external override returns (uint256 proposalId) {
// Calls public function for permission check.
proposalId = createProposal(_metadata, _actions, 0, false, false, _startDate, _endDate);
}

/// @inheritdoc IMultisig
Expand Down Expand Up @@ -328,7 +379,9 @@
}

/// @inheritdoc IMultisig
function canExecute(uint256 _proposalId) external view returns (bool) {
function canExecute(
uint256 _proposalId
) external view override(IMultisig, IProposal) returns (bool) {
return _canExecute(_proposalId);
}

Expand Down Expand Up @@ -382,19 +435,41 @@
return isListed(_account);
}

/// @notice Hashing function used to (re)build the proposal id from the proposal details..
/// @dev The proposal id is produced by hashing the ABI encoded `targets` array, the `values` array, the `calldatas` array

Check failure on line 439 in packages/contracts/src/Multisig.sol

View workflow job for this annotation

GitHub Actions / checks

Line length must be no more than 120 but current length is 126

Check failure on line 439 in packages/contracts/src/Multisig.sol

View workflow job for this annotation

GitHub Actions / formatting-linting / checks

Line length must be no more than 120 but current length is 126

Check failure on line 439 in packages/contracts/src/Multisig.sol

View workflow job for this annotation

GitHub Actions / formatting-linting / checks

Line length must be no more than 120 but current length is 126
/// and the descriptionHash (bytes32 which itself is the keccak256 hash of the description string). This proposal id
/// can be produced from the proposal data which is part of the {ProposalCreated} event. It can even be computed in
/// advance, before the proposal is submitted.
/// The chainId and the governor address are not part of the proposal id computation. Consequently, the
/// same proposal (with same operation and same description) will have the same id if submitted on multiple governors

Check failure on line 444 in packages/contracts/src/Multisig.sol

View workflow job for this annotation

GitHub Actions / checks

Line length must be no more than 120 but current length is 121

Check failure on line 444 in packages/contracts/src/Multisig.sol

View workflow job for this annotation

GitHub Actions / formatting-linting / checks

Line length must be no more than 120 but current length is 121

Check failure on line 444 in packages/contracts/src/Multisig.sol

View workflow job for this annotation

GitHub Actions / formatting-linting / checks

Line length must be no more than 120 but current length is 121
/// across multiple networks. This also means that in order to execute the same operation twice (on the same
/// governor) the proposer will have to change the description in order to avoid proposal id conflicts.
/// @param _actions The actions that will be executed after the proposal passes.
/// @param _metadata The metadata of the proposal.
/// @return proposalId The ID of the proposal.
function createProposalId(
IDAO.Action[] calldata _actions,
bytes memory _metadata
) public pure override returns (uint256) {
return uint256(keccak256(abi.encode(_actions, _metadata)));
}

/// @notice Internal function to execute a vote. It assumes the queried proposal exists.
/// @param _proposalId The ID of the proposal.
function _execute(uint256 _proposalId) internal {
Proposal storage proposal_ = proposals[_proposalId];

proposal_.executed = true;

_executeProposal(
dao(),
_proposalId,
_execute(
proposal_.target,
bytes32(_proposalId),
novaknole marked this conversation as resolved.
Show resolved Hide resolved
proposals[_proposalId].actions,
proposals[_proposalId].allowFailureMap
proposals[_proposalId].allowFailureMap,
proposal_.operation
);

emit ProposalExecuted(_proposalId);
}

/// @notice Internal function to check if an account can approve. It assumes the queried proposal exists.
Expand Down
Loading
Loading