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

refactor: added reinitializer and storage gap to PluginRepo and moved events to IPluginRepo #584

Merged
merged 13 commits into from
May 10, 2024
Merged
3 changes: 3 additions & 0 deletions packages/contracts/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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

Expand Down
17 changes: 17 additions & 0 deletions packages/contracts/src/framework/plugin/repo/IPluginRepo.sol
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,21 @@ interface IPluginRepo {
bytes calldata _buildMetadata,
bytes calldata _releaseMetadata
) external;

/// @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.
/// @param buildMetadata The build metadata URI.
event VersionCreated(
uint8 release,
uint16 build,
address indexed pluginSetup,
bytes buildMetadata
);

/// @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);
}
36 changes: 19 additions & 17 deletions packages/contracts/src/framework/plugin/repo/PluginRepo.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -129,6 +112,22 @@ contract PluginRepo is
_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)).
/// @dev This function is a placeholder until we require reinitialization.
function initializeFrom(
uint8[3] calldata _previousProtocolVersion,
josemarinas marked this conversation as resolved.
Show resolved Hide resolved
bytes calldata _initData
) external reinitializer(2) {
// Silences the unused function parameter warning.
_previousProtocolVersion;
_initData;

// Revert because this is a placeholder until this contract requires reinitialization.
revert();
}

/// @inheritdoc IPluginRepo
function createVersion(
uint8 _release,
Expand Down Expand Up @@ -272,4 +271,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;
heueristik marked this conversation as resolved.
Show resolved Hide resolved
}
10 changes: 9 additions & 1 deletion packages/contracts/test/framework/plugin/plugin-repo.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -152,6 +153,13 @@ describe('PluginRepo', function () {
expect(toProtocolVersion).to.deep.equal(osxContractsVersion());
});
});
describe('InitializeFrom', () => {
it('Should revert because the function is a placeholder', async () => {
josemarinas marked this conversation as resolved.
Show resolved Hide resolved
// 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 () => {
Expand Down Expand Up @@ -515,7 +523,7 @@ describe('PluginRepo', function () {
).to.be.revertedWithCustomError(pluginRepo, 'EmptyReleaseMetadata');
});

it('successfuly updates metadata for the release that already exists', async () => {
it('updates metadata for the release that already exists and emits the "ReleaseMetadataUpdated" event', async () => {
josemarinas marked this conversation as resolved.
Show resolved Hide resolved
await pluginRepo.createVersion(
1,
pluginSetupMock.address,
Expand Down
Loading