Merge pull request #19 from aragon/feature/fix-create-proposal-id

fix contract and test

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();
 

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)) {

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 {

packages/contracts/test/10_unit-testing/11_plugin.ts

@@ -23,17 +23,46 @@ import {
 import {
   findEvent,
   findEventTopicLog,
-  proposalIdToBytes32,
   getInterfaceId,
   DAO_PERMISSIONS,
 } from '@aragon/osx-commons-sdk';
 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<BigNumber> {
+  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
         );