fix: disable storage provider changes for GA (#309)

rjan90 · rvagg · web-flow · commit d58232dc244f · 2025-10-21T10:22:12.000+02:00
Disables storage provider changes by reverting in the `FilecoinWarmStorageService.storageProviderChanged()` listener callback. PDPVerifier remains unchanged, so no contract upgrade is needed for that layer. ### Changes - Modified `FilecoinWarmStorageService.storageProviderChanged()` to revert with "Storage provider changes are not yet supported" - Updated test expectations to verify the revert behavior Ref: #203 --------- Co-authored-by: Rod Vagg <rod@vagg.org>
diff --git a/service_contracts/abi/FilecoinWarmStorageService.abi.json b/service_contracts/abi/FilecoinWarmStorageService.abi.json
@@ -719,17 +719,17 @@
     "name": "storageProviderChanged",
     "inputs": [
       {
-        "name": "dataSetId",
+        "name": "",
         "type": "uint256",
         "internalType": "uint256"
       },
       {
-        "name": "oldServiceProvider",
+        "name": "",
         "type": "address",
         "internalType": "address"
       },
       {
-        "name": "newServiceProvider",
+        "name": "",
         "type": "address",
         "internalType": "address"
       },
@@ -1965,27 +1965,6 @@
     "name": "NotInitializing",
     "inputs": []
   },
-  {
-    "type": "error",
-    "name": "OldServiceProviderMismatch",
-    "inputs": [
-      {
-        "name": "dataSetId",
-        "type": "uint256",
-        "internalType": "uint256"
-      },
-      {
-        "name": "expected",
-        "type": "address",
-        "internalType": "address"
-      },
-      {
-        "name": "actual",
-        "type": "address",
-        "internalType": "address"
-      }
-    ]
-  },
   {
     "type": "error",
     "name": "OnlyFilBeamControllerAllowed",
diff --git a/service_contracts/src/FilecoinWarmStorageService.sol b/service_contracts/src/FilecoinWarmStorageService.sol
@@ -919,43 +919,18 @@ contract FilecoinWarmStorageService is
     }
 
     /**
-     * @notice Handles data set service provider changes by updating internal state only
-     * @dev Called by the PDPVerifier contract when data set service provider is transferred.
-     * NOTE: The PDPVerifier contract emits events and exposes methods in terms of "storage providers",
-     * because its scope is specifically the Proof-of-Data-Possession for storage services.
-     * In FilecoinWarmStorageService (and the broader service registry architecture), we use the term
-     * "service provider" to support a future where multiple types of services may exist (not just storage).
-     * As a result, some parameters and events reflect this terminology shift and this method represents
-     * a transition point in the language, from PDPVerifier to FilecoinWarmStorageService.
-     * @param dataSetId The ID of the data set whose service provider is changing
-     * @param oldServiceProvider The previous service provider address
-     * @param newServiceProvider The new service provider address (must be an approved provider)
+     * @notice Handles data set service provider changes (currently disabled for GA)
+     * @dev Storage provider changes are disabled for GA. This will be re-enabled post-GA
+     * with proper client authorization. See: https://github.com/FilOzone/filecoin-services/issues/203
+     * Called by the PDPVerifier contract when data set service provider is transferred.
      */
     function storageProviderChanged(
-        uint256 dataSetId,
-        address oldServiceProvider,
-        address newServiceProvider,
+        uint256, // dataSetId
+        address, // oldServiceProvider
+        address, // newServiceProvider
         bytes calldata // extraData - not used
     ) external override onlyPDPVerifier {
-        // Verify the data set exists and validate the old service provider
-        DataSetInfo storage info = dataSetInfo[dataSetId];
-        require(
-            info.serviceProvider == oldServiceProvider,
-            Errors.OldServiceProviderMismatch(dataSetId, info.serviceProvider, oldServiceProvider)
-        );
-        require(newServiceProvider != address(0), Errors.ZeroAddress(Errors.AddressField.ServiceProvider));
-
-        // Verify new service provider is registered
-        uint256 newProviderId = serviceProviderRegistry.getProviderIdByAddress(newServiceProvider);
-
-        // Check if provider is registered
-        require(newProviderId != 0, Errors.ProviderNotRegistered(newServiceProvider));
-
-        // Update the data set service provider
-        info.serviceProvider = newServiceProvider;
-
-        // Emit event for off-chain tracking
-        emit DataSetServiceProviderChanged(dataSetId, oldServiceProvider, newServiceProvider);
+        revert("Storage provider changes are not yet supported");
     }
 
     function terminateService(uint256 dataSetId) external {
diff --git a/service_contracts/test/FilecoinWarmStorageService.t.sol b/service_contracts/test/FilecoinWarmStorageService.t.sol
@@ -1172,44 +1172,23 @@ contract FilecoinWarmStorageServiceTest is MockFVMTest {
      * @notice Test successful service provider change between two approved providers
      * @dev Verifies only the data set's payee is updated, event is emitted, and serviceProviderRegistry state is unchanged.
      */
+    // NOTE: Disabled for GA - Storage provider changes are not permitted
+    // See: https://github.com/FilOzone/filecoin-services/issues/203
     function testServiceProviderChangedSuccessDecoupled() public {
         // Create a data set with sp1 as the service provider
         uint256 testDataSetId = createDataSetForServiceProviderTest(sp1, client, "Test Data Set");
 
-        // Change service provider from sp1 to sp2
+        // Change service provider from sp1 to sp2 should revert
         bytes memory testExtraData = new bytes(0);
-        vm.expectEmit(true, true, true, true);
-        emit FilecoinWarmStorageService.DataSetServiceProviderChanged(testDataSetId, sp1, sp2);
         vm.prank(sp2);
+        vm.expectRevert("Storage provider changes are not yet supported");
         mockPDPVerifier.changeDataSetServiceProvider(testDataSetId, sp2, address(pdpServiceWithPayments), testExtraData);
-
-        // Only the data set's service provider is updated
-        FilecoinWarmStorageService.DataSetInfoView memory dataSet = viewContract.getDataSet(testDataSetId);
-        assertEq(dataSet.serviceProvider, sp2, "Service provider should be updated to new service provider");
-        // Payee should remain unchanged (still sp1's beneficiary)
-        assertEq(dataSet.payee, sp1, "Payee should remain unchanged");
-    }
-
-    /**
-     * @notice Test service provider change reverts if new service provider is not an approved provider
-     */
-    function testServiceProviderChangedNoLongerChecksApproval() public {
-        // Create a data set with sp1 as the service provider
-        uint256 testDataSetId = createDataSetForServiceProviderTest(sp1, client, "Test Data Set");
-        address newProvider = address(0x9999);
-        bytes memory testExtraData = new bytes(0);
-
-        // The change should now fail because the new provider is not registered
-        vm.prank(newProvider);
-        vm.expectRevert(abi.encodeWithSelector(Errors.ProviderNotRegistered.selector, newProvider));
-        mockPDPVerifier.changeDataSetServiceProvider(
-            testDataSetId, newProvider, address(pdpServiceWithPayments), testExtraData
-        );
     }
 
     /**
      * @notice Test service provider change reverts if new service provider is zero address
      */
+    // NOTE: The mock PDPVerifier checks for zero address before calling the listener
     function testServiceProviderChangedRevertsIfNewServiceProviderZeroAddress() public {
         uint256 testDataSetId = createDataSetForServiceProviderTest(sp1, client, "Test Data Set");
         bytes memory testExtraData = new bytes(0);
@@ -1221,65 +1200,44 @@ contract FilecoinWarmStorageServiceTest is MockFVMTest {
     }
 
     /**
-     * @notice Test service provider change reverts if old service provider mismatch
+     * @notice Test service provider change reverts (feature not yet supported)
      */
+    // NOTE: Disabled for GA - Storage provider changes are not permitted
+    // See: https://github.com/FilOzone/filecoin-services/issues/203
     function testServiceProviderChangedRevertsIfOldServiceProviderMismatch() public {
         uint256 testDataSetId = createDataSetForServiceProviderTest(sp1, client, "Test Data Set");
         bytes memory testExtraData = new bytes(0);
-        // Call directly as PDPVerifier with wrong old service provider
+        // Call directly as PDPVerifier - should now revert before validation
         vm.prank(address(mockPDPVerifier));
-        vm.expectRevert(abi.encodeWithSelector(Errors.OldServiceProviderMismatch.selector, 1, sp1, sp2));
+        vm.expectRevert("Storage provider changes are not yet supported");
         pdpServiceWithPayments.storageProviderChanged(testDataSetId, sp2, sp2, testExtraData);
     }
 
     /**
      * @notice Test service provider change reverts if called by unauthorized address
      */
+    // NOTE: This test for the onlyPDPVerifier modifier validation remains important
     function testServiceProviderChangedRevertsIfUnauthorizedCaller() public {
         uint256 testDataSetId = createDataSetForServiceProviderTest(sp1, client, "Test Data Set");
         bytes memory testExtraData = new bytes(0);
-        // Call directly as sp2 (not PDPVerifier)
+        // Call directly as sp2 (not PDPVerifier) - should fail on modifier before main revert
         vm.prank(sp2);
         vm.expectRevert(abi.encodeWithSelector(Errors.OnlyPDPVerifierAllowed.selector, address(mockPDPVerifier), sp2));
         pdpServiceWithPayments.storageProviderChanged(testDataSetId, sp1, sp2, testExtraData);
     }
 
-    /**
-     * @notice Test multiple data sets per provider: only the targeted data set's payee is updated
-     */
-    function testMultipleDataSetsPerProviderServiceProviderChange() public {
-        // Create two data sets for sp1
-        uint256 ps1 = createDataSetForServiceProviderTest(sp1, client, "Data Set 1");
-        uint256 ps2 = createDataSetForServiceProviderTest(sp1, client, "Data Set 2");
-        // Change service provider of ps1 to sp2
-        bytes memory testExtraData = new bytes(0);
-        vm.expectEmit(true, true, true, true);
-        emit FilecoinWarmStorageService.DataSetServiceProviderChanged(ps1, sp1, sp2);
-        vm.prank(sp2);
-        mockPDPVerifier.changeDataSetServiceProvider(ps1, sp2, address(pdpServiceWithPayments), testExtraData);
-        // ps1 service provider updated, ps2 service provider unchanged
-        FilecoinWarmStorageService.DataSetInfoView memory dataSet1 = viewContract.getDataSet(ps1);
-        FilecoinWarmStorageService.DataSetInfoView memory dataSet2 = viewContract.getDataSet(ps2);
-        assertEq(dataSet1.serviceProvider, sp2, "ps1 service provider should be sp2");
-        assertEq(dataSet1.payee, sp1, "ps1 payee should remain sp1");
-        assertEq(dataSet2.serviceProvider, sp1, "ps2 service provider should remain sp1");
-        assertEq(dataSet2.payee, sp1, "ps2 payee should remain sp1");
-    }
-
     /**
      * @notice Test service provider change works with arbitrary extra data
      */
+    // NOTE: Disabled for GA - Storage provider changes are not permitted
+    // See: https://github.com/FilOzone/filecoin-services/issues/203
     function testServiceProviderChangedWithArbitraryExtraData() public {
         uint256 testDataSetId = createDataSetForServiceProviderTest(sp1, client, "Test Data Set");
         // Use arbitrary extra data
         bytes memory testExtraData = abi.encode("arbitrary", 123, address(this));
-        vm.expectEmit(true, true, true, true);
-        emit FilecoinWarmStorageService.DataSetServiceProviderChanged(testDataSetId, sp1, sp2);
         vm.prank(sp2);
+        vm.expectRevert("Storage provider changes are not yet supported");
         mockPDPVerifier.changeDataSetServiceProvider(testDataSetId, sp2, address(pdpServiceWithPayments), testExtraData);
-        FilecoinWarmStorageService.DataSetInfoView memory dataSet = viewContract.getDataSet(testDataSetId);
-        assertEq(dataSet.serviceProvider, sp2, "Service provider should be updated to new service provider");
-        assertEq(dataSet.payee, sp1, "Payee should remain unchanged");
     }
 
     function testProvenPeriods() public {
diff --git a/service_contracts/test/FilecoinWarmStorageServiceOwner.t.sol b/service_contracts/test/FilecoinWarmStorageServiceOwner.t.sol
@@ -12,7 +12,6 @@ import {SessionKeyRegistry} from "@session-key-registry/SessionKeyRegistry.sol";
 import {PDPListener} from "@pdp/PDPVerifier.sol";
 import {MyERC1967Proxy} from "@pdp/ERC1967Proxy.sol";
 import {FilecoinPayV1} from "@fws-payments/FilecoinPayV1.sol";
-import {Errors} from "../src/Errors.sol";
 import {MockERC20, MockPDPVerifier} from "./mocks/SharedMocks.sol";
 import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
 
@@ -217,139 +216,70 @@ contract FilecoinWarmStorageServiceOwnerTest is MockFVMTest {
         console.log("Service provider field correctly set to creator:", provider1);
     }
 
+    // NOTE: Disabled for GA - Storage provider changes are not permitted
+    // See: https://github.com/FilOzone/filecoin-services/issues/203
     function testStorageProviderChangedUpdatesOnlyOwnerField() public {
-        console.log("=== Test: storageProviderChanged updates only owner field ===");
+        console.log("=== Test: storageProviderChanged reverts (not yet supported) ===");
 
         uint256 dataSetId = createDataSet(provider1, client);
 
-        // Get initial state
-        FilecoinWarmStorageService.DataSetInfoView memory infoBefore = viewContract.getDataSet(dataSetId);
-        assertEq(infoBefore.serviceProvider, provider1, "Initial owner should be provider1");
-
-        // Change storage provider
-        vm.expectEmit(true, true, true, true);
-        emit DataSetServiceProviderChanged(dataSetId, provider1, provider2);
-
+        // Change storage provider should revert at FWSS listener
         vm.prank(provider2);
+        vm.expectRevert("Storage provider changes are not yet supported");
         pdpVerifier.changeDataSetServiceProvider(dataSetId, provider2, address(serviceContract), new bytes(0));
 
-        // Check updated state
-        FilecoinWarmStorageService.DataSetInfoView memory infoAfter = viewContract.getDataSet(dataSetId);
-
-        assertEq(infoAfter.serviceProvider, provider2, "Service provider should be updated to provider2");
-        assertEq(infoAfter.payee, provider1, "Payee should remain unchanged");
-        assertEq(infoAfter.payer, client, "Payer should remain unchanged");
-
-        console.log("Service provider updated from", provider1, "to", provider2);
-        console.log("Payee remained unchanged:", provider1);
+        console.log("Storage provider change correctly rejected");
     }
 
+    // NOTE: Disabled for GA - Storage provider changes are not permitted
+    // See: https://github.com/FilOzone/filecoin-services/issues/203
     function testStorageProviderChangedRevertsForUnregisteredProvider() public {
-        console.log("=== Test: storageProviderChanged reverts for unregistered provider ===");
+        console.log("=== Test: storageProviderChanged reverts (not yet supported) ===");
 
         uint256 dataSetId = createDataSet(provider1, client);
 
         address unregisteredAddress = address(0x999);
 
-        // Try to change to unregistered provider
+        // Try to change to unregistered provider, expect a revert
         vm.prank(address(pdpVerifier));
-        vm.expectRevert(abi.encodeWithSelector(Errors.ProviderNotRegistered.selector, unregisteredAddress));
+        vm.expectRevert("Storage provider changes are not yet supported");
         serviceContract.storageProviderChanged(dataSetId, provider1, unregisteredAddress, new bytes(0));
 
-        console.log("Correctly reverted for unregistered provider");
+        console.log("Correctly reverted (feature not yet supported)");
     }
 
+    // NOTE: Disabled for GA - Storage provider changes are not permitted
+    // See: https://github.com/FilOzone/filecoin-services/issues/203
     function testStorageProviderChangedSucceedsForAnyRegisteredProvider() public {
-        console.log("=== Test: storageProviderChanged succeeds for any registered provider ===");
+        console.log("=== Test: storageProviderChanged reverts (not yet supported) ===");
 
         uint256 dataSetId = createDataSet(provider1, client);
 
-        // Change to shouldn't require provider be approved
+        // Change to registered provider should revert
         vm.prank(address(pdpVerifier));
+        vm.expectRevert("Storage provider changes are not yet supported");
         serviceContract.storageProviderChanged(dataSetId, provider1, unauthorizedProvider, new bytes(0));
 
-        // Verify the service provider was changed
-        FilecoinWarmStorageService.DataSetInfoView memory info = viewContract.getDataSet(dataSetId);
-        assertEq(info.serviceProvider, unauthorizedProvider, "Service provider should be updated");
-
-        console.log("Successfully changed to registered provider (approval not required)");
+        console.log("Correctly reverted (feature not yet supported)");
     }
 
+    // NOTE: Disabled for GA - Storage provider changes are not permitted
+    // See: https://github.com/FilOzone/filecoin-services/issues/203
     function testStorageProviderChangedRevertsForWrongOldOwner() public {
-        console.log("=== Test: storageProviderChanged reverts for wrong old owner ===");
+        console.log("=== Test: storageProviderChanged reverts (not yet supported) ===");
 
         uint256 dataSetId = createDataSet(provider1, client);
 
-        // Try to change with wrong old owner
+        // Try to change with wrong old owner - now reverts before validation
         vm.prank(address(pdpVerifier));
-        vm.expectRevert(
-            abi.encodeWithSelector(
-                Errors.OldServiceProviderMismatch.selector,
-                dataSetId,
-                provider1, // actual owner
-                provider3 // wrong old owner passed
-            )
-        );
+        vm.expectRevert("Storage provider changes are not yet supported");
         serviceContract.storageProviderChanged(
             dataSetId,
             provider3, // wrong old owner
             provider2,
             new bytes(0)
         );
 
-        console.log("Correctly reverted for wrong old owner");
-    }
-
-    function testTerminateServiceUsesOwnerForAuthorization() public {
-        console.log("=== Test: terminateService uses owner for authorization ===");
-
-        uint256 dataSetId = createDataSet(provider1, client);
-
-        // Change owner to provider2
-        vm.prank(provider2);
-        pdpVerifier.changeDataSetServiceProvider(dataSetId, provider2, address(serviceContract), new bytes(0));
-
-        // Provider1 (original creator but no longer owner) should not be able to terminate
-        vm.prank(provider1);
-        vm.expectRevert(
-            abi.encodeWithSelector(
-                Errors.CallerNotPayerOrPayee.selector,
-                dataSetId,
-                client, // payer
-                provider2, // current owner
-                provider1 // caller
-            )
-        );
-        serviceContract.terminateService(dataSetId);
-
-        // Provider2 (current owner) should be able to terminate
-        vm.prank(provider2);
-        serviceContract.terminateService(dataSetId);
-
-        console.log("Only current owner (provider2) could terminate, not original creator (provider1)");
-    }
-
-    function testMultipleOwnerChanges() public {
-        console.log("=== Test: Multiple owner changes ===");
-
-        uint256 dataSetId = createDataSet(provider1, client);
-
-        // First change: provider1 -> provider2
-        vm.prank(provider2);
-        pdpVerifier.changeDataSetServiceProvider(dataSetId, provider2, address(serviceContract), new bytes(0));
-
-        FilecoinWarmStorageService.DataSetInfoView memory info1 = viewContract.getDataSet(dataSetId);
-        assertEq(info1.serviceProvider, provider2, "Service provider should be provider2 after first change");
-
-        // Second change: provider2 -> provider3
-        vm.prank(provider3);
-        pdpVerifier.changeDataSetServiceProvider(dataSetId, provider3, address(serviceContract), new bytes(0));
-
-        FilecoinWarmStorageService.DataSetInfoView memory info2 = viewContract.getDataSet(dataSetId);
-        assertEq(info2.serviceProvider, provider3, "Service provider should be provider3 after second change");
-        assertEq(info2.payee, provider1, "Payee should still be original provider1");
-
-        console.log("Service provider changed successfully: provider1 -> provider2 -> provider3");
-        console.log("Payee remained as provider1 throughout");
+        console.log("Correctly reverted (feature not yet supported)");
     }
 }
