Contract upgrade scripts (storyprotocol#241)

* oz upgrades annotations * output for storage layout checks * scripts for upgrades * docs * addressing comments
kingster-will · Sep 18, 2024 · 4c57a25 · 4c57a25
1 parent fed581a
commit 4c57a25
Show file tree

Hide file tree

Showing 11 changed files with 677 additions and 23 deletions.
diff --git a/UPGRADES.md b/UPGRADES.md
@@ -0,0 +1,213 @@
+# Upgrade process
+
+When performing an upgrade on EVM contracts, the biggest risk is to introduce storage layout collisions. This may degrade a contract storage, which can do things like change token balances, change a stored address to other contract for garbage data, etc.
+
+These errors might harder to detect, even happening some time after deployment.
+
+In order to prevent this:
+
+## 1. Make sure the implementations follow ERC7201
+
+check the project README.md for more info
+
+## 2. [Use OZ upgrades](https://docs.openzeppelin.com/upgrades-plugins/1.x/api-core) to test for storage layout incompatibility
+
+2.1. Get diff between tags to scope the changes, for example
+
+https://github.com/storyprotocol/protocol-core-v1/compare/v1.0.0...v1.1.0
+
+2.2. Keep a file with the versions of the old tag and dependencies, so the test compiles and oz upgrades can compare storage layouts.
+
+example:
+
+https://github.com/Ramarti/protocol-core-v1/blob/v1.1.0_upgrade_script/contracts/old/v1.0.0.sol
+
+This is largely a manual task, but there is a process and some commands that could help
+
+2.2.1 First, git clone the old tag in a folder inside the repo. Make sure it’s gitignored, you must delete later
+
+```jsx
+git clone --depth 1 --branch v1.0.0 git@github.com:storyprotocol/protocol-core-v1.git v1.0.0
+```
+
+2.2.2 Now we should find and rename the old contract names to reflect the version, like `DisputeModule_V1_0_0` 
+
+After this, there are 2 ways to make them compile
+
+a) Fix the stray absolute import path for relative ones.
+
+b) Flatten everything into a file. You can flatten the files with
+
+```jsx
+forge flatten contracts/old/v1.0.0/contracts/modules/dispute/DisputeModule.sol > contracts/old/DisputeModule.sol
+```
+
+then you can use the VSCode extension [Combine Code in Folder](https://marketplace.visualstudio.com/items?itemName=ToanBui.combine-code-in-folder) and manual labor.
+
+2.2.3 Add this tags to the newer implementations of the contracts, so the script can compare. For example
+
+```solidity
+/// @custom:oz-upgrades-from contracts/old/v1.0.0.sol:AccessController_V1_0_0
+contract AccessController is IAccessController, ProtocolPausableUpgradeable, UUPSUpgradeable {
+```
+
+2.2.4 Now when we run the tests, they will run the storage layout checker script and we will get a list with errors to correct.
+
+Note, some are going to be false positives, especially Solady and UpgradeableBeacon. Once we fix all of them, we may need to disable the verification so the tests can run
+
+## 3. Write a script to deploy the new contracts and implementations
+
+Inherit from UpgradedImplHelper to compile the upgrade structs that `_writeUpgradeProposals()` need to generate the output file
+Upgrading is a multi step process, we need to schedule first, then execute. Having an intermediary file helps the auditability
+of the process.
+
+Remember to use CREATE3 for new proxy contracts
+
+Example:
+
+```solidity
+contract DeployerV1_2 is JsonDeploymentHandler, BroadcastManager, UpgradedImplHelper {
+    ///
+
+    string constant PREV_VERSION = "v1.1.1";
+    string constant PROPOSAL_VERSION = "v1.2.0";
+
+
+    constructor() JsonDeploymentHandler("main") {
+        create3Deployer = ICreate3Deployer(CREATE3_DEPLOYER);
+    }
+
+    function run() public virtual {
+        _readDeployment(PREV_VERSION); // JsonDeploymentHandler.s.sol
+        // Load existing contracts
+        protocolAccessManager = AccessManager(_readAddress("ProtocolAccessManager"));
+        /// ...
+
+        _beginBroadcast(); // BroadcastManager.s.sol
+
+        UpgradeProposal[] memory proposals = deploy();
+        _writeUpgradeProposals(PREV_VERSION, PROPOSAL_VERSION, proposals); // JsonDeploymentHandler.s.sol
+
+        _endBroadcast(); // BroadcastManager.s.sol
+    }
+
+    function deploy() public returns (UpgradeProposal[] memory) {
+        string memory contractKey;
+        address impl;
+
+        // Deploy new contracts
+        _predeploy("RoyaltyPolicyLRP");
+        impl = address(new RoyaltyPolicyLRP(address(royaltyModule)));
+        royaltyPolicyLRP = RoyaltyPolicyLRP(
+            TestProxyHelper.deployUUPSProxy(
+                create3Deployer,
+                _getSalt(type(RoyaltyPolicyLRP).name),
+                impl,
+                abi.encodeCall(RoyaltyPolicyLRP.initialize, address(protocolAccessManager))
+            )
+        );
+        require(
+            _getDeployedAddress(type(RoyaltyPolicyLRP).name) == address(royaltyPolicyLRP),
+            "Deploy: Royalty Policy Address Mismatch"
+        );
+        require(_loadProxyImpl(address(royaltyPolicyLRP)) == impl, "RoyaltyPolicyLRP Proxy Implementation Mismatch");
+        impl = address(0);
+        _postdeploy("RoyaltyPolicyLRP", address(royaltyPolicyLRP));
+        
+        //...
+
+        // Deploy new implementations
+        contractKey = "LicenseToken";
+        _predeploy(contractKey);
+        impl = address(new LicenseToken(licensingModule, disputeModule));
+        upgradeProposals.push(UpgradeProposal({ key: contractKey, proxy: address(licenseToken), newImpl: impl }));
+        impl = address(0);
+
+        //...
+
+        _logUpgradeProposals();
+        
+        return upgradeProposals;
+    }
+}
+```
+
+For `IPRoyaltyVault`, set as proxy the address of the contract that is `IVaultController`
+
+Output will look something like:
+
+`deploy-out/upgrade-v1.1.1-to-v1.2.0-1513.json`
+```
+{
+  "main": {
+    "GroupingModule-NewImpl": "0xa1A9b2cBb4fFEeF7226Eaee9A5b71007bDCa721F",
+    "GroupingModule-Proxy": "0xeD1eF5749468B1805952757F53aB4C9037cD3ed6",
+    // ...
+  }
+}
+```
+
+## 4. Write contracts inheriting UpgradeExecutor
+
+`script/foundry/utils/upgrades/UpgradeExecutor.s.sol` has the logic to read the upgrade proposal file, and act on Access Manager
+
+```solidity
+/// @notice Upgrade modes
+enum UpgradeModes {
+     SCHEDULE, // Schedule upgrades in AccessManager
+    EXECUTE, // Execute scheduled upgrades
+    CANCEL // Cancel scheduled upgrades
+}
+/// @notice End result of the script
+enum Output {
+    TX_EXECUTION, // One Tx per operation
+    BATCH_TX_EXECUTION, // Use AccessManager to batch actions in 1 tx through (multicall)
+    BATCH_TX_JSON // Prepare raw bytes for multisig. Multisig may batch txs (e.g. Gnosis Safe JSON input in tx builder)
+}
+```
+
+Example of concrete version upgrade (depending on the mode, one of the xxxUpgrades() methods will be called)
+
+```solidity
+contract ExecuteV1_2 is UpgradeExecutor {
+    
+    constructor() UpgradeExecutor(
+        "v1.1.1", // From version
+        "v1.2.0", // To version
+        UpgradeModes.EXECUTE, // Schedule, Cancel or Execute upgrade
+        Output.BATCH_TX_EXECUTION // Output mode
+    ) {}
+
+    function _scheduleUpgrades() internal virtual override {
+        console2.log("Scheduling upgrades  -------------");
+        _scheduleUpgrade("GroupingModule");
+        /...
+    }
+
+    function _executeUpgrades() internal virtual override {
+        console2.log("Executing upgrades  -------------");
+        _executeUpgrade("IpRoyaltyVault");
+        /...
+    }
+
+    function _cancelScheduledUpgrades() internal virtual override {
+        console2.log("Cancelling upgrades  -------------");
+        _cancelScheduledUpgrade("GroupingModule");
+        /...
+    }
+}
+```
+
+## 5. Execute the scripts
+
+Script name will deppend on your file names. For example:
+
+Deployment (remember to verify)
+```
+forge script script/foundry/deployment/upgrades/DeployerV1_2.s.sol --fork-url https://testnet.storyrpc.io --broadcast --verify --verifier blockscout --verifier-url https://testnet.storyscan.xyz/api\?
+```
+
+Executing the transaction
+```
+forge script script/foundry/deployment/upgrades/ExecuteV1_2.s.sol --fork-url https://testnet.storyrpc.io --broadcast
+```
diff --git a/contracts/IPAccountImpl.sol b/contracts/IPAccountImpl.sol
@@ -18,7 +18,10 @@ import { IPAccountStorage } from "./IPAccountStorage.sol";
 
 /// @title IPAccountImpl
 /// @notice The Story Protocol's implementation of the IPAccount.
+/// @dev This impl is not part of an upgradeable proxy/impl setup. We are
+/// adding OZ annotations to avoid false positives when running oz-foundry-upgrades
 contract IPAccountImpl is ERC6551, IPAccountStorage, IIPAccount {
+    /// @custom:oz-upgrades-unsafe-allow state-variable-immutable
     address public immutable ACCESS_CONTROLLER;
 
     receive() external payable override(Receiver, IIPAccount) {}
@@ -29,6 +32,7 @@ contract IPAccountImpl is ERC6551, IPAccountStorage, IIPAccount {
     /// This means that each cloned IPAccount will inherently use the same AccessController
     /// without the need for individual configuration.
     /// @param accessController The address of the AccessController contract to be used for permission checks
+    /// @custom:oz-upgrades-unsafe-allow constructor
     constructor(
         address accessController,
         address ipAssetRegistry,

diff --git a/contracts/IPAccountStorage.sol b/contracts/IPAccountStorage.sol
@@ -12,11 +12,15 @@ import { ShortStrings } from "@openzeppelin/contracts/utils/ShortStrings.sol";
 /// This contract allows Modules to store and retrieve data in a structured and conflict-free manner
 /// by utilizing namespaces, where the default namespace is determined by the
 /// `msg.sender` (the caller Module's address).
+/// This impl is not part of an upgradeable proxy/impl setup. We are
+/// adding OZ annotations to avoid false positives when running oz-foundry-upgrades
 contract IPAccountStorage is ERC165, IIPAccountStorage {
     using ShortStrings for *;
-
+    /// @custom:oz-upgrades-unsafe-allow state-variable-immutable
     address public immutable MODULE_REGISTRY;
+    /// @custom:oz-upgrades-unsafe-allow state-variable-immutable
     address public immutable LICENSE_REGISTRY;
+    /// @custom:oz-upgrades-unsafe-allow state-variable-immutable
     address public immutable IP_ASSET_REGISTRY;
 
     mapping(bytes32 => mapping(bytes32 => bytes)) public bytesData;
@@ -33,6 +37,7 @@ contract IPAccountStorage is ERC165, IIPAccountStorage {
         _;
     }
 
+    /// @custom:oz-upgrades-unsafe-allow constructor
     constructor(address ipAssetRegistry, address licenseRegistry, address moduleRegistry) {
         if (ipAssetRegistry == address(0)) revert Errors.IPAccountStorage__ZeroIpAssetRegistry();
         if (licenseRegistry == address(0)) revert Errors.IPAccountStorage__ZeroLicenseRegistry();

diff --git a/foundry.toml b/foundry.toml
@@ -9,6 +9,10 @@ optimizer_runs = 20000
 test = 'test'
 solc = '0.8.26'
 fs_permissions = [{ access = 'read-write', path = './deploy-out' }, { access = 'read', path = './out' }]
+##### Uncomment if running storage layout verification
+# ffi = true
+# ast = true
+# build_info = true
 extra_output = ["storageLayout"]
 evm_version = "cancun"
 

diff --git a/script/foundry/deployment/Main.s.sol b/script/foundry/deployment/Main.s.sol
@@ -16,6 +16,7 @@ contract Main is DeployHelper {
     address internal constant TREASURY_ADDRESS = address(200);
     // For royalty policy
     uint256 internal constant MAX_ROYALTY_APPROVAL = 10000 ether;
+    string internal constant VERSION = "v1.2";
 
     constructor()
         DeployHelper(
@@ -44,8 +45,8 @@ contract Main is DeployHelper {
         super.run(
             seed, // create3 seed
             false, // runStorageLayoutCheck
-            true // writeDeployments
+            true // writeDeployments,
         );
-        _writeDeployment(); // write deployment json to deployments/deployment-{chainId}.json
+        _writeDeployment(VERSION); // write deployment json to deployments/deployment-{chainId}.json
     }
 }
diff --git a/script/foundry/deployment/MockAssets.s.sol b/script/foundry/deployment/MockAssets.s.sol
@@ -26,7 +26,7 @@ contract MockAssets is Script, BroadcastManager, JsonDeploymentHandler {
 
         _deployProtocolContracts();
 
-        _writeDeployment(); // write deployment json to deploy-out/deployment-{chainId}.json
+        _writeDeployment("mock"); // write deployment json to deploy-out/deployment-{chainId}.json
         _endBroadcast(); // BroadcastManager.s.sol
     }
 

diff --git a/script/foundry/utils/DeployHelper.sol b/script/foundry/utils/DeployHelper.sol
@@ -123,6 +123,8 @@ contract DeployHelper is Script, BroadcastManager, JsonDeploymentHandler, Storag
     // DeployHelper variable
     bool private writeDeploys;
 
+    string private version;
+
     constructor(
         address erc6551Registry_,
         address create3Deployer_,
@@ -179,7 +181,7 @@ contract DeployHelper is Script, BroadcastManager, JsonDeploymentHandler, Storag
             revert RoleConfigError("Multisig roles not granted");
         }
 
-        if (writeDeploys) _writeDeployment();
+        if (writeDeploys) _writeDeployment(version);
         _endBroadcast(); // BroadcastManager.s.sol
     }
 
@@ -738,7 +740,6 @@ contract DeployHelper is Script, BroadcastManager, JsonDeploymentHandler, Storag
             ProtocolAdmin.UPGRADER_ROLE
         );
         protocolAccessManager.setTargetFunctionRole(address(licensingModule), selectors, ProtocolAdmin.UPGRADER_ROLE);
-        protocolAccessManager.setTargetFunctionRole(address(royaltyModule), selectors, ProtocolAdmin.UPGRADER_ROLE);
         protocolAccessManager.setTargetFunctionRole(address(royaltyPolicyLAP), selectors, ProtocolAdmin.UPGRADER_ROLE);
         protocolAccessManager.setTargetFunctionRole(address(royaltyPolicyLRP), selectors, ProtocolAdmin.UPGRADER_ROLE);
         protocolAccessManager.setTargetFunctionRole(address(licenseRegistry), selectors, ProtocolAdmin.UPGRADER_ROLE);

diff --git a/script/foundry/utils/JsonBatchTxHelper.s.sol b/script/foundry/utils/JsonBatchTxHelper.s.sol
@@ -0,0 +1,61 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.23;
+
+import { Script } from "forge-std/Script.sol";
+import { stdJson } from "forge-std/StdJson.sol";
+import { console2 } from "forge-std/console2.sol";
+
+import { StringUtil } from "./StringUtil.sol";
+
+contract JsonBatchTxHelper is Script {
+    using StringUtil for uint256;
+    using stdJson for string;
+
+    struct Transaction {
+        address to;
+        uint256 value;
+        bytes data;
+        uint8 operation;
+    }
+
+    Transaction[] private transactions;
+    string private chainId;
+
+    constructor() {
+        chainId = (block.chainid).toString();
+    }
+
+    function _writeTx(address _to, uint256 _value, bytes memory _data) internal {
+        transactions.push(Transaction({
+            to: _to,
+            value: _value,
+            data: _data,
+            operation: 0
+        }));
+        console2.log("Added tx to ", _to);
+        console2.log("Value: ", _value);
+        console2.log("Data: ");
+        console2.logBytes(_data);
+        console2.log("Operation: 0");
+    }
+
+    function _writeBatchTxsOutput(string memory _action) internal {
+        string memory json = "[";
+        for (uint i = 0; i < transactions.length; i++) {
+            if (i > 0) {
+                json = string(abi.encodePacked(json, ","));
+            }
+            json = string(abi.encodePacked(json, "{"));
+            json = string(abi.encodePacked(json, '"to":"', vm.toString(transactions[i].to), '",'));
+            json = string(abi.encodePacked(json, '"value":', vm.toString(transactions[i].value), ','));
+            json = string(abi.encodePacked(json, '"data":"', vm.toString(transactions[i].data), '",'));
+            json = string(abi.encodePacked(json, '"operation":', vm.toString(transactions[i].operation)));
+            json = string(abi.encodePacked(json, "}"));
+        }
+        json = string(abi.encodePacked(json, "]"));
+
+        string memory filename = string(abi.encodePacked("./deploy-out/", _action, "-", chainId, ".json"));
+        vm.writeFile(filename, json);
+        console2.log("Wrote batch txs to ", filename);
+    }
+}