diff --git a/CHANGELOG.md b/CHANGELOG.md index 1e10081..cde9424 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,20 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## v1.2 + +### Changed + +- Recompiled against an amended `osx-commons` `RuledCondition._evalLogic`. The IF_ELSE starting rule now evaluates with `_where`/`_who` in the same order as the surrounding `_evalRule` call. No setup interface change. + +### Added + +- SPP-level regression tests for `SPPRuleCondition.isGranted` covering an asymmetric IF_ELSE predicate (success/failure routing and a swapped-args caller path). +- Unit and fork tests for `prepareUpdate`. +- `script/NewVersion.s.sol` now also prints the management DAO multisig `createProposal` calldata wrapping the `createVersion` action — including the pinned `PROPOSAL_METADATA` URI as the proposal metadata — so a multisig member can submit it directly. +- `script/Deploy.s.sol` now publishes `PlaceholderSetup` builds for builds 1..VERSION_BUILD-1 on a fresh repo before publishing the real `SPPSetup` build, keeping on-chain build numbers aligned across networks. +- `PROPOSAL_METADATA` and `PLACEHOLDER_BUILD_METADATA` constants in `PluginSettings.sol`, and `script/new-version-proposal-metadata.json` as the v1.2 proposal metadata source. + ## v1.1 ### Added diff --git a/README.md b/README.md index e483f6b..f0137e6 100644 --- a/README.md +++ b/README.md @@ -40,18 +40,40 @@ forge build ```shell just test # unit tests just test-fork # fork tests (requires RPC_URL) -just validate-upgrade SPPStorageV1 StagedProposalProcessor # storage layout check +just check-upgrade SPPStorageV1 StagedProposalProcessor # storage layout compatibility check ``` ## Deploy ```shell just deploy # initial deployment (creates plugin repo, publishes v1) -just new-version # deploy new setup + print DAO proposal calldata +just new-version # deploy new setup + print management DAO multisig proposal calldata ``` Set `SPP_ENS_SUBDOMAIN=spp` in `.env` for production deployments. Omitting it generates a unique name (`spp-`), which is useful for testing. +### Publishing a new build + +1. Bump `VERSION_BUILD` in `src/utils/PluginSettings.sol`. +2. Edit `src/build-metadata.json` and `script/new-version-proposal-metadata.json` for this build (and `src/release-metadata.json` if shipping a new release). +3. Pin and update the matching constants in `PluginSettings.sol`: + ```shell + just ipfs-pin src/build-metadata.json # → BUILD_METADATA + just ipfs-pin script/new-version-proposal-metadata.json # → PROPOSAL_METADATA + just ipfs-pin src/release-metadata.json # → RELEASE_METADATA (only on a new release) + ``` +4. Run `just new-version`. The script deploys the new `SPPSetup` and prints two calldata blobs: + - the inner `createVersion` action (`to = SPP_PLUGIN_REPO_ADDRESS`), and + - the outer management DAO multisig `createProposal` call (`to = MANAGEMENT_DAO_MULTISIG_ADDRESS`) including the pinned `PROPOSAL_METADATA` URI — submit it from any listed multisig member to publish the version. + +On a brand-new network, `just deploy` automatically publishes `PlaceholderSetup` builds for any build numbers below `VERSION_BUILD` before publishing the real one, so build numbers stay aligned with networks where prior builds shipped. + +### Upgrading existing installations + +Publishing a new build does not upgrade installed plugins. Each DAO running an older build needs a proposal that calls `psp.applyUpdate(...)`. + +Version 1.2 is published with the same `IMPLEMENTATION` as 1.1 (bytecode is identical), so `applyUpdate` skips the proxy upgrade — no `UPGRADE_PLUGIN_PERMISSION` grant/revoke bracket is required. + ### Deployment Checklist - [ ] I have cloned the official repository on my computer and I have checked out the `main` branch @@ -73,6 +95,7 @@ Set `SPP_ENS_SUBDOMAIN=spp` in `.env` for production deployments. Omitting it ge - [ ] I have created a new burner wallet with `cast wallet new` and used its private key as `DEPLOYER_KEY` - [ ] I am the only person of the ceremony that will operate the deployment wallet - [ ] All the tests run clean (`just test`) +- [ ] `just check-upgrade OldContract NewContract` reports the storage layout check passed - My computer: - [ ] Is running in a safe location and using a trusted network - [ ] It exposes no services or ports diff --git a/justfile b/justfile index c536923..e02be9a 100644 --- a/justfile +++ b/justfile @@ -8,6 +8,12 @@ docs: cd docs-gen && bun install && bash prepare-docs.sh && bun prepare-docs.js DEPLOY_SCRIPT := "script/Deploy.s.sol:Deploy" +NEW_VERSION_SCRIPT := "script/NewVersion.s.sol:NewVersion" + +# Dry-run the new-version script (no broadcast) — eyeball the printed multisig calldata +[group('upgrade')] +pre-new-version: + just dry-run {{ NEW_VERSION_SCRIPT }} # Publish a new SPP plugin version (deploys setup, prints DAO proposal calldata) [group('upgrade')] @@ -18,6 +24,6 @@ new-version *args: mkdir -p logs LOG_FILE="logs/new-version-$NETWORK_NAME-$(date +"%y-%m-%d-%H-%M").log" just test 2>&1 | tee -a "$LOG_FILE" - just run script/NewVersion.s.sol:NewVersion {{ args }} 2>&1 | tee -a "$LOG_FILE" + just run {{ NEW_VERSION_SCRIPT }} {{ args }} 2>&1 | tee -a "$LOG_FILE" echo "Logs saved in $LOG_FILE" diff --git a/script/Deploy.s.sol b/script/Deploy.s.sol index 5f80eb3..1d0897e 100644 --- a/script/Deploy.s.sol +++ b/script/Deploy.s.sol @@ -10,6 +10,9 @@ import {StagedProposalProcessorSetup as SPPSetup} from "../src/StagedProposalPro import {PluginRepo} from "@aragon/osx/framework/plugin/repo/PluginRepo.sol"; import {PluginRepoFactory} from "@aragon/osx/framework/plugin/repo/PluginRepoFactory.sol"; +import { + PlaceholderSetup +} from "@aragon/osx/framework/plugin/repo/placeholder/PlaceholderSetup.sol"; import {PermissionLib} from "@aragon/osx-commons-contracts/src/permission/PermissionLib.sol"; import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; @@ -18,6 +21,8 @@ contract Deploy is BaseScript { error InvalidVersionBuild(uint8 build, uint8 latestBuild); error VersionPublishFailed(); + PlaceholderSetup public placeholderSetup; + function run() external { address pluginRepoFactory = vm.envAddress("PLUGIN_REPO_FACTORY_ADDRESS"); address managementDao = vm.envAddress("MANAGEMENT_DAO_ADDRESS"); @@ -37,6 +42,20 @@ contract Deploy is BaseScript { revert InvalidVersionBuild(PluginSettings.VERSION_BUILD, uint8(latestBuild)); } + // Fill builds 1..VERSION_BUILD-1 with PlaceholderSetup so build numbers stay + // aligned across networks (a fresh chain still ends up with build N == VERSION_BUILD). + if (PluginSettings.VERSION_BUILD > latestBuild + 1) { + placeholderSetup = new PlaceholderSetup(); + for (uint8 i = uint8(latestBuild) + 1; i < PluginSettings.VERSION_BUILD; ++i) { + sppRepo.createVersion( + PluginSettings.VERSION_RELEASE, + address(placeholderSetup), + bytes(PluginSettings.PLACEHOLDER_BUILD_METADATA), + bytes(PluginSettings.RELEASE_METADATA) + ); + } + } + sppRepo.createVersion( PluginSettings.VERSION_RELEASE, address(sppSetup), @@ -59,6 +78,9 @@ contract Deploy is BaseScript { console.log("- SPP PluginRepo: ", address(sppRepo)); console.log("- SPP PluginSetup: ", address(sppSetup)); console.log("- Implementation: ", sppSetup.implementation()); + if (address(placeholderSetup) != address(0)) { + console.log("- PlaceholderSetup: ", address(placeholderSetup)); + } console.log( "- Version: ", _versionString(PluginSettings.VERSION_RELEASE, PluginSettings.VERSION_BUILD) diff --git a/script/NewVersion.s.sol b/script/NewVersion.s.sol index 6df6661..948e348 100644 --- a/script/NewVersion.s.sol +++ b/script/NewVersion.s.sol @@ -9,33 +9,112 @@ import {StagedProposalProcessor as SPP} from "../src/StagedProposalProcessor.sol import {StagedProposalProcessorSetup as SPPSetup} from "../src/StagedProposalProcessorSetup.sol"; import {PluginRepo} from "@aragon/osx/framework/plugin/repo/PluginRepo.sol"; +import {Action} from "@aragon/osx-commons-contracts/src/executors/IExecutor.sol"; +import {IPluginSetup} from "@aragon/osx-commons-contracts/src/plugin/setup/IPluginSetup.sol"; -/// @notice Deploys a new SPPSetup implementation and prints the DAO proposal calldata. -/// Submit the printed calldata as a management DAO proposal to publish the new version. +/// @dev Minimal subset of the management DAO Multisig plugin ABI used by this script. +/// Mirrors the 7-arg `createProposal` overload (selector `0xfbd56e41`); using this typed +/// interface with `abi.encodeCall` makes the compiler enforce the signature match instead +/// of trusting a string we hand to `abi.encodeWithSignature`. +interface IManagementDaoMultisig { + function createProposal( + bytes calldata _metadata, + Action[] calldata _actions, + uint256 _allowFailureMap, + bool _approveProposal, + bool _tryExecution, + uint64 _startDate, + uint64 _endDate + ) external returns (uint256 proposalId); +} + +/// @notice Deploys a new SPPSetup implementation and prints both the inner +/// `createVersion` action and the outer management DAO multisig +/// `createProposal` calldata. Submit the printed multisig calldata from any +/// listed multisig member to publish this version. contract NewVersion is BaseScript { function run() external { sppRepo = PluginRepo(vm.envAddress("SPP_PLUGIN_REPO_ADDRESS")); + address managementDaoMultisig = vm.envAddress("MANAGEMENT_DAO_MULTISIG_ADDRESS"); + + // Reuse the previous build's plugin implementation. v1.2's bytecode is identical to v1.1's + SPP existingImpl = _readLatestImplementation(); vm.startBroadcast(deployerPrivateKey); - sppSetup = new SPPSetup(new SPP()); + sppSetup = new SPPSetup(existingImpl); vm.stopBroadcast(); - console.log("- SPP PluginSetup: ", address(sppSetup)); + console.log("- SPP PluginSetup: ", address(sppSetup)); console.log( - "- Version: ", + "- Version: ", _versionString(PluginSettings.VERSION_RELEASE, PluginSettings.VERSION_BUILD) ); - console.log("\nDAO proposal to publish this version:"); - console.log(" to: ", address(sppRepo)); - console.log(" value: ", uint256(0)); - console.logBytes( - abi.encodeWithSelector( - sppRepo.createVersion.selector, + + bytes memory createVersionData = abi.encodeCall( + sppRepo.createVersion, + ( PluginSettings.VERSION_RELEASE, address(sppSetup), - PluginSettings.BUILD_METADATA, - PluginSettings.RELEASE_METADATA + bytes(PluginSettings.BUILD_METADATA), + bytes(PluginSettings.RELEASE_METADATA) ) ); + + console.log("\nDAO action to publish this version:"); + console.log(" to: ", address(sppRepo)); + console.log(" value: 0"); + console.log(" data: "); + console.logBytes(createVersionData); + + // Wrap the action in a management DAO multisig proposal. The 7-arg + // `createProposal` is the multisig-specific overload; passing + // `_approveProposal=true` means the submitter also casts their vote + // in the same transaction. + Action[] memory actions = new Action[](1); + actions[0] = Action({to: address(sppRepo), value: 0, data: createVersionData}); + + bytes memory metadata = bytes(PluginSettings.PROPOSAL_METADATA); + uint64 endDate = uint64(vm.envOr("PROPOSAL_END_DATE", block.timestamp + 30 days)); + bytes memory multisigCalldata = abi.encodeCall( + IManagementDaoMultisig.createProposal, + ( + metadata, + actions, + uint256(0), // _allowFailureMap + true, // _approveProposal + false, // _tryExecution + uint64(0), // _startDate (0 = now, evaluated at submission time) + endDate + ) + ); + + // The Multisig derives proposalId as + // keccak256(abi.encode(chainid, block.number, multisig, keccak256(abi.encode(actions, metadata)))) + // (see Multisig.createProposal -> Proposal._createProposalId in osx-commons). + // Submission block isn't known at script time, so we print the deterministic + // salt; the actual proposalId is also surfaced via the `ProposalCreated` event + // on the submission tx receipt. + bytes32 proposalSalt = keccak256(abi.encode(actions, metadata)); + + console.log("\nManagement DAO multisig proposal to publish this version:"); + console.log(" to: ", managementDaoMultisig); + console.log(" value: 0"); + console.log(" data: "); + console.logBytes(multisigCalldata); + console.log("\n proposal metadata: ", PluginSettings.PROPOSAL_METADATA); + console.log(" defaults: allowFailureMap=0, approveProposal=true, tryExecution=false, startDate=0"); + console.log(" endDate (unix): ", uint256(endDate)); + console.log("\n proposal id (deterministic salt):"); + console.logBytes32(proposalSalt); + console.log(" full id = keccak256(abi.encode(chainid, block.number @ submission, multisig, salt))"); + console.log(" or read it from the `ProposalCreated` event on the submission tx receipt."); + } + + function _readLatestImplementation() internal view returns (SPP) { + uint8 latestRelease = sppRepo.latestRelease(); + uint16 latestBuild = uint16(sppRepo.buildCount(latestRelease)); + PluginRepo.Tag memory latestTag = PluginRepo.Tag({release: latestRelease, build: latestBuild}); + address latestSetup = sppRepo.getVersion(latestTag).pluginSetup; + return SPP(IPluginSetup(latestSetup).implementation()); } } diff --git a/script/new-version-proposal-metadata.json b/script/new-version-proposal-metadata.json new file mode 100644 index 0000000..962e946 --- /dev/null +++ b/script/new-version-proposal-metadata.json @@ -0,0 +1,15 @@ +{ + "title": "Publish StagedProposalProcessor v1.2", + "summary": "Publishes build 2 of the StagedProposalProcessor (release 1) on the SPP repo.", + "description": "Calls `createVersion` on the SPP plugin repo to publish v1.2 (release 1, build 2).\n\nBuild 2 is recompiled against an amended `osx-commons` `RuledCondition`. The installation parameters are unchanged versus build 1.\n\nExisting v1.1 installations can update in place via the setup's `prepareUpdate` flow, which deploys a fresh `SPPRuleCondition` seeded with the existing rules and migrates the `CREATE_PROPOSAL` and `UPDATE_RULES` permissions from the old helper to the new one.", + "resources": [ + { + "name": "changelog", + "url": "https://github.com/aragon/staged-proposal-processor-plugin/blob/main/CHANGELOG.md" + }, + { + "name": "audit report", + "url": "https://github.com/aragon/osx/tree/main/audits" + } + ] +} diff --git a/src/StagedProposalProcessorSetup.sol b/src/StagedProposalProcessorSetup.sol index 7929554..8bbd463 100644 --- a/src/StagedProposalProcessorSetup.sol +++ b/src/StagedProposalProcessorSetup.sol @@ -20,7 +20,7 @@ import { /// @title StagedProposalProcessorSetup /// @author Aragon X - 2024 /// @notice The setup contract of the `StagedProposalProcessor` plugin. -/// @dev Release 1, Build 1 +/// @dev Release 1, Build 2 contract StagedProposalProcessorSetup is PluginUpgradeableSetup { using ProxyLib for address; @@ -91,14 +91,71 @@ contract StagedProposalProcessorSetup is PluginUpgradeableSetup { } /// @inheritdoc IPluginSetup - /// @dev The default implementation for the initial build 1 that reverts because no earlier build exists. + /// @dev v1.1 → v1.2: deploys a fresh `SPPRuleCondition` seeded with the existing rules and migrates + /// `CREATE_PROPOSAL_PERMISSION` (on the plugin) and `UPDATE_RULES_PERMISSION` (on the helper) from + /// the old condition to the new one. The plugin proxy itself is upgraded to the new implementation + /// by the `PluginSetupProcessor` automatically; no reinitializer is required because no new storage + /// is introduced in build 2. Existing rules are read from the old helper, so no caller-supplied data + /// is required — `_payload.data` is ignored. function prepareUpdate( address _dao, uint16 _fromBuild, SetupPayload calldata _payload - ) external pure virtual returns (bytes memory, PreparedSetupData memory) { - (_dao, _fromBuild, _payload); - revert InvalidUpdatePath({fromBuild: 0, thisBuild: 1}); + ) external virtual override returns (bytes memory initData, PreparedSetupData memory preparedSetupData) { + if (_fromBuild != 1) { + revert InvalidUpdatePath({fromBuild: _fromBuild, thisBuild: 2}); + } + + address oldCondition = _payload.currentHelpers[0]; + RuledCondition.Rule[] memory rules = SPPRuleCondition(oldCondition).getRules(); + + bytes memory conditionInitData = abi.encodeCall( + SPPRuleCondition.initialize, + (_dao, rules) + ); + address newCondition = CLONES_SUPPORTED + ? CONDITION_IMPLEMENTATION.deployMinimalProxy(conditionInitData) + : CONDITION_IMPLEMENTATION.deployUUPSProxy(conditionInitData); + + preparedSetupData.helpers = new address[](1); + preparedSetupData.helpers[0] = newCondition; + + preparedSetupData.permissions = new PermissionLib.MultiTargetPermission[](4); + + // Move CREATE_PROPOSAL_PERMISSION on the plugin from the old condition to the new one. + preparedSetupData.permissions[0] = PermissionLib.MultiTargetPermission({ + operation: PermissionLib.Operation.Revoke, + where: _payload.plugin, + who: ANY_ADDR, + condition: oldCondition, + permissionId: Permissions.CREATE_PROPOSAL_PERMISSION_ID + }); + preparedSetupData.permissions[1] = PermissionLib.MultiTargetPermission({ + operation: PermissionLib.Operation.GrantWithCondition, + where: _payload.plugin, + who: ANY_ADDR, + condition: newCondition, + permissionId: Permissions.CREATE_PROPOSAL_PERMISSION_ID + }); + + // Move UPDATE_RULES_PERMISSION (DAO is the rule manager) from the old condition to the new one. + preparedSetupData.permissions[2] = PermissionLib.MultiTargetPermission({ + operation: PermissionLib.Operation.Revoke, + where: oldCondition, + who: _dao, + condition: PermissionLib.NO_CONDITION, + permissionId: Permissions.UPDATE_RULES_PERMISSION_ID + }); + preparedSetupData.permissions[3] = PermissionLib.MultiTargetPermission({ + operation: PermissionLib.Operation.Grant, + where: newCondition, + who: _dao, + condition: PermissionLib.NO_CONDITION, + permissionId: Permissions.UPDATE_RULES_PERMISSION_ID + }); + + // initData stays empty — applyUpdate triggers the proxy implementation upgrade on its own. + initData = ""; } /// @inheritdoc IPluginSetup diff --git a/src/build-metadata.json b/src/build-metadata.json index 7b8633c..e1f14dc 100644 --- a/src/build-metadata.json +++ b/src/build-metadata.json @@ -1,6 +1,6 @@ { "ui": {}, - "change": "Initial build.", + "change": "Recompiled against an amended osx-commons RuledCondition. Adds an in-place v1.1 -> v1.2 update path: the setup's prepareUpdate deploys a fresh SPPRuleCondition seeded with the existing rules and migrates CREATE_PROPOSAL / UPDATE_RULES permissions from the old helper to the new one. The setup interface for new installations is unchanged versus build 1.", "pluginSetup": { "prepareInstallation": { "description": "The information required for the installation of build 1.", diff --git a/src/utils/PluginSettings.sol b/src/utils/PluginSettings.sol index a8679a1..08323e0 100644 --- a/src/utils/PluginSettings.sol +++ b/src/utils/PluginSettings.sol @@ -14,13 +14,29 @@ library PluginSettings { // Specify the version of your plugin that you are currently working on. The first version is v1.1. // For more details, visit https://devs.aragon.org/docs/osx/how-it-works/framework/plugin-management/plugin-repo. uint8 public constant VERSION_RELEASE = 1; - uint8 public constant VERSION_BUILD = 1; + uint8 public constant VERSION_BUILD = 2; - // 1. upload build-metadata and release-metadata jsons to the IPFS. - // 2. use ethers to convert it to utf8 bytes: - // ethers.utils.hexlify(ethers.utils.toUtf8Bytes(`ipfs://${cid}`)) - // 3. Copy/paste the bytes into BUILD_METADATA and RELEASE_METADATA - - string public constant BUILD_METADATA = "ipfs://bafkreifia6hhz7klfbaqawd4vcplkoiesycbmrf5c2x24zfuivyn35mfsu"; - string public constant RELEASE_METADATA = "ipfs://bafkreif23p6yw325rkwwlhgkudiasvq64lonqmfnt7ls5ksfam5hedcb4m"; + // Per-build flow when bumping VERSION_BUILD (or VERSION_RELEASE): + // 1. Edit the matching JSON file: + // - `src/build-metadata.json` → BUILD_METADATA + // - `script/new-version-proposal-metadata.json` → PROPOSAL_METADATA + // - `src/release-metadata.json` (only on a new release) → RELEASE_METADATA + // 2. Pin via `just ipfs-pin `. + // 3. Paste the returned `ipfs://` into the matching constant below. + + string public constant BUILD_METADATA = + "ipfs://QmaxGSvvnTAZcDLYz2BMtaXmcx3i1GcaKGaxNEpfQe3Vyv"; + string public constant RELEASE_METADATA = + "ipfs://bafkreif23p6yw325rkwwlhgkudiasvq64lonqmfnt7ls5ksfam5hedcb4m"; + + /// @notice Title/summary/description/resources JSON pinned for this version's management DAO proposal. + /// @dev Re-pin and update on every VERSION_BUILD bump. Source: `script/new-version-proposal-metadata.json`. + string public constant PROPOSAL_METADATA = + "ipfs://QmTS3Nrjrs8nuMeqUqSRjBxbGUhZB4nW6N1GiK8vFmfDcD"; + + /// @notice Aragon's canonical empty-schema placeholder build metadata, used when filling skipped builds + /// on a fresh-network deploy so on-chain build numbers stay aligned across networks. + /// @dev Content-addressed; the file at `lib/osx/.../placeholder/placeholder-build-metadata.json` always pins to this CID. + string public constant PLACEHOLDER_BUILD_METADATA = + "ipfs://QmZDx8G5xuF9vqVbFGZ3KhF5nioL8gXwV3JbsEsSHvNMiz"; } diff --git a/test/fork/ForkBaseTest.t.sol b/test/fork/ForkBaseTest.t.sol index 7a10518..00aaf1c 100644 --- a/test/fork/ForkBaseTest.t.sol +++ b/test/fork/ForkBaseTest.t.sol @@ -83,13 +83,20 @@ contract ForkBaseTest is Assertions, Constants, Events, Fuzzers, Test { target = new Target(); trustedForwarder = new TrustedForwarder(); - // publish new spp version - sppSetup = new SPPSetup(new SPP()); // Check release number uint256 latestRelease = sppRepo.latestRelease(); uint256 latestBuild = sppRepo.buildCount(uint8(latestRelease)); + // Publish a test build that reuses the latest published SPP implementation. Mirrors + // NewVersion.s.sol so that PSP.applyUpdate sees `currentImpl == newImpl` and skips + // the proxy upgrade — i.e., per-DAO upgrades work without UPGRADE_PLUGIN_PERMISSION. + address latestSetup = sppRepo + .getVersion(PluginRepo.Tag({release: uint8(latestRelease), build: uint16(latestBuild)})) + .pluginSetup; + SPP existingImpl = SPP(IPluginSetup(latestSetup).implementation()); + sppSetup = new SPPSetup(existingImpl); + // create plugin version resetPrank(managementDao); sppRepo.createVersion( diff --git a/test/fork/upgradeV1_1ToV1_2.t.sol b/test/fork/upgradeV1_1ToV1_2.t.sol new file mode 100644 index 0000000..92fe643 --- /dev/null +++ b/test/fork/upgradeV1_1ToV1_2.t.sol @@ -0,0 +1,261 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.18; + +import {ForkBaseTest} from "./ForkBaseTest.t.sol"; +import {Permissions} from "../../src/libraries/Permissions.sol"; +import {SPPRuleCondition} from "../../src/utils/SPPRuleCondition.sol"; +import {StagedProposalProcessor as SPP} from "../../src/StagedProposalProcessor.sol"; + +import {DAO} from "@aragon/osx/core/dao/DAO.sol"; +import {DAOFactory} from "@aragon/osx/framework/dao/DAOFactory.sol"; +import { + hashHelpers, + PluginSetupRef +} from "@aragon/osx/framework/plugin/setup/PluginSetupProcessorHelpers.sol"; +import {PluginRepo} from "@aragon/osx/framework/plugin/repo/PluginRepo.sol"; +import {IPlugin} from "@aragon/osx-commons-contracts/src/plugin/IPlugin.sol"; +import {IPluginSetup} from "@aragon/osx-commons-contracts/src/plugin/setup/IPluginSetup.sol"; +import {PluginSetupProcessor} from "@aragon/osx/framework/plugin/setup/PluginSetupProcessor.sol"; +import { + RuledCondition +} from "@aragon/osx-commons-contracts/src/permission/condition/extensions/RuledCondition.sol"; +import { + AddressCheckConditionMock +} from "@aragon/osx-commons-contracts/src/mocks/permission/condition/AddressCheckConditionMock.sol"; + +/// @notice Forks a network where v1.1 is deployed (set RPC_URL accordingly), installs the SPP at +/// build 1, exercises the v1.1 → v1.2 upgrade through the PSP, and asserts that the helper has +/// been swapped, the rules preserved, and the IF_ELSE `_where`/`_who` swap fixed end-to-end. Run +/// with e.g. `RPC_URL= just test-fork --match-test test_upgradeFromBuild1`. +contract UpgradeV1_1ToV1_2_ForkTest is ForkBaseTest { + DAO internal dao; + address internal installedAdminPlugin; + + // Op enum and rule-id values mirrored as raw uint8 to keep the test independent of internal + // enum ordering — a change there would itself be a bug. + uint8 internal constant OP_RET = 7; + uint8 internal constant OP_IF_ELSE = 12; + uint8 internal constant LOGIC_OP_RULE_ID = 203; + uint8 internal constant VALUE_RULE_ID = 204; + + function setUp() public override { + super.setUp(); + + DAOFactory.InstalledPlugin[] memory installed; + (dao, installed) = _createDummyDaoAdmin(); + installedAdminPlugin = installed[0].plugin; + } + + function test_upgradeFromBuild1_replacesHelper_preservesRules_andFixesIfElseSwap() external { + // ---- 1. Install the SPP at build 1 (the v1.1 setup that lives on the fork). ---- + PluginSetupRef memory build1Ref = PluginSetupRef({ + versionTag: PluginRepo.Tag({release: 1, build: 1}), + pluginSetupRepo: sppRepo + }); + + (address plugin, address[] memory helpers) = _installSPPAtRef( + dao, + _prepareSimpleInstallData(), + build1Ref + ); + address oldCondition = helpers[0]; + assertEq(SPPRuleCondition(oldCondition).getRules().length, 0, "starts with no rules"); + + // ---- 2. Seed the old condition with an IF_ELSE rule whose predicate is asymmetric in + // (`_where`, `_who`). Build 1 ships with the swap bug, so the predicate sees the + // arguments in the wrong order and the IF_ELSE routes to the failure branch. ---- + AddressCheckConditionMock asymCondition = new AddressCheckConditionMock(); + address aliceWho = makeAddr("aliceWho"); + asymCondition.setExpected(plugin, aliceWho); + + RuledCondition.Rule[] memory rules = new RuledCondition.Rule[](4); + rules[0] = RuledCondition.Rule({ + id: LOGIC_OP_RULE_ID, + op: OP_IF_ELSE, + value: SPPRuleCondition(oldCondition).encodeIfElse(1, 2, 3), + permissionId: Permissions.CREATE_PROPOSAL_PERMISSION_ID + }); + rules[1] = RuledCondition.Rule({ + id: 202, // CONDITION_RULE_ID + op: 1, // EQ + value: uint160(address(asymCondition)), + permissionId: Permissions.CREATE_PROPOSAL_PERMISSION_ID + }); + rules[2] = RuledCondition.Rule({ + id: VALUE_RULE_ID, + op: OP_RET, + value: 1, + permissionId: Permissions.CREATE_PROPOSAL_PERMISSION_ID + }); + rules[3] = RuledCondition.Rule({ + id: VALUE_RULE_ID, + op: OP_RET, + value: 0, + permissionId: Permissions.CREATE_PROPOSAL_PERMISSION_ID + }); + + resetPrank(address(dao)); + SPPRuleCondition(oldCondition).updateRules(rules); + resetPrank(deployer); + + // Bug witness on v1.1: `isGranted(plugin, aliceWho, ...)` should evaluate the predicate + // with `(_where=plugin, _who=aliceWho)` and route to the success branch (return true). + // Because of the swap, the predicate sees `(_where=aliceWho, _who=plugin)`, doesn't + // match the expected pair, and the IF_ELSE returns false instead. + assertFalse( + SPPRuleCondition(oldCondition).isGranted( + plugin, + aliceWho, + Permissions.CREATE_PROPOSAL_PERMISSION_ID, + bytes("") + ), + "v1.1: IF_ELSE swap bug returns false where it should return true" + ); + + // PSP rejects an update in the same block as the install. + vm.roll(block.number + 1); + + // ---- 3. prepareUpdate(1 -> 2). The new setup published in setUp handles the migration. ---- + PluginSetupRef memory build2Ref = PluginSetupRef({ + versionTag: PluginRepo.Tag({release: 1, build: 2}), + pluginSetupRepo: sppRepo + }); + + ( + bytes memory initData, + IPluginSetup.PreparedSetupData memory preparedSetupData + ) = psp.prepareUpdate( + address(dao), + PluginSetupProcessor.PrepareUpdateParams({ + currentVersionTag: build1Ref.versionTag, + newVersionTag: build2Ref.versionTag, + pluginSetupRepo: sppRepo, + setupPayload: IPluginSetup.SetupPayload({ + plugin: plugin, + currentHelpers: helpers, + data: "" + }) + }) + ); + + address newCondition = preparedSetupData.helpers[0]; + assertNotEq(newCondition, oldCondition, "helper replaced"); + assertEq(initData.length, 0, "initData empty (no reinitializer)"); + assertEq(preparedSetupData.permissions.length, 4, "four permission migrations"); + + // ---- 4. applyUpdate. NewVersion.s.sol publishes v1.2 with the same `IMPLEMENTATION` + // as v1.1 (bytecode is identical), so PSP.applyUpdate sees the proxy already + // points at the right impl and skips the upgrade — no UPGRADE_PLUGIN_PERMISSION + // bracket needed. The PSP only needs ROOT temporarily to apply the helper-swap + // permissions. ---- + resetPrank(address(dao)); + dao.grant(address(dao), address(psp), dao.ROOT_PERMISSION_ID()); + + psp.applyUpdate( + address(dao), + PluginSetupProcessor.ApplyUpdateParams({ + plugin: plugin, + pluginSetupRef: build2Ref, + initData: initData, + permissions: preparedSetupData.permissions, + helpersHash: hashHelpers(preparedSetupData.helpers) + }) + ); + + dao.revoke(address(dao), address(psp), dao.ROOT_PERMISSION_ID()); + resetPrank(deployer); + + // ---- 5. Post-upgrade assertions: rules preserved, permissions migrated, bug fixed. ---- + assertEq(SPPRuleCondition(newCondition).getRules(), rules, "rules preserved on new helper"); + + assertTrue( + dao.hasPermission( + newCondition, + address(dao), + Permissions.UPDATE_RULES_PERMISSION_ID, + bytes("") + ), + "new helper grants UPDATE_RULES to DAO" + ); + assertFalse( + dao.hasPermission( + oldCondition, + address(dao), + Permissions.UPDATE_RULES_PERMISSION_ID, + bytes("") + ), + "old helper no longer grants UPDATE_RULES to DAO" + ); + + // The same call that returned the wrong answer on v1.1 must now return the right one. + assertTrue( + SPPRuleCondition(newCondition).isGranted( + plugin, + aliceWho, + Permissions.CREATE_PROPOSAL_PERMISSION_ID, + bytes("") + ), + "v1.2: IF_ELSE predicate now evaluates with the correct (_where, _who) order" + ); + } + + function _prepareSimpleInstallData() internal view returns (bytes memory) { + SPP.Stage[] memory stages = new SPP.Stage[](1); + SPP.Body[] memory bodies = new SPP.Body[](1); + bodies[0] = SPP.Body({ + addr: installedAdminPlugin, + isManual: true, + tryAdvance: true, + resultType: SPP.ResultType.Approval + }); + stages[0] = SPP.Stage({ + bodies: bodies, + maxAdvance: 100, + minAdvance: 30, + voteDuration: 10, + approvalThreshold: 1, + vetoThreshold: 0, + cancelable: false, + editable: false + }); + + return + abi.encode( + "dummy spp metadata", + stages, + new RuledCondition.Rule[](0), + IPlugin.TargetConfig({target: address(0), operation: IPlugin.Operation.Call}) + ); + } + + function _installSPPAtRef( + DAO _dao, + bytes memory _data, + PluginSetupRef memory _ref + ) internal returns (address plugin, address[] memory helpers) { + resetPrank(address(_dao)); + + IPluginSetup.PreparedSetupData memory preparedSetupData; + (plugin, preparedSetupData) = psp.prepareInstallation( + address(_dao), + PluginSetupProcessor.PrepareInstallationParams(_ref, _data) + ); + + helpers = preparedSetupData.helpers; + + _dao.grant(address(_dao), address(psp), _dao.ROOT_PERMISSION_ID()); + + psp.applyInstallation( + address(_dao), + PluginSetupProcessor.ApplyInstallationParams( + _ref, + plugin, + preparedSetupData.permissions, + hashHelpers(preparedSetupData.helpers) + ) + ); + + _dao.revoke(address(_dao), address(psp), _dao.ROOT_PERMISSION_ID()); + resetPrank(deployer); + } +} diff --git a/test/integration/concrete/upgradeability/upgradeability.t.sol b/test/integration/concrete/upgradeability/upgradeability.t.sol index f235df6..2222419 100644 --- a/test/integration/concrete/upgradeability/upgradeability.t.sol +++ b/test/integration/concrete/upgradeability/upgradeability.t.sol @@ -84,5 +84,5 @@ contract Upgradeability_SPP_IntegrationTest is BaseTest { } // Storage layout compatibility (SPPStorageV1 → StagedProposalProcessor) is - // validated separately: just validate-upgrade SPPStorageV1 StagedProposalProcessor + // validated separately: just check-upgrade SPPStorageV1 StagedProposalProcessor } diff --git a/test/unit/stagedProposalProcessorSetup/concrete/prepareUpdate/prepareUpdate.t.sol b/test/unit/stagedProposalProcessorSetup/concrete/prepareUpdate/prepareUpdate.t.sol index 2396294..a78f2c9 100644 --- a/test/unit/stagedProposalProcessorSetup/concrete/prepareUpdate/prepareUpdate.t.sol +++ b/test/unit/stagedProposalProcessorSetup/concrete/prepareUpdate/prepareUpdate.t.sol @@ -2,15 +2,21 @@ pragma solidity ^0.8.18; import {BaseTest} from "../../../../BaseTest.t.sol"; +import {Permissions} from "../../../../../src/libraries/Permissions.sol"; +import {SPPRuleCondition} from "../../../../../src/utils/SPPRuleCondition.sol"; import {StagedProposalProcessor as SPP} from "../../../../../src/StagedProposalProcessor.sol"; import { StagedProposalProcessorSetup as SPPSetup } from "../../../../../src/StagedProposalProcessorSetup.sol"; +import {PermissionLib} from "@aragon/osx-commons-contracts/src/permission/PermissionLib.sol"; import { PluginUpgradeableSetup } from "@aragon/osx-commons-contracts/src/plugin/setup/PluginUpgradeableSetup.sol"; import {IPluginSetup} from "@aragon/osx-commons-contracts/src/plugin/setup/IPluginSetup.sol"; +import { + RuledCondition +} from "@aragon/osx-commons-contracts/src/permission/condition/extensions/RuledCondition.sol"; contract PrepareUpdate_SPPSetup_UnitTest is BaseTest { SPPSetup sppSetup; @@ -18,25 +24,186 @@ contract PrepareUpdate_SPPSetup_UnitTest is BaseTest { function setUp() public override { super.setUp(); - // deploy SPPSetup contract. sppSetup = new SPPSetup(new SPP()); } - function test_RevertWhen_PreparingUpdate() external { - // it should revert. + function test_RevertWhen_FromBuildIsNotOne() external { + // it should revert for any build other than 1 — there is only one supported update path. + + IPluginSetup.SetupPayload memory payload = IPluginSetup.SetupPayload({ + plugin: address(0), + currentHelpers: new address[](0), + data: "" + }); + + uint16[3] memory invalidFromBuilds = [uint16(0), uint16(2), uint16(3)]; + for (uint256 i = 0; i < invalidFromBuilds.length; i++) { + vm.expectRevert( + abi.encodeWithSelector( + PluginUpgradeableSetup.InvalidUpdatePath.selector, + invalidFromBuilds[i], + 2 + ) + ); + sppSetup.prepareUpdate(address(dao), invalidFromBuilds[i], payload); + } + } + + function test_WhenFromBuildIsOne() external { + // it should deploy a new condition seeded with the existing rules, + // return it as the single helper, and emit empty initData. + + SPPRuleCondition oldCondition = _deployOldConditionWithRules(); + address fakePlugin = makeAddr("fakePlugin"); + + address[] memory currentHelpers = new address[](1); + currentHelpers[0] = address(oldCondition); + IPluginSetup.SetupPayload memory payload = IPluginSetup.SetupPayload({ + plugin: fakePlugin, + currentHelpers: currentHelpers, + data: "" + }); + + (bytes memory initData, IPluginSetup.PreparedSetupData memory setupData) = sppSetup + .prepareUpdate(address(dao), 1, payload); + + // initData stays empty: no reinitializer needed. + assertEq(initData.length, 0, "initData should be empty"); + + // a brand new helper is returned (not the old one). + assertEq(setupData.helpers.length, 1, "helpers length"); + assertNotEq(setupData.helpers[0], address(0), "helper non-zero"); + assertNotEq(setupData.helpers[0], address(oldCondition), "helper differs from old"); + + // the new helper carries the same rules as the old one. + RuledCondition.Rule[] memory oldRules = oldCondition.getRules(); + RuledCondition.Rule[] memory newRules = SPPRuleCondition(setupData.helpers[0]).getRules(); + assertEq(oldRules, newRules, "rules copied to new condition"); + } + + function test_WhenFromBuildIsOne_ItMigratesPermissions() external { + // it should revoke CREATE_PROPOSAL/UPDATE_RULES from the old condition + // and grant them on the new one, in that order. + + SPPRuleCondition oldCondition = _deployOldConditionWithRules(); + address fakePlugin = makeAddr("fakePlugin"); + + address[] memory currentHelpers = new address[](1); + currentHelpers[0] = address(oldCondition); + IPluginSetup.SetupPayload memory payload = IPluginSetup.SetupPayload({ + plugin: fakePlugin, + currentHelpers: currentHelpers, + data: "" + }); + + (, IPluginSetup.PreparedSetupData memory setupData) = sppSetup.prepareUpdate( + address(dao), + 1, + payload + ); + address newCondition = setupData.helpers[0]; + + assertEq(setupData.permissions.length, 4, "four permission migrations expected"); + + PermissionLib.MultiTargetPermission memory revokeCreate = setupData.permissions[0]; + assertEq( + uint256(revokeCreate.operation), + uint256(PermissionLib.Operation.Revoke), + "[0] operation" + ); + assertEq(revokeCreate.where, fakePlugin, "[0] where"); + assertEq(revokeCreate.who, ANY_ADDR, "[0] who"); + assertEq(revokeCreate.condition, address(oldCondition), "[0] condition"); + assertEq( + revokeCreate.permissionId, + Permissions.CREATE_PROPOSAL_PERMISSION_ID, + "[0] permissionId" + ); + + PermissionLib.MultiTargetPermission memory grantCreate = setupData.permissions[1]; + assertEq( + uint256(grantCreate.operation), + uint256(PermissionLib.Operation.GrantWithCondition), + "[1] operation" + ); + assertEq(grantCreate.where, fakePlugin, "[1] where"); + assertEq(grantCreate.who, ANY_ADDR, "[1] who"); + assertEq(grantCreate.condition, newCondition, "[1] condition"); + assertEq( + grantCreate.permissionId, + Permissions.CREATE_PROPOSAL_PERMISSION_ID, + "[1] permissionId" + ); + + PermissionLib.MultiTargetPermission memory revokeUpdateRules = setupData.permissions[2]; + assertEq( + uint256(revokeUpdateRules.operation), + uint256(PermissionLib.Operation.Revoke), + "[2] operation" + ); + assertEq(revokeUpdateRules.where, address(oldCondition), "[2] where"); + assertEq(revokeUpdateRules.who, address(dao), "[2] who"); + assertEq( + revokeUpdateRules.permissionId, + Permissions.UPDATE_RULES_PERMISSION_ID, + "[2] permissionId" + ); + + PermissionLib.MultiTargetPermission memory grantUpdateRules = setupData.permissions[3]; + assertEq( + uint256(grantUpdateRules.operation), + uint256(PermissionLib.Operation.Grant), + "[3] operation" + ); + assertEq(grantUpdateRules.where, newCondition, "[3] where"); + assertEq(grantUpdateRules.who, address(dao), "[3] who"); + assertEq( + grantUpdateRules.permissionId, + Permissions.UPDATE_RULES_PERMISSION_ID, + "[3] permissionId" + ); + } + + function test_WhenFromBuildIsOneAndRulesAreEmpty() external { + // it should still produce a valid update with an empty rules set on the new helper. - // reverts due to there is no build before current one. - vm.expectRevert( - abi.encodeWithSelector(PluginUpgradeableSetup.InvalidUpdatePath.selector, 0, 1) + SPPRuleCondition oldCondition = new SPPRuleCondition( + address(dao), + new RuledCondition.Rule[](0) ); - sppSetup.prepareUpdate( + + address[] memory currentHelpers = new address[](1); + currentHelpers[0] = address(oldCondition); + IPluginSetup.SetupPayload memory payload = IPluginSetup.SetupPayload({ + plugin: makeAddr("fakePlugin"), + currentHelpers: currentHelpers, + data: "" + }); + + (, IPluginSetup.PreparedSetupData memory setupData) = sppSetup.prepareUpdate( address(dao), - 0, - IPluginSetup.SetupPayload({ - plugin: address(0), - currentHelpers: new address[](0), - data: "" - }) + 1, + payload ); + + assertEq(SPPRuleCondition(setupData.helpers[0]).getRules().length, 0, "rules empty"); + } + + function _deployOldConditionWithRules() private returns (SPPRuleCondition oldCondition) { + RuledCondition.Rule[] memory rules = new RuledCondition.Rule[](2); + rules[0] = RuledCondition.Rule({ + id: 204, // VALUE_RULE_ID + op: 7, // RET + value: 1, + permissionId: Permissions.CREATE_PROPOSAL_PERMISSION_ID + }); + rules[1] = RuledCondition.Rule({ + id: 200, // BLOCK_NUMBER_RULE_ID + op: 5, // GTE + value: 100, + permissionId: Permissions.CREATE_PROPOSAL_PERMISSION_ID + }); + + oldCondition = new SPPRuleCondition(address(dao), rules); } } diff --git a/test/unit/stagedProposalProcessorSetup/concrete/prepareUpdate/prepareUpdate.tree b/test/unit/stagedProposalProcessorSetup/concrete/prepareUpdate/prepareUpdate.tree index c73fb1f..672c7a1 100644 --- a/test/unit/stagedProposalProcessorSetup/concrete/prepareUpdate/prepareUpdate.tree +++ b/test/unit/stagedProposalProcessorSetup/concrete/prepareUpdate/prepareUpdate.tree @@ -1,3 +1,11 @@ PrepareUpdate_SPPSetup_UnitTest -└── when preparing update - └── it should revert. \ No newline at end of file +├── when fromBuild is not 1 +│ └── it should revert with InvalidUpdatePath(fromBuild, 2). +└── when fromBuild is 1 + ├── it should deploy a new condition seeded with the existing rules + │ and return it as the single helper, with empty initData. + ├── it should migrate CREATE_PROPOSAL and UPDATE_RULES permissions + │ from the old condition to the new one (4 entries, in order: + │ Revoke create, GrantWithCondition create, Revoke update-rules, Grant update-rules). + └── when the old condition has no rules + └── it should still succeed and emit an empty rules set on the new helper.