From 8d8ba3f7ab66391fbb3a3b443b08f097d83be1de Mon Sep 17 00:00:00 2001 From: Kunal Arora <55632507+aroralanuk@users.noreply.github.com> Date: Wed, 10 Jan 2024 15:18:10 -0500 Subject: [PATCH] fix:`HyperlaneIsmFactory` doesn't fail with missing chainMetadata errors (#3142) ### Description HyperlaneIsmFactory is now wary of (try)getDomainId or (try)getChainName calls which may fail and handles them appropriately. - switched from chainName to domain in `routingModuleDelta` - replace `getDomainId` with `tryGetDomainId` and `getChainName` with `tryGetChainName` wherever necessary. ### Drive-by changes None ### Related issues None ### Backward compatibility Yes ### Testing Hardhat unit tests --- .changeset/old-lizards-melt.md | 5 + .../ism/HyperlaneIsmFactory.hardhat-test.ts | 104 +++++++++++++++++- typescript/sdk/src/ism/HyperlaneIsmFactory.ts | 79 ++++++++----- typescript/sdk/src/ism/types.ts | 8 +- 4 files changed, 163 insertions(+), 33 deletions(-) create mode 100644 .changeset/old-lizards-melt.md diff --git a/.changeset/old-lizards-melt.md b/.changeset/old-lizards-melt.md new file mode 100644 index 0000000000..739b78c897 --- /dev/null +++ b/.changeset/old-lizards-melt.md @@ -0,0 +1,5 @@ +--- +'@hyperlane-xyz/sdk': minor +--- + +HyperlaneIsmFactory is now wary of (try)getDomainId or (try)getChainName calls which may fail and handles them appropriately. diff --git a/typescript/sdk/src/ism/HyperlaneIsmFactory.hardhat-test.ts b/typescript/sdk/src/ism/HyperlaneIsmFactory.hardhat-test.ts index df6967c83f..0541d7d755 100644 --- a/typescript/sdk/src/ism/HyperlaneIsmFactory.hardhat-test.ts +++ b/typescript/sdk/src/ism/HyperlaneIsmFactory.hardhat-test.ts @@ -1,6 +1,7 @@ import { expect } from 'chai'; import { ethers } from 'hardhat'; +import { DomainRoutingIsm } from '@hyperlane-xyz/core'; import { Address, error } from '@hyperlane-xyz/utils'; import { TestChains } from '../consts/chains'; @@ -77,6 +78,7 @@ describe('HyperlaneIsmFactory', async () => { let ismFactory: HyperlaneIsmFactory; let coreApp: TestCoreApp; let multiProvider: MultiProvider; + let ismFactoryDeployer: HyperlaneProxyFactoryDeployer; let exampleRoutingConfig: RoutingIsmConfig; let mailboxAddress: Address, newMailboxAddress: Address; const chain = 'test1'; @@ -84,7 +86,7 @@ describe('HyperlaneIsmFactory', async () => { beforeEach(async () => { const [signer] = await ethers.getSigners(); multiProvider = MultiProvider.createTestMultiProvider({ signer }); - const ismFactoryDeployer = new HyperlaneProxyFactoryDeployer(multiProvider); + ismFactoryDeployer = new HyperlaneProxyFactoryDeployer(multiProvider); ismFactory = new HyperlaneIsmFactory( await ismFactoryDeployer.deploy(multiProvider.mapKnownChains(() => ({}))), multiProvider, @@ -207,6 +209,59 @@ describe('HyperlaneIsmFactory', async () => { expect(matches).to.be.true; }); + it(`should skip deployment with warning if no chain metadata configured ${type}`, async () => { + exampleRoutingConfig.type = type as + | IsmType.ROUTING + | IsmType.FALLBACK_ROUTING; + let matches = true; + exampleRoutingConfig.domains['test4'] = { + type: IsmType.MESSAGE_ID_MULTISIG, + threshold: 1, + validators: [randomAddress()], + }; + let ism = await ismFactory.deploy({ + destination: chain, + config: exampleRoutingConfig, + mailbox: mailboxAddress, + }); + const existingIsm = ism.address; + matches = + matches && + existingIsm === ism.address && + (await moduleMatchesConfig( + chain, + ism.address, + exampleRoutingConfig, + ismFactory.multiProvider, + ismFactory.getContracts(chain), + mailboxAddress, + )); + + exampleRoutingConfig.domains['test5'] = { + type: IsmType.MESSAGE_ID_MULTISIG, + threshold: 1, + validators: [randomAddress()], + }; + ism = await ismFactory.deploy({ + destination: chain, + config: exampleRoutingConfig, + existingIsmAddress: ism.address, + mailbox: mailboxAddress, + }); + matches = + matches && + existingIsm === ism.address && + (await moduleMatchesConfig( + chain, + ism.address, + exampleRoutingConfig, + ismFactory.multiProvider, + ismFactory.getContracts(chain), + mailboxAddress, + )); + expect(matches).to.be.true; + }); + it(`deletes route in an existing ${type}`, async () => { exampleRoutingConfig.type = type as | IsmType.ROUTING @@ -240,6 +295,53 @@ describe('HyperlaneIsmFactory', async () => { expect(matches).to.be.true; }); + it(`deletes route in an existing ${type} even if not in multiprovider`, async () => { + exampleRoutingConfig.type = type as + | IsmType.ROUTING + | IsmType.FALLBACK_ROUTING; + let matches = true; + let ism = await ismFactory.deploy({ + destination: chain, + config: exampleRoutingConfig, + mailbox: mailboxAddress, + }); + const existingIsm = ism.address; + const domainsBefore = await (ism as DomainRoutingIsm).domains(); + + // deleting the domain and removing from multiprovider should unenroll the domain + // NB: we'll deploy new multisigIsms for the domains bc of new factories but the routingIsm address should be the same because of existingIsmAddress + delete exampleRoutingConfig.domains['test3']; + multiProvider = multiProvider.intersect(['test1', 'test2']).result; + ismFactoryDeployer = new HyperlaneProxyFactoryDeployer(multiProvider); + ismFactory = new HyperlaneIsmFactory( + await ismFactoryDeployer.deploy( + multiProvider.mapKnownChains(() => ({})), + ), + multiProvider, + ); + ism = await ismFactory.deploy({ + destination: chain, + config: exampleRoutingConfig, + existingIsmAddress: ism.address, + mailbox: mailboxAddress, + }); + const domainsAfter = await (ism as DomainRoutingIsm).domains(); + + matches = + matches && + existingIsm == ism.address && + (await moduleMatchesConfig( + chain, + ism.address, + exampleRoutingConfig, + ismFactory.multiProvider, + ismFactory.getContracts(chain), + mailboxAddress, + )); + expect(domainsBefore.length - 1).to.equal(domainsAfter.length); + expect(matches).to.be.true; + }); + it(`updates owner in an existing ${type}`, async () => { exampleRoutingConfig.type = type as | IsmType.ROUTING diff --git a/typescript/sdk/src/ism/HyperlaneIsmFactory.ts b/typescript/sdk/src/ism/HyperlaneIsmFactory.ts index 42391df177..f5a0ae43bf 100644 --- a/typescript/sdk/src/ism/HyperlaneIsmFactory.ts +++ b/typescript/sdk/src/ism/HyperlaneIsmFactory.ts @@ -23,9 +23,13 @@ import { } from '@hyperlane-xyz/core'; import { Address, + Domain, eqAddress, formatMessage, normalizeAddress, + objFilter, + objMap, + warn, } from '@hyperlane-xyz/utils'; import { HyperlaneApp } from '../app/HyperlaneApp'; @@ -201,6 +205,22 @@ export class HyperlaneIsmFactory extends HyperlaneApp { const overrides = this.multiProvider.getTransactionOverrides(destination); const routingIsmFactory = this.getContracts(destination).routingIsmFactory; let routingIsm: DomainRoutingIsm | DefaultFallbackRoutingIsm; + // filtering out domains which are not part of the multiprovider + config.domains = objFilter( + config.domains, + (domain, config): config is IsmConfig => { + const domainId = this.multiProvider.tryGetDomainId(domain); + if (domainId === null) { + warn( + `Domain ${domain} doesn't have chain metadata provided, skipping ...`, + ); + } + return domainId !== null; + }, + ); + const safeConfigDomains = Object.keys(config.domains).map((domain) => + this.multiProvider.getDomainId(domain), + ); const delta: RoutingIsmDelta = existingIsmAddress ? await routingModuleDelta( destination, @@ -212,7 +232,7 @@ export class HyperlaneIsmFactory extends HyperlaneApp { ) : { domainsToUnenroll: [], - domainsToEnroll: Object.keys(config.domains), + domainsToEnroll: safeConfigDomains, }; const signer = this.multiProvider.getSigner(destination); @@ -227,13 +247,14 @@ export class HyperlaneIsmFactory extends HyperlaneApp { } // reconfiguring existing routing ISM if (existingIsmAddress && isOwner && !delta.mailbox) { - const isms: ChainMap
= {}; + const isms: Record = {}; routingIsm = DomainRoutingIsm__factory.connect( existingIsmAddress, this.multiProvider.getSigner(destination), ); // deploying all the ISMs which have to be updated - for (const origin of delta.domainsToEnroll) { + for (const originDomain of delta.domainsToEnroll) { + const origin = this.multiProvider.getChainName(originDomain); // already filtered to only include domains in the multiprovider logger( `Reconfiguring preexisting routing ISM at for origin ${origin}...`, ); @@ -243,23 +264,20 @@ export class HyperlaneIsmFactory extends HyperlaneApp { origin, mailbox, }); - isms[origin] = ism.address; + isms[originDomain] = ism.address; const tx = await routingIsm.set( - this.multiProvider.getDomainId(origin), - isms[origin], + originDomain, + isms[originDomain], overrides, ); await this.multiProvider.handleTx(destination, tx); } // unenrolling domains if needed - for (const origin of delta.domainsToUnenroll) { + for (const originDomain of delta.domainsToUnenroll) { logger( - `Unenrolling origin ${origin} from preexisting routing ISM at ${existingIsmAddress}...`, - ); - const tx = await routingIsm.remove( - this.multiProvider.getDomainId(origin), - overrides, + `Unenrolling originDomain ${originDomain} from preexisting routing ISM at ${existingIsmAddress}...`, ); + const tx = await routingIsm.remove(originDomain, overrides); await this.multiProvider.handleTx(destination, tx); } // transfer ownership if needed @@ -280,9 +298,6 @@ export class HyperlaneIsmFactory extends HyperlaneApp { isms[origin] = ism.address; } const submoduleAddresses = Object.values(isms); - const domains = Object.keys(isms).map((chain) => - this.multiProvider.getDomainId(chain), - ); let receipt: ethers.providers.TransactionReceipt; if (config.type === IsmType.FALLBACK_ROUTING) { // deploying new fallback routing ISM @@ -302,7 +317,7 @@ export class HyperlaneIsmFactory extends HyperlaneApp { destination, routingIsm['initialize(address,uint32[],address[])']( config.owner, - domains, + safeConfigDomains, submoduleAddresses, overrides, ), @@ -311,7 +326,7 @@ export class HyperlaneIsmFactory extends HyperlaneApp { // deploying new domain routing ISM const tx = await routingIsmFactory.deploy( config.owner, - domains, + safeConfigDomains, submoduleAddresses, overrides, ); @@ -428,12 +443,17 @@ export async function moduleCanCertainlyVerify( origin: ChainName, destination: ChainName, ): Promise { + const originDomainId = multiProvider.tryGetDomainId(origin); + const destinationDomainId = multiProvider.tryGetDomainId(destination); + if (!originDomainId || !destinationDomainId) { + return false; + } const message = formatMessage( 0, 0, - multiProvider.getDomainId(origin), + originDomainId, ethers.constants.AddressZero, - multiProvider.getDomainId(destination), + destinationDomainId, ethers.constants.AddressZero, '0x', ); @@ -689,7 +709,11 @@ export async function routingModuleDelta( const routingIsm = DomainRoutingIsm__factory.connect(moduleAddress, provider); const owner = await routingIsm.owner(); const deployedDomains = (await routingIsm.domains()).map((domain) => - multiProvider.getChainName(domain.toNumber()), + domain.toNumber(), + ); + // config.domains is already filtered to only include domains in the multiprovider + const safeConfigDomains = objMap(config.domains, (domain) => + multiProvider.getDomainId(domain), ); const delta: RoutingIsmDelta = { @@ -707,16 +731,15 @@ export async function routingModuleDelta( } // check for exclusion of domains in the config delta.domainsToUnenroll = deployedDomains.filter( - (domain) => !Object.keys(config.domains).includes(domain), + (domain) => !Object.values(safeConfigDomains).includes(domain), ); // check for inclusion of domains in the config for (const [origin, subConfig] of Object.entries(config.domains)) { - if (!deployedDomains.includes(origin)) { - delta.domainsToEnroll.push(origin); + const originDomain = safeConfigDomains[origin]; + if (!deployedDomains.includes(originDomain)) { + delta.domainsToEnroll.push(originDomain); } else { - const subModule = await routingIsm.module( - multiProvider.getDomainId(origin), - ); + const subModule = await routingIsm.module(originDomain); // Recursively check that the submodule for each configured // domain matches the submodule config. const subModuleMatches = await moduleMatchesConfig( @@ -725,9 +748,9 @@ export async function routingModuleDelta( subConfig, multiProvider, contracts, - origin, + mailbox, ); - if (!subModuleMatches) delta.domainsToEnroll.push(origin); + if (!subModuleMatches) delta.domainsToEnroll.push(originDomain); } } return delta; diff --git a/typescript/sdk/src/ism/types.ts b/typescript/sdk/src/ism/types.ts index 74bec60870..8cd7886f22 100644 --- a/typescript/sdk/src/ism/types.ts +++ b/typescript/sdk/src/ism/types.ts @@ -5,9 +5,9 @@ import { OPStackIsm, TestIsm, } from '@hyperlane-xyz/core'; -import type { Address, ValueOf } from '@hyperlane-xyz/utils'; +import type { Address, Domain, ValueOf } from '@hyperlane-xyz/utils'; -import { ChainMap, ChainName } from '../types'; +import { ChainMap } from '../types'; // this enum should match the IInterchainSecurityModule.sol enum // meant for the relayer @@ -106,8 +106,8 @@ export type DeployedIsm = ValueOf; // for finding the difference between the onchain deployment and the config provided export type RoutingIsmDelta = { - domainsToUnenroll: ChainName[]; // new or updated isms for the domain - domainsToEnroll: ChainName[]; // isms to remove + domainsToUnenroll: Domain[]; // new or updated isms for the domain + domainsToEnroll: Domain[]; // isms to remove owner?: Address; // is the owner different mailbox?: Address; // is the mailbox different (only for fallback routing) };