Add unenroll to Router (#2760)

Fixes https://github.com/hyperlane-xyz/issues/issues/630

solidity/contracts/client/Router.sol

@@ -14,6 +14,7 @@ import {Strings} from "@openzeppelin/contracts/utils/Strings.sol";
 
 abstract contract Router is MailboxClient, IMessageRecipient {
     using EnumerableMapExtended for EnumerableMapExtended.UintToBytes32Map;
+    using Strings for uint32;
 
     // ============ Mutable Storage ============
     EnumerableMapExtended.UintToBytes32Map internal _routers;
@@ -38,6 +39,14 @@ abstract contract Router is MailboxClient, IMessageRecipient {
         return _router;
     }
 
+    /**
+     * @notice Unregister the domain
+     * @param _domain The domain of the remote Application Router
+     */
+    function unenrollRemoteRouter(uint32 _domain) external virtual onlyOwner {
+        _unenrollRemoteRouter(_domain);
+    }
+
     /**
      * @notice Register the address of a Router contract for the same Application on a remote chain
      * @param _domain The domain of the remote Application Router
@@ -53,7 +62,7 @@ abstract contract Router is MailboxClient, IMessageRecipient {
 
     /**
      * @notice Batch version of `enrollRemoteRouter`
-     * @param _domains The domaisn of the remote Application Routers
+     * @param _domains The domains of the remote Application Routers
      * @param _addresses The addresses of the remote Application Routers
      */
     function enrollRemoteRouters(
@@ -67,6 +76,21 @@ abstract contract Router is MailboxClient, IMessageRecipient {
         }
     }
 
+    /**
+     * @notice Batch version of `unenrollRemoteRouter`
+     * @param _domains The domains of the remote Application Routers
+     */
+    function unenrollRemoteRouters(uint32[] calldata _domains)
+        external
+        virtual
+        onlyOwner
+    {
+        uint256 length = _domains.length;
+        for (uint256 i = 0; i < length; i += 1) {
+            _unenrollRemoteRouter(_domains[i]);
+        }
+    }
+
     /**
      * @notice Handles an incoming message
      * @param _origin The origin domain
@@ -104,6 +128,14 @@ abstract contract Router is MailboxClient, IMessageRecipient {
         _routers.set(_domain, _address);
     }
 
+    /**
+     * @notice Remove the router for a given domain
+     * @param _domain The domain
+     */
+    function _unenrollRemoteRouter(uint32 _domain) internal virtual {
+        require(_routers.remove(_domain), _domainNotFoundError(_domain));
+    }
+
     /**
      * @notice Return true if the given domain / router is the address of a remote Application Router
      * @param _domain The domain of the potential remote Application Router
@@ -128,14 +160,20 @@ abstract contract Router is MailboxClient, IMessageRecipient {
         returns (bytes32)
     {
         (bool contained, bytes32 _router) = _routers.tryGet(_domain);
-        require(
-            contained,
+        require(contained, _domainNotFoundError(_domain));
+        return _router;
+    }
+
+    function _domainNotFoundError(uint32 _domain)
+        internal
+        pure
+        returns (string memory)
+    {
+        return
             string.concat(
                 "No router enrolled for domain: ",
-                Strings.toString(_domain)
-            )
-        );
-        return _router;
+                _domain.toString()
+            );
     }
 
     function _dispatch(uint32 _destinationDomain, bytes memory _messageBody)

solidity/contracts/isms/routing/DefaultFallbackRoutingIsm.sol

@@ -1,29 +1,24 @@
 // SPDX-License-Identifier: MIT OR Apache-2.0
 pragma solidity >=0.8.0;
 
+// ============ Internal Imports ============
 import {DomainRoutingIsm} from "./DomainRoutingIsm.sol";
-import {IMailbox} from "../../Mailbox.sol";
 import {IInterchainSecurityModule} from "../../interfaces/IInterchainSecurityModule.sol";
 import {EnumerableMapExtended} from "../../libs/EnumerableMapExtended.sol";
 import {TypeCasts} from "../../libs/TypeCasts.sol";
+import {MailboxClient} from "../../client/MailboxClient.sol";
+
+// ============ External Imports ============
 import {Address} from "@openzeppelin/contracts/utils/Address.sol";
 
-contract DefaultFallbackRoutingIsm is DomainRoutingIsm {
+contract DefaultFallbackRoutingIsm is DomainRoutingIsm, MailboxClient {
     using EnumerableMapExtended for EnumerableMapExtended.UintToBytes32Map;
     using Address for address;
     using TypeCasts for bytes32;
 
-    IMailbox public immutable mailbox;
-
-    constructor(address _mailbox) {
-        require(
-            _mailbox.isContract(),
-            "DefaultFallbackRoutingIsm: INVALID_MAILBOX"
-        );
-        mailbox = IMailbox(_mailbox);
-    }
+    constructor(address _mailbox) MailboxClient(_mailbox) {}
 
-    function modules(uint32 origin)
+    function module(uint32 origin)
         public
         view
         override

solidity/contracts/isms/routing/DomainRoutingIsm.sol

@@ -1,8 +1,10 @@
 // SPDX-License-Identifier: MIT OR Apache-2.0
 pragma solidity >=0.8.0;
+
 // ============ External Imports ============
 import {Address} from "@openzeppelin/contracts/utils/Address.sol";
 import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
+import {Strings} from "@openzeppelin/contracts/utils/Strings.sol";
 
 // ============ Internal Imports ============
 import {AbstractRoutingIsm} from "./AbstractRoutingIsm.sol";
@@ -20,19 +22,11 @@ contract DomainRoutingIsm is AbstractRoutingIsm, OwnableUpgradeable {
     using TypeCasts for bytes32;
     using TypeCasts for address;
     using Address for address;
+    using Strings for uint32;
 
     // ============ Mutable Storage ============
     EnumerableMapExtended.UintToBytes32Map internal _modules;
 
-    // ============ Events ============
-
-    /**
-     * @notice Emitted when a module is set for a domain
-     * @param domain The origin domain.
-     * @param module The ISM to use.
-     */
-    event ModuleSet(uint32 indexed domain, address module);
-
     // ============ External Functions ============
 
     /**
@@ -75,18 +69,26 @@ contract DomainRoutingIsm is AbstractRoutingIsm, OwnableUpgradeable {
         _set(_domain, address(_module));
     }
 
+    /**
+     * @notice Removes the specified origin domain
+     * @param _domain The origin domain
+     */
+    function remove(uint32 _domain) external onlyOwner {
+        _remove(_domain);
+    }
+
     function domains() external view returns (uint256[] memory) {
         return _modules.keys();
     }
 
-    function modules(uint32 origin)
+    function module(uint32 origin)
         public
         view
         virtual
         returns (IInterchainSecurityModule)
     {
         (bool contained, bytes32 _module) = _modules.tryGet(origin);
-        require(contained, "No ISM found for origin domain");
+        require(contained, _originNotFoundError(origin));
         return IInterchainSecurityModule(_module.bytes32ToAddress());
     }
 
@@ -103,19 +105,34 @@ contract DomainRoutingIsm is AbstractRoutingIsm, OwnableUpgradeable {
         override
         returns (IInterchainSecurityModule)
     {
-        return modules(_message.origin());
+        return module(_message.origin());
     }
 
     // ============ Internal Functions ============
 
+    /**
+     * @notice Removes the specified origin domain's ISM
+     * @param _domain The origin domain
+     */
+    function _remove(uint32 _domain) internal {
+        require(_modules.remove(_domain), _originNotFoundError(_domain));
+    }
+
+    function _originNotFoundError(uint32 _origin)
+        internal
+        pure
+        returns (string memory)
+    {
+        return string.concat("No ISM found for origin: ", _origin.toString());
+    }
+
     /**
      * @notice Sets the ISM to be used for the specified origin domain
      * @param _domain The origin domain
      * @param _module The ISM to use to verify messages
      */
     function _set(uint32 _domain, address _module) internal {
-        require(_module.isContract(), "!contract");
+        require(_module.isContract(), "ISM must be a contract");
         _modules.set(_domain, _module.addressToBytes32());
-        emit ModuleSet(_domain, _module);
     }
 }

solidity/hardhat.config.ts

@@ -13,10 +13,6 @@ module.exports = {
       {
         version: '0.8.19',
       },
-      {
-        // for @eth-optimism
-        version: '0.8.15',
-      },
     ],
     settings: {
       optimizer: {

solidity/test/isms/DomainRoutingIsm.t.sol

@@ -2,7 +2,6 @@
 pragma solidity ^0.8.13;
 
 import "forge-std/Test.sol";
-import "forge-std/console.sol";
 
 import {DomainRoutingIsm} from "../../contracts/isms/routing/DomainRoutingIsm.sol";
 import {DefaultFallbackRoutingIsm} from "../../contracts/isms/routing/DefaultFallbackRoutingIsm.sol";
@@ -15,7 +14,6 @@ import {TestPostDispatchHook} from "../../contracts/test/TestPostDispatchHook.so
 contract DomainRoutingIsmTest is Test {
     address private constant NON_OWNER =
         0xCAfEcAfeCAfECaFeCaFecaFecaFECafECafeCaFe;
-    event ModuleSet(uint32 indexed domain, IInterchainSecurityModule module);
     DomainRoutingIsm internal ism;
 
     function setUp() public virtual {
@@ -31,15 +29,22 @@ contract DomainRoutingIsmTest is Test {
     }
 
     function getMetadata(uint32 domain) internal view returns (bytes memory) {
-        return TestIsm(address(ism.modules(domain))).requiredMetadata();
+        return TestIsm(address(ism.module(domain))).requiredMetadata();
     }
 
     function testSet(uint32 domain) public {
         TestIsm _ism = deployTestIsm(bytes32(0));
-        vm.expectEmit(true, true, false, true);
-        emit ModuleSet(domain, _ism);
         ism.set(domain, _ism);
-        assertEq(address(ism.modules(domain)), address(_ism));
+        assertEq(address(ism.module(domain)), address(_ism));
+    }
+
+    function testRemove(uint32 domain) public {
+        vm.expectRevert();
+        ism.remove(domain);
+
+        TestIsm _ism = deployTestIsm(bytes32(0));
+        ism.set(domain, _ism);
+        ism.remove(domain);
     }
 
     function testSetManyViaFactory(uint8 count, uint32 domain) public {
@@ -51,12 +56,10 @@ contract DomainRoutingIsmTest is Test {
         for (uint32 i = 0; i < count; ++i) {
             _domains[i] = domain - i;
             _isms[i] = deployTestIsm(bytes32(0));
-            vm.expectEmit(true, true, false, true);
-            emit ModuleSet(_domains[i], _isms[i]);
         }
         ism = factory.deploy(_domains, _isms);
         for (uint256 i = 0; i < count; ++i) {
-            assertEq(address(ism.modules(_domains[i])), address(_isms[i]));
+            assertEq(address(ism.module(_domains[i])), address(_isms[i]));
         }
     }
 
@@ -72,7 +75,10 @@ contract DomainRoutingIsmTest is Test {
         ism.set(domain, deployTestIsm(seed));
 
         bytes memory metadata = getMetadata(domain);
+        uint256 gasBefore = gasleft();
         assertTrue(ism.verify(metadata, MessageUtils.build(domain)));
+        uint256 gasAfter = gasleft();
+        console.log("Overhead gas usage: %d", gasBefore - gasAfter);
     }
 
     function testVerifyNoIsm(uint32 domain, bytes32 seed) public virtual {
@@ -80,7 +86,7 @@ contract DomainRoutingIsmTest is Test {
         ism.set(domain, deployTestIsm(seed));
 
         bytes memory metadata = getMetadata(domain);
-        vm.expectRevert("No ISM found for origin domain");
+        vm.expectRevert();
         ism.verify(metadata, MessageUtils.build(domain - 1));
     }
 
@@ -113,7 +119,7 @@ contract DefaultFallbackRoutingIsmTest is DomainRoutingIsmTest {
     }
 
     function testConstructorReverts() public {
-        vm.expectRevert("DefaultFallbackRoutingIsm: INVALID_MAILBOX");
+        vm.expectRevert("MailboxClient: invalid mailbox");
         new DefaultFallbackRoutingIsm(address(0));
     }
 

solidity/test/router.test.ts

@@ -110,6 +110,17 @@ describe('Router', async () => {
     expect(await router.mustHaveRemoteRouter(origin)).to.equal(remoteBytes);
   });
 
+  it('owner can unenroll remote router', async () => {
+    const remote = nonOwner.address;
+    const remoteBytes = addressToBytes32(remote);
+    await expect(router.unenrollRemoteRouter(origin)).to.be.revertedWith(
+      `No router enrolled for domain: ${origin}`,
+    );
+    await router.enrollRemoteRouter(origin, remoteBytes);
+    await router.unenrollRemoteRouter(origin);
+    expect(await router.isRemoteRouter(origin, remoteBytes)).to.equal(false);
+  });
+
   it('owner can enroll remote router using batch function', async () => {
     const remote = nonOwner.address;
     const remoteBytes = addressToBytes32(nonOwner.address);
@@ -122,6 +133,17 @@ describe('Router', async () => {
     expect(await router.mustHaveRemoteRouter(origin)).to.equal(remoteBytes);
   });
 
+  it('owner can unenroll remote router using batch function', async () => {
+    const remote = nonOwner.address;
+    const remoteBytes = addressToBytes32(remote);
+    await expect(router.unenrollRemoteRouters([origin])).to.be.revertedWith(
+      `No router enrolled for domain: ${origin}`,
+    );
+    await router.enrollRemoteRouter(origin, remoteBytes);
+    await router.unenrollRemoteRouters([origin]);
+    expect(await router.isRemoteRouter(origin, remoteBytes)).to.equal(false);
+  });
+
   describe('#domains', () => {
     it('returns the domains', async () => {
       await router.enrollRemoteRouters(

typescript/sdk/src/ism/HyperlaneIsmFactory.ts

@@ -382,7 +382,7 @@ export async function moduleMatchesConfig(
       // Recursively check that the submodule for each configured
       // domain matches the submodule config.
       for (const [origin, subConfig] of Object.entries(config.domains)) {
-        const subModule = await routingIsm.modules(
+        const subModule = await routingIsm.module(
           multiProvider.getDomainId(origin),
         );
         const subModuleMatches = await moduleMatchesConfig(