From c193c5b40368305031bd7aff505f4cabd0c601ff Mon Sep 17 00:00:00 2001 From: Giorgi Lagidze Date: Fri, 11 Oct 2024 01:33:30 +0400 Subject: [PATCH 1/4] fix contract and test --- packages/contracts/src/Admin.sol | 21 +--------- packages/contracts/src/AdminSetup.sol | 6 +-- .../test/10_unit-testing/11_plugin.ts | 41 ++++++++++++++----- 3 files changed, 35 insertions(+), 33 deletions(-) diff --git a/packages/contracts/src/Admin.sol b/packages/contracts/src/Admin.sol index 5eeb1daf..8c613112 100644 --- a/packages/contracts/src/Admin.sol +++ b/packages/contracts/src/Admin.sol @@ -62,25 +62,6 @@ 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) { @@ -123,7 +104,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(_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/test/10_unit-testing/11_plugin.ts b/packages/contracts/test/10_unit-testing/11_plugin.ts index 774f7b8f..f1dc5b69 100644 --- a/packages/contracts/test/10_unit-testing/11_plugin.ts +++ b/packages/contracts/test/10_unit-testing/11_plugin.ts @@ -31,9 +31,33 @@ 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 {ethers} from 'hardhat'; +let chainId: number; + +async function createProposalId(pluginAddress: string, metadata: string) { + let blockNumber = (await ethers.provider.getBlock('latest')).number; + return BigNumber.from( + ethers.utils.keccak256( + ethers.utils.defaultAbiCoder.encode( + ['uint256', 'uint256', 'address', 'bytes32'], + [ + chainId, + blockNumber + 1, + pluginAddress, + ethers.utils.keccak256(metadata), + ] + ) + ) + ); +} + 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,8 +241,8 @@ describe(PLUGIN_CONTRACT_NAME, function () { DAO_PERMISSIONS.EXECUTE_PERMISSION_ID ); - const currentExpectedProposalId = await plugin.createProposalId( - dummyActions, + const currentExpectedProposalId = await createProposalId( + plugin.address, dummyMetadata ); @@ -263,8 +287,8 @@ describe(PLUGIN_CONTRACT_NAME, function () { DAO_PERMISSIONS.EXECUTE_PERMISSION_ID ); - const currentExpectedProposalId = await plugin.createProposalId( - dummyActions, + const currentExpectedProposalId = await createProposalId( + plugin.address, dummyMetadata ); @@ -299,8 +323,8 @@ describe(PLUGIN_CONTRACT_NAME, function () { const newPlugin = plugin.connect(alice); { - const proposalId = await plugin.createProposalId( - dummyActions, + const proposalId = await createProposalId( + plugin.address, dummyMetadata ); @@ -329,10 +353,7 @@ describe(PLUGIN_CONTRACT_NAME, function () { { const newMetadata = dummyMetadata + '11'; - const proposalId = await plugin.createProposalId( - dummyActions, - newMetadata - ); + const proposalId = await createProposalId(plugin.address, newMetadata); const tx = await newPlugin .connect(alice) From 9896454490e210288ae204b0341f83f8da658b96 Mon Sep 17 00:00:00 2001 From: Giorgi Lagidze Date: Fri, 11 Oct 2024 01:36:36 +0400 Subject: [PATCH 2/4] unused import --- packages/contracts/src/mocks/CustomExecutorMock.sol | 2 +- packages/contracts/test/10_unit-testing/11_plugin.ts | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-) 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 f1dc5b69..71c11df9 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'; @@ -36,8 +35,11 @@ import {ethers} from 'hardhat'; let chainId: number; -async function createProposalId(pluginAddress: string, metadata: string) { - let blockNumber = (await ethers.provider.getBlock('latest')).number; +async function createProposalId( + pluginAddress: string, + metadata: string +): Promise { + const blockNumber = (await ethers.provider.getBlock('latest')).number; return BigNumber.from( ethers.utils.keccak256( ethers.utils.defaultAbiCoder.encode( From c0e69f260745986979f724d26e4acbe6dc0458c2 Mon Sep 17 00:00:00 2001 From: Giorgi Lagidze Date: Fri, 11 Oct 2024 15:19:09 +0400 Subject: [PATCH 3/4] add actions in createproposalid --- packages/contracts/src/Admin.sol | 2 +- .../test/10_unit-testing/11_plugin.ts | 28 +++++++++++++------ 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/packages/contracts/src/Admin.sol b/packages/contracts/src/Admin.sol index 8c613112..f953ae65 100644 --- a/packages/contracts/src/Admin.sol +++ b/packages/contracts/src/Admin.sol @@ -104,7 +104,7 @@ contract Admin is IMembership, PluginCloneable, ProposalUpgradeable { ) public auth(EXECUTE_PROPOSAL_PERMISSION_ID) returns (uint256 proposalId) { uint64 currentTimestamp = block.timestamp.toUint64(); - proposalId = _createProposalId(keccak256(_metadata)); + proposalId = _createProposalId(keccak256(abi.encode(_actions, _metadata))); TargetConfig memory targetConfig = getTargetConfig(); diff --git a/packages/contracts/test/10_unit-testing/11_plugin.ts b/packages/contracts/test/10_unit-testing/11_plugin.ts index 71c11df9..dc52294a 100644 --- a/packages/contracts/test/10_unit-testing/11_plugin.ts +++ b/packages/contracts/test/10_unit-testing/11_plugin.ts @@ -31,25 +31,28 @@ 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( - ethers.utils.keccak256( - ethers.utils.defaultAbiCoder.encode( + keccak256( + defaultAbiCoder.encode( ['uint256', 'uint256', 'address', 'bytes32'], - [ - chainId, - blockNumber + 1, - pluginAddress, - ethers.utils.keccak256(metadata), - ] + [chainId, blockNumber + 1, pluginAddress, salt] ) ) ); @@ -245,6 +248,7 @@ describe(PLUGIN_CONTRACT_NAME, function () { const currentExpectedProposalId = await createProposalId( plugin.address, + dummyActions, dummyMetadata ); @@ -291,6 +295,7 @@ describe(PLUGIN_CONTRACT_NAME, function () { const currentExpectedProposalId = await createProposalId( plugin.address, + dummyActions, dummyMetadata ); @@ -327,6 +332,7 @@ describe(PLUGIN_CONTRACT_NAME, function () { { const proposalId = await createProposalId( plugin.address, + dummyActions, dummyMetadata ); @@ -355,7 +361,11 @@ describe(PLUGIN_CONTRACT_NAME, function () { { const newMetadata = dummyMetadata + '11'; - const proposalId = await createProposalId(plugin.address, newMetadata); + const proposalId = await createProposalId( + plugin.address, + dummyActions, + newMetadata + ); const tx = await newPlugin .connect(alice) From 59ea7277f71057e30c800b13b091444090eeb998 Mon Sep 17 00:00:00 2001 From: Giorgi Lagidze Date: Fri, 11 Oct 2024 16:39:31 +0400 Subject: [PATCH 4/4] rename --- packages/contracts/src/Admin.sol | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/contracts/src/Admin.sol b/packages/contracts/src/Admin.sol index f953ae65..09d9c65e 100644 --- a/packages/contracts/src/Admin.sol +++ b/packages/contracts/src/Admin.sol @@ -63,8 +63,7 @@ contract Admin is IMembership, PluginCloneable, ProposalUpgradeable { } /// @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)"; }