From 0d8fdec59d69aa7b43f49c1b6725004a344aab43 Mon Sep 17 00:00:00 2001 From: noel Date: Thu, 16 May 2024 15:20:47 +0900 Subject: [PATCH 1/6] feat: support multiple rollup chain IDs close #136 --- contracts/src/core/MachServiceManager.sol | 37 ++++++++++--------- .../src/core/MachServiceManagerStorage.sol | 15 +++++++- .../src/interfaces/IMachServiceManager.sol | 4 +- contracts/test/AVSDeployer.sol | 5 ++- contracts/test/MachServiceManager.t.sol | 17 +++++---- 5 files changed, 48 insertions(+), 30 deletions(-) diff --git a/contracts/src/core/MachServiceManager.sol b/contracts/src/core/MachServiceManager.sol index 1bf7d4b..4748030 100644 --- a/contracts/src/core/MachServiceManager.sol +++ b/contracts/src/core/MachServiceManager.sol @@ -84,18 +84,22 @@ contract MachServiceManager is } function initialize( - IPauserRegistry _pauserRegistry, - uint256 _initialPausedStatus, - address _initialOwner, - address _alertConfirmer, - address _whitelister, - uint256 _rollupChainId + IPauserRegistry pauserRegistry_, + uint256 initialPausedStatus_, + address initialOwner_, + address alertConfirmer_, + address whitelister_, + uint256[] calldata rollupChainIDs_ ) public initializer { - _initializePauser(_pauserRegistry, _initialPausedStatus); - __ServiceManagerBase_init(_initialOwner); - _setAlertConfirmer(_alertConfirmer); - _setWhitelister(_whitelister); - _updateRollupChainId(_rollupChainId); + _initializePauser(pauserRegistry_, initialPausedStatus_); + __ServiceManagerBase_init(initialOwner_); + _setAlertConfirmer(alertConfirmer_); + _setWhitelister(whitelister_); + + for (uint256 i; i < rollupChainIDs_.length; ++i) { + _setRollupChainID(rollupChainIDs_[i], true); + } + allowlistEnabled = true; quorumThresholdPercentage = 66; } @@ -193,8 +197,8 @@ contract MachServiceManager is emit QuorumThresholdPercentageChanged(thresholdPercentage); } - function updateRollupChainId(uint256 newChainid) external onlyOwner { - _updateRollupChainId(newChainid); + function setRollupChainID(uint256 rollupChainid, bool status) external onlyOwner { + _setRollupChainID(rollupChainid, status); } ////////////////////////////////////////////////////////////////////////////// @@ -383,9 +387,8 @@ contract MachServiceManager is }); } - function _updateRollupChainId(uint256 newChainid) internal { - uint256 previousRollupChainId = rollupChainId; - rollupChainId = newChainid; - emit RollupChainIdUpdated(previousRollupChainId, newChainid); + function _setRollupChainID(uint256 rollupChainId, bool status) internal { + rollupChainIDs[rollupChainId] = status; + emit RollupChainIDUpdated(rollupChainId, status); } } diff --git a/contracts/src/core/MachServiceManagerStorage.sol b/contracts/src/core/MachServiceManagerStorage.sol index 41fb60f..0d7073b 100644 --- a/contracts/src/core/MachServiceManagerStorage.sol +++ b/contracts/src/core/MachServiceManagerStorage.sol @@ -14,14 +14,18 @@ abstract contract MachServiceManagerStorage { // CONSTANTS uint256 public constant THRESHOLD_DENOMINATOR = 100; + // Slot 0 EnumerableSet.Bytes32Set internal _messageHashes; + // Slot 1 /// @notice Ethereum addresses of currently register operators EnumerableSet.AddressSet internal _operators; + // Slot 2 /// @notice Set of operators that are allowed to register mapping(address => bool) public allowlist; + // Slot 3 /// @notice address that is permissioned to confirm alerts address public alertConfirmer; @@ -31,16 +35,23 @@ abstract contract MachServiceManagerStorage { /// @notice Minimal quorum threshold percentage uint8 public quorumThresholdPercentage; + // slot 4 /// @notice Resolved message hashes, prevent aggregator from replay any resolved alert EnumerableSet.Bytes32Set internal _resolvedMessageHashes; + // slot 5 /// @notice Role for whitelisting operators address public whitelister; + // slot 6 /// @notice Rollup chain id - uint256 public rollupChainId; + uint256 private __deprecatedSlot6; // rollupChainId + + // slot 7 + /// @notice Allowed rollup chain IDs + mapping(uint256 => bool) public rollupChainIDs; // storage gap for upgradeability // slither-disable-next-line shadowing-state - uint256[43] private __GAP; + uint256[42] private __GAP; } diff --git a/contracts/src/interfaces/IMachServiceManager.sol b/contracts/src/interfaces/IMachServiceManager.sol index ae59389..9bf5cb1 100644 --- a/contracts/src/interfaces/IMachServiceManager.sol +++ b/contracts/src/interfaces/IMachServiceManager.sol @@ -79,9 +79,9 @@ interface IMachServiceManager is IServiceManager { event AllowlistDisabled(); /** - * @notice Emitted when rollup chain id is changed + * @notice Emitted when rollup chain id is updated */ - event RollupChainIdUpdated(uint256 previousRollupChainId, uint256 newRollupChainId); + event RollupChainIDUpdated(uint256 rollupChainId, bool status); /** * @notice Emitted when a Alert is confirmed. diff --git a/contracts/test/AVSDeployer.sol b/contracts/test/AVSDeployer.sol index 944620f..858752c 100644 --- a/contracts/test/AVSDeployer.sol +++ b/contracts/test/AVSDeployer.sol @@ -271,7 +271,10 @@ contract AVSDeployer is Test { TransparentUpgradeableProxy(payable(address(serviceManager))), address(serviceManagerImplementation) ); - serviceManager.initialize(pauserRegistry, 0, proxyAdminOwner, proxyAdminOwner, proxyAdminOwner, 1); + uint256[] memory ids = new uint256[](2); + ids[0] = 1; + ids[1] = 2; + serviceManager.initialize(pauserRegistry, 0, proxyAdminOwner, proxyAdminOwner, proxyAdminOwner, ids); cheats.stopPrank(); } diff --git a/contracts/test/MachServiceManager.t.sol b/contracts/test/MachServiceManager.t.sol index e514560..c918598 100644 --- a/contracts/test/MachServiceManager.t.sol +++ b/contracts/test/MachServiceManager.t.sol @@ -15,7 +15,7 @@ contract MachServiceManagerTest is BLSAVSDeployer { event AlertConfirmerChanged(address previousAddress, address newAddress); event WhitelisterChanged(address previousAddress, address newAddress); event QuorumThresholdPercentageChanged(uint8 thresholdPercentages); - event RollupChainIdUpdated(uint256 previousRollupChainId, uint256 newRollupChainId); + event RollupChainIDUpdated(uint256 rollupChainId, bool status); event AlertConfirmed(bytes32 indexed alertHeaderHash, bytes32 messageHash); event AlertRemoved(bytes32 messageHash, address sender); @@ -207,19 +207,20 @@ contract MachServiceManagerTest is BLSAVSDeployer { vm.stopPrank(); } - function test_UpdateRollupChainId() public { + function test_SetRollupChainID() public { + assertTrue(serviceManager.rollupChainIDs(1), "mismatch"); + assertTrue(serviceManager.rollupChainIDs(2), "mismatch"); vm.startPrank(proxyAdminOwner); - assertTrue(serviceManager.rollupChainId() == 1, "mismatch"); vm.expectEmit(); - emit RollupChainIdUpdated(1, 42); - serviceManager.updateRollupChainId(42); - assertTrue(serviceManager.rollupChainId() == 42, "mismatch"); + emit RollupChainIDUpdated(42, true); + serviceManager.setRollupChainID(42, true); + assertTrue(serviceManager.rollupChainIDs(42), "mismatch"); vm.stopPrank(); } - function test_UpdateRollupChainId_RevertIfNotOwner() public { + function test_SetRollupChainID_RevertIfNotOwner() public { vm.expectRevert("Ownable: caller is not the owner"); - serviceManager.updateRollupChainId(42); + serviceManager.setRollupChainID(42, true); } function test_confirmAlert() public { From d64fa64a42f4bc69dace0336afb292c504ba427c Mon Sep 17 00:00:00 2001 From: noel Date: Thu, 16 May 2024 17:23:33 +0900 Subject: [PATCH 2/6] feat: alert per rollup chain ID --- contracts/src/core/MachServiceManager.sol | 37 +++++++--- .../src/core/MachServiceManagerStorage.sol | 4 +- contracts/src/error/Errors.sol | 2 + .../src/interfaces/IMachServiceManager.sol | 12 ++- contracts/test/MachServiceManager.t.sol | 74 ++++++++++++------- 5 files changed, 86 insertions(+), 43 deletions(-) diff --git a/contracts/src/core/MachServiceManager.sol b/contracts/src/core/MachServiceManager.sol index 4748030..963dc29 100644 --- a/contracts/src/core/MachServiceManager.sol +++ b/contracts/src/core/MachServiceManager.sol @@ -25,6 +25,8 @@ import { ZeroAddress, AlreadyInAllowlist, NotInAllowlist, + NoStatusChange, + InvalidRollupChainID, InvalidReferenceBlockNum, InsufficientThreshold, InvalidStartIndex, @@ -177,10 +179,10 @@ contract MachServiceManager is * @notice Remove an Alert. * @param messageHash The message hash of the alert */ - function removeAlert(bytes32 messageHash) external onlyOwner { - bool ret = _messageHashes.remove(messageHash); + function removeAlert(uint256 rollupChainId, bytes32 messageHash) external onlyOwner { + bool ret = _messageHashes[rollupChainId].remove(messageHash); if (ret) { - _resolvedMessageHashes.add(messageHash); + _resolvedMessageHashes[rollupChainId].add(messageHash); emit AlertRemoved(messageHash, _msgSender()); } } @@ -252,6 +254,7 @@ contract MachServiceManager is * - and check whether quorum has been achieved or not. */ function confirmAlert( + uint256 rollupChainId, AlertHeader calldata alertHeader, NonSignerStakesAndSignature memory nonSignerStakesAndSignature ) external whenNotPaused onlyAlertConfirmer { @@ -261,7 +264,7 @@ contract MachServiceManager is } // check is it is the resolved alert before - if (_resolvedMessageHashes.contains(alertHeader.messageHash)) { + if (_resolvedMessageHashes[rollupChainId].contains(alertHeader.messageHash)) { revert ResolvedAlert(); } @@ -305,7 +308,7 @@ contract MachServiceManager is } // store alert - bool success = _messageHashes.add(alertHeader.messageHash); + bool success = _messageHashes[rollupChainId].add(alertHeader.messageHash); if (!success) { revert AlreadyAdded(); } @@ -318,18 +321,22 @@ contract MachServiceManager is ////////////////////////////////////////////////////////////////////////////// /// @notice Returns the length of total alerts - function totalAlerts() external view returns (uint256) { - return _messageHashes.length(); + function totalAlerts(uint256 rollupChainId) external view returns (uint256) { + return _messageHashes[rollupChainId].length(); } /// @notice Checks if messageHash exists - function contains(bytes32 messageHash) external view returns (bool) { - return _messageHashes.contains(messageHash); + function contains(uint256 rollupChainId, bytes32 messageHash) external view returns (bool) { + return _messageHashes[rollupChainId].contains(messageHash); } /// @notice Returns an array of messageHash - function queryMessageHashes(uint256 start, uint256 querySize) external view returns (bytes32[] memory) { - uint256 length = _messageHashes.length(); + function queryMessageHashes(uint256 rollupChainId, uint256 start, uint256 querySize) + external + view + returns (bytes32[] memory) + { + uint256 length = _messageHashes[rollupChainId].length(); if (start >= length) { revert InvalidStartIndex(); @@ -343,7 +350,7 @@ contract MachServiceManager is bytes32[] memory output = new bytes32[](end - start); for (uint256 i = start; i < end; ++i) { - output[i - start] = _messageHashes.at(i); + output[i - start] = _messageHashes[rollupChainId].at(i); } return output; @@ -388,6 +395,12 @@ contract MachServiceManager is } function _setRollupChainID(uint256 rollupChainId, bool status) internal { + if (rollupChainId < 1) { + revert InvalidRollupChainID(); + } + if (rollupChainIDs[rollupChainId] == status) { + revert NoStatusChange(); + } rollupChainIDs[rollupChainId] = status; emit RollupChainIDUpdated(rollupChainId, status); } diff --git a/contracts/src/core/MachServiceManagerStorage.sol b/contracts/src/core/MachServiceManagerStorage.sol index 0d7073b..1227ecd 100644 --- a/contracts/src/core/MachServiceManagerStorage.sol +++ b/contracts/src/core/MachServiceManagerStorage.sol @@ -15,7 +15,7 @@ abstract contract MachServiceManagerStorage { uint256 public constant THRESHOLD_DENOMINATOR = 100; // Slot 0 - EnumerableSet.Bytes32Set internal _messageHashes; + mapping(uint256 => EnumerableSet.Bytes32Set) internal _messageHashes; // Slot 1 /// @notice Ethereum addresses of currently register operators @@ -37,7 +37,7 @@ abstract contract MachServiceManagerStorage { // slot 4 /// @notice Resolved message hashes, prevent aggregator from replay any resolved alert - EnumerableSet.Bytes32Set internal _resolvedMessageHashes; + mapping(uint256 => EnumerableSet.Bytes32Set) internal _resolvedMessageHashes; // slot 5 /// @notice Role for whitelisting operators diff --git a/contracts/src/error/Errors.sol b/contracts/src/error/Errors.sol index 9ac3246..7a5389d 100644 --- a/contracts/src/error/Errors.sol +++ b/contracts/src/error/Errors.sol @@ -6,6 +6,8 @@ error InvalidStartIndex(); error InvalidConfirmer(); error NotWhitelister(); error InvalidSender(); +error NoStatusChange(); +error InvalidRollupChainID(); error InvalidReferenceBlockNum(); error InsufficientThreshold(); error InsufficientThresholdPercentages(); diff --git a/contracts/src/interfaces/IMachServiceManager.sol b/contracts/src/interfaces/IMachServiceManager.sol index 9bf5cb1..9607258 100644 --- a/contracts/src/interfaces/IMachServiceManager.sol +++ b/contracts/src/interfaces/IMachServiceManager.sol @@ -123,7 +123,7 @@ interface IMachServiceManager is IServiceManager { * @notice Remove an Alert. * @param messageHash The message hash of the alert */ - function removeAlert(bytes32 messageHash) external; + function removeAlert(uint256 rollupChainId, bytes32 messageHash) external; /** * @notice Update quorum threshold percentage @@ -138,16 +138,20 @@ interface IMachServiceManager is IServiceManager { * - and check whether quorum has been achieved or not. */ function confirmAlert( + uint256 rollupChainId, AlertHeader calldata alertHeader, BLSSignatureChecker.NonSignerStakesAndSignature memory nonSignerStakesAndSignature ) external; /// @notice Returns the length of total alerts - function totalAlerts() external view returns (uint256); + function totalAlerts(uint256 rollupChainId) external view returns (uint256); /// @notice Checks if messageHash exists - function contains(bytes32 messageHash) external view returns (bool); + function contains(uint256 rollupChainId, bytes32 messageHash) external view returns (bool); /// @notice Returns an array of messageHash - function queryMessageHashes(uint256 start, uint256 querySize) external view returns (bytes32[] memory); + function queryMessageHashes(uint256 rollupChainId, uint256 start, uint256 querySize) + external + view + returns (bytes32[] memory); } diff --git a/contracts/test/MachServiceManager.t.sol b/contracts/test/MachServiceManager.t.sol index c918598..d0df13b 100644 --- a/contracts/test/MachServiceManager.t.sol +++ b/contracts/test/MachServiceManager.t.sol @@ -34,6 +34,14 @@ contract MachServiceManagerTest is BLSAVSDeployer { _setAggregatePublicKeysAndSignature(); } + function test_Init_RevertIfImpleBeingInitialized() public { + uint256[] memory ids = new uint256[](0); + vm.expectRevert("Initializable: contract is already initialized"); + serviceManagerImplementation.initialize( + pauserRegistry, 0, proxyAdminOwner, proxyAdminOwner, proxyAdminOwner, ids + ); + } + function test_SetConfirmer() public { address newConfirmer = address(42); vm.startPrank(proxyAdminOwner); @@ -223,6 +231,22 @@ contract MachServiceManagerTest is BLSAVSDeployer { serviceManager.setRollupChainID(42, true); } + function test_SetRollupChainID_RevertIfInvalidRollupChainID() public { + vm.startPrank(proxyAdminOwner); + + vm.expectRevert(InvalidRollupChainID.selector); + serviceManager.setRollupChainID(0, true); + vm.stopPrank(); + } + + function test_SetRollupChainID_RevertIfNoStatusChange() public { + vm.startPrank(proxyAdminOwner); + + vm.expectRevert(NoStatusChange.selector); + serviceManager.setRollupChainID(1, true); + vm.stopPrank(); + } + function test_confirmAlert() public { vm.startPrank(proxyAdminOwner); serviceManager.disableAllowlist(); @@ -245,14 +269,14 @@ contract MachServiceManagerTest is BLSAVSDeployer { vm.startPrank(proxyAdminOwner); vm.expectEmit(); - assertEq(serviceManager.totalAlerts(), 0); - assertFalse(serviceManager.contains("foo")); + assertEq(serviceManager.totalAlerts(1), 0); + assertFalse(serviceManager.contains(1, "foo")); emit AlertConfirmed(msgHash, alertHeader.messageHash); - serviceManager.confirmAlert(alertHeader, nonSignerStakesAndSignature); + serviceManager.confirmAlert(1, alertHeader, nonSignerStakesAndSignature); - assertEq(serviceManager.totalAlerts(), 1); - assertTrue(serviceManager.contains("foo")); + assertEq(serviceManager.totalAlerts(1), 1); + assertTrue(serviceManager.contains(1, "foo")); vm.stopPrank(); } @@ -277,7 +301,7 @@ contract MachServiceManagerTest is BLSAVSDeployer { referenceBlockNumber: referenceBlockNumber }); vm.expectRevert(InvalidConfirmer.selector); - serviceManager.confirmAlert(alertHeader, nonSignerStakesAndSignature); + serviceManager.confirmAlert(1, alertHeader, nonSignerStakesAndSignature); } function test_confirmAlert_RevertIfInvalidSender() public { @@ -306,7 +330,7 @@ contract MachServiceManagerTest is BLSAVSDeployer { vm.startPrank(address(this)); vm.expectRevert(InvalidSender.selector); - serviceManager.confirmAlert(alertHeader, nonSignerStakesAndSignature); + serviceManager.confirmAlert(1, alertHeader, nonSignerStakesAndSignature); vm.stopPrank(); } @@ -331,9 +355,9 @@ contract MachServiceManagerTest is BLSAVSDeployer { }); vm.startPrank(proxyAdminOwner); - serviceManager.confirmAlert(alertHeader, nonSignerStakesAndSignature); + serviceManager.confirmAlert(1, alertHeader, nonSignerStakesAndSignature); vm.expectRevert(AlreadyAdded.selector); - serviceManager.confirmAlert(alertHeader, nonSignerStakesAndSignature); + serviceManager.confirmAlert(1, alertHeader, nonSignerStakesAndSignature); vm.stopPrank(); } @@ -359,7 +383,7 @@ contract MachServiceManagerTest is BLSAVSDeployer { vm.startPrank(proxyAdminOwner); vm.expectRevert(InvalidQuorumParam.selector); - serviceManager.confirmAlert(alertHeader, nonSignerStakesAndSignature); + serviceManager.confirmAlert(1, alertHeader, nonSignerStakesAndSignature); vm.stopPrank(); } @@ -384,11 +408,11 @@ contract MachServiceManagerTest is BLSAVSDeployer { }); vm.startPrank(proxyAdminOwner); - serviceManager.confirmAlert(alertHeader, nonSignerStakesAndSignature); - serviceManager.removeAlert("foo"); + serviceManager.confirmAlert(1, alertHeader, nonSignerStakesAndSignature); + serviceManager.removeAlert(1, "foo"); vm.expectRevert(ResolvedAlert.selector); - serviceManager.confirmAlert(alertHeader, nonSignerStakesAndSignature); + serviceManager.confirmAlert(1, alertHeader, nonSignerStakesAndSignature); vm.stopPrank(); } @@ -414,7 +438,7 @@ contract MachServiceManagerTest is BLSAVSDeployer { vm.startPrank(proxyAdminOwner); vm.expectRevert(InvalidReferenceBlockNum.selector); - serviceManager.confirmAlert(alertHeader, nonSignerStakesAndSignature); + serviceManager.confirmAlert(1, alertHeader, nonSignerStakesAndSignature); vm.stopPrank(); } @@ -440,7 +464,7 @@ contract MachServiceManagerTest is BLSAVSDeployer { vm.startPrank(proxyAdminOwner); vm.expectRevert(InvalidQuorumThresholdPercentage.selector); - serviceManager.confirmAlert(alertHeader, nonSignerStakesAndSignature); + serviceManager.confirmAlert(1, alertHeader, nonSignerStakesAndSignature); vm.stopPrank(); } @@ -466,7 +490,7 @@ contract MachServiceManagerTest is BLSAVSDeployer { vm.startPrank(proxyAdminOwner); vm.expectRevert(InsufficientThresholdPercentages.selector); - serviceManager.confirmAlert(alertHeader, nonSignerStakesAndSignature); + serviceManager.confirmAlert(1, alertHeader, nonSignerStakesAndSignature); vm.stopPrank(); } @@ -492,34 +516,34 @@ contract MachServiceManagerTest is BLSAVSDeployer { vm.startPrank(proxyAdminOwner); vm.expectRevert(InsufficientThreshold.selector); - serviceManager.confirmAlert(alertHeader, nonSignerStakesAndSignature); + serviceManager.confirmAlert(1, alertHeader, nonSignerStakesAndSignature); vm.stopPrank(); } function test_removeAlert() public { test_confirmAlert(); vm.startPrank(proxyAdminOwner); - assertEq(serviceManager.totalAlerts(), 1); - assertTrue(serviceManager.contains("foo")); + assertEq(serviceManager.totalAlerts(1), 1); + assertTrue(serviceManager.contains(1, "foo")); vm.expectEmit(); emit AlertRemoved("foo", msg.sender); - serviceManager.removeAlert("foo"); + serviceManager.removeAlert(1, "foo"); - assertFalse(serviceManager.contains("foo")); - assertEq(serviceManager.totalAlerts(), 0); + assertFalse(serviceManager.contains(1, "foo")); + assertEq(serviceManager.totalAlerts(1), 0); vm.stopPrank(); } function test_removeAlert_RevertIfNotOwner() public { test_confirmAlert(); vm.expectRevert("Ownable: caller is not the owner"); - serviceManager.removeAlert("foo"); + serviceManager.removeAlert(1, "foo"); } function test_QueryMessageHashes() public { test_confirmAlert(); - bytes32[] memory results = serviceManager.queryMessageHashes(0, 2); + bytes32[] memory results = serviceManager.queryMessageHashes(1, 0, 2); assertTrue(results.length == 1); assertTrue(results[0] == "foo"); } @@ -527,6 +551,6 @@ contract MachServiceManagerTest is BLSAVSDeployer { function test_QueryMessageHashes_RevertIfInvalidStartIndex() public { test_confirmAlert(); vm.expectRevert(InvalidStartIndex.selector); - bytes32[] memory results = serviceManager.queryMessageHashes(1, 2); + bytes32[] memory results = serviceManager.queryMessageHashes(1, 1, 2); } } From 3d153976ae5043ce575e620f36af880b0b784ccd Mon Sep 17 00:00:00 2001 From: noel Date: Thu, 16 May 2024 19:35:23 +0900 Subject: [PATCH 3/6] fix: check if rollup chain ID is valid --- contracts/src/core/MachServiceManager.sol | 16 +++++- contracts/test/MachServiceManager.t.sol | 66 +++++++++++++++++------ 2 files changed, 64 insertions(+), 18 deletions(-) diff --git a/contracts/src/core/MachServiceManager.sol b/contracts/src/core/MachServiceManager.sol index 963dc29..9371af7 100644 --- a/contracts/src/core/MachServiceManager.sol +++ b/contracts/src/core/MachServiceManager.sol @@ -74,6 +74,14 @@ contract MachServiceManager is _; } + /// @notice when applied to a function, ensures that the `rollupChainID` is valid. + modifier onlyValidRollupChainID(uint256 rollupChainID) { + if (!rollupChainIDs[rollupChainID]) { + revert InvalidRollupChainID(); + } + _; + } + constructor( IAVSDirectory __avsDirectory, IRegistryCoordinator __registryCoordinator, @@ -179,7 +187,11 @@ contract MachServiceManager is * @notice Remove an Alert. * @param messageHash The message hash of the alert */ - function removeAlert(uint256 rollupChainId, bytes32 messageHash) external onlyOwner { + function removeAlert(uint256 rollupChainId, bytes32 messageHash) + external + onlyValidRollupChainID(rollupChainId) + onlyOwner + { bool ret = _messageHashes[rollupChainId].remove(messageHash); if (ret) { _resolvedMessageHashes[rollupChainId].add(messageHash); @@ -257,7 +269,7 @@ contract MachServiceManager is uint256 rollupChainId, AlertHeader calldata alertHeader, NonSignerStakesAndSignature memory nonSignerStakesAndSignature - ) external whenNotPaused onlyAlertConfirmer { + ) external whenNotPaused onlyAlertConfirmer onlyValidRollupChainID(rollupChainId) { // make sure the information needed to derive the non-signers and batch is in calldata to avoid emitting events if (tx.origin != msg.sender) { revert InvalidSender(); diff --git a/contracts/test/MachServiceManager.t.sol b/contracts/test/MachServiceManager.t.sol index d0df13b..07fde4b 100644 --- a/contracts/test/MachServiceManager.t.sol +++ b/contracts/test/MachServiceManager.t.sol @@ -247,7 +247,7 @@ contract MachServiceManagerTest is BLSAVSDeployer { vm.stopPrank(); } - function test_confirmAlert() public { + function test_ConfirmAlert() public { vm.startPrank(proxyAdminOwner); serviceManager.disableAllowlist(); vm.stopPrank(); @@ -281,7 +281,7 @@ contract MachServiceManagerTest is BLSAVSDeployer { vm.stopPrank(); } - function test_confirmAlert_RevertIfInvalidConfirmer() public { + function test_ConfirmAlert_RevertIfInvalidConfirmer() public { vm.startPrank(proxyAdminOwner); serviceManager.disableAllowlist(); vm.stopPrank(); @@ -304,7 +304,7 @@ contract MachServiceManagerTest is BLSAVSDeployer { serviceManager.confirmAlert(1, alertHeader, nonSignerStakesAndSignature); } - function test_confirmAlert_RevertIfInvalidSender() public { + function test_ConfirmAlert_RevertIfInvalidSender() public { vm.startPrank(proxyAdminOwner); serviceManager.disableAllowlist(); vm.stopPrank(); @@ -334,7 +334,7 @@ contract MachServiceManagerTest is BLSAVSDeployer { vm.stopPrank(); } - function test_confirmAlert_RevertIfAlreadyAdded() public { + function test_ConfirmAlert_RevertIfAlreadyAdded() public { vm.startPrank(proxyAdminOwner); serviceManager.disableAllowlist(); vm.stopPrank(); @@ -361,7 +361,7 @@ contract MachServiceManagerTest is BLSAVSDeployer { vm.stopPrank(); } - function test_confirmAlert_RevertIfInvalidQuorumParam() public { + function test_ConfirmAlert_RevertIfInvalidQuorumParam() public { vm.startPrank(proxyAdminOwner); serviceManager.disableAllowlist(); vm.stopPrank(); @@ -387,7 +387,7 @@ contract MachServiceManagerTest is BLSAVSDeployer { vm.stopPrank(); } - function test_confirmAlert_RevertIfResolvedAlert() public { + function test_ConfirmAlert_RevertIfResolvedAlert() public { vm.startPrank(proxyAdminOwner); serviceManager.disableAllowlist(); vm.stopPrank(); @@ -416,7 +416,7 @@ contract MachServiceManagerTest is BLSAVSDeployer { vm.stopPrank(); } - function test_confirmAlert_RevertIfInvalidReferenceBlockNum() public { + function test_ConfirmAlert_RevertIfInvalidReferenceBlockNum() public { vm.startPrank(proxyAdminOwner); serviceManager.disableAllowlist(); vm.stopPrank(); @@ -442,7 +442,7 @@ contract MachServiceManagerTest is BLSAVSDeployer { vm.stopPrank(); } - function test_confirmAlert_RevertIfInvalidQuorumThresholdPercentage() public { + function test_ConfirmAlert_RevertIfInvalidQuorumThresholdPercentage() public { vm.startPrank(proxyAdminOwner); serviceManager.disableAllowlist(); vm.stopPrank(); @@ -468,7 +468,7 @@ contract MachServiceManagerTest is BLSAVSDeployer { vm.stopPrank(); } - function test_confirmAlert_RevertIfInsufficientThresholdPercentages() public { + function test_ConfirmAlert_RevertIfInsufficientThresholdPercentages() public { vm.startPrank(proxyAdminOwner); serviceManager.disableAllowlist(); vm.stopPrank(); @@ -494,7 +494,7 @@ contract MachServiceManagerTest is BLSAVSDeployer { vm.stopPrank(); } - function test_confirmAlert_RevertIfInsufficientThreshold() public { + function test_ConfirmAlert_RevertIfInsufficientThreshold() public { vm.startPrank(proxyAdminOwner); serviceManager.disableAllowlist(); vm.stopPrank(); @@ -520,8 +520,34 @@ contract MachServiceManagerTest is BLSAVSDeployer { vm.stopPrank(); } - function test_removeAlert() public { - test_confirmAlert(); + function test_ConfirmAlert_RevertIfInvalidRollupChainID() public { + vm.startPrank(proxyAdminOwner); + serviceManager.disableAllowlist(); + vm.stopPrank(); + + ( + uint32 referenceBlockNumber, + BLSSignatureChecker.NonSignerStakesAndSignature memory nonSignerStakesAndSignature + ) = _registerSignatoriesAndGetNonSignerStakeAndSignatureRandom(nonRandomNumber, 1, quorumBitmap); + + bytes memory quorumThresholdPercentages = new bytes(1); + quorumThresholdPercentages[0] = bytes1(uint8(67)); + + IMachServiceManager.AlertHeader memory alertHeader = IMachServiceManager.AlertHeader({ + messageHash: "foo", + quorumNumbers: quorumNumbers, + quorumThresholdPercentages: quorumThresholdPercentages, + referenceBlockNumber: referenceBlockNumber + }); + + vm.startPrank(proxyAdminOwner); + vm.expectRevert(InvalidRollupChainID.selector); + serviceManager.confirmAlert(99, alertHeader, nonSignerStakesAndSignature); + vm.stopPrank(); + } + + function test_RemoveAlert() public { + test_ConfirmAlert(); vm.startPrank(proxyAdminOwner); assertEq(serviceManager.totalAlerts(1), 1); assertTrue(serviceManager.contains(1, "foo")); @@ -535,21 +561,29 @@ contract MachServiceManagerTest is BLSAVSDeployer { vm.stopPrank(); } - function test_removeAlert_RevertIfNotOwner() public { - test_confirmAlert(); + function test_RemoveAlert_RevertIfInvalidRollupChainID() public { + test_ConfirmAlert(); + vm.startPrank(proxyAdminOwner); + vm.expectRevert(InvalidRollupChainID.selector); + serviceManager.removeAlert(42, "foo"); + vm.stopPrank(); + } + + function test_RemoveAlert_RevertIfNotOwner() public { + test_ConfirmAlert(); vm.expectRevert("Ownable: caller is not the owner"); serviceManager.removeAlert(1, "foo"); } function test_QueryMessageHashes() public { - test_confirmAlert(); + test_ConfirmAlert(); bytes32[] memory results = serviceManager.queryMessageHashes(1, 0, 2); assertTrue(results.length == 1); assertTrue(results[0] == "foo"); } function test_QueryMessageHashes_RevertIfInvalidStartIndex() public { - test_confirmAlert(); + test_ConfirmAlert(); vm.expectRevert(InvalidStartIndex.selector); bytes32[] memory results = serviceManager.queryMessageHashes(1, 1, 2); } From 373200a25a016858e15f8ba97de8be33b121f933 Mon Sep 17 00:00:00 2001 From: noel Date: Fri, 17 May 2024 00:49:28 +0900 Subject: [PATCH 4/6] feat: IMachServiceManager --- contracts/src/core/MachServiceManager.sol | 64 +++++++++++-------- .../src/interfaces/IMachServiceManager.sol | 17 +++++ 2 files changed, 53 insertions(+), 28 deletions(-) diff --git a/contracts/src/core/MachServiceManager.sol b/contracts/src/core/MachServiceManager.sol index 9371af7..66298cd 100644 --- a/contracts/src/core/MachServiceManager.sol +++ b/contracts/src/core/MachServiceManager.sol @@ -119,8 +119,7 @@ contract MachServiceManager is ////////////////////////////////////////////////////////////////////////////// /** - * @notice Add an operator to the allowlist. - * @param operator The operator to add + * @inheritdoc IMachServiceManager */ function addToAllowlist(address operator) external onlyWhitelister { if (operator == address(0)) { @@ -134,8 +133,7 @@ contract MachServiceManager is } /** - * @notice Remove an operator from the allowlist. - * @param operator The operator to remove + * @inheritdoc IMachServiceManager */ function removeFromAllowlist(address operator) external onlyWhitelister { if (!allowlist[operator]) { @@ -146,7 +144,7 @@ contract MachServiceManager is } /** - * @notice Enable the allowlist. + * @inheritdoc IMachServiceManager */ function enableAllowlist() external onlyOwner { if (allowlistEnabled) { @@ -158,7 +156,7 @@ contract MachServiceManager is } /** - * @notice Disable the allowlist. + * @inheritdoc IMachServiceManager */ function disableAllowlist() external onlyOwner { if (!allowlistEnabled) { @@ -170,22 +168,28 @@ contract MachServiceManager is } /** - * @notice Set confirmer address. + * @inheritdoc IMachServiceManager */ function setConfirmer(address confirmer) external onlyOwner { _setAlertConfirmer(confirmer); } /** - * @notice Set whitelister address. + * @inheritdoc IMachServiceManager */ function setWhitelister(address whitelister) external onlyOwner { _setWhitelister(whitelister); } /** - * @notice Remove an Alert. - * @param messageHash The message hash of the alert + * @inheritdoc IMachServiceManager + */ + function setRollupChainID(uint256 rollupChainId, bool status) external onlyOwner { + _setRollupChainID(rollupChainId, status); + } + + /** + * @inheritdoc IMachServiceManager */ function removeAlert(uint256 rollupChainId, bytes32 messageHash) external @@ -200,8 +204,7 @@ contract MachServiceManager is } /** - * @notice Update quorum threshold percentage - * @param thresholdPercentage The new quorum threshold percentage + * @inheritdoc IMachServiceManager */ function updateQuorumThresholdPercentage(uint8 thresholdPercentage) external onlyOwner { if (thresholdPercentage > 100) { @@ -211,10 +214,6 @@ contract MachServiceManager is emit QuorumThresholdPercentageChanged(thresholdPercentage); } - function setRollupChainID(uint256 rollupChainid, bool status) external onlyOwner { - _setRollupChainID(rollupChainid, status); - } - ////////////////////////////////////////////////////////////////////////////// // Operator Registration // ////////////////////////////////////////////////////////////////////////////// @@ -260,10 +259,7 @@ contract MachServiceManager is ////////////////////////////////////////////////////////////////////////////// /** - * @notice This function is used for - * - submitting alert, - * - check that the aggregate signature is valid, - * - and check whether quorum has been achieved or not. + * @inheritdoc IMachServiceManager */ function confirmAlert( uint256 rollupChainId, @@ -332,17 +328,23 @@ contract MachServiceManager is // View Functions // ////////////////////////////////////////////////////////////////////////////// - /// @notice Returns the length of total alerts + /** + * @inheritdoc IMachServiceManager + */ function totalAlerts(uint256 rollupChainId) external view returns (uint256) { return _messageHashes[rollupChainId].length(); } - /// @notice Checks if messageHash exists + /** + * @inheritdoc IMachServiceManager + */ function contains(uint256 rollupChainId, bytes32 messageHash) external view returns (bool) { return _messageHashes[rollupChainId].contains(messageHash); } - /// @notice Returns an array of messageHash + /** + * @inheritdoc IMachServiceManager + */ function queryMessageHashes(uint256 rollupChainId, uint256 start, uint256 querySize) external view @@ -372,29 +374,35 @@ contract MachServiceManager is // Internal Functions // ////////////////////////////////////////////////////////////////////////////// - /// @notice hash the alert header + /** + * @dev hash the alert header + */ function _hashAlertHeader(AlertHeader calldata alertHeader) internal pure returns (bytes32) { return keccak256(abi.encode(_convertAlertHeaderToReducedAlertHeader(alertHeader))); } - /// @notice changes the alert confirmer + /** + * @dev changes the alert confirmer + */ function _setAlertConfirmer(address _alertConfirmer) internal { address previousBatchConfirmer = alertConfirmer; alertConfirmer = _alertConfirmer; emit AlertConfirmerChanged(previousBatchConfirmer, alertConfirmer); } - /// @notice changes the whitelister + /** + * @dev changes the whitelister + */ function _setWhitelister(address _whitelister) internal { address previousWhitelister = whitelister; whitelister = _whitelister; emit WhitelisterChanged(previousWhitelister, _whitelister); } + /** - * @notice converts a alert header to a reduced alert header + * @dev converts a alert header to a reduced alert header * @param alertHeader the alert header to convert */ - function _convertAlertHeaderToReducedAlertHeader(AlertHeader calldata alertHeader) internal pure diff --git a/contracts/src/interfaces/IMachServiceManager.sol b/contracts/src/interfaces/IMachServiceManager.sol index 9607258..e4d8ab3 100644 --- a/contracts/src/interfaces/IMachServiceManager.sol +++ b/contracts/src/interfaces/IMachServiceManager.sol @@ -119,6 +119,23 @@ interface IMachServiceManager is IServiceManager { */ function disableAllowlist() external; + /** + * @notice Set confirmer address. + */ + function setConfirmer(address confirmer) external; + + /** + * @notice Set whitelister address. + */ + function setWhitelister(address whitelister) external; + + /** + * @notice Set the status of a rollup chain ID + * @param rollupChainId The ID of the rollup chain to be updated + * @param status The new status for the rollup chain ID (true for active, false for inactive) + */ + function setRollupChainID(uint256 rollupChainId, bool status) external; + /** * @notice Remove an Alert. * @param messageHash The message hash of the alert From 95e824e4c5c9c946772577ae0ce192ceb7d4e1ba Mon Sep 17 00:00:00 2001 From: noel Date: Fri, 17 May 2024 11:04:08 +0900 Subject: [PATCH 5/6] refactor: natspec comments --- contracts/src/core/MachServiceManager.sol | 27 +++++++++++++---------- 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/contracts/src/core/MachServiceManager.sol b/contracts/src/core/MachServiceManager.sol index 66298cd..14000f4 100644 --- a/contracts/src/core/MachServiceManager.sol +++ b/contracts/src/core/MachServiceManager.sol @@ -58,7 +58,9 @@ contract MachServiceManager is using EnumerableSet for EnumerableSet.Bytes32Set; using EnumerableSet for EnumerableSet.AddressSet; - /// @notice when applied to a function, ensures that the function is only callable by the `alertConfirmer`. + /** + * @dev Ensures that the function is only callable by the `alertConfirmer`. + */ modifier onlyAlertConfirmer() { if (_msgSender() != alertConfirmer) { revert InvalidConfirmer(); @@ -66,7 +68,9 @@ contract MachServiceManager is _; } - /// @notice when applied to a function, ensures that the function is only callable by the `whitelister`. + /** + * @dev Ensures that the function is only callable by the `whitelister`. + */ modifier onlyWhitelister() { if (_msgSender() != whitelister) { revert NotWhitelister(); @@ -74,7 +78,9 @@ contract MachServiceManager is _; } - /// @notice when applied to a function, ensures that the `rollupChainID` is valid. + /** + * @dev Ensures that the `rollupChainID` is valid. + */ modifier onlyValidRollupChainID(uint256 rollupChainID) { if (!rollupChainIDs[rollupChainID]) { revert InvalidRollupChainID(); @@ -219,9 +225,7 @@ contract MachServiceManager is ////////////////////////////////////////////////////////////////////////////// /** - * @notice Register an operator with the AVS. Forwards call to EigenLayer' AVSDirectory. - * @param operator The address of the operator to register. - * @param operatorSignature The signature, salt, and expiry of the operator's signature. + * @inheritdoc IServiceManager */ function registerOperatorToAVS( address operator, @@ -240,8 +244,7 @@ contract MachServiceManager is } /** - * @notice Deregister an operator from the AVS. Forwards a call to EigenLayer's AVSDirectory. - * @param operator The address of the operator to register. + * @inheritdoc IServiceManager */ function deregisterOperatorFromAVS(address operator) public @@ -375,14 +378,14 @@ contract MachServiceManager is ////////////////////////////////////////////////////////////////////////////// /** - * @dev hash the alert header + * @dev Hashes an alert header */ function _hashAlertHeader(AlertHeader calldata alertHeader) internal pure returns (bytes32) { return keccak256(abi.encode(_convertAlertHeaderToReducedAlertHeader(alertHeader))); } /** - * @dev changes the alert confirmer + * @dev Changes the alert confirmer */ function _setAlertConfirmer(address _alertConfirmer) internal { address previousBatchConfirmer = alertConfirmer; @@ -391,7 +394,7 @@ contract MachServiceManager is } /** - * @dev changes the whitelister + * @dev Changes the whitelister */ function _setWhitelister(address _whitelister) internal { address previousWhitelister = whitelister; @@ -400,7 +403,7 @@ contract MachServiceManager is } /** - * @dev converts a alert header to a reduced alert header + * @dev Converts a alert header to a reduced alert header * @param alertHeader the alert header to convert */ function _convertAlertHeaderToReducedAlertHeader(AlertHeader calldata alertHeader) From bf7c3aa005f8cbf4a1246b3559693e8b1e491607 Mon Sep 17 00:00:00 2001 From: noel Date: Fri, 17 May 2024 11:10:44 +0900 Subject: [PATCH 6/6] refactor: __DEPRECATED_SLOT6 --- contracts/src/core/MachServiceManagerStorage.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/src/core/MachServiceManagerStorage.sol b/contracts/src/core/MachServiceManagerStorage.sol index 1227ecd..3f1902a 100644 --- a/contracts/src/core/MachServiceManagerStorage.sol +++ b/contracts/src/core/MachServiceManagerStorage.sol @@ -45,7 +45,7 @@ abstract contract MachServiceManagerStorage { // slot 6 /// @notice Rollup chain id - uint256 private __deprecatedSlot6; // rollupChainId + uint256 private __DEPRECATED_SLOT6; // rollupChainId // slot 7 /// @notice Allowed rollup chain IDs