diff --git a/service_contracts/abi/ServiceProviderRegistry.abi.json b/service_contracts/abi/ServiceProviderRegistry.abi.json index b605b3f4..a903bf5e 100644 --- a/service_contracts/abi/ServiceProviderRegistry.abi.json +++ b/service_contracts/abi/ServiceProviderRegistry.abi.json @@ -156,6 +156,31 @@ ], "stateMutability": "view" }, + { + "type": "function", + "name": "announcePlannedUpgrade", + "inputs": [ + { + "name": "plannedUpgrade", + "type": "tuple", + "internalType": "struct ServiceProviderRegistry.PlannedUpgrade", + "components": [ + { + "name": "nextImplementation", + "type": "address", + "internalType": "address" + }, + { + "name": "afterEpoch", + "type": "uint96", + "internalType": "uint96" + } + ] + } + ], + "outputs": [], + "stateMutability": "nonpayable" + }, { "type": "function", "name": "eip712Domain", @@ -794,6 +819,24 @@ "outputs": [], "stateMutability": "nonpayable" }, + { + "type": "function", + "name": "nextUpgrade", + "inputs": [], + "outputs": [ + { + "name": "nextImplementation", + "type": "address", + "internalType": "address" + }, + { + "name": "afterEpoch", + "type": "uint96", + "internalType": "uint96" + } + ], + "stateMutability": "view" + }, { "type": "function", "name": "owner", @@ -1304,6 +1347,31 @@ ], "anonymous": false }, + { + "type": "event", + "name": "UpgradeAnnounced", + "inputs": [ + { + "name": "plannedUpgrade", + "type": "tuple", + "indexed": false, + "internalType": "struct ServiceProviderRegistry.PlannedUpgrade", + "components": [ + { + "name": "nextImplementation", + "type": "address", + "internalType": "address" + }, + { + "name": "afterEpoch", + "type": "uint96", + "internalType": "uint96" + } + ] + } + ], + "anonymous": false + }, { "type": "event", "name": "Upgraded", diff --git a/service_contracts/src/ServiceProviderRegistry.sol b/service_contracts/src/ServiceProviderRegistry.sol index a8b03baa..c52db78e 100644 --- a/service_contracts/src/ServiceProviderRegistry.sol +++ b/service_contracts/src/ServiceProviderRegistry.sol @@ -102,6 +102,18 @@ contract ServiceProviderRegistry is /// @notice Emitted when the contract is upgraded event ContractUpgraded(string version, address implementation); + // Used for announcing upgrades, packed into one slot + struct PlannedUpgrade { + // Address of the new implementation contract + address nextImplementation; + // Upgrade will not occur until at least this epoch + uint96 afterEpoch; + } + + PlannedUpgrade public nextUpgrade; + + event UpgradeAnnounced(PlannedUpgrade plannedUpgrade); + /// @notice Ensures the caller is the service provider modifier onlyServiceProvider(uint256 providerId) { require(providers[providerId].serviceProvider == msg.sender, "Only service provider can call this function"); @@ -752,18 +764,32 @@ contract ServiceProviderRegistry is } } + /// @notice Announce a planned upgrade + /// @dev Can only be called by the contract owner + /// @param plannedUpgrade The planned upgrade details + function announcePlannedUpgrade(PlannedUpgrade calldata plannedUpgrade) external onlyOwner { + require(plannedUpgrade.nextImplementation.code.length > 3000); + require(plannedUpgrade.afterEpoch > block.number); + nextUpgrade = plannedUpgrade; + emit UpgradeAnnounced(plannedUpgrade); + } + /// @notice Authorizes an upgrade to a new implementation /// @dev Can only be called by the contract owner + /// @dev Supports both one-step (legacy) and two-step (announcePlannedUpgrade) upgrade mechanisms /// @param newImplementation Address of the new implementation contract function _authorizeUpgrade(address newImplementation) internal override onlyOwner { - // Authorization logic is handled by the onlyOwner modifier + // zero address already checked by ERC1967Utils._setImplementation + require(newImplementation == nextUpgrade.nextImplementation); + require(block.number >= nextUpgrade.afterEpoch); + delete nextUpgrade; } /// @notice Migration function for contract upgrades /// @dev This function should be called during upgrades to emit version tracking events + /// Only callable during proxy upgrade process /// @param newVersion The version string for the new implementation - function migrate(string memory newVersion) public onlyProxy reinitializer(2) { - require(msg.sender == address(this), "Only self can call migrate"); + function migrate(string memory newVersion) public onlyProxy onlyOwner reinitializer(2) { emit ContractUpgraded(newVersion, ERC1967Utils.getImplementation()); } } diff --git a/service_contracts/test/ServiceProviderRegistry.t.sol b/service_contracts/test/ServiceProviderRegistry.t.sol index 5db937c1..a16da2d8 100644 --- a/service_contracts/test/ServiceProviderRegistry.t.sol +++ b/service_contracts/test/ServiceProviderRegistry.t.sol @@ -2,6 +2,7 @@ pragma solidity ^0.8.20; import {MockFVMTest} from "@fvm-solidity/mocks/MockFVMTest.sol"; +import {Vm} from "forge-std/Test.sol"; import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; @@ -52,6 +53,99 @@ contract ServiceProviderRegistryTest is MockFVMTest { registry.initialize(); } + function testAnnouncePlannedUpgrade() public { + // Initially, no upgrade is planned + (address nextImplementation, uint96 afterEpoch) = registry.nextUpgrade(); + assertEq(nextImplementation, address(0)); + assertEq(afterEpoch, uint96(0)); + + // Deploy new implementation + ServiceProviderRegistry newImplementation = new ServiceProviderRegistry(); + + // Announce upgrade + ServiceProviderRegistry.PlannedUpgrade memory plan; + plan.nextImplementation = address(newImplementation); + plan.afterEpoch = uint96(vm.getBlockNumber()) + 2000; + + vm.expectEmit(false, false, false, true); + emit ServiceProviderRegistry.UpgradeAnnounced(plan); + registry.announcePlannedUpgrade(plan); + + // Verify upgrade plan is stored + (nextImplementation, afterEpoch) = registry.nextUpgrade(); + assertEq(nextImplementation, plan.nextImplementation); + assertEq(afterEpoch, plan.afterEpoch); + + // Cannot upgrade before afterEpoch + bytes memory migrateData = + abi.encodeWithSelector(ServiceProviderRegistry.migrate.selector, newImplementation.VERSION()); + vm.expectRevert(); + registry.upgradeToAndCall(plan.nextImplementation, migrateData); + + // Still cannot upgrade at afterEpoch - 1 + vm.roll(plan.afterEpoch - 1); + vm.expectRevert(); + registry.upgradeToAndCall(plan.nextImplementation, migrateData); + + // Can upgrade at afterEpoch + vm.roll(plan.afterEpoch); + // Note: reinitializer(2) emits Initialized event first, then ContractUpgraded + // We use recordLogs to capture all events and verify ContractUpgraded is present + vm.recordLogs(); + registry.upgradeToAndCall(plan.nextImplementation, migrateData); + + // Verify ContractUpgraded event was emitted + Vm.Log[] memory logs = vm.getRecordedLogs(); + bytes32 expectedTopic = keccak256("ContractUpgraded(string,address)"); + bool foundEvent = false; + for (uint256 i = 0; i < logs.length; i++) { + if (logs[i].topics[0] == expectedTopic) { + (string memory version, address impl) = abi.decode(logs[i].data, (string, address)); + assertEq(version, newImplementation.VERSION(), "Version should match"); + assertEq(impl, plan.nextImplementation, "Implementation should match"); + foundEvent = true; + break; + } + } + assertTrue(foundEvent, "ContractUpgraded event should be emitted"); + + // After upgrade, nextUpgrade should be cleared + (nextImplementation, afterEpoch) = registry.nextUpgrade(); + assertEq(nextImplementation, address(0)); + assertEq(afterEpoch, uint96(0)); + } + + function testAnnouncePlannedUpgradeOnlyOwner() public { + ServiceProviderRegistry newImplementation = new ServiceProviderRegistry(); + ServiceProviderRegistry.PlannedUpgrade memory plan; + plan.nextImplementation = address(newImplementation); + plan.afterEpoch = uint96(vm.getBlockNumber()) + 2000; + + // Non-owner cannot announce upgrade + vm.prank(user1); + vm.expectRevert(); + registry.announcePlannedUpgrade(plan); + } + + function testAnnouncePlannedUpgradeInvalidImplementation() public { + ServiceProviderRegistry.PlannedUpgrade memory plan; + plan.nextImplementation = address(0x123); // Invalid address with no code + plan.afterEpoch = uint96(vm.getBlockNumber()) + 2000; + + vm.expectRevert(); + registry.announcePlannedUpgrade(plan); + } + + function testAnnouncePlannedUpgradeInvalidEpoch() public { + ServiceProviderRegistry newImplementation = new ServiceProviderRegistry(); + ServiceProviderRegistry.PlannedUpgrade memory plan; + plan.nextImplementation = address(newImplementation); + plan.afterEpoch = uint96(vm.getBlockNumber()); // Must be in the future + + vm.expectRevert(); + registry.announcePlannedUpgrade(plan); + } + function testIsRegisteredProviderReturnsFalse() public view { // Should return false for unregistered addresses assertFalse(registry.isRegisteredProvider(user1), "Should return false for unregistered address"); @@ -241,13 +335,22 @@ contract ServiceProviderRegistryTest is MockFVMTest { // Deploy new implementation ServiceProviderRegistry newImplementation = new ServiceProviderRegistry(); - // Non-owner cannot upgrade + // Non-owner cannot upgrade (will fail in _authorizeUpgrade due to onlyOwner) vm.prank(user1); vm.expectRevert(); registry.upgradeToAndCall(address(newImplementation), ""); - // Owner can upgrade - registry.upgradeToAndCall(address(newImplementation), ""); + // Owner can upgrade (but needs to announce first or it will fail in _authorizeUpgrade) + // Since we're testing the onlyOwner check, we need to announce the upgrade first + ServiceProviderRegistry.PlannedUpgrade memory plan; + plan.nextImplementation = address(newImplementation); + plan.afterEpoch = uint96(vm.getBlockNumber()) + 1; + registry.announcePlannedUpgrade(plan); + + vm.roll(plan.afterEpoch); + bytes memory migrateData = + abi.encodeWithSelector(ServiceProviderRegistry.migrate.selector, newImplementation.VERSION()); + registry.upgradeToAndCall(address(newImplementation), migrateData); } function testTransferOwnership() public { diff --git a/service_contracts/tools/announce-planned-upgrade-registry.sh b/service_contracts/tools/announce-planned-upgrade-registry.sh new file mode 100755 index 00000000..a6bd5d93 --- /dev/null +++ b/service_contracts/tools/announce-planned-upgrade-registry.sh @@ -0,0 +1,77 @@ +#!/bin/bash + +# announce-planned-upgrade-registry.sh: Announces a planned upgrade for ServiceProviderRegistry +# Required args: ETH_RPC_URL, REGISTRY_PROXY_ADDRESS, ETH_KEYSTORE, PASSWORD, NEW_REGISTRY_IMPLEMENTATION_ADDRESS, AFTER_EPOCH + +if [ -z "$ETH_RPC_URL" ]; then + echo "Error: ETH_RPC_URL is not set" + exit 1 +fi + +if [ -z "$ETH_KEYSTORE" ]; then + echo "Error: ETH_KEYSTORE is not set" + exit 1 +fi + +if [ -z "$PASSWORD" ]; then + echo "Error: PASSWORD is not set" + exit 1 +fi + +if [ -z "$CHAIN" ]; then + CHAIN=$(cast chain-id) + if [ -z "$CHAIN" ]; then + echo "Error: Failed to detect chain ID from RPC" + exit 1 + fi +fi + +if [ -z "$NEW_REGISTRY_IMPLEMENTATION_ADDRESS" ]; then + echo "NEW_REGISTRY_IMPLEMENTATION_ADDRESS is not set" + exit 1 +fi + +if [ -z "$AFTER_EPOCH" ]; then + echo "AFTER_EPOCH is not set" + exit 1 +fi + +CURRENT_EPOCH=$(cast block-number 2>/dev/null) + +if [ "$CURRENT_EPOCH" -gt "$AFTER_EPOCH" ]; then + echo "Already past AFTER_EPOCH ($CURRENT_EPOCH > $AFTER_EPOCH)" + exit 1 +else + echo "Announcing planned upgrade after $(($AFTER_EPOCH - $CURRENT_EPOCH)) epochs" +fi + + +ADDR=$(cast wallet address --password "$PASSWORD") +echo "Sending announcement from owner address: $ADDR" + +# Get current nonce +NONCE=$(cast nonce "$ADDR") + +if [ -z "$REGISTRY_PROXY_ADDRESS" ]; then + echo "Error: REGISTRY_PROXY_ADDRESS is not set" + exit 1 +fi + +PROXY_OWNER=$(cast call -f 0x0000000000000000000000000000000000000000 "$REGISTRY_PROXY_ADDRESS" "owner()(address)" 2>/dev/null) +if [ "$PROXY_OWNER" != "$ADDR" ]; then + echo "Supplied ETH_KEYSTORE ($ADDR) is not the proxy owner ($PROXY_OWNER)." + exit 1 +fi + +TX_HASH=$(cast send "$REGISTRY_PROXY_ADDRESS" "announcePlannedUpgrade((address,uint96))" "($NEW_REGISTRY_IMPLEMENTATION_ADDRESS,$AFTER_EPOCH)" \ + --password "$PASSWORD" \ + --nonce "$NONCE" \ + --json | jq -r '.transactionHash') + +if [ -z "$TX_HASH" ]; then + echo "Error: Failed to send announcePlannedUpgrade transaction" + exit 1 +fi + +echo "announcePlannedUpgrade transaction sent: $TX_HASH" + diff --git a/service_contracts/tools/announce-planned-upgrade.sh b/service_contracts/tools/announce-planned-upgrade.sh index cd87555d..5ee6f9b4 100755 --- a/service_contracts/tools/announce-planned-upgrade.sh +++ b/service_contracts/tools/announce-planned-upgrade.sh @@ -70,6 +70,7 @@ TX_HASH=$(cast send "$WARM_STORAGE_PROXY_ADDRESS" "announcePlannedUpgrade((addre if [ -z "$TX_HASH" ]; then echo "Error: Failed to send announcePlannedUpgrade transaction" + exit 1 fi echo "announcePlannedUpgrade transaction sent: $TX_HASH" diff --git a/service_contracts/tools/upgrade-registry.sh b/service_contracts/tools/upgrade-registry.sh new file mode 100755 index 00000000..f891d764 --- /dev/null +++ b/service_contracts/tools/upgrade-registry.sh @@ -0,0 +1,140 @@ +#!/bin/bash + +# upgrade-registry.sh: Completes a pending upgrade for ServiceProviderRegistry +# Required args: ETH_RPC_URL, REGISTRY_PROXY_ADDRESS, ETH_KEYSTORE, PASSWORD, NEW_REGISTRY_IMPLEMENTATION_ADDRESS +# Optional args: NEW_VERSION +# Calculated if unset: CHAIN + +if [ -z "$ETH_RPC_URL" ]; then + echo "Error: ETH_RPC_URL is not set" + exit 1 +fi + +if [ -z "$ETH_KEYSTORE" ]; then + echo "Error: ETH_KEYSTORE is not set" + exit 1 +fi + +if [ -z "$PASSWORD" ]; then + echo "Error: PASSWORD is not set" + exit 1 +fi + +if [ -z "$CHAIN" ]; then + CHAIN=$(cast chain-id) + if [ -z "$CHAIN" ]; then + echo "Error: Failed to detect chain ID from RPC" + exit 1 + fi +fi + +ADDR=$(cast wallet address --password "$PASSWORD") +echo "Using owner address: $ADDR" + +# Get current nonce +NONCE=$(cast nonce "$ADDR") + +if [ -z "$REGISTRY_PROXY_ADDRESS" ]; then + echo "Error: REGISTRY_PROXY_ADDRESS is not set" + exit 1 +fi + +PROXY_OWNER=$(cast call -f 0x0000000000000000000000000000000000000000 "$REGISTRY_PROXY_ADDRESS" "owner()(address)" 2>/dev/null) +if [ "$PROXY_OWNER" != "$ADDR" ]; then + echo "Supplied ETH_KEYSTORE ($ADDR) is not the proxy owner ($PROXY_OWNER)." + exit 1 +fi + +# Get the upgrade plan (if any) +# Try to call nextUpgrade() - this will fail if the method doesn't exist (old contracts) +UPGRADE_PLAN_OUTPUT=$(cast call -f 0x0000000000000000000000000000000000000000 "$REGISTRY_PROXY_ADDRESS" "nextUpgrade()(address,uint96)" 2>&1) +CAST_CALL_EXIT_CODE=$? + +ZERO_ADDRESS="0x0000000000000000000000000000000000000000" + +# Check if cast call succeeded (method exists) +if [ $CAST_CALL_EXIT_CODE -eq 0 ] && [ -n "$UPGRADE_PLAN_OUTPUT" ]; then + # Method exists - parse the result + UPGRADE_PLAN=($UPGRADE_PLAN_OUTPUT) + PLANNED_REGISTRY_IMPLEMENTATION_ADDRESS=${UPGRADE_PLAN[0]} + AFTER_EPOCH=${UPGRADE_PLAN[1]} + + # Check if there's a planned upgrade (non-zero address) + # Zero address means either no upgrade was announced or the upgrade was already completed + if [ -n "$PLANNED_REGISTRY_IMPLEMENTATION_ADDRESS" ] && [ "$PLANNED_REGISTRY_IMPLEMENTATION_ADDRESS" != "$ZERO_ADDRESS" ]; then + # New two-step mechanism: validate planned upgrade + echo "Detected planned upgrade (two-step mechanism)" + + if [ "$PLANNED_REGISTRY_IMPLEMENTATION_ADDRESS" != "$NEW_REGISTRY_IMPLEMENTATION_ADDRESS" ]; then + echo "NEW_REGISTRY_IMPLEMENTATION_ADDRESS ($NEW_REGISTRY_IMPLEMENTATION_ADDRESS) != planned ($PLANNED_REGISTRY_IMPLEMENTATION_ADDRESS)" + exit 1 + else + echo "Upgrade plan matches ($NEW_REGISTRY_IMPLEMENTATION_ADDRESS)" + fi + + CURRENT_EPOCH=$(cast block-number 2>/dev/null) + + if [ "$CURRENT_EPOCH" -lt "$AFTER_EPOCH" ]; then + echo "Not time yet ($CURRENT_EPOCH < $AFTER_EPOCH)" + exit 1 + else + echo "Upgrade ready ($CURRENT_EPOCH >= $AFTER_EPOCH)" + fi + else + # Method exists but returns zero - no planned upgrade or already completed + # On new contracts, _authorizeUpgrade requires a planned upgrade, so one-step will fail + echo "No planned upgrade detected (nextUpgrade returns zero)" + echo "Error: This contract requires a planned upgrade. Please call announce-planned-upgrade-registry.sh first." + exit 1 + fi +else + # Method doesn't exist (old contract without nextUpgrade) or call failed + echo "nextUpgrade() method not found or call failed, using one-step mechanism (direct upgrade)" + echo "WARNING: This is the legacy upgrade path. For new deployments, use announce-planned-upgrade-registry.sh first." +fi + +if [ -n "$NEW_VERSION" ]; then + echo "Using provided version: $NEW_VERSION" + MIGRATE_DATA=$(cast calldata "migrate(string)" "$NEW_VERSION") +else + echo "Warning: NEW_VERSION is not set. Using empty string for version." + MIGRATE_DATA=$(cast calldata "migrate(string)" "") +fi + +# Call upgradeToAndCall on the proxy with migrate function +echo "Upgrading proxy and calling migrate..." +TX_HASH=$(cast send "$REGISTRY_PROXY_ADDRESS" "upgradeToAndCall(address,bytes)" "$NEW_REGISTRY_IMPLEMENTATION_ADDRESS" "$MIGRATE_DATA" \ + --password "$PASSWORD" \ + --nonce "$NONCE" \ + --json | jq -r '.transactionHash') + +if [ -z "$TX_HASH" ]; then + echo "Error: Failed to send upgrade transaction" + echo "The transaction may have failed due to:" + echo "- Insufficient permissions (not owner)" + echo "- Proxy is paused or locked" + echo "- Implementation address is invalid" + exit 1 +fi + +echo "Upgrade transaction sent: $TX_HASH" +echo "Waiting for confirmation..." + +# Wait for transaction receipt +cast receipt "$TX_HASH" --confirmations 1 > /dev/null + +# Verify the upgrade by checking the implementation address +echo "Verifying upgrade..." +NEW_IMPL=$(cast rpc eth_getStorageAt "$REGISTRY_PROXY_ADDRESS" 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc latest | sed 's/"//g' | sed 's/0x000000000000000000000000/0x/') + +# Compare to lowercase +export EXPECTED_IMPL=$(echo $NEW_REGISTRY_IMPLEMENTATION_ADDRESS | tr '[:upper:]' '[:lower:]') + +if [ "$NEW_IMPL" = "$EXPECTED_IMPL" ]; then + echo "✅ Upgrade successful! Proxy now points to: $NEW_REGISTRY_IMPLEMENTATION_ADDRESS" +else + echo "⚠️ Warning: Could not verify upgrade. Please check manually." + echo "Expected: $NEW_REGISTRY_IMPLEMENTATION_ADDRESS" + echo "Got: $NEW_IMPL" +fi +