diff --git a/service_contracts/foundry.toml b/service_contracts/foundry.toml index 38a02e58..c4e2f404 100644 --- a/service_contracts/foundry.toml +++ b/service_contracts/foundry.toml @@ -5,12 +5,25 @@ script = 'script' out = 'out' libs = ['lib'] cache_path = 'cache' -solc = "0.8.30" +solc = "0.8.37" via_ir = true optimizer = true optimizer_runs = 200 bytecode_hash = "none" +ignored_error_codes = [ + "license", + "code-size", + "init-code-size", + "transient-storage", + "transfer-deprecated", + "natspec-memory-safe-assembly-deprecated", + 9170, # Solidity 0.8.37 warning from fws-payments + 6335, # Solidity 0.8.37 warning from OpenZeppelin/forge-std + 8506, # Solidity 0.8.37 warning from forge-std + 4591, # Warning limit reached +] + # For dependencies remappings = [ '@openzeppelin/contracts/=lib/openzeppelin-contracts/contracts/', diff --git a/service_contracts/src/lib/LibAccessControl.sol b/service_contracts/src/lib/LibAccessControl.sol new file mode 100644 index 00000000..26919eda --- /dev/null +++ b/service_contracts/src/lib/LibAccessControl.sol @@ -0,0 +1,44 @@ +// SPDX-License-Identifier: Apache-2.0 OR MIT +pragma solidity 0.8.37; + +/// @title LibAccessControl +/// @notice Shared access-control checks for FWSS modules. +library LibAccessControl { + /// @custom:storage-location erc7201:openzeppelin.storage.Ownable + struct OwnableStorage { + address owner; + } + + // keccak256(abi.encode(uint256(keccak256("openzeppelin.storage.Ownable")) - 1)) & ~bytes32(uint256(0xff)) + bytes32 private constant OWNABLE_STORAGE_LOCATION = + 0x9016d09d72d40fdae2fd8ceac6b6234c7706214fd39c1cd1e609a0528c199300; + + /// @notice The caller account is not authorized to perform an operation. + /// @param account The unauthorized account. + error OwnableUnauthorizedAccount(address account); + + /** + * @notice Reverts when `account` is not the FWSS owner. + * @param account The account requiring owner authorization. + */ + function requireOwner(address account) internal view { + if (account != owner()) { + revert OwnableUnauthorizedAccount(account); + } + } + + /** + * @notice Returns the owner stored by OpenZeppelin OwnableUpgradeable. + * @return The current owner address. + */ + function owner() internal view returns (address) { + OwnableStorage storage ownableStorage; + bytes32 location = OWNABLE_STORAGE_LOCATION; + + assembly ("memory-safe") { + ownableStorage.slot := location + } + + return ownableStorage.owner; + } +} diff --git a/service_contracts/src/modules/FWSSMetadataModule.sol b/service_contracts/src/modules/FWSSMetadataModule.sol new file mode 100644 index 00000000..81ffb8b0 --- /dev/null +++ b/service_contracts/src/modules/FWSSMetadataModule.sol @@ -0,0 +1,27 @@ +// SPDX-License-Identifier: Apache-2.0 OR MIT +pragma solidity 0.8.37; + +import {IFilecoinServiceMetadata} from "../IFilecoinServiceMetadata.sol"; + +/// @title FWSSMetadataModule +/// @notice Exposes static FWSS service metadata. +contract FWSSMetadataModule is IFilecoinServiceMetadata { + // Version tracking + string public constant VERSION = "1.4.0"; + string internal constant SERVICE_NAME = "Filecoin Warm Storage Service"; + string internal constant SERVICE_DESCRIPTION = + "Warm storage service for the Filecoin Onchain Cloud. Manages PDP-backed datasets, Filecoin Pay storage rails, lifecycle fees, and optional CDN payment rails."; + string private constant SERVICE_HOMEPAGE = "https://github.com/FilOzone/filecoin-services"; + + function name() external pure override returns (string memory) { + return SERVICE_NAME; + } + + function description() external pure override returns (string memory) { + return SERVICE_DESCRIPTION; + } + + function homepage() external pure override returns (string memory) { + return SERVICE_HOMEPAGE; + } +} diff --git a/service_contracts/src/modules/FilecoinWarmStorageServiceProviderManagementModule.sol b/service_contracts/src/modules/FilecoinWarmStorageServiceProviderManagementModule.sol new file mode 100644 index 00000000..1c3ee231 --- /dev/null +++ b/service_contracts/src/modules/FilecoinWarmStorageServiceProviderManagementModule.sol @@ -0,0 +1,58 @@ +// SPDX-License-Identifier: Apache-2.0 OR MIT +pragma solidity 0.8.37; + +import {Errors} from "../Errors.sol"; +import {LibAccessControl} from "../lib/LibAccessControl.sol"; +import {FWSSStorage} from "../storage/FWSSStorage.sol"; + +/// @title FilecoinWarmStorageServiceProviderManagementModule +/// @notice Manages the set of provider IDs approved to use FWSS. +contract FilecoinWarmStorageServiceProviderManagementModule is FWSSStorage { + event ProviderApproved(uint256 indexed providerId); + event ProviderUnapproved(uint256 indexed providerId); + + /// @notice Ensures the caller is the FWSS owner + modifier onlyOwner() { + LibAccessControl.requireOwner(msg.sender); + _; + } + + /** + * @notice Adds a provider ID to the approved list + * @dev Only callable by the contract owner. Reverts if already approved. + * @param providerId The provider ID to approve + */ + function addApprovedProvider(uint256 providerId) external onlyOwner { + if (approvedProviders[providerId]) { + revert Errors.ProviderAlreadyApproved(providerId); + } + approvedProviders[providerId] = true; + approvedProviderIds.push(providerId); + emit ProviderApproved(providerId); + } + + /** + * @notice Removes a provider ID from the approved list + * @dev Only callable by the contract owner. Reverts if not in list. + * @param providerId The provider ID to remove + * @param index The index of the provider ID in the approvedProviderIds array + */ + function removeApprovedProvider(uint256 providerId, uint256 index) external onlyOwner { + if (!approvedProviders[providerId]) { + revert Errors.ProviderNotInApprovedList(providerId); + } + + require(approvedProviderIds[index] == providerId, Errors.ProviderIdMismatchAtIndex(index, providerId)); + + approvedProviders[providerId] = false; + + // Remove from array using swap-and-pop pattern + uint256 length = approvedProviderIds.length; + if (index != length - 1) { + approvedProviderIds[index] = approvedProviderIds[length - 1]; + } + approvedProviderIds.pop(); + + emit ProviderUnapproved(providerId); + } +} diff --git a/service_contracts/src/storage/FWSSStorage.sol b/service_contracts/src/storage/FWSSStorage.sol index 8cc5fd19..18c20720 100644 --- a/service_contracts/src/storage/FWSSStorage.sol +++ b/service_contracts/src/storage/FWSSStorage.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: Apache-2.0 OR MIT -pragma solidity 0.8.30; +pragma solidity 0.8.37; /// @dev Legacy slots 0-23. Preserve field order, types and packing in every inheriting module. abstract contract FWSSStorage { diff --git a/service_contracts/src/storage/README.md b/service_contracts/src/storage/README.md index 1026716b..11fdcbbb 100644 --- a/service_contracts/src/storage/README.md +++ b/service_contracts/src/storage/README.md @@ -1,13 +1,16 @@ # FWSS shared storage -`FWSSStorage` declares the legacy application layout used by -`FilecoinWarmStorageService`, including retired fields and stored structs. -Contracts inheriting it share the same field declarations and storage positions. +`FWSSStorage` declares the complete legacy application layout, +including retired fields and stored structs. FWSS inherits it and uses the same field +names. Previously private fields become internal to allow inherited access. -Preserve root slots 0–23, field order, types, struct members and packing. Retired -fields remain reserved. Appending ordinary fields independently in different -modules can make them write to the same proxy slots. OpenZeppelin state uses its -existing bases and namespaces. +Preserve the 24 root slots (0–23), field order, types, struct members and packing. +Retired fields must remain in place. Constants and constructor immutables stay in +FWSS; OpenZeppelin state remains in its existing bases and namespaces. + +Future modules must inherit the shared declarations. Independently appending fields +in different modules can make them write to the same proxy slots. Their actual +compiled layouts and any new storage must be reviewed when modules are introduced. ## Verification @@ -20,11 +23,17 @@ forge test make update-abi ``` -`make gen` regenerates the compiler layout snapshot and read helpers. -`make check-layout` compares the snapshot with the PR base selected by -`GITHUB_BASE_REF`, or with `HEAD~1` locally. The comparison includes inherited -fields, offsets, packing and nested types. The normalizer preserves historical -names for `DataSetInfo` and `PlannedUpgrade` when comparing compiler output. +Existing CI regenerates FWSS's compiler layout, verifies generated files are current, +and compares the snapshot against the PR base. Inherited fields are included, so +shifts caused by this extraction are checked by the same workflow. Local +`make check-layout` compares against HEAD~1 when available; it is not a deployed +implementation check. No new upgrade validator is required by this extraction. + +The snapshot normalizer maps only the declaring-contract names of `DataSetInfo` and +`PlannedUpgrade` to their historical names. Slots, offsets, widths and recursive +member types remain part of the comparison. The ABI's `PlannedUpgrade.internalType` +changes its declaring-contract name; tuple encoding is unchanged. -These checks detect layout changes; they do not execute an upgrade or verify -migration behavior against a deployed proxy. +Layout checks do not execute a historical upgrade or prove migration behavior. +This PR changes declarations and inheritance; dispatcher and module behavior belong +to later changes with their own integration tests. diff --git a/service_contracts/test/modules/FWSSMetadataModule.t.sol b/service_contracts/test/modules/FWSSMetadataModule.t.sol new file mode 100644 index 00000000..8267fa52 --- /dev/null +++ b/service_contracts/test/modules/FWSSMetadataModule.t.sol @@ -0,0 +1,31 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.13; + +import {Test} from "forge-std/Test.sol"; +import {FWSSMetadataModule} from "../../src/modules/FWSSMetadataModule.sol"; +import {IFilecoinServiceMetadata} from "../../src/IFilecoinServiceMetadata.sol"; + +contract FWSSMetadataModuleTest is Test { + FWSSMetadataModule public metadataModule; + + function setUp() public { + metadataModule = new FWSSMetadataModule(); + } + + function testServiceMetadata() public view { + IFilecoinServiceMetadata metadata = IFilecoinServiceMetadata(address(metadataModule)); + string memory serviceName = metadata.name(); + string memory serviceDescription = metadata.description(); + string memory serviceHomepage = metadata.homepage(); + + assertEq(serviceName, "Filecoin Warm Storage Service", "Service name should match"); + assertEq( + serviceDescription, + "Warm storage service for the Filecoin Onchain Cloud. Manages PDP-backed datasets, Filecoin Pay storage rails, lifecycle fees, and optional CDN payment rails.", + "Service description should match" + ); + assertEq(serviceHomepage, "https://github.com/FilOzone/filecoin-services", "Service homepage should match"); + assertLe(bytes(serviceDescription).length, 256, "Service description should not exceed 256 bytes"); + assertLe(bytes(serviceHomepage).length, 256, "Service homepage should not exceed 256 bytes"); + } +} diff --git a/service_contracts/test/modules/FilecoinWarmStorageServiceProviderManagementModule.t.sol b/service_contracts/test/modules/FilecoinWarmStorageServiceProviderManagementModule.t.sol new file mode 100644 index 00000000..a3760a7f --- /dev/null +++ b/service_contracts/test/modules/FilecoinWarmStorageServiceProviderManagementModule.t.sol @@ -0,0 +1,232 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity 0.8.37; + +import {MyERC1967Proxy} from "@pdp/ERC1967Proxy.sol"; +import {Test} from "forge-std/Test.sol"; + +import {Errors} from "../../src/Errors.sol"; +import { + FilecoinWarmStorageServiceProviderManagementModule +} from "../../src/modules/FilecoinWarmStorageServiceProviderManagementModule.sol"; + +contract FilecoinWarmStorageServiceProviderManagementModuleHarness is + FilecoinWarmStorageServiceProviderManagementModule +{ + function isProviderApproved(uint256 providerId) external view returns (bool) { + return approvedProviders[providerId]; + } + + function getApprovedProviders() external view returns (uint256[] memory) { + return approvedProviderIds; + } + + function getApprovedProvidersLength() external view returns (uint256) { + return approvedProviderIds.length; + } +} + +contract FilecoinWarmStorageServiceProviderManagementModuleTest is Test { + bytes32 private constant OWNABLE_STORAGE_LOCATION = + 0x9016d09d72d40fdae2fd8ceac6b6234c7706214fd39c1cd1e609a0528c199300; + + FilecoinWarmStorageServiceProviderManagementModuleHarness public providerManagementModule; + + address public owner; + address public provider1; + + function setUp() public { + owner = address(this); + provider1 = address(0x1); + + FilecoinWarmStorageServiceProviderManagementModuleHarness implementation = + new FilecoinWarmStorageServiceProviderManagementModuleHarness(); + MyERC1967Proxy proxy = new MyERC1967Proxy(address(implementation), ""); + providerManagementModule = FilecoinWarmStorageServiceProviderManagementModuleHarness(address(proxy)); + + vm.store(address(proxy), OWNABLE_STORAGE_LOCATION, bytes32(uint256(uint160(owner)))); + } + + function testAddAndRemoveApprovedProvider() public { + // Test adding provider + providerManagementModule.addApprovedProvider(1); + assertTrue(providerManagementModule.isProviderApproved(1), "Provider 1 should be approved"); + + // Test adding already approved provider (should revert) + vm.expectRevert(abi.encodeWithSelector(Errors.ProviderAlreadyApproved.selector, 1)); + providerManagementModule.addApprovedProvider(1); + + // Test removing provider + providerManagementModule.removeApprovedProvider(1, 0); // Provider 1 is at index 0 + assertFalse(providerManagementModule.isProviderApproved(1), "Provider 1 should not be approved"); + + // Test removing non-approved provider (should revert) + vm.expectRevert(abi.encodeWithSelector(Errors.ProviderNotInApprovedList.selector, 2)); + providerManagementModule.removeApprovedProvider(2, 0); + + // Test removing already removed provider (should revert) + vm.expectRevert(abi.encodeWithSelector(Errors.ProviderNotInApprovedList.selector, 1)); + providerManagementModule.removeApprovedProvider(1, 0); + } + + function testOnlyOwnerCanManageApprovedProviders() public { + // Non-owner tries to add provider + vm.prank(provider1); + vm.expectRevert(); + providerManagementModule.addApprovedProvider(1); + + // Non-owner tries to remove provider + providerManagementModule.addApprovedProvider(1); + vm.prank(provider1); + vm.expectRevert(); + providerManagementModule.removeApprovedProvider(1, 0); + } + + function testAddApprovedProviderAlreadyApproved() public { + // First add should succeed + providerManagementModule.addApprovedProvider(5); + assertTrue(providerManagementModule.isProviderApproved(5), "Provider 5 should be approved"); + + // Second add should revert with ProviderAlreadyApproved error + vm.expectRevert(abi.encodeWithSelector(Errors.ProviderAlreadyApproved.selector, 5)); + providerManagementModule.addApprovedProvider(5); + } + + function testGetApprovedProviders() public { + // Test empty list initially + uint256[] memory providers = providerManagementModule.getApprovedProviders(); + assertEq(providers.length, 0, "Should have no approved providers initially"); + + // Add some providers + providerManagementModule.addApprovedProvider(1); + providerManagementModule.addApprovedProvider(5); + providerManagementModule.addApprovedProvider(10); + + // Test retrieval + providers = providerManagementModule.getApprovedProviders(); + assertEq(providers.length, 3, "Should have 3 approved providers"); + assertEq(providers[0], 1, "First provider should be 1"); + assertEq(providers[1], 5, "Second provider should be 5"); + assertEq(providers[2], 10, "Third provider should be 10"); + + // Remove one provider (provider 5 is at index 1) + providerManagementModule.removeApprovedProvider(5, 1); + + // Test after removal (should have provider 10 in place of 5 due to swap-and-pop) + providers = providerManagementModule.getApprovedProviders(); + assertEq(providers.length, 2, "Should have 2 approved providers after removal"); + assertEq(providers[0], 1, "First provider should still be 1"); + assertEq(providers[1], 10, "Second provider should be 10 (moved from last position)"); + + // Remove another (provider 1 is at index 0) + providerManagementModule.removeApprovedProvider(1, 0); + providers = providerManagementModule.getApprovedProviders(); + assertEq(providers.length, 1, "Should have 1 approved provider"); + assertEq(providers[0], 10, "Remaining provider should be 10"); + + // Remove last one (provider 10 is at index 0) + providerManagementModule.removeApprovedProvider(10, 0); + providers = providerManagementModule.getApprovedProviders(); + assertEq(providers.length, 0, "Should have no approved providers after removing all"); + } + + function testGetApprovedProvidersWithSingleProvider() public { + // Add single provider and verify + providerManagementModule.addApprovedProvider(42); + uint256[] memory providers = providerManagementModule.getApprovedProviders(); + assertEq(providers.length, 1, "Should have 1 approved provider"); + assertEq(providers[0], 42, "Provider should be 42"); + + // Remove and verify empty (provider 42 is at index 0) + providerManagementModule.removeApprovedProvider(42, 0); + providers = providerManagementModule.getApprovedProviders(); + assertEq(providers.length, 0, "Should have no approved providers"); + } + + function testConsistencyBetweenIsApprovedAndGetAll() public { + // Add multiple providers + uint256[] memory idsToAdd = new uint256[](5); + idsToAdd[0] = 1; + idsToAdd[1] = 3; + idsToAdd[2] = 7; + idsToAdd[3] = 15; + idsToAdd[4] = 100; + + for (uint256 i = 0; i < idsToAdd.length; i++) { + providerManagementModule.addApprovedProvider(idsToAdd[i]); + } + + // Verify consistency - all providers in the array should return true for isProviderApproved + uint256[] memory providers = providerManagementModule.getApprovedProviders(); + assertEq(providers.length, 5, "Should have 5 approved providers"); + + for (uint256 i = 0; i < providers.length; i++) { + assertTrue( + providerManagementModule.isProviderApproved(providers[i]), + string.concat("Provider ", vm.toString(providers[i]), " should be approved") + ); + } + + // Verify that non-approved providers return false + assertFalse(providerManagementModule.isProviderApproved(2), "Provider 2 should not be approved"); + assertFalse(providerManagementModule.isProviderApproved(50), "Provider 50 should not be approved"); + + // Remove some providers and verify consistency + // Find indices of providers 3 and 15 in the array + // Based on adding order: [1, 3, 7, 15, 100] + providerManagementModule.removeApprovedProvider(3, 1); // provider 3 is at index 1 + // After removing 3 with swap-and-pop, array becomes: [1, 100, 7, 15] + providerManagementModule.removeApprovedProvider(15, 3); // provider 15 is now at index 3 + + providers = providerManagementModule.getApprovedProviders(); + assertEq(providers.length, 3, "Should have 3 approved providers after removal"); + + // Verify all remaining are still approved + for (uint256 i = 0; i < providers.length; i++) { + assertTrue( + providerManagementModule.isProviderApproved(providers[i]), + string.concat("Remaining provider ", vm.toString(providers[i]), " should be approved") + ); + } + + // Verify removed ones are not approved + assertFalse(providerManagementModule.isProviderApproved(3), "Provider 3 should not be approved after removal"); + assertFalse(providerManagementModule.isProviderApproved(15), "Provider 15 should not be approved after removal"); + } + + function testRemoveApprovedProviderNotInList() public { + // Trying to remove a provider that was never approved should revert + vm.expectRevert(abi.encodeWithSelector(Errors.ProviderNotInApprovedList.selector, 10)); + providerManagementModule.removeApprovedProvider(10, 0); + + // Add and then remove a provider + providerManagementModule.addApprovedProvider(6); + providerManagementModule.removeApprovedProvider(6, 0); // provider 6 is at index 0 + + // Trying to remove the same provider again should revert + vm.expectRevert(abi.encodeWithSelector(Errors.ProviderNotInApprovedList.selector, 6)); + providerManagementModule.removeApprovedProvider(6, 0); + } + + function testGetApprovedProvidersLength() public { + // Initially should be 0 + assertEq(providerManagementModule.getApprovedProvidersLength(), 0, "Initial length should be 0"); + + // Add providers and check length + providerManagementModule.addApprovedProvider(1); + assertEq( + providerManagementModule.getApprovedProvidersLength(), 1, "Length should be 1 after adding one provider" + ); + + providerManagementModule.addApprovedProvider(2); + providerManagementModule.addApprovedProvider(3); + assertEq( + providerManagementModule.getApprovedProvidersLength(), 3, "Length should be 3 after adding three providers" + ); + + // Remove one and check length + providerManagementModule.removeApprovedProvider(2, 1); // provider 2 is at index 1 + assertEq( + providerManagementModule.getApprovedProvidersLength(), 2, "Length should be 2 after removing one provider" + ); + } +}