From 836060240bc4d2ea0871e633b5802419cadcc95b Mon Sep 17 00:00:00 2001 From: Yorke Rhodes Date: Wed, 6 Nov 2024 16:16:19 -0500 Subject: [PATCH] feat: mutable storage ISMs (#4577) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### Description Some chains like zkSync do not support eip1167 (minimal/meta) proxies. This PR adds an alternative storage based multisig and aggregation ISM for use on these chains. ### Drive-by changes Simplify CLI multisig interactive config builder. Remove stale multisig config. ### Related issues None ### Backward compatibility Yes, relayer already supports this module type ### Testing Contract unit tests Manual CLI tests ![Screenshot 2024-10-02 at 4 05 08 PM](https://github.com/user-attachments/assets/c7fec896-ea7c-4fd9-a313-463168e66a82) --- .changeset/lazy-carpets-nail.md | 7 + .../interfaces/IThresholdAddressFactory.sol | 9 ++ .../aggregation/AbstractAggregationIsm.sol | 3 +- .../isms/aggregation/StaticAggregationIsm.sol | 2 +- .../aggregation/StorageAggregationIsm.sol | 89 +++++++++++ .../isms/multisig/AbstractMultisigIsm.sol | 2 +- .../isms/multisig/StorageMultisigIsm.sol | 143 ++++++++++++++++++ .../libs/StaticAddressSetFactory.sol | 10 +- solidity/test/hooks/AggregationHook.t.sol | 2 + solidity/test/isms/AggregationIsm.t.sol | 30 +++- solidity/test/isms/MultisigIsm.t.sol | 111 +++++++++++++- solidity/test/isms/WeightedMultisigIsm.t.sol | 16 ++ typescript/cli/src/config/ism.ts | 88 +++++------ typescript/cli/src/tests/multisig.test.ts | 41 ----- .../tests/multisig/invalid-address-fail.yaml | 8 - .../src/tests/multisig/safe-parse-fail.yaml | 6 - .../src/tests/multisig/threshold-gt-fail.yaml | 8 - typescript/sdk/src/index.ts | 6 +- .../sdk/src/ism/EvmIsmModule.hardhat-test.ts | 8 +- typescript/sdk/src/ism/HyperlaneIsmFactory.ts | 100 +++++++++--- .../sdk/src/ism/metadata/aggregation.ts | 2 +- typescript/sdk/src/ism/schemas.ts | 2 + typescript/sdk/src/ism/types.ts | 11 +- 23 files changed, 549 insertions(+), 155 deletions(-) create mode 100644 .changeset/lazy-carpets-nail.md create mode 100644 solidity/contracts/interfaces/IThresholdAddressFactory.sol create mode 100644 solidity/contracts/isms/aggregation/StorageAggregationIsm.sol create mode 100644 solidity/contracts/isms/multisig/StorageMultisigIsm.sol delete mode 100644 typescript/cli/src/tests/multisig.test.ts delete mode 100644 typescript/cli/src/tests/multisig/invalid-address-fail.yaml delete mode 100644 typescript/cli/src/tests/multisig/safe-parse-fail.yaml delete mode 100644 typescript/cli/src/tests/multisig/threshold-gt-fail.yaml diff --git a/.changeset/lazy-carpets-nail.md b/.changeset/lazy-carpets-nail.md new file mode 100644 index 000000000..e838512e8 --- /dev/null +++ b/.changeset/lazy-carpets-nail.md @@ -0,0 +1,7 @@ +--- +'@hyperlane-xyz/cli': minor +'@hyperlane-xyz/sdk': minor +'@hyperlane-xyz/core': minor +--- + +Add storage based multisig ISM types diff --git a/solidity/contracts/interfaces/IThresholdAddressFactory.sol b/solidity/contracts/interfaces/IThresholdAddressFactory.sol new file mode 100644 index 000000000..f013c519a --- /dev/null +++ b/solidity/contracts/interfaces/IThresholdAddressFactory.sol @@ -0,0 +1,9 @@ +// SPDX-License-Identifier: MIT OR Apache-2.0 +pragma solidity >=0.8.0; + +interface IThresholdAddressFactory { + function deploy( + address[] calldata _values, + uint8 _threshold + ) external returns (address); +} diff --git a/solidity/contracts/isms/aggregation/AbstractAggregationIsm.sol b/solidity/contracts/isms/aggregation/AbstractAggregationIsm.sol index a4181b473..2ea79ec29 100644 --- a/solidity/contracts/isms/aggregation/AbstractAggregationIsm.sol +++ b/solidity/contracts/isms/aggregation/AbstractAggregationIsm.sol @@ -8,13 +8,14 @@ import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; import {IInterchainSecurityModule} from "../../interfaces/IInterchainSecurityModule.sol"; import {IAggregationIsm} from "../../interfaces/isms/IAggregationIsm.sol"; import {AggregationIsmMetadata} from "../../isms/libs/AggregationIsmMetadata.sol"; +import {PackageVersioned} from "../../PackageVersioned.sol"; /** * @title AggregationIsm * @notice Manages per-domain m-of-n ISM sets that are used to verify * interchain messages. */ -abstract contract AbstractAggregationIsm is IAggregationIsm { +abstract contract AbstractAggregationIsm is IAggregationIsm, PackageVersioned { // ============ Constants ============ // solhint-disable-next-line const-name-snakecase diff --git a/solidity/contracts/isms/aggregation/StaticAggregationIsm.sol b/solidity/contracts/isms/aggregation/StaticAggregationIsm.sol index 7a64369af..07679093a 100644 --- a/solidity/contracts/isms/aggregation/StaticAggregationIsm.sol +++ b/solidity/contracts/isms/aggregation/StaticAggregationIsm.sol @@ -12,7 +12,7 @@ import {PackageVersioned} from "contracts/PackageVersioned.sol"; * @notice Manages per-domain m-of-n ISM sets that are used to verify * interchain messages. */ -contract StaticAggregationIsm is AbstractAggregationIsm, PackageVersioned { +contract StaticAggregationIsm is AbstractAggregationIsm { // ============ Public Functions ============ /** diff --git a/solidity/contracts/isms/aggregation/StorageAggregationIsm.sol b/solidity/contracts/isms/aggregation/StorageAggregationIsm.sol new file mode 100644 index 000000000..c69128a9d --- /dev/null +++ b/solidity/contracts/isms/aggregation/StorageAggregationIsm.sol @@ -0,0 +1,89 @@ +// SPDX-License-Identifier: MIT OR Apache-2.0 +pragma solidity >=0.8.0; + +// ============ Internal Imports ============ +import {AbstractAggregationIsm} from "./AbstractAggregationIsm.sol"; +import {IInterchainSecurityModule} from "../../interfaces/IInterchainSecurityModule.sol"; +import {IThresholdAddressFactory} from "../../interfaces/IThresholdAddressFactory.sol"; +import {MinimalProxy} from "../../libs/MinimalProxy.sol"; +import {PackageVersioned} from "../../PackageVersioned.sol"; + +// ============ External Imports ============ +import {Ownable2StepUpgradeable} from "@openzeppelin/contracts-upgradeable/access/Ownable2StepUpgradeable.sol"; + +contract StorageAggregationIsm is + AbstractAggregationIsm, + Ownable2StepUpgradeable +{ + address[] public modules; + uint8 public threshold; + + event ModulesAndThresholdSet(address[] modules, uint8 threshold); + + constructor( + address[] memory _modules, + uint8 _threshold + ) Ownable2StepUpgradeable() { + modules = _modules; + threshold = _threshold; + _disableInitializers(); + } + + function initialize( + address _owner, + address[] memory _modules, + uint8 _threshold + ) external initializer { + __Ownable2Step_init(); + setModulesAndThreshold(_modules, _threshold); + _transferOwnership(_owner); + } + + function setModulesAndThreshold( + address[] memory _modules, + uint8 _threshold + ) public onlyOwner { + require( + 0 < _threshold && _threshold <= _modules.length, + "Invalid threshold" + ); + modules = _modules; + threshold = _threshold; + emit ModulesAndThresholdSet(_modules, _threshold); + } + + function modulesAndThreshold( + bytes calldata /* _message */ + ) public view override returns (address[] memory, uint8) { + return (modules, threshold); + } +} + +contract StorageAggregationIsmFactory is + IThresholdAddressFactory, + PackageVersioned +{ + address public immutable implementation; + + constructor() { + implementation = address( + new StorageAggregationIsm(new address[](1), 1) + ); + } + + /** + * @notice Emitted when a multisig module is deployed + * @param module The deployed ISM + */ + event ModuleDeployed(address module); + + // ============ External Functions ============ + function deploy( + address[] calldata _modules, + uint8 _threshold + ) external returns (address ism) { + ism = MinimalProxy.create(implementation); + emit ModuleDeployed(ism); + StorageAggregationIsm(ism).initialize(msg.sender, _modules, _threshold); + } +} diff --git a/solidity/contracts/isms/multisig/AbstractMultisigIsm.sol b/solidity/contracts/isms/multisig/AbstractMultisigIsm.sol index 3b59d130c..d8cf5b8e4 100644 --- a/solidity/contracts/isms/multisig/AbstractMultisigIsm.sol +++ b/solidity/contracts/isms/multisig/AbstractMultisigIsm.sol @@ -68,7 +68,7 @@ abstract contract AbstractMultisig is PackageVersioned { * @notice Manages per-domain m-of-n Validator sets of AbstractMultisig that are used to verify * interchain messages. */ -abstract contract AbstractMultisigIsm is AbstractMultisig { +abstract contract AbstractMultisigIsm is AbstractMultisig, IMultisigIsm { // ============ Virtual Functions ============ // ======= OVERRIDE THESE TO IMPLEMENT ======= diff --git a/solidity/contracts/isms/multisig/StorageMultisigIsm.sol b/solidity/contracts/isms/multisig/StorageMultisigIsm.sol new file mode 100644 index 000000000..d35a38e3d --- /dev/null +++ b/solidity/contracts/isms/multisig/StorageMultisigIsm.sol @@ -0,0 +1,143 @@ +// SPDX-License-Identifier: MIT OR Apache-2.0 +pragma solidity >=0.8.0; + +// ============ Internal Imports ============ +import {AbstractMultisigIsm} from "./AbstractMultisigIsm.sol"; +import {AbstractMerkleRootMultisigIsm} from "./AbstractMerkleRootMultisigIsm.sol"; +import {AbstractMessageIdMultisigIsm} from "./AbstractMessageIdMultisigIsm.sol"; +import {IInterchainSecurityModule} from "../../interfaces/IInterchainSecurityModule.sol"; +import {IThresholdAddressFactory} from "../../interfaces/IThresholdAddressFactory.sol"; +import {MinimalProxy} from "../../libs/MinimalProxy.sol"; +import {PackageVersioned} from "../../PackageVersioned.sol"; + +// ============ External Imports ============ +import {Ownable2StepUpgradeable} from "@openzeppelin/contracts-upgradeable/access/Ownable2StepUpgradeable.sol"; + +abstract contract AbstractStorageMultisigIsm is + AbstractMultisigIsm, + Ownable2StepUpgradeable +{ + address[] public validators; + uint8 public threshold; + + event ValidatorsAndThresholdSet(address[] validators, uint8 threshold); + + constructor( + address[] memory _validators, + uint8 _threshold + ) Ownable2StepUpgradeable() { + validators = _validators; + threshold = _threshold; + _disableInitializers(); + } + + function initialize( + address _owner, + address[] memory _validators, + uint8 _threshold + ) external initializer { + __Ownable2Step_init(); + setValidatorsAndThreshold(_validators, _threshold); + _transferOwnership(_owner); + } + + function setValidatorsAndThreshold( + address[] memory _validators, + uint8 _threshold + ) public onlyOwner { + require( + 0 < _threshold && _threshold <= _validators.length, + "Invalid threshold" + ); + validators = _validators; + threshold = _threshold; + emit ValidatorsAndThresholdSet(_validators, _threshold); + } + + function validatorsAndThreshold( + bytes calldata /* _message */ + ) public view override returns (address[] memory, uint8) { + return (validators, threshold); + } +} + +contract StorageMerkleRootMultisigIsm is + AbstractMerkleRootMultisigIsm, + AbstractStorageMultisigIsm +{ + uint8 public constant moduleType = + uint8(IInterchainSecurityModule.Types.MERKLE_ROOT_MULTISIG); + + constructor( + address[] memory _validators, + uint8 _threshold + ) AbstractStorageMultisigIsm(_validators, _threshold) {} +} + +contract StorageMessageIdMultisigIsm is + AbstractMessageIdMultisigIsm, + AbstractStorageMultisigIsm +{ + uint8 public constant moduleType = + uint8(IInterchainSecurityModule.Types.MESSAGE_ID_MULTISIG); + + constructor( + address[] memory _validators, + uint8 _threshold + ) AbstractStorageMultisigIsm(_validators, _threshold) {} +} + +abstract contract StorageMultisigIsmFactory is + IThresholdAddressFactory, + PackageVersioned +{ + /** + * @notice Emitted when a multisig module is deployed + * @param module The deployed ISM + */ + event ModuleDeployed(address module); + + // ============ External Functions ============ + function deploy( + address[] calldata _validators, + uint8 _threshold + ) external returns (address ism) { + ism = MinimalProxy.create(implementation()); + emit ModuleDeployed(ism); + AbstractStorageMultisigIsm(ism).initialize( + msg.sender, + _validators, + _threshold + ); + } + + function implementation() public view virtual returns (address); +} + +contract StorageMerkleRootMultisigIsmFactory is StorageMultisigIsmFactory { + address internal immutable _implementation; + + constructor() { + _implementation = address( + new StorageMerkleRootMultisigIsm(new address[](0), 0) + ); + } + + function implementation() public view override returns (address) { + return _implementation; + } +} + +contract StorageMessageIdMultisigIsmFactory is StorageMultisigIsmFactory { + address internal immutable _implementation; + + constructor() { + _implementation = address( + new StorageMessageIdMultisigIsm(new address[](0), 0) + ); + } + + function implementation() public view override returns (address) { + return _implementation; + } +} diff --git a/solidity/contracts/libs/StaticAddressSetFactory.sol b/solidity/contracts/libs/StaticAddressSetFactory.sol index 38da45b8b..67c45607d 100644 --- a/solidity/contracts/libs/StaticAddressSetFactory.sol +++ b/solidity/contracts/libs/StaticAddressSetFactory.sol @@ -7,8 +7,12 @@ import {Create2} from "@openzeppelin/contracts/utils/Create2.sol"; // ============ Internal Imports ============ import {MetaProxy} from "./MetaProxy.sol"; import {PackageVersioned} from "../PackageVersioned.sol"; +import {IThresholdAddressFactory} from "../interfaces/IThresholdAddressFactory.sol"; -abstract contract StaticThresholdAddressSetFactory is PackageVersioned { +abstract contract StaticThresholdAddressSetFactory is + PackageVersioned, + IThresholdAddressFactory +{ // ============ Immutables ============ address public immutable implementation; @@ -32,6 +36,10 @@ abstract contract StaticThresholdAddressSetFactory is PackageVersioned { address[] calldata _values, uint8 _threshold ) public returns (address) { + require( + 0 < _threshold && _threshold <= _values.length, + "Invalid threshold" + ); (bytes32 _salt, bytes memory _bytecode) = _saltAndBytecode( _values, _threshold diff --git a/solidity/test/hooks/AggregationHook.t.sol b/solidity/test/hooks/AggregationHook.t.sol index a37b20f1a..879f3de30 100644 --- a/solidity/test/hooks/AggregationHook.t.sol +++ b/solidity/test/hooks/AggregationHook.t.sol @@ -22,6 +22,8 @@ contract AggregationHookTest is Test { uint8 n, uint256 fee ) internal returns (address[] memory) { + vm.assume(n > 0); + address[] memory hooks = new address[](n); for (uint8 i = 0; i < n; i++) { TestPostDispatchHook subHook = new TestPostDispatchHook(); diff --git a/solidity/test/isms/AggregationIsm.t.sol b/solidity/test/isms/AggregationIsm.t.sol index 8806d9b66..fa380d3dc 100644 --- a/solidity/test/isms/AggregationIsm.t.sol +++ b/solidity/test/isms/AggregationIsm.t.sol @@ -7,6 +7,8 @@ import "@openzeppelin/contracts/utils/Strings.sol"; import {IAggregationIsm} from "../../contracts/interfaces/isms/IAggregationIsm.sol"; import {StaticAggregationIsmFactory} from "../../contracts/isms/aggregation/StaticAggregationIsmFactory.sol"; +import {IThresholdAddressFactory} from "../../contracts/interfaces/IThresholdAddressFactory.sol"; +import {StorageAggregationIsmFactory} from "../../contracts/isms/aggregation/StorageAggregationIsm.sol"; import {AggregationIsmMetadata} from "../../contracts/isms/libs/AggregationIsmMetadata.sol"; import {TestIsm, ThresholdTestUtils} from "./IsmTestUtils.sol"; @@ -16,10 +18,10 @@ contract AggregationIsmTest is Test { string constant fixtureKey = "fixture"; - StaticAggregationIsmFactory factory; + IThresholdAddressFactory factory; IAggregationIsm ism; - function setUp() public { + function setUp() public virtual { factory = new StaticAggregationIsmFactory(); } @@ -46,6 +48,8 @@ contract AggregationIsmTest is Test { uint8 n, bytes32 seed ) internal returns (address[] memory) { + vm.assume(m > 0 && m <= n && n < 10); + bytes32 randomness = seed; address[] memory isms = new address[](n); for (uint256 i = 0; i < n; i++) { @@ -91,7 +95,6 @@ contract AggregationIsmTest is Test { } function testVerify(uint8 m, uint8 n, bytes32 seed) public { - vm.assume(0 < m && m <= n && n < 10); deployIsms(m, n, seed); bytes memory metadata = getMetadata(m, seed); @@ -104,7 +107,7 @@ contract AggregationIsmTest is Test { uint8 i, bytes32 seed ) public { - vm.assume(0 < m && m <= n && n < 10 && i < n); + vm.assume(i < n); deployIsms(m, n, seed); (address[] memory modules, ) = ism.modulesAndThreshold(""); bytes memory noMetadata; @@ -115,7 +118,6 @@ contract AggregationIsmTest is Test { } function testVerifyMissingMetadata(uint8 m, uint8 n, bytes32 seed) public { - vm.assume(0 < m && m <= n && n < 10); deployIsms(m, n, seed); // Populate metadata for one fewer ISMs than needed. @@ -129,7 +131,6 @@ contract AggregationIsmTest is Test { uint8 n, bytes32 seed ) public { - vm.assume(0 < m && m <= n && n < 10); deployIsms(m, n, seed); bytes memory metadata = getMetadata(m, seed); @@ -141,11 +142,26 @@ contract AggregationIsmTest is Test { } function testModulesAndThreshold(uint8 m, uint8 n, bytes32 seed) public { - vm.assume(0 < m && m <= n && n < 10); address[] memory expectedIsms = deployIsms(m, n, seed); (address[] memory actualIsms, uint8 actualThreshold) = ism .modulesAndThreshold(""); assertEq(abi.encode(actualIsms), abi.encode(expectedIsms)); assertEq(actualThreshold, m); } + + function testZeroThreshold() public { + vm.expectRevert("Invalid threshold"); + factory.deploy(new address[](1), 0); + } + + function testThresholdExceedsLength() public { + vm.expectRevert("Invalid threshold"); + factory.deploy(new address[](1), 2); + } +} + +contract StorageAggregationIsmTest is AggregationIsmTest { + function setUp() public override { + factory = new StorageAggregationIsmFactory(); + } } diff --git a/solidity/test/isms/MultisigIsm.t.sol b/solidity/test/isms/MultisigIsm.t.sol index 29098475f..86a3eddf6 100644 --- a/solidity/test/isms/MultisigIsm.t.sol +++ b/solidity/test/isms/MultisigIsm.t.sol @@ -11,13 +11,16 @@ import {TestMailbox} from "../../contracts/test/TestMailbox.sol"; import {StaticMerkleRootMultisigIsmFactory, StaticMessageIdMultisigIsmFactory} from "../../contracts/isms/multisig/StaticMultisigIsm.sol"; import {MerkleRootMultisigIsmMetadata} from "../../contracts/isms/libs/MerkleRootMultisigIsmMetadata.sol"; import {CheckpointLib} from "../../contracts/libs/CheckpointLib.sol"; -import {StaticThresholdAddressSetFactory} from "../../contracts/libs/StaticAddressSetFactory.sol"; +import {IThresholdAddressFactory} from "../../contracts/interfaces/IThresholdAddressFactory.sol"; import {TypeCasts} from "../../contracts/libs/TypeCasts.sol"; import {MerkleTreeHook} from "../../contracts/hooks/MerkleTreeHook.sol"; import {TestMerkleTreeHook} from "../../contracts/test/TestMerkleTreeHook.sol"; import {TestPostDispatchHook} from "../../contracts/test/TestPostDispatchHook.sol"; import {Message} from "../../contracts/libs/Message.sol"; import {ThresholdTestUtils} from "./IsmTestUtils.sol"; +import {StorageMessageIdMultisigIsm, StorageMerkleRootMultisigIsm, StorageMessageIdMultisigIsmFactory, StorageMerkleRootMultisigIsmFactory, AbstractStorageMultisigIsm} from "../../contracts/isms/multisig/StorageMultisigIsm.sol"; + +uint8 constant MAX_VALIDATORS = 20; /// @notice since we removed merkle tree from the mailbox, we need to include the MerkleTreeHook in the test abstract contract AbstractMultisigIsmTest is Test { @@ -32,7 +35,7 @@ abstract contract AbstractMultisigIsmTest is Test { string constant prefixKey = "prefix"; uint32 constant ORIGIN = 11; - StaticThresholdAddressSetFactory factory; + IThresholdAddressFactory factory; IInterchainSecurityModule ism; TestMerkleTreeHook internal merkleTreeHook; TestPostDispatchHook internal noopHook; @@ -163,7 +166,7 @@ abstract contract AbstractMultisigIsmTest is Test { uint8 n, bytes32 seed ) public { - vm.assume(0 < m && m <= n && n < 10); + vm.assume(0 < m && m <= n && n < MAX_VALIDATORS); bytes memory message = getMessage(destination, recipient, body); bytes memory metadata = getMetadata(m, n, seed, message); assertTrue(ism.verify(metadata, message)); @@ -177,7 +180,7 @@ abstract contract AbstractMultisigIsmTest is Test { uint8 n, bytes32 seed ) public { - vm.assume(0 < m && m <= n && n < 10); + vm.assume(0 < m && m <= n && n < MAX_VALIDATORS); bytes memory message = getMessage(destination, recipient, body); bytes memory metadata = getMetadata(m, n, seed, message); @@ -212,6 +215,16 @@ abstract contract AbstractMultisigIsmTest is Test { vm.expectRevert("!threshold"); ism.verify(duplicateMetadata, message); } + + function testZeroThreshold() public virtual { + vm.expectRevert("Invalid threshold"); + factory.deploy(new address[](1), 0); + } + + function testThresholdExceedsLength() public virtual { + vm.expectRevert("Invalid threshold"); + factory.deploy(new address[](1), 2); + } } contract MerkleRootMultisigIsmTest is AbstractMultisigIsmTest { @@ -308,3 +321,93 @@ contract MessageIdMultisigIsmTest is AbstractMultisigIsmTest { return abi.encodePacked(merkleTreeAddress, root, index); } } + +abstract contract StorageMultisigIsmTest is AbstractMultisigIsmTest { + event ValidatorsAndThresholdSet(address[] validators, uint8 threshold); + event Initialized(uint8 version); + event OwnershipTransferred( + address indexed previousOwner, + address indexed newOwner + ); + + function test_initialize( + bytes32 seed, + address[] memory validators, + uint8 threshold + ) public { + vm.assume( + 0 < threshold && + threshold <= validators.length && + validators.length <= MAX_VALIDATORS + ); + + addValidators(threshold, uint8(validators.length), seed); + + vm.expectRevert("Initializable: contract is already initialized"); + AbstractStorageMultisigIsm(address(ism)).initialize( + address(this), + validators, + threshold + ); + } + + function test_setValidatorsAndThreshold( + bytes32 seed, + address[] memory validators, + uint8 threshold + ) public { + vm.assume( + 0 < threshold && + threshold <= validators.length && + validators.length <= MAX_VALIDATORS + ); + + addValidators(threshold, uint8(validators.length), seed); + + AbstractStorageMultisigIsm storageIsm = AbstractStorageMultisigIsm( + address(ism) + ); + + address owner = storageIsm.owner(); + address antiOwner = address(~bytes20(owner)); + vm.expectRevert("Ownable: caller is not the owner"); + vm.prank(antiOwner); + storageIsm.setValidatorsAndThreshold(validators, threshold); + + vm.expectRevert("Invalid threshold"); + vm.prank(owner); + storageIsm.setValidatorsAndThreshold( + validators, + uint8(validators.length + 1) + ); + + vm.prank(owner); + vm.expectEmit(true, true, false, false); + emit ValidatorsAndThresholdSet(validators, threshold); + storageIsm.setValidatorsAndThreshold(validators, threshold); + (address[] memory _validators, uint8 _threshold) = storageIsm + .validatorsAndThreshold("0x"); + assertEq(_threshold, threshold); + assertEq(_validators, validators); + } +} + +contract StorageMessageIdMultisigIsmTest is + StorageMultisigIsmTest, + MessageIdMultisigIsmTest +{ + function setUp() public override { + super.setUp(); + factory = new StorageMessageIdMultisigIsmFactory(); + } +} + +contract StorageMerkleRootMultisigIsmTest is + StorageMultisigIsmTest, + MerkleRootMultisigIsmTest +{ + function setUp() public override { + super.setUp(); + factory = new StorageMerkleRootMultisigIsmFactory(); + } +} diff --git a/solidity/test/isms/WeightedMultisigIsm.t.sol b/solidity/test/isms/WeightedMultisigIsm.t.sol index 0c5fd7ee4..b2d763bc1 100644 --- a/solidity/test/isms/WeightedMultisigIsm.t.sol +++ b/solidity/test/isms/WeightedMultisigIsm.t.sol @@ -243,6 +243,14 @@ contract StaticMerkleRootWeightedMultisigIsmTest is seed ); } + + function testThresholdExceedsLength() public override { + // no-op + } + + function testZeroThreshold() public override { + // no-op + } } contract StaticMessageIdWeightedMultisigIsmTest is @@ -298,4 +306,12 @@ contract StaticMessageIdWeightedMultisigIsmTest is seed ); } + + function testThresholdExceedsLength() public override { + // no-op + } + + function testZeroThreshold() public override { + // no-op + } } diff --git a/typescript/cli/src/config/ism.ts b/typescript/cli/src/config/ism.ts index e40a00c26..f7f6bab9a 100644 --- a/typescript/cli/src/config/ism.ts +++ b/typescript/cli/src/config/ism.ts @@ -8,6 +8,7 @@ import { IsmConfigSchema, IsmType, MultisigIsmConfig, + MultisigIsmConfigSchema, TrustedRelayerIsmConfig, } from '@hyperlane-xyz/sdk'; @@ -72,7 +73,11 @@ const ISM_TYPE_DESCRIPTIONS: Record = { "You can specify ISM type for specific chains you like and fallback to mailbox's default ISM for other chains via DefaultFallbackRoutingISM", [IsmType.MERKLE_ROOT_MULTISIG]: 'Validators need to sign the root of the merkle tree of all messages from origin chain', + [IsmType.STORAGE_MERKLE_ROOT_MULTISIG]: + 'Mutable validators in storage need to sign the root of the merkle tree of all messages from origin chain', [IsmType.MESSAGE_ID_MULTISIG]: 'Validators need to sign just this messageId', + [IsmType.STORAGE_MESSAGE_ID_MULTISIG]: + 'Mutable validators in storage need to sign just this messageId', [IsmType.ROUTING]: 'Each origin chain can be verified by the specified ISM type via RoutingISM', [IsmType.TEST_ISM]: @@ -106,9 +111,10 @@ export async function createAdvancedIsmConfig( case IsmType.FALLBACK_ROUTING: return createFallbackRoutingConfig(context); case IsmType.MERKLE_ROOT_MULTISIG: - return createMerkleRootMultisigConfig(context); case IsmType.MESSAGE_ID_MULTISIG: - return createMessageIdMultisigConfig(context); + case IsmType.STORAGE_MERKLE_ROOT_MULTISIG: + case IsmType.STORAGE_MESSAGE_ID_MULTISIG: + return createMultisigConfig(moduleType); case IsmType.ROUTING: return createRoutingConfig(context); case IsmType.TEST_ISM: @@ -116,58 +122,40 @@ export async function createAdvancedIsmConfig( case IsmType.TRUSTED_RELAYER: return createTrustedRelayerConfig(context, true); default: - throw new Error(`Invalid ISM type: ${moduleType}.`); + throw new Error(`Unsupported ISM type: ${moduleType}.`); } } -export const createMerkleRootMultisigConfig = callWithConfigCreationLogs( - async (): Promise => { - const validatorsInput = await input({ - message: - 'Enter validator addresses (comma separated list) for merkle root multisig ISM:', - }); - const validators = validatorsInput.split(',').map((v) => v.trim()); - const thresholdInput = await input({ - message: - 'Enter threshold of validators (number) for merkle root multisig ISM:', - }); - const threshold = parseInt(thresholdInput, 10); - if (threshold > validators.length) { - errorRed( - `Merkle root multisig signer threshold (${threshold}) cannot be greater than total number of validators (${validators.length}).`, - ); - throw new Error('Invalid protocol fee.'); - } - return { - type: IsmType.MERKLE_ROOT_MULTISIG, - threshold, - validators, - }; - }, - IsmType.MERKLE_ROOT_MULTISIG, -); - -export const createMessageIdMultisigConfig = callWithConfigCreationLogs( - async (): Promise => { - const thresholdInput = await input({ - message: - 'Enter threshold of validators (number) for message ID multisig ISM', - }); - const threshold = parseInt(thresholdInput, 10); +export const createMultisigConfig = async ( + ismType: MultisigIsmConfig['type'], +): Promise => { + const validatorsInput = await input({ + message: + 'Enter validator addresses (comma separated list) for multisig ISM:', + }); + const validators = validatorsInput.split(',').map((v) => v.trim()); + const threshold = parseInt( + await input({ + message: 'Enter threshold of validators (number) for multisig ISM:', + }), + 10, + ); + const result = MultisigIsmConfigSchema.safeParse({ + type: ismType, + validators, + threshold, + }); + if (!result.success) { + errorRed( + result.error.issues + .map((input, index) => `input[${index}]: ${input.message}`) + .join('\n'), + ); + return createMultisigConfig(ismType); + } - const validatorsInput = await input({ - message: - 'Enter validator addresses (comma separated list) for message ID multisig ISM', - }); - const validators = validatorsInput.split(',').map((v) => v.trim()); - return { - type: IsmType.MESSAGE_ID_MULTISIG, - threshold, - validators, - }; - }, - IsmType.MESSAGE_ID_MULTISIG, -); + return result.data; +}; export const createTrustedRelayerConfig = callWithConfigCreationLogs( async ( diff --git a/typescript/cli/src/tests/multisig.test.ts b/typescript/cli/src/tests/multisig.test.ts deleted file mode 100644 index f56b3e7bd..000000000 --- a/typescript/cli/src/tests/multisig.test.ts +++ /dev/null @@ -1,41 +0,0 @@ -import { expect } from 'chai'; - -import { ChainMap, MultisigConfig } from '@hyperlane-xyz/sdk'; - -import { readMultisigConfig } from '../config/multisig.js'; - -describe('readMultisigConfig', () => { - it('parses and validates example correctly', () => { - const multisig = readMultisigConfig('examples/ism.yaml'); - - const exampleMultisigConfig: ChainMap = { - anvil1: { - threshold: 1, - validators: ['0xa0Ee7A142d267C1f36714E4a8F75612F20a79720'], - }, - anvil2: { - threshold: 1, - validators: ['0xa0Ee7A142d267C1f36714E4a8F75612F20a79720'], - }, - }; - expect(multisig).to.deep.equal(exampleMultisigConfig); - }); - - it('parsing failure', () => { - expect(function () { - readMultisigConfig('src/tests/multisig/safe-parse-fail.yaml'); - }).to.throw('Invalid multisig config: anvil2,validators => Required'); - }); - - it('threshold cannot be greater than the # of validators', () => { - expect(function () { - readMultisigConfig('src/tests/multisig/threshold-gt-fail.yaml'); - }).to.throw('Threshold cannot be greater than number of validators'); - }); - - it('invalid address', () => { - expect(function () { - readMultisigConfig('src/tests/multisig/invalid-address-fail.yaml'); - }).to.throw('Invalid multisig config: anvil2,validators,0 => Invalid'); - }); -}); diff --git a/typescript/cli/src/tests/multisig/invalid-address-fail.yaml b/typescript/cli/src/tests/multisig/invalid-address-fail.yaml deleted file mode 100644 index 073a76189..000000000 --- a/typescript/cli/src/tests/multisig/invalid-address-fail.yaml +++ /dev/null @@ -1,8 +0,0 @@ -anvil1: - threshold: 1 # Number: Signatures required to approve a message - validators: # Array: List of validator addresses - - '0xa0ee7a142d267c1f36714e4a8f75612f20a79720' -anvil2: - threshold: 1 - validators: - - '0xa0ee7a142d267c1n36714e4a8f7561f20a79720' diff --git a/typescript/cli/src/tests/multisig/safe-parse-fail.yaml b/typescript/cli/src/tests/multisig/safe-parse-fail.yaml deleted file mode 100644 index a19e8868c..000000000 --- a/typescript/cli/src/tests/multisig/safe-parse-fail.yaml +++ /dev/null @@ -1,6 +0,0 @@ -anvil1: - threshold: 1 # Number: Signatures required to approve a message - validators: # Array: List of validator addresses - - '0xa0ee7a142d267c1f36714e4a8f75612f20a79720' -anvil2: - threshold: 1 diff --git a/typescript/cli/src/tests/multisig/threshold-gt-fail.yaml b/typescript/cli/src/tests/multisig/threshold-gt-fail.yaml deleted file mode 100644 index ae3a3fb00..000000000 --- a/typescript/cli/src/tests/multisig/threshold-gt-fail.yaml +++ /dev/null @@ -1,8 +0,0 @@ -anvil1: - threshold: 1 # Number: Signatures required to approve a message - validators: # Array: List of validator addresses - - '0xa0ee7a142d267c1f36714e4a8f75612f20a79720' -anvil2: - threshold: 3 - validators: - - '0xa0ee7a142d267c1f36714e4a8f75612f20a79720' diff --git a/typescript/sdk/src/index.ts b/typescript/sdk/src/index.ts index b2358f080..e52b6109a 100644 --- a/typescript/sdk/src/index.ts +++ b/typescript/sdk/src/index.ts @@ -510,7 +510,11 @@ export { } from './warp/types.js'; export { WarpCore, WarpCoreOptions } from './warp/WarpCore.js'; -export { AggregationIsmConfigSchema, IsmConfigSchema } from './ism/schemas.js'; +export { + AggregationIsmConfigSchema, + IsmConfigSchema, + MultisigIsmConfigSchema, +} from './ism/schemas.js'; export { MailboxClientConfigSchema as mailboxClientConfigSchema } from './router/schemas.js'; export { CollateralConfig, diff --git a/typescript/sdk/src/ism/EvmIsmModule.hardhat-test.ts b/typescript/sdk/src/ism/EvmIsmModule.hardhat-test.ts index fc03107cc..00c31fc71 100644 --- a/typescript/sdk/src/ism/EvmIsmModule.hardhat-test.ts +++ b/typescript/sdk/src/ism/EvmIsmModule.hardhat-test.ts @@ -466,9 +466,11 @@ describe('EvmIsmModule', async () => { ); // update the validators for a domain - ( - exampleRoutingConfig.domains[TestChainName.test2] as MultisigIsmConfig - ).validators = [randomAddress(), randomAddress()]; + exampleRoutingConfig.domains[TestChainName.test2] = { + type: IsmType.MERKLE_ROOT_MULTISIG, + validators: [randomAddress(), randomAddress()], + threshold: 2, + }; // expect 1 tx to update validator set for test2 domain await expectTxsAndUpdate(ism, exampleRoutingConfig, 1); diff --git a/typescript/sdk/src/ism/HyperlaneIsmFactory.ts b/typescript/sdk/src/ism/HyperlaneIsmFactory.ts index f9a0b42d4..2835e4a35 100644 --- a/typescript/sdk/src/ism/HyperlaneIsmFactory.ts +++ b/typescript/sdk/src/ism/HyperlaneIsmFactory.ts @@ -19,6 +19,9 @@ import { StaticAddressSetFactory, StaticThresholdAddressSetFactory, StaticWeightedValidatorSetFactory, + StorageAggregationIsm__factory, + StorageMerkleRootMultisigIsm__factory, + StorageMessageIdMultisigIsm__factory, TestIsm__factory, TrustedRelayerIsm__factory, } from '@hyperlane-xyz/core'; @@ -135,6 +138,8 @@ export class HyperlaneIsmFactory extends HyperlaneApp { switch (ismType) { case IsmType.MESSAGE_ID_MULTISIG: case IsmType.MERKLE_ROOT_MULTISIG: + case IsmType.STORAGE_MESSAGE_ID_MULTISIG: + case IsmType.STORAGE_MERKLE_ROOT_MULTISIG: contract = await this.deployMultisigIsm(destination, config, logger); break; case IsmType.WEIGHTED_MESSAGE_ID_MULTISIG: @@ -157,6 +162,7 @@ export class HyperlaneIsmFactory extends HyperlaneApp { }); break; case IsmType.AGGREGATION: + case IsmType.STORAGE_AGGREGATION: contract = await this.deployAggregationIsm({ destination, config, @@ -227,18 +233,55 @@ export class HyperlaneIsmFactory extends HyperlaneApp { logger: Logger, ): Promise { const signer = this.multiProvider.getSigner(destination); - const multisigIsmFactory = - config.type === IsmType.MERKLE_ROOT_MULTISIG - ? this.getContracts(destination).staticMerkleRootMultisigIsmFactory - : this.getContracts(destination).staticMessageIdMultisigIsmFactory; - const address = await this.deployStaticAddressSet( - destination, - multisigIsmFactory, - config.validators, - logger, - config.threshold, - ); + const deployStatic = (factory: StaticThresholdAddressSetFactory) => + this.deployStaticAddressSet( + destination, + factory, + config.validators, + logger, + config.threshold, + ); + + const deployStorage = async ( + factory: + | StorageMerkleRootMultisigIsm__factory + | StorageMessageIdMultisigIsm__factory, + ) => { + const contract = await this.multiProvider.handleDeploy( + destination, + factory, + [config.validators, config.threshold], + ); + return contract.address; + }; + + let address: string; + switch (config.type) { + case IsmType.MERKLE_ROOT_MULTISIG: + address = await deployStatic( + this.getContracts(destination).staticMerkleRootMultisigIsmFactory, + ); + break; + case IsmType.MESSAGE_ID_MULTISIG: + address = await deployStatic( + this.getContracts(destination).staticMessageIdMultisigIsmFactory, + ); + break; + // TODO: support using minimal proxy factories for storage multisig ISMs too + case IsmType.STORAGE_MERKLE_ROOT_MULTISIG: + address = await deployStorage( + new StorageMerkleRootMultisigIsm__factory(), + ); + break; + case IsmType.STORAGE_MESSAGE_ID_MULTISIG: + address = await deployStorage( + new StorageMessageIdMultisigIsm__factory(), + ); + break; + default: + throw new Error(`Unsupported multisig ISM type ${config.type}`); + } return IMultisigIsm__factory.connect(address, signer); } @@ -453,8 +496,7 @@ export class HyperlaneIsmFactory extends HyperlaneApp { }): Promise { const { destination, config, origin, mailbox } = params; const signer = this.multiProvider.getSigner(destination); - const staticAggregationIsmFactory = - this.getContracts(destination).staticAggregationIsmFactory; + const addresses: Address[] = []; for (const module of config.modules) { const submodule = await this.deploy({ @@ -465,14 +507,30 @@ export class HyperlaneIsmFactory extends HyperlaneApp { }); addresses.push(submodule.address); } - const address = await this.deployStaticAddressSet( - destination, - staticAggregationIsmFactory, - addresses, - params.logger, - config.threshold, - ); - return IAggregationIsm__factory.connect(address, signer); + + let ismAddress: string; + if (config.type === IsmType.STORAGE_AGGREGATION) { + // TODO: support using minimal proxy factories for storage aggregation ISMs too + const factory = new StorageAggregationIsm__factory().connect(signer); + const ism = await this.multiProvider.handleDeploy(destination, factory, [ + addresses, + config.threshold, + ]); + ismAddress = ism.address; + } else { + const staticAggregationIsmFactory = + this.getContracts(destination).staticAggregationIsmFactory; + + ismAddress = await this.deployStaticAddressSet( + destination, + staticAggregationIsmFactory, + addresses, + params.logger, + config.threshold, + ); + } + + return IAggregationIsm__factory.connect(ismAddress, signer); } async deployStaticAddressSet( diff --git a/typescript/sdk/src/ism/metadata/aggregation.ts b/typescript/sdk/src/ism/metadata/aggregation.ts index baa4d91ea..5c347a600 100644 --- a/typescript/sdk/src/ism/metadata/aggregation.ts +++ b/typescript/sdk/src/ism/metadata/aggregation.ts @@ -20,7 +20,7 @@ import { // null indicates that metadata is NOT INCLUDED for this submodule // empty or 0x string indicates that metadata is INCLUDED but NULL export interface AggregationMetadata { - type: IsmType.AGGREGATION; + type: AggregationIsmConfig['type']; submoduleMetadata: Array; } diff --git a/typescript/sdk/src/ism/schemas.ts b/typescript/sdk/src/ism/schemas.ts index 46f6ad180..e9d2e1cda 100644 --- a/typescript/sdk/src/ism/schemas.ts +++ b/typescript/sdk/src/ism/schemas.ts @@ -51,6 +51,8 @@ export const MultisigIsmConfigSchema = MultisigConfigSchema.and( type: z.union([ z.literal(IsmType.MERKLE_ROOT_MULTISIG), z.literal(IsmType.MESSAGE_ID_MULTISIG), + z.literal(IsmType.STORAGE_MERKLE_ROOT_MULTISIG), + z.literal(IsmType.STORAGE_MESSAGE_ID_MULTISIG), ]), }), ); diff --git a/typescript/sdk/src/ism/types.ts b/typescript/sdk/src/ism/types.ts index 9558af089..82e025a04 100644 --- a/typescript/sdk/src/ism/types.ts +++ b/typescript/sdk/src/ism/types.ts @@ -52,8 +52,11 @@ export enum IsmType { ROUTING = 'domainRoutingIsm', FALLBACK_ROUTING = 'defaultFallbackRoutingIsm', AGGREGATION = 'staticAggregationIsm', + STORAGE_AGGREGATION = 'storageAggregationIsm', MERKLE_ROOT_MULTISIG = 'merkleRootMultisigIsm', MESSAGE_ID_MULTISIG = 'messageIdMultisigIsm', + STORAGE_MERKLE_ROOT_MULTISIG = 'storageMerkleRootMultisigIsm', + STORAGE_MESSAGE_ID_MULTISIG = 'storageMessageIdMultisigIsm', TEST_ISM = 'testIsm', PAUSABLE = 'pausableIsm', TRUSTED_RELAYER = 'trustedRelayerIsm', @@ -77,10 +80,13 @@ export function ismTypeToModuleType(ismType: IsmType): ModuleType { case IsmType.FALLBACK_ROUTING: return ModuleType.ROUTING; case IsmType.AGGREGATION: + case IsmType.STORAGE_AGGREGATION: return ModuleType.AGGREGATION; case IsmType.MERKLE_ROOT_MULTISIG: + case IsmType.STORAGE_MERKLE_ROOT_MULTISIG: return ModuleType.MERKLE_ROOT_MULTISIG; case IsmType.MESSAGE_ID_MULTISIG: + case IsmType.STORAGE_MESSAGE_ID_MULTISIG: return ModuleType.MESSAGE_ID_MULTISIG; case IsmType.OP_STACK: case IsmType.TEST_ISM: @@ -126,7 +132,7 @@ export type RoutingIsmConfig = OwnableConfig & { }; export type AggregationIsmConfig = { - type: IsmType.AGGREGATION; + type: IsmType.AGGREGATION | IsmType.STORAGE_AGGREGATION; modules: Array; threshold: number; }; @@ -138,8 +144,11 @@ export type DeployedIsmType = { [IsmType.ROUTING]: IRoutingIsm; [IsmType.FALLBACK_ROUTING]: IRoutingIsm; [IsmType.AGGREGATION]: IAggregationIsm; + [IsmType.STORAGE_AGGREGATION]: IAggregationIsm; [IsmType.MERKLE_ROOT_MULTISIG]: IMultisigIsm; [IsmType.MESSAGE_ID_MULTISIG]: IMultisigIsm; + [IsmType.STORAGE_MERKLE_ROOT_MULTISIG]: IMultisigIsm; + [IsmType.STORAGE_MESSAGE_ID_MULTISIG]: IMultisigIsm; [IsmType.OP_STACK]: OPStackIsm; [IsmType.TEST_ISM]: TestIsm; [IsmType.PAUSABLE]: PausableIsm;