From 0754632692c4fa984e0482ccb5836743d58c9aaf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jose=20Manuel=20Mari=C3=B1as=20Bascoy?= Date: Tue, 30 Apr 2024 10:20:09 +0200 Subject: [PATCH 01/13] chore: move VersionCreated and ReleaseMetadataUpdated events to IPluginRepo --- .../src/framework/plugin/repo/IPluginRepo.sol | 17 +++++++++++++++++ .../src/framework/plugin/repo/PluginRepo.sol | 17 ----------------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/packages/contracts/src/framework/plugin/repo/IPluginRepo.sol b/packages/contracts/src/framework/plugin/repo/IPluginRepo.sol index 6536be3fa..1706dcdd0 100644 --- a/packages/contracts/src/framework/plugin/repo/IPluginRepo.sol +++ b/packages/contracts/src/framework/plugin/repo/IPluginRepo.sol @@ -23,4 +23,21 @@ interface IPluginRepo { bytes calldata _buildMetadata, bytes calldata _releaseMetadata ) external; + + /// @notice Thrown if the same plugin setup exists in previous releases. + /// @param release The release number. + /// @param build The build number. + /// @param pluginSetup The address of the plugin setup contract. + /// @param buildMetadata The build metadata URI. + event VersionCreated( + uint8 release, + uint16 build, + address indexed pluginSetup, + bytes buildMetadata + ); + + /// @notice Thrown when a release's metadata was updated. + /// @param release The release number. + /// @param releaseMetadata The release metadata URI. + event ReleaseMetadataUpdated(uint8 release, bytes releaseMetadata); } diff --git a/packages/contracts/src/framework/plugin/repo/PluginRepo.sol b/packages/contracts/src/framework/plugin/repo/PluginRepo.sol index 73dca68d4..4f09bd642 100644 --- a/packages/contracts/src/framework/plugin/repo/PluginRepo.sol +++ b/packages/contracts/src/framework/plugin/repo/PluginRepo.sol @@ -96,23 +96,6 @@ contract PluginRepo is /// @notice Thrown if release does not exist. error ReleaseDoesNotExist(); - /// @notice Thrown if the same plugin setup exists in previous releases. - /// @param release The release number. - /// @param build The build number. - /// @param pluginSetup The address of the plugin setup contract. - /// @param buildMetadata The build metadata URI. - event VersionCreated( - uint8 release, - uint16 build, - address indexed pluginSetup, - bytes buildMetadata - ); - - /// @notice Thrown when a release's metadata was updated. - /// @param release The release number. - /// @param releaseMetadata The release metadata URI. - event ReleaseMetadataUpdated(uint8 release, bytes releaseMetadata); - /// @dev Used to disallow initializing the implementation contract by an attacker for extra safety. constructor() { _disableInitializers(); From fce603a83e844597766f7b71fe0a8098dad7b429 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jose=20Manuel=20Mari=C3=B1as=20Bascoy?= Date: Tue, 30 Apr 2024 10:20:47 +0200 Subject: [PATCH 02/13] chore: add storage gap --- packages/contracts/src/framework/plugin/repo/PluginRepo.sol | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/contracts/src/framework/plugin/repo/PluginRepo.sol b/packages/contracts/src/framework/plugin/repo/PluginRepo.sol index 4f09bd642..6e4407a79 100644 --- a/packages/contracts/src/framework/plugin/repo/PluginRepo.sol +++ b/packages/contracts/src/framework/plugin/repo/PluginRepo.sol @@ -255,4 +255,7 @@ contract PluginRepo is _interfaceId == type(IProtocolVersion).interfaceId || super.supportsInterface(_interfaceId); } + + /// @notice This empty reserved space is put in place to allow future versions to add new variables without shifting down storage in the inheritance chain (see [OpenZeppelin's guide about storage gaps](https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps)). + uint256[46] private __gap; } From 7272b9b3ec020a3c7f37d1450cb31c2b5564bccc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jose=20Manuel=20Mari=C3=B1as=20Bascoy?= Date: Tue, 30 Apr 2024 10:24:04 +0200 Subject: [PATCH 03/13] chore: add 'initializeFrom' function and 'reinitializer' modifyers --- .../src/framework/plugin/repo/PluginRepo.sol | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/packages/contracts/src/framework/plugin/repo/PluginRepo.sol b/packages/contracts/src/framework/plugin/repo/PluginRepo.sol index 6e4407a79..b63446f84 100644 --- a/packages/contracts/src/framework/plugin/repo/PluginRepo.sol +++ b/packages/contracts/src/framework/plugin/repo/PluginRepo.sol @@ -95,6 +95,8 @@ contract PluginRepo is /// @notice Thrown if release does not exist. error ReleaseDoesNotExist(); + /// @notice Thrown if an upgrade is not supported from a specific protocol version . + error ProtocolVersionUpgradeNotSupported(uint8[3] protocolVersion); /// @dev Used to disallow initializing the implementation contract by an attacker for extra safety. constructor() { @@ -105,13 +107,28 @@ contract PluginRepo is /// - initializing the permission manager /// - granting the `MAINTAINER_PERMISSION_ID` permission to the initial owner. /// @dev This method is required to support [ERC-1822](https://eips.ethereum.org/EIPS/eip-1822). - function initialize(address initialOwner) external initializer { + function initialize(address initialOwner) external reinitializer(2) { __PermissionManager_init(initialOwner); _grant(address(this), initialOwner, MAINTAINER_PERMISSION_ID); _grant(address(this), initialOwner, UPGRADE_REPO_PERMISSION_ID); } + /// @notice Initializes the pluginRepo after an upgrade from a previous protocol version. + /// @param _previousProtocolVersion The semantic protocol version number of the previous DAO 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( + uint8[3] calldata _previousProtocolVersion, + bytes calldata _initData + ) external reinitializer(2) { + _initData; // Silences the unused function parameter warning. + + // Check that the contract is not upgrading from a different major release. + if (_previousProtocolVersion[0] != 1) { + revert ProtocolVersionUpgradeNotSupported(_previousProtocolVersion); + } + } + /// @inheritdoc IPluginRepo function createVersion( uint8 _release, From 7bc8b2d98e931f9cd7521ab212e3ce43e299b8b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jose=20Manuel=20Mari=C3=B1as=20Bascoy?= Date: Tue, 30 Apr 2024 12:33:13 +0200 Subject: [PATCH 04/13] chore: update test name --- packages/contracts/test/framework/plugin/plugin-repo.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/contracts/test/framework/plugin/plugin-repo.ts b/packages/contracts/test/framework/plugin/plugin-repo.ts index 161748590..22a3da781 100644 --- a/packages/contracts/test/framework/plugin/plugin-repo.ts +++ b/packages/contracts/test/framework/plugin/plugin-repo.ts @@ -515,7 +515,7 @@ describe('PluginRepo', function () { ).to.be.revertedWithCustomError(pluginRepo, 'EmptyReleaseMetadata'); }); - it('successfuly updates metadata for the release that already exists', async () => { + it('successfuly updates metadata for the release that already exists and emits the "ReleaseMetadataUpdated" event', async () => { await pluginRepo.createVersion( 1, pluginSetupMock.address, From ce6b46c542cf8f8183c35dec3fe1bc9d8e89a0e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jose=20Manuel=20Mari=C3=B1as=20Bascoy?= Date: Tue, 30 Apr 2024 12:35:01 +0200 Subject: [PATCH 05/13] fix: typo --- packages/contracts/test/framework/plugin/plugin-repo.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/contracts/test/framework/plugin/plugin-repo.ts b/packages/contracts/test/framework/plugin/plugin-repo.ts index 22a3da781..19a1e7566 100644 --- a/packages/contracts/test/framework/plugin/plugin-repo.ts +++ b/packages/contracts/test/framework/plugin/plugin-repo.ts @@ -515,7 +515,7 @@ describe('PluginRepo', function () { ).to.be.revertedWithCustomError(pluginRepo, 'EmptyReleaseMetadata'); }); - it('successfuly updates metadata for the release that already exists and emits the "ReleaseMetadataUpdated" event', async () => { + it('successfully updates metadata for the release that already exists and emits the "ReleaseMetadataUpdated" event', async () => { await pluginRepo.createVersion( 1, pluginSetupMock.address, From 8f72e5bb46f10b7ab20413063a55bad01f44dfa6 Mon Sep 17 00:00:00 2001 From: josemarinas <36479864+josemarinas@users.noreply.github.com> Date: Wed, 1 May 2024 00:56:24 +0200 Subject: [PATCH 06/13] Apply suggestions from code review Co-authored-by: Michael Heuer <20623991+Michael-A-Heuer@users.noreply.github.com> --- .../src/framework/plugin/repo/PluginRepo.sol | 15 ++++++++------- .../test/framework/plugin/plugin-repo.ts | 2 +- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/packages/contracts/src/framework/plugin/repo/PluginRepo.sol b/packages/contracts/src/framework/plugin/repo/PluginRepo.sol index b63446f84..735993f38 100644 --- a/packages/contracts/src/framework/plugin/repo/PluginRepo.sol +++ b/packages/contracts/src/framework/plugin/repo/PluginRepo.sol @@ -107,7 +107,7 @@ contract PluginRepo is /// - initializing the permission manager /// - granting the `MAINTAINER_PERMISSION_ID` permission to the initial owner. /// @dev This method is required to support [ERC-1822](https://eips.ethereum.org/EIPS/eip-1822). - function initialize(address initialOwner) external reinitializer(2) { + function initialize(address initialOwner) external initializer { __PermissionManager_init(initialOwner); _grant(address(this), initialOwner, MAINTAINER_PERMISSION_ID); @@ -117,16 +117,17 @@ contract PluginRepo is /// @notice Initializes the pluginRepo after an upgrade from a previous protocol version. /// @param _previousProtocolVersion The semantic protocol version number of the previous DAO 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)). + /// @dev This function is a placeholder until we require reinitialization. function initializeFrom( uint8[3] calldata _previousProtocolVersion, bytes calldata _initData ) external reinitializer(2) { - _initData; // Silences the unused function parameter warning. - - // Check that the contract is not upgrading from a different major release. - if (_previousProtocolVersion[0] != 1) { - revert ProtocolVersionUpgradeNotSupported(_previousProtocolVersion); - } + // Silences the unused function parameter warning. + _previousProtocolVersion; + _initData; + + // Revert because this is a placeholder until this contract requires reinitialization. + revert(); } /// @inheritdoc IPluginRepo diff --git a/packages/contracts/test/framework/plugin/plugin-repo.ts b/packages/contracts/test/framework/plugin/plugin-repo.ts index 19a1e7566..6df5df3b0 100644 --- a/packages/contracts/test/framework/plugin/plugin-repo.ts +++ b/packages/contracts/test/framework/plugin/plugin-repo.ts @@ -515,7 +515,7 @@ describe('PluginRepo', function () { ).to.be.revertedWithCustomError(pluginRepo, 'EmptyReleaseMetadata'); }); - it('successfully updates metadata for the release that already exists and emits the "ReleaseMetadataUpdated" event', async () => { + it('updates metadata for the release that already exists and emits the "ReleaseMetadataUpdated" event', async () => { await pluginRepo.createVersion( 1, pluginSetupMock.address, From 1393796d513fdfd8e808a16d0e3ca2afb424db66 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jose=20Manuel=20Mari=C3=B1as=20Bascoy?= Date: Thu, 2 May 2024 12:28:49 +0200 Subject: [PATCH 07/13] chore: add initializeFrom test --- .../test/framework/plugin/plugin-repo.ts | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/packages/contracts/test/framework/plugin/plugin-repo.ts b/packages/contracts/test/framework/plugin/plugin-repo.ts index 6df5df3b0..1fbd13754 100644 --- a/packages/contracts/test/framework/plugin/plugin-repo.ts +++ b/packages/contracts/test/framework/plugin/plugin-repo.ts @@ -11,6 +11,7 @@ import { } from '../../../typechain'; import {PluginRepo__factory as PluginRepo_V1_0_0__factory} from '../../../typechain/@aragon/osx-v1.0.1/framework/plugin/repo/PluginRepo.sol'; import {PluginRepo__factory as PluginRepo_V1_3_0__factory} from '../../../typechain/@aragon/osx-v1.3.0/framework/plugin/repo/PluginRepo.sol'; +import {OZ_INITIALIZED_SLOT_POSITION} from '../../../utils/storage'; import {ZERO_BYTES32} from '../../test-utils/dao'; import {osxContractsVersion} from '../../test-utils/protocol-version'; import {tagHash} from '../../test-utils/psp/hash-helpers'; @@ -152,6 +153,23 @@ describe('PluginRepo', function () { expect(toProtocolVersion).to.deep.equal(osxContractsVersion()); }); }); + describe('InitializeFrom', () => { + it('increments `_initialized` to `1`', async () => { + // Expect the contract to be initialized with `_initialized = 1`. + expect( + ethers.BigNumber.from( + await ethers.provider.getStorageAt( + pluginRepo.address, + OZ_INITIALIZED_SLOT_POSITION + ) + ).toNumber() + ).to.equal(1); + + // Call `initializeFrom` with version 1.3.0. and revert + await expect(pluginRepo.initializeFrom([1, 3, 0], emptyBytes)).to.be + .reverted; + }); + }); describe('ERC-165', async () => { it('does not support the empty interface', async () => { From d8e2ae577c0541ad11655f4b422b4baa6e3031de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jose=20Manuel=20Mari=C3=B1as=20Bascoy?= Date: Thu, 2 May 2024 12:29:30 +0200 Subject: [PATCH 08/13] style: fix prettier --- packages/contracts/src/framework/plugin/repo/PluginRepo.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/contracts/src/framework/plugin/repo/PluginRepo.sol b/packages/contracts/src/framework/plugin/repo/PluginRepo.sol index 735993f38..1542f462e 100644 --- a/packages/contracts/src/framework/plugin/repo/PluginRepo.sol +++ b/packages/contracts/src/framework/plugin/repo/PluginRepo.sol @@ -125,7 +125,7 @@ contract PluginRepo is // Silences the unused function parameter warning. _previousProtocolVersion; _initData; - + // Revert because this is a placeholder until this contract requires reinitialization. revert(); } From 454860932c70e9afb44bb5d626d429c8ddc9cb91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jose=20Manuel=20Mari=C3=B1as=20Bascoy?= Date: Thu, 2 May 2024 12:40:53 +0200 Subject: [PATCH 09/13] chore: update changelog --- packages/contracts/CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/contracts/CHANGELOG.md b/packages/contracts/CHANGELOG.md index e6faa1a4b..5e2630ca4 100644 --- a/packages/contracts/CHANGELOG.md +++ b/packages/contracts/CHANGELOG.md @@ -12,6 +12,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Added `VersionComparisonLib` to compare semantic versioning numbers. - Inherit `ProtocolVersion` in `Plugin`, `PluginCloneable`, `PluginUUPSUpgradeable`, `PluginSetup`, `PermissionCondition`, `PermissionConditionUpgradeable` `PluginSetupProcessor`, `PluginRepoRegistry`, `DAORegistry`, and `ENSSubdomainRegistrar`. - Added the `FunctionDeprecated` error to `DAO`. +- Added a storage gap to `PluginRepo`. +- Added `initializeFrom` placeholder function to `PluginRepo`. ### Changed @@ -23,6 +25,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Use the DAOs permission manager functionality to validate signatures. - Renamed `managingDAO` during deployment to `managmentDAO`. - Aligned contract names during deployment with the names given in `@aragon/osx-commons-configs`. +- Move events `ReleaseMetadataUpdated`and `VersionCreated` from `PluginRepo` to `IPluginRepo`. ### Removed From 734cca92c167b15495190fbe2fed23a46c574f01 Mon Sep 17 00:00:00 2001 From: josemarinas <36479864+josemarinas@users.noreply.github.com> Date: Mon, 6 May 2024 15:11:49 +0200 Subject: [PATCH 10/13] Update packages/contracts/src/framework/plugin/repo/IPluginRepo.sol Co-authored-by: Jordan <45881807+jordaniza@users.noreply.github.com> --- packages/contracts/src/framework/plugin/repo/IPluginRepo.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/contracts/src/framework/plugin/repo/IPluginRepo.sol b/packages/contracts/src/framework/plugin/repo/IPluginRepo.sol index 1706dcdd0..d78489745 100644 --- a/packages/contracts/src/framework/plugin/repo/IPluginRepo.sol +++ b/packages/contracts/src/framework/plugin/repo/IPluginRepo.sol @@ -24,7 +24,7 @@ interface IPluginRepo { bytes calldata _releaseMetadata ) external; - /// @notice Thrown if the same plugin setup exists in previous releases. + /// @notice Emitted if the same plugin setup exists in previous releases. /// @param release The release number. /// @param build The build number. /// @param pluginSetup The address of the plugin setup contract. From 9c4d2a917d785809ee9eb4c6c87185b522c25cac Mon Sep 17 00:00:00 2001 From: josemarinas <36479864+josemarinas@users.noreply.github.com> Date: Mon, 6 May 2024 15:11:54 +0200 Subject: [PATCH 11/13] Update packages/contracts/src/framework/plugin/repo/IPluginRepo.sol Co-authored-by: Jordan <45881807+jordaniza@users.noreply.github.com> --- packages/contracts/src/framework/plugin/repo/IPluginRepo.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/contracts/src/framework/plugin/repo/IPluginRepo.sol b/packages/contracts/src/framework/plugin/repo/IPluginRepo.sol index d78489745..6f5557a0d 100644 --- a/packages/contracts/src/framework/plugin/repo/IPluginRepo.sol +++ b/packages/contracts/src/framework/plugin/repo/IPluginRepo.sol @@ -36,7 +36,7 @@ interface IPluginRepo { bytes buildMetadata ); - /// @notice Thrown when a release's metadata was updated. + /// @notice Emitted when a release's metadata was updated. /// @param release The release number. /// @param releaseMetadata The release metadata URI. event ReleaseMetadataUpdated(uint8 release, bytes releaseMetadata); From c104eabd8cf3eeeabf73845f1b669b4d8eb750e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jose=20Manuel=20Mari=C3=B1as=20Bascoy?= Date: Mon, 6 May 2024 15:15:20 +0200 Subject: [PATCH 12/13] fix: tests --- .../src/framework/plugin/repo/PluginRepo.sol | 2 -- .../contracts/test/framework/plugin/plugin-repo.ts | 12 +----------- 2 files changed, 1 insertion(+), 13 deletions(-) diff --git a/packages/contracts/src/framework/plugin/repo/PluginRepo.sol b/packages/contracts/src/framework/plugin/repo/PluginRepo.sol index 1542f462e..b777b505d 100644 --- a/packages/contracts/src/framework/plugin/repo/PluginRepo.sol +++ b/packages/contracts/src/framework/plugin/repo/PluginRepo.sol @@ -95,8 +95,6 @@ contract PluginRepo is /// @notice Thrown if release does not exist. error ReleaseDoesNotExist(); - /// @notice Thrown if an upgrade is not supported from a specific protocol version . - error ProtocolVersionUpgradeNotSupported(uint8[3] protocolVersion); /// @dev Used to disallow initializing the implementation contract by an attacker for extra safety. constructor() { diff --git a/packages/contracts/test/framework/plugin/plugin-repo.ts b/packages/contracts/test/framework/plugin/plugin-repo.ts index 1fbd13754..de58b6ee6 100644 --- a/packages/contracts/test/framework/plugin/plugin-repo.ts +++ b/packages/contracts/test/framework/plugin/plugin-repo.ts @@ -154,17 +154,7 @@ describe('PluginRepo', function () { }); }); describe('InitializeFrom', () => { - it('increments `_initialized` to `1`', async () => { - // Expect the contract to be initialized with `_initialized = 1`. - expect( - ethers.BigNumber.from( - await ethers.provider.getStorageAt( - pluginRepo.address, - OZ_INITIALIZED_SLOT_POSITION - ) - ).toNumber() - ).to.equal(1); - + it('Should revert because the function is a placeholder', async () => { // Call `initializeFrom` with version 1.3.0. and revert await expect(pluginRepo.initializeFrom([1, 3, 0], emptyBytes)).to.be .reverted; From 9df22353f3a17232c813477dff692bcbada6875a Mon Sep 17 00:00:00 2001 From: josemarinas <36479864+josemarinas@users.noreply.github.com> Date: Thu, 9 May 2024 08:57:35 +0200 Subject: [PATCH 13/13] Update packages/contracts/test/framework/plugin/plugin-repo.ts Co-authored-by: Michael Heuer <20623991+Michael-A-Heuer@users.noreply.github.com> --- packages/contracts/test/framework/plugin/plugin-repo.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/contracts/test/framework/plugin/plugin-repo.ts b/packages/contracts/test/framework/plugin/plugin-repo.ts index de58b6ee6..f2b435b46 100644 --- a/packages/contracts/test/framework/plugin/plugin-repo.ts +++ b/packages/contracts/test/framework/plugin/plugin-repo.ts @@ -154,7 +154,7 @@ describe('PluginRepo', function () { }); }); describe('InitializeFrom', () => { - it('Should revert because the function is a placeholder', async () => { + it('reverts because the function is a placeholder', async () => { // Call `initializeFrom` with version 1.3.0. and revert await expect(pluginRepo.initializeFrom([1, 3, 0], emptyBytes)).to.be .reverted;