diff --git a/packages/contracts/src/Admin.sol b/packages/contracts/src/Admin.sol index 5eeb1daf..09d9c65e 100644 --- a/packages/contracts/src/Admin.sol +++ b/packages/contracts/src/Admin.sol @@ -62,28 +62,8 @@ contract Admin is IMembership, PluginCloneable, ProposalUpgradeable { }); } - /// @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 - /// 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 - /// 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( - Action[] calldata _actions, - bytes memory _metadata - ) public pure override returns (uint256) { - return uint256(keccak256(abi.encode(_actions, _metadata))); - } - /// @inheritdoc IProposal - /// @dev Admin doesn't allow creating a proposal, so we return empty string. - function createProposalParamsABI() external pure override returns (string memory) { + function customProposalParamsABI() external pure override returns (string memory) { return "(uint256 allowFailureMap)"; } @@ -123,7 +103,7 @@ contract Admin is IMembership, PluginCloneable, ProposalUpgradeable { ) public auth(EXECUTE_PROPOSAL_PERMISSION_ID) returns (uint256 proposalId) { uint64 currentTimestamp = block.timestamp.toUint64(); - proposalId = createProposalId(_actions, _metadata); + proposalId = _createProposalId(keccak256(abi.encode(_actions, _metadata))); TargetConfig memory targetConfig = getTargetConfig(); diff --git a/packages/contracts/src/AdminSetup.sol b/packages/contracts/src/AdminSetup.sol index d5edb808..a6c34fb8 100644 --- a/packages/contracts/src/AdminSetup.sol +++ b/packages/contracts/src/AdminSetup.sol @@ -7,7 +7,7 @@ import {PluginSetup} from "@aragon/osx-commons-contracts/src/plugin/setup/Plugin import {PermissionLib} from "@aragon/osx-commons-contracts/src/permission/PermissionLib.sol"; import {ProxyLib} from "@aragon/osx-commons-contracts/src/utils/deployment/ProxyLib.sol"; import {IDAO} from "@aragon/osx-commons-contracts/src/dao/IDAO.sol"; -import {PluginCloneable} from "@aragon/osx-commons-contracts/src/plugin/PluginCloneable.sol"; +import {IPlugin} from "@aragon/osx-commons-contracts/src/plugin/IPlugin.sol"; import {Admin} from "./Admin.sol"; @@ -39,9 +39,9 @@ contract AdminSetup is PluginSetup { bytes calldata _data ) external returns (address plugin, PreparedSetupData memory preparedSetupData) { // Decode `_data` to extract the params needed for cloning and initializing the `Admin` plugin. - (address admin, PluginCloneable.TargetConfig memory targetConfig) = abi.decode( + (address admin, IPlugin.TargetConfig memory targetConfig) = abi.decode( _data, - (address, PluginCloneable.TargetConfig) + (address, IPlugin.TargetConfig) ); if (admin == address(0)) { diff --git a/packages/contracts/src/mocks/CustomExecutorMock.sol b/packages/contracts/src/mocks/CustomExecutorMock.sol index e59fa539..31986842 100644 --- a/packages/contracts/src/mocks/CustomExecutorMock.sol +++ b/packages/contracts/src/mocks/CustomExecutorMock.sol @@ -2,7 +2,7 @@ pragma solidity ^0.8.8; -import {IExecutor, Action} from "@aragon/osx-commons-contracts/src/executors/IExecutor.sol"; +import {Action} from "@aragon/osx-commons-contracts/src/executors/IExecutor.sol"; /// @dev DO NOT USE IN PRODUCTION! contract CustomExecutorMock { diff --git a/packages/contracts/test/10_unit-testing/11_plugin.ts b/packages/contracts/test/10_unit-testing/11_plugin.ts index 774f7b8f..dc52294a 100644 --- a/packages/contracts/test/10_unit-testing/11_plugin.ts +++ b/packages/contracts/test/10_unit-testing/11_plugin.ts @@ -23,7 +23,6 @@ import { import { findEvent, findEventTopicLog, - proposalIdToBytes32, getInterfaceId, DAO_PERMISSIONS, } from '@aragon/osx-commons-sdk'; @@ -31,9 +30,39 @@ import {DAO, DAOEvents, DAOStructs} from '@aragon/osx-ethers'; import {loadFixture} from '@nomicfoundation/hardhat-network-helpers'; import {SignerWithAddress} from '@nomiclabs/hardhat-ethers/signers'; import {expect} from 'chai'; +import {BigNumber} from 'ethers'; +import {defaultAbiCoder, keccak256} from 'ethers/lib/utils'; import {ethers} from 'hardhat'; +let chainId: number; + +async function createProposalId( + pluginAddress: string, + actions: DAOStructs.ActionStruct[], + metadata: string +): Promise { + const blockNumber = (await ethers.provider.getBlock('latest')).number; + const salt = keccak256( + defaultAbiCoder.encode( + ['tuple(address to,uint256 value,bytes data)[]', 'bytes'], + [actions, metadata] + ) + ); + return BigNumber.from( + keccak256( + defaultAbiCoder.encode( + ['uint256', 'uint256', 'address', 'bytes32'], + [chainId, blockNumber + 1, pluginAddress, salt] + ) + ) + ); +} + describe(PLUGIN_CONTRACT_NAME, function () { + before(async () => { + chainId = (await ethers.provider.getNetwork()).chainId; + }); + describe('initialize', async () => { it('reverts if trying to re-initialize', async () => { const { @@ -217,7 +246,8 @@ describe(PLUGIN_CONTRACT_NAME, function () { DAO_PERMISSIONS.EXECUTE_PERMISSION_ID ); - const currentExpectedProposalId = await plugin.createProposalId( + const currentExpectedProposalId = await createProposalId( + plugin.address, dummyActions, dummyMetadata ); @@ -263,7 +293,8 @@ describe(PLUGIN_CONTRACT_NAME, function () { DAO_PERMISSIONS.EXECUTE_PERMISSION_ID ); - const currentExpectedProposalId = await plugin.createProposalId( + const currentExpectedProposalId = await createProposalId( + plugin.address, dummyActions, dummyMetadata ); @@ -299,7 +330,8 @@ describe(PLUGIN_CONTRACT_NAME, function () { const newPlugin = plugin.connect(alice); { - const proposalId = await plugin.createProposalId( + const proposalId = await createProposalId( + plugin.address, dummyActions, dummyMetadata ); @@ -329,7 +361,8 @@ describe(PLUGIN_CONTRACT_NAME, function () { { const newMetadata = dummyMetadata + '11'; - const proposalId = await plugin.createProposalId( + const proposalId = await createProposalId( + plugin.address, dummyActions, newMetadata );