From 7919417ecd68d7f6a1bd0be4249fba8065b6f30e Mon Sep 17 00:00:00 2001 From: Kunal Arora <55632507+aroralanuk@users.noreply.github.com> Date: Wed, 13 Dec 2023 13:26:52 -0500 Subject: [PATCH] feat: add routing delta for more granular routingIsm config updates (#3017) ### Description - add support for `routingIsmDelta` which filters out the incompatibility between the onchain deployed config and the desired config. - based on the above, you either update the deployed Ism with new routes, delete old routes, change owners, etc. - moduleMatchesConfig uses the same ### Drive-by changes - checking exclusion of domains in the onchain deployed config ### Related issues - fixes https://github.com/hyperlane-xyz/issues/issues/818 ### Backward compatibility Yes ### Testing Unit tests --------- Co-authored-by: Yorke Rhodes --- .changeset/breezy-bats-type.md | 8 + solidity/test/testSendReceiver.test.ts | 40 --- .../ism/HyperlaneIsmFactory.hardhat-test.ts | 279 ++++++++++++++--- typescript/sdk/src/ism/HyperlaneIsmFactory.ts | 293 +++++++++++++----- typescript/sdk/src/ism/types.ts | 10 +- 5 files changed, 465 insertions(+), 165 deletions(-) create mode 100644 .changeset/breezy-bats-type.md delete mode 100644 solidity/test/testSendReceiver.test.ts diff --git a/.changeset/breezy-bats-type.md b/.changeset/breezy-bats-type.md new file mode 100644 index 000000000..4b1dd9ca3 --- /dev/null +++ b/.changeset/breezy-bats-type.md @@ -0,0 +1,8 @@ +--- +'@hyperlane-xyz/sdk': patch +--- + +Granular control of updating predeployed routingIsms based on routing config mismatch +- Add support for routingIsmDelta which filters out the incompatibility between the onchain deployed config and the desired config. +- Based on the above, you either update the deployed Ism with new routes, delete old routes, change owners, etc. +- `moduleMatchesConfig` uses the same diff --git a/solidity/test/testSendReceiver.test.ts b/solidity/test/testSendReceiver.test.ts deleted file mode 100644 index 527b96859..000000000 --- a/solidity/test/testSendReceiver.test.ts +++ /dev/null @@ -1,40 +0,0 @@ -import { expect } from 'chai'; -import { ethers } from 'hardhat'; - -import { addressToBytes32 } from '@hyperlane-xyz/utils'; - -import { TestSendReceiver__factory } from '../types'; - -describe('TestSendReceiver', () => { - it('randomly handles a message', async () => { - const [signer] = await ethers.getSigners(); - const signerAddress = await signer.getAddress(); - const recipientFactory = new TestSendReceiver__factory(signer); - const recipient = await recipientFactory.deploy(); - - // Didn't know how else to test the randomness - let successes = 0; - let failures = 0; - for (let i = 0; i < 100; i++) { - try { - // "Inject randomness" - await signer.sendTransaction({ - from: signerAddress, - to: signerAddress, - value: 1, - }); - await recipient.handle( - 0, - addressToBytes32(recipient.address), - '0x1234', - ); - successes += 1; - } catch (error) { - failures += 1; - } - } - - expect(successes).to.be.greaterThan(5); - expect(failures).to.be.greaterThan(1); - }); -}); diff --git a/typescript/sdk/src/ism/HyperlaneIsmFactory.hardhat-test.ts b/typescript/sdk/src/ism/HyperlaneIsmFactory.hardhat-test.ts index c5061e08d..df6967c83 100644 --- a/typescript/sdk/src/ism/HyperlaneIsmFactory.hardhat-test.ts +++ b/typescript/sdk/src/ism/HyperlaneIsmFactory.hardhat-test.ts @@ -1,7 +1,7 @@ import { expect } from 'chai'; import { ethers } from 'hardhat'; -import { error } from '@hyperlane-xyz/utils'; +import { Address, error } from '@hyperlane-xyz/utils'; import { TestChains } from '../consts/chains'; import { TestCoreApp } from '../core/TestCoreApp'; @@ -76,21 +76,37 @@ const randomIsmConfig = (depth = 0, maxDepth = 2): IsmConfig => { describe('HyperlaneIsmFactory', async () => { let ismFactory: HyperlaneIsmFactory; let coreApp: TestCoreApp; - + let multiProvider: MultiProvider; + let exampleRoutingConfig: RoutingIsmConfig; + let mailboxAddress: Address, newMailboxAddress: Address; const chain = 'test1'; - before(async () => { + beforeEach(async () => { const [signer] = await ethers.getSigners(); - - const multiProvider = MultiProvider.createTestMultiProvider({ signer }); - + multiProvider = MultiProvider.createTestMultiProvider({ signer }); const ismFactoryDeployer = new HyperlaneProxyFactoryDeployer(multiProvider); ismFactory = new HyperlaneIsmFactory( await ismFactoryDeployer.deploy(multiProvider.mapKnownChains(() => ({}))), multiProvider, ); - const coreDeployer = new TestCoreDeployer(multiProvider, ismFactory); + let coreDeployer = new TestCoreDeployer(multiProvider, ismFactory); + coreApp = await coreDeployer.deployApp(); + mailboxAddress = coreApp.getContracts(chain).mailbox.address; + + coreDeployer = new TestCoreDeployer(multiProvider, ismFactory); coreApp = await coreDeployer.deployApp(); + newMailboxAddress = coreApp.getContracts(chain).mailbox.address; + + exampleRoutingConfig = { + type: IsmType.ROUTING, + owner: await multiProvider.getSignerAddress(chain), + domains: Object.fromEntries( + TestChains.filter((c) => c !== 'test1').map((c) => [ + c, + randomMultisigIsmConfig(3, 5), + ]), + ), + }; }); it('deploys a simple ism', async () => { @@ -136,47 +152,218 @@ describe('HyperlaneIsmFactory', async () => { }); } - it('deploys routingIsm with correct routes', async () => { - const config: RoutingIsmConfig = { - type: IsmType.ROUTING, - owner: randomAddress(), - domains: Object.fromEntries( - TestChains.map((c) => [c, randomIsmConfig()]), - ), - }; - const ism = await ismFactory.deploy({ destination: chain, config }); - const matches = await moduleMatchesConfig( - chain, - ism.address, - config, - ismFactory.multiProvider, - ismFactory.getContracts(chain), - ); - expect(matches).to.be.true; - }); + for (const type of [IsmType.ROUTING, IsmType.FALLBACK_ROUTING]) { + it(`deploys ${type} routingIsm with correct routes`, async () => { + exampleRoutingConfig.type = type as + | IsmType.ROUTING + | IsmType.FALLBACK_ROUTING; + const ism = await ismFactory.deploy({ + destination: chain, + config: exampleRoutingConfig, + mailbox: mailboxAddress, + }); + const matches = await moduleMatchesConfig( + chain, + ism.address, + exampleRoutingConfig, + ismFactory.multiProvider, + ismFactory.getContracts(chain), + mailboxAddress, + ); + expect(matches).to.be.true; + }); - it('deploys defaultFallbackRoutingIsm with correct routes and fallback to mailbox', async () => { - const config: RoutingIsmConfig = { - type: IsmType.FALLBACK_ROUTING, - owner: randomAddress(), - domains: Object.fromEntries( - TestChains.map((c) => [c, randomIsmConfig()]), - ), - }; - const mailbox = await coreApp.getContracts(chain).mailbox; - const ism = await ismFactory.deploy({ + it(`update route in an existing ${type}`, 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; + // changing the type of a domain should enroll the domain + (exampleRoutingConfig.domains['test2'] as MultisigIsmConfig).type = + IsmType.MESSAGE_ID_MULTISIG; + 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 + | IsmType.FALLBACK_ROUTING; + let matches = true; + let ism = await ismFactory.deploy({ + destination: chain, + config: exampleRoutingConfig, + mailbox: mailboxAddress, + }); + const existingIsm = ism.address; + // deleting the domain should unenroll the domain + delete exampleRoutingConfig.domains['test3']; + 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(`updates owner in an existing ${type}`, 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; + // change the owner + exampleRoutingConfig.owner = 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(`no changes to an existing ${type} means no redeployment or updates`, 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; + // using the same config should not change anything + 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(`redeploy same config if the deployer doesn't have ownership of ${type}`, async () => { + exampleRoutingConfig.type = type as + | IsmType.ROUTING + | IsmType.FALLBACK_ROUTING; + let matches = true; + exampleRoutingConfig.owner = randomAddress(); + let ism = await ismFactory.deploy({ + destination: chain, + config: exampleRoutingConfig, + mailbox: mailboxAddress, + }); + const existingIsm = ism.address; + 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(`redeploy same config if the mailbox address changes for defaultFallbackRoutingIsm`, async () => { + exampleRoutingConfig.type = IsmType.FALLBACK_ROUTING; + let matches = true; + let ism = await ismFactory.deploy({ destination: chain, - config, - mailbox: mailbox.address, - }); // not through an actual factory just for maintaining consistency in naming - const matches = await moduleMatchesConfig( - chain, - ism.address, - config, - ismFactory.multiProvider, - ismFactory.getContracts(chain), - mailbox.address, - ); + config: exampleRoutingConfig, + mailbox: mailboxAddress, + }); + const existingIsm = ism.address; + ism = await ismFactory.deploy({ + destination: chain, + config: exampleRoutingConfig, + existingIsmAddress: ism.address, + mailbox: newMailboxAddress, + }); + matches = + matches && + existingIsm !== ism.address && + (await moduleMatchesConfig( + chain, + ism.address, + exampleRoutingConfig, + ismFactory.multiProvider, + ismFactory.getContracts(chain), + newMailboxAddress, + )); expect(matches).to.be.true; }); }); diff --git a/typescript/sdk/src/ism/HyperlaneIsmFactory.ts b/typescript/sdk/src/ism/HyperlaneIsmFactory.ts index b7ada8b58..42391df17 100644 --- a/typescript/sdk/src/ism/HyperlaneIsmFactory.ts +++ b/typescript/sdk/src/ism/HyperlaneIsmFactory.ts @@ -21,7 +21,12 @@ import { StaticThresholdAddressSetFactory, TestIsm__factory, } from '@hyperlane-xyz/core'; -import { Address, eqAddress, formatMessage, warn } from '@hyperlane-xyz/utils'; +import { + Address, + eqAddress, + formatMessage, + normalizeAddress, +} from '@hyperlane-xyz/utils'; import { HyperlaneApp } from '../app/HyperlaneApp'; import { @@ -34,6 +39,7 @@ import { ProxyFactoryFactories, proxyFactoryFactories, } from '../deploy/contracts'; +import { logger } from '../logger'; import { MultiProvider } from '../providers/MultiProvider'; import { ChainMap, ChainName } from '../types'; @@ -47,6 +53,7 @@ import { MultisigIsmConfig, OpStackIsmConfig, RoutingIsmConfig, + RoutingIsmDelta, ismTypeToModuleType, } from './types'; @@ -89,8 +96,9 @@ export class HyperlaneIsmFactory extends HyperlaneApp { config: C; origin?: ChainName; mailbox?: Address; + existingIsmAddress?: Address; }): Promise { - const { destination, config, origin, mailbox } = params; + const { destination, config, origin, mailbox, existingIsmAddress } = params; if (typeof config === 'string') { // @ts-ignore return IInterchainSecurityModule__factory.connect( @@ -119,6 +127,7 @@ export class HyperlaneIsmFactory extends HyperlaneApp { config, origin, mailbox, + existingIsmAddress, }); break; case IsmType.AGGREGATION: @@ -186,77 +195,151 @@ export class HyperlaneIsmFactory extends HyperlaneApp { config: RoutingIsmConfig; origin?: ChainName; mailbox?: Address; + existingIsmAddress?: Address; }): Promise { - const { destination, config, mailbox } = params; - const routingIsmFactory = this.getContracts(destination).routingIsmFactory; - const isms: ChainMap
= {}; - for (const origin of Object.keys(config.domains)) { - const ism = await this.deploy({ - destination, - config: config.domains[origin], - origin, - mailbox, - }); - isms[origin] = ism.address; - } - const domains = Object.keys(isms).map((chain) => - this.multiProvider.getDomainId(chain), - ); - const submoduleAddresses = Object.values(isms); + const { destination, config, mailbox, existingIsmAddress } = params; const overrides = this.multiProvider.getTransactionOverrides(destination); - let receipt: ethers.providers.TransactionReceipt; + const routingIsmFactory = this.getContracts(destination).routingIsmFactory; let routingIsm: DomainRoutingIsm | DefaultFallbackRoutingIsm; - if (config.type === IsmType.FALLBACK_ROUTING) { - if (!mailbox) { - throw new Error( - 'Mailbox address is required for deploying fallback routing ISM', + const delta: RoutingIsmDelta = existingIsmAddress + ? await routingModuleDelta( + destination, + existingIsmAddress, + config, + this.multiProvider, + this.getContracts(destination), + mailbox, + ) + : { + domainsToUnenroll: [], + domainsToEnroll: Object.keys(config.domains), + }; + + const signer = this.multiProvider.getSigner(destination); + const provider = this.multiProvider.getProvider(destination); + let isOwner = false; + if (existingIsmAddress) { + const owner = await DomainRoutingIsm__factory.connect( + existingIsmAddress, + provider, + ).owner(); + isOwner = eqAddress(await signer.getAddress(), owner); + } + // reconfiguring existing routing ISM + if (existingIsmAddress && isOwner && !delta.mailbox) { + const isms: ChainMap
= {}; + routingIsm = DomainRoutingIsm__factory.connect( + existingIsmAddress, + this.multiProvider.getSigner(destination), + ); + // deploying all the ISMs which have to be updated + for (const origin of delta.domainsToEnroll) { + logger( + `Reconfiguring preexisting routing ISM at for origin ${origin}...`, ); + const ism = await this.deploy({ + destination, + config: config.domains[origin], + origin, + mailbox, + }); + isms[origin] = ism.address; + const tx = await routingIsm.set( + this.multiProvider.getDomainId(origin), + isms[origin], + overrides, + ); + await this.multiProvider.handleTx(destination, tx); } - debug('Deploying fallback routing ISM ...'); - routingIsm = await this.multiProvider.handleDeploy( - destination, - new DefaultFallbackRoutingIsm__factory(), - [mailbox], + // unenrolling domains if needed + for (const origin of delta.domainsToUnenroll) { + logger( + `Unenrolling origin ${origin} from preexisting routing ISM at ${existingIsmAddress}...`, + ); + const tx = await routingIsm.remove( + this.multiProvider.getDomainId(origin), + overrides, + ); + await this.multiProvider.handleTx(destination, tx); + } + // transfer ownership if needed + if (delta.owner) { + logger(`Transferring ownership of routing ISM...`); + const tx = await routingIsm.transferOwnership(delta.owner, overrides); + await this.multiProvider.handleTx(destination, tx); + } + } else { + const isms: ChainMap
= {}; + for (const origin of Object.keys(config.domains)) { + const ism = await this.deploy({ + destination, + config: config.domains[origin], + origin, + mailbox, + }); + isms[origin] = ism.address; + } + const submoduleAddresses = Object.values(isms); + const domains = Object.keys(isms).map((chain) => + this.multiProvider.getDomainId(chain), ); - debug('Initialising fallback routing ISM ...'); - receipt = await this.multiProvider.handleTx( - destination, - routingIsm['initialize(address,uint32[],address[])']( + let receipt: ethers.providers.TransactionReceipt; + if (config.type === IsmType.FALLBACK_ROUTING) { + // deploying new fallback routing ISM + if (!mailbox) { + throw new Error( + 'Mailbox address is required for deploying fallback routing ISM', + ); + } + logger('Deploying fallback routing ISM ...'); + routingIsm = await this.multiProvider.handleDeploy( + destination, + new DefaultFallbackRoutingIsm__factory(), + [mailbox], + ); + logger('Initialising fallback routing ISM ...'); + receipt = await this.multiProvider.handleTx( + destination, + routingIsm['initialize(address,uint32[],address[])']( + config.owner, + domains, + submoduleAddresses, + overrides, + ), + ); + } else { + // deploying new domain routing ISM + const tx = await routingIsmFactory.deploy( config.owner, domains, submoduleAddresses, overrides, - ), - ); - } else { - const tx = await routingIsmFactory.deploy( - config.owner, - domains, - submoduleAddresses, - overrides, - ); - receipt = await this.multiProvider.handleTx(destination, tx); - - // TODO: Break this out into a generalized function - const dispatchLogs = receipt.logs - .map((log) => { - try { - return routingIsmFactory.interface.parseLog(log); - } catch (e) { - return undefined; - } - }) - .filter( - (log): log is ethers.utils.LogDescription => - !!log && log.name === 'ModuleDeployed', ); - const moduleAddress = dispatchLogs[0].args['module']; - routingIsm = DomainRoutingIsm__factory.connect( - moduleAddress, - this.multiProvider.getSigner(destination), - ); + receipt = await this.multiProvider.handleTx(destination, tx); + + // TODO: Break this out into a generalized function + const dispatchLogs = receipt.logs + .map((log) => { + try { + return routingIsmFactory.interface.parseLog(log); + } catch (e) { + return undefined; + } + }) + .filter( + (log): log is ethers.utils.LogDescription => + !!log && log.name === 'ModuleDeployed', + ); + if (dispatchLogs.length === 0) { + throw new Error('No ModuleDeployed event found'); + } + const moduleAddress = dispatchLogs[0].args['module']; + routingIsm = DomainRoutingIsm__factory.connect( + moduleAddress, + this.multiProvider.getSigner(destination), + ); + } } - return routingIsm; } @@ -410,7 +493,7 @@ export async function moduleCanCertainlyVerify( throw new Error(`Unsupported module type: ${moduleType}`); } } catch (e) { - warn(`Error checking module ${destModule}: ${e}`); + logger(`Error checking module ${destModule}: ${e}`); return false; } } else { @@ -523,22 +606,20 @@ export async function moduleMatchesConfig( mailbox !== undefined && eqAddress(mailboxAddress, mailbox); } - // 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.module( - multiProvider.getDomainId(origin), - ); - const subModuleMatches = await moduleMatchesConfig( - chain, - subModule, - subConfig, - multiProvider, - contracts, - mailbox, - ); - matches = matches && subModuleMatches; - } + const delta = await routingModuleDelta( + chain, + moduleAddress, + config, + multiProvider, + contracts, + mailbox, + ); + matches = + matches && + delta.domainsToEnroll.length === 0 && + delta.domainsToUnenroll.length === 0 && + !delta.mailbox && + !delta.owner; break; } case IsmType.AGGREGATION: { @@ -596,6 +677,62 @@ export async function moduleMatchesConfig( return matches; } +export async function routingModuleDelta( + destination: ChainName, + moduleAddress: Address, + config: RoutingIsmConfig, + multiProvider: MultiProvider, + contracts: HyperlaneContracts, + mailbox?: Address, +): Promise { + const provider = multiProvider.getProvider(destination); + const routingIsm = DomainRoutingIsm__factory.connect(moduleAddress, provider); + const owner = await routingIsm.owner(); + const deployedDomains = (await routingIsm.domains()).map((domain) => + multiProvider.getChainName(domain.toNumber()), + ); + + const delta: RoutingIsmDelta = { + domainsToUnenroll: [], + domainsToEnroll: [], + }; + + // if owners don't match, we need to transfer ownership + if (!eqAddress(owner, normalizeAddress(config.owner))) + delta.owner = config.owner; + if (config.type === IsmType.FALLBACK_ROUTING) { + const client = MailboxClient__factory.connect(moduleAddress, provider); + const mailboxAddress = await client.mailbox(); + if (mailbox && !eqAddress(mailboxAddress, mailbox)) delta.mailbox = mailbox; + } + // check for exclusion of domains in the config + delta.domainsToUnenroll = deployedDomains.filter( + (domain) => !Object.keys(config.domains).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); + } else { + const subModule = await routingIsm.module( + multiProvider.getDomainId(origin), + ); + // Recursively check that the submodule for each configured + // domain matches the submodule config. + const subModuleMatches = await moduleMatchesConfig( + destination, + subModule, + subConfig, + multiProvider, + contracts, + origin, + ); + if (!subModuleMatches) delta.domainsToEnroll.push(origin); + } + } + return delta; +} + export function collectValidators( origin: ChainName, config: IsmConfig, diff --git a/typescript/sdk/src/ism/types.ts b/typescript/sdk/src/ism/types.ts index 86023080c..74bec6087 100644 --- a/typescript/sdk/src/ism/types.ts +++ b/typescript/sdk/src/ism/types.ts @@ -7,7 +7,7 @@ import { } from '@hyperlane-xyz/core'; import type { Address, ValueOf } from '@hyperlane-xyz/utils'; -import { ChainMap } from '../types'; +import { ChainMap, ChainName } from '../types'; // this enum should match the IInterchainSecurityModule.sol enum // meant for the relayer @@ -103,3 +103,11 @@ export type DeployedIsmType = { }; 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 + owner?: Address; // is the owner different + mailbox?: Address; // is the mailbox different (only for fallback routing) +};