feat: announcePlannedUpgrade #260

wjmelements · 2025-10-04T05:10:33Z

Reviewer @rvagg
Per a discussion with @jennijuju, we want to make the upgrade process more transparent.
This changes upgrades to a two step process.
First, we announce the planned upgrade.
Then, after the designated time, we can complete the upgrade.
The delay gives us time to check the pending implementation address. It also gives users notice even if we are not in communication with them.
I also fix #259 in this change, because I was already adding tests for upgradeToAndCall. I can fix it separately if this change is rejected.
After validating that this is a good design that the team likes, I can replicate it for the other proxies in separate PRs.

Test Plan

I tested the new deployment scripts by running them on calibnet.

Changes

announcePlannedUpgrade
update _authorizeUpgrade
new view method for pending upgrade
make gen
add and fix tests covering upgradeToAndCall with migrate
separate announce and upgrade scripts
update implementation-only script

wjmelements · 2025-10-04T05:17:52Z

service_contracts/src/FilecoinWarmStorageService.sol

-    function _authorizeUpgrade(address newImplementation) internal override onlyOwner {}
+    function announcePlannedUpgrade(PlannedUpgrade calldata plannedUpgrade) external onlyOwner {
+        require(plannedUpgrade.nextImplementation.code.length > 3000);
+        require(plannedUpgrade.afterEpoch > block.number);


We could also require a minimum delay, but in the event of an emergency we might want a shorter delay. I can add it if someone else wants it. Mainly if we discovered a problem right after a deployment a mandatory delay would become mandatory downtime.

Such a minimum delay would help facilitate security by allowing time to check, for example, that the bytecode matches the source code. It would enforce transparency by mandating a notice period for service providers, who can watch for the UpgradeAnnounced event log.

However such a delay could also just be enforced off-chain by best-practices. This is why I don't enforce it in the code.

wjmelements · 2025-10-04T05:18:18Z

service_contracts/src/FilecoinWarmStorageService.sol

     * @param _viewContract Address of the view contract (optional, can be address(0))
     */
-    function migrate(address _viewContract) public onlyProxy reinitializer(4) {
-        require(msg.sender == address(this), Errors.OnlySelf(address(this), msg.sender));


This is the fix for #259

wjmelements · 2025-10-04T05:18:56Z

After validating that this is a good design, I can replicate it for the other proxies in separate PRs.

wjmelements · 2025-10-04T05:20:25Z

service_contracts/test/FilecoinWarmStorageService.t.sol

    }
-
-    // Event declaration for testing (must match the contract's event)
-    event ContractUpgraded(string version, address implementation);


this is unused, and you can match the contract more easily by referring to the original, eg FilecoinWarmStorageService.ContractUpgraded.

wjmelements · 2025-10-04T05:22:07Z

service_contracts/tools/upgrade.sh

+export EXPECTED_IMPL=$(echo $NEW_WARM_STORAGE_IMPLEMENTATION_ADDRESS | tr '[:upper:]' '[:lower:]')
+
+if [ "$NEW_IMPL" = "$EXPECTED_IMPL" ]; then
+    echo "✅ Upgrade successful! Proxy now points to: $NEW_WARM_STORAGE_IMPLEMENTATION_ADDRESS"


output when I ran this locally:

Warning: NEW_WARM_STORAGE_VIEW_ADDRESS is not set. Keeping previous view contract. Using owner address: 0x3B559cf662fdFe485eC216C036D4Cee91FBd1D39 Upgrade plan matches (0x9D4af62664A69db9c21eFCdf9fb43029D43A7f68) Upgrade ready (3074256 > 3074254) Keeping previous view contract address (0x85DB31F04dE9625b94b80bA20F1041f1A301D024) Upgrading proxy and calling migrate... Upgrade transaction sent: 0xbfb790ed434d1a1be7cc85fdab9cba40f00f1fbd9a4c0c10a39886cba74c70cc Waiting for confirmation... Verifying upgrade... ✅ Upgrade successful! Proxy now points to: 0x9D4af62664A69db9c21eFCdf9fb43029D43A7f68

wjmelements · 2025-10-04T05:23:01Z

service_contracts/tools/announce-planned-upgrade.sh

+  echo "Error: Failed to send announcePlannedUpgrade transaction"
+fi
+
+echo "announcePlannedUpgrade transaction sent: $TX_HASH"


successful output log

Announcing planned upgrade after 4 epochs Sending announcement from owner address: 0x3B559cf662fdFe485eC216C036D4Cee91FBd1D39 announcePlannedUpgrade transaction sent: 0xee4f2cb731c4102454a57356d86df2cd4bc3ab3a8c8bc229d8126fdb279fe70c

wjmelements · 2025-10-04T05:25:49Z

service_contracts/src/FilecoinWarmStorageService.sol

+    }
+
+    function _authorizeUpgrade(address newImplementation) internal override onlyOwner {
+        // zero address already checked by ERC1967Utils._setImplementation


I also test for this case in testUpgrade.

(address nextImplementation, uint96 afterEpoch) = viewContract.nextUpgrade(); assertEq(nextImplementation, address(0)); assertEq(afterEpoch, uint96(0)); // Do not allow upgrade to zero address even if it's the nextImplementation vm.expectRevert(); service.upgradeToAndCall(nextImplementation, migrateData);

service_contracts/src/FilecoinWarmStorageService.sol

service_contracts/test/FilecoinWarmStorageService.t.sol

Co-authored-by: Rod Vagg <[email protected]>

rvagg

script names could be more consistent and have warm-storage in them, but that's a weakly held opinion and I hope that problem mostly goes away with #229

wjmelements · 2025-10-07T04:07:05Z

weird that 351cc6e caused the size check to fail

wjmelements · 2025-10-07T04:23:01Z

weird that 351cc6e caused the size check to fail

I reproduce this after merging with the main branch but not on the tip commit. Maybe it was testing a merged PR?

I'll look for ways to cut codesize.

wjmelements · 2025-10-07T04:35:42Z

Probably worth cutting codesize more than the minimum but will do it in a separate PR

docs: add #260 to the changelog after rebase

@rvagg

Reviewer @rvagg Per a discussion with @jennijuju, we want to make the upgrade process more transparent. This changes upgrades to a two step process. First, we announce the planned upgrade. Then, after the designated time, we can complete the upgrade. The delay gives us time to check the pending implementation address. It also gives users notice even if we are not in communication with them. I also fix FilOzone#259 in this change, because I was already adding tests for `upgradeToAndCall`. I can fix it separately if this change is rejected. After validating that this is a good design that the team likes, I can replicate it for the other proxies in separate PRs. #### Test Plan I tested the new deployment scripts by running them on calibnet. * [announcePlannedUpgrade](https://calibration.filfox.info/en/message/bafy2bzaceb6vpcsy563vhpwig5g6jy7gb6fr2ssxhjgwiagj7k3vbjrg3dg56?t=3) * [upgradeToAndCall](https://calibration.filfox.info/en/message/bafy2bzaceadjmlemjyrv7nmwh5lzavdbntjmywvblrzmtuh3wsyldqlvxbcyo?t=3) #### Changes * announcePlannedUpgrade * update _authorizeUpgrade * new view method for pending upgrade * make gen * add and fix tests covering upgradeToAndCall with migrate * separate announce and upgrade scripts * update implementation-only script --------- Co-authored-by: Rod Vagg <[email protected]>

feat: announcePlannedUpgrade

57e744f

wjmelements requested a review from rvagg October 4, 2025 05:10

FilOzzy added this to FS Oct 4, 2025

github-project-automation bot moved this to 📌 Triage in FS Oct 4, 2025

wjmelements commented Oct 4, 2025

View reviewed changes

make update-abi

090e985

wjmelements commented Oct 4, 2025

View reviewed changes

rjan90 moved this from 📌 Triage to 🔎 Awaiting review in FS Oct 4, 2025

wjmelements commented Oct 4, 2025

View reviewed changes

wjmelements changed the title ~~announcePlannedUpgrade~~ feat: announcePlannedUpgrade Oct 4, 2025

rvagg reviewed Oct 7, 2025

View reviewed changes

service_contracts/src/FilecoinWarmStorageService.sol Show resolved Hide resolved

rvagg reviewed Oct 7, 2025

View reviewed changes

service_contracts/test/FilecoinWarmStorageService.t.sol Outdated Show resolved Hide resolved

wjmelements and others added 2 commits October 6, 2025 22:59

Update service_contracts/src/FilecoinWarmStorageService.sol

351cc6e

Co-authored-by: Rod Vagg <[email protected]>

Update service_contracts/test/FilecoinWarmStorageService.t.sol

467039a

Co-authored-by: Rod Vagg <[email protected]>

rvagg approved these changes Oct 7, 2025

View reviewed changes

github-project-automation bot moved this from 🔎 Awaiting review to ✔️ Approved by reviewer in FS Oct 7, 2025

Merge remote-tracking branch 'origin/main' into announce-planned-upgrade

ebfd0f6

move serviceCommissionBps to view contract

f0be07e

wjmelements merged commit 722943e into main Oct 7, 2025
5 checks passed

wjmelements deleted the announce-planned-upgrade branch October 7, 2025 04:35

github-project-automation bot moved this from ✔️ Approved by reviewer to 🎉 Done in FS Oct 7, 2025

rjan90 added a commit that referenced this pull request Oct 7, 2025

docs: add #260 to the changelog after rebase

1260bec

docs: add #260 to the changelog after rebase

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: announcePlannedUpgrade #260

feat: announcePlannedUpgrade #260

Uh oh!

wjmelements commented Oct 4, 2025 •

edited

Loading

Uh oh!

wjmelements Oct 4, 2025

Uh oh!

rvagg Oct 7, 2025

Uh oh!

wjmelements Oct 4, 2025

Uh oh!

wjmelements commented Oct 4, 2025

Uh oh!

wjmelements Oct 4, 2025

Uh oh!

wjmelements Oct 4, 2025

Uh oh!

wjmelements Oct 4, 2025

Uh oh!

wjmelements Oct 4, 2025

Uh oh!

Uh oh!

Uh oh!

rvagg left a comment

Uh oh!

wjmelements commented Oct 7, 2025

Uh oh!

wjmelements commented Oct 7, 2025

Uh oh!

Uh oh!

wjmelements commented Oct 7, 2025

Uh oh!

Uh oh!

feat: announcePlannedUpgrade #260

feat: announcePlannedUpgrade #260

Uh oh!

Conversation

wjmelements commented Oct 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Plan

Changes

Uh oh!

wjmelements Oct 4, 2025

Choose a reason for hiding this comment

Uh oh!

rvagg Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

wjmelements Oct 4, 2025

Choose a reason for hiding this comment

Uh oh!

wjmelements commented Oct 4, 2025

Uh oh!

wjmelements Oct 4, 2025

Choose a reason for hiding this comment

Uh oh!

wjmelements Oct 4, 2025

Choose a reason for hiding this comment

Uh oh!

wjmelements Oct 4, 2025

Choose a reason for hiding this comment

Uh oh!

wjmelements Oct 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rvagg left a comment

Choose a reason for hiding this comment

Uh oh!

wjmelements commented Oct 7, 2025

Uh oh!

wjmelements commented Oct 7, 2025

Uh oh!

Uh oh!

wjmelements commented Oct 7, 2025

Uh oh!

Uh oh!

wjmelements commented Oct 4, 2025 •

edited

Loading