diff --git a/.changeset/twenty-jokes-relax.md b/.changeset/twenty-jokes-relax.md new file mode 100644 index 0000000000..46f60744e3 --- /dev/null +++ b/.changeset/twenty-jokes-relax.md @@ -0,0 +1,5 @@ +--- +'@hyperlane-xyz/sdk': minor +--- + +Fix an issue with HookModule that causes HookModule trigger triggering a new deployment due to unnormalized config despite configs being the same diff --git a/typescript/sdk/src/hook/EvmHookModule.hardhat-test.ts b/typescript/sdk/src/hook/EvmHookModule.hardhat-test.ts index 2b31d62346..56cc71fa33 100644 --- a/typescript/sdk/src/hook/EvmHookModule.hardhat-test.ts +++ b/typescript/sdk/src/hook/EvmHookModule.hardhat-test.ts @@ -2,7 +2,13 @@ import { expect } from 'chai'; import { Signer } from 'ethers'; import hre from 'hardhat'; -import { Address, assert, deepEquals, eqAddress } from '@hyperlane-xyz/utils'; +import { + Address, + WithAddress, + assert, + deepEquals, + eqAddress, +} from '@hyperlane-xyz/utils'; import { TestChainName, testChains } from '../consts/testChains.js'; import { HyperlaneAddresses, HyperlaneContracts } from '../contracts/types.js'; @@ -747,6 +753,33 @@ describe('EvmHookModule', async () => { await expectTxsAndUpdate(hook, config, 0); }); + it('should not update a hook if given address matches actual config', async () => { + // create a new agg hook with the owner hardcoded + const hookConfig: AggregationHookConfig = { + type: HookType.AGGREGATION, + hooks: [ + { + ...(randomHookConfig( + 0, + 2, + HookType.FALLBACK_ROUTING, + ) as FallbackRoutingHookConfig), + owner: '0xD1e6626310fD54Eceb5b9a51dA2eC329D6D4B68A', + }, + ], + }; + const { hook } = await createHook(hookConfig); + const deployedHookBefore = + (await hook.read()) as WithAddress; + + // expect 0 updates, but actually deploys a new hook + await expectTxsAndUpdate(hook, hookConfig, 0); + const deployedHookAfter = + (await hook.read()) as WithAddress; + + expect(deployedHookBefore.address).to.equal(deployedHookAfter.address); + }); + // generate a random config for each ownable hook type const ownableHooks = hookTypes .filter((hookType) => MUTABLE_HOOK_TYPE.includes(hookType)) diff --git a/typescript/sdk/src/hook/EvmHookModule.ts b/typescript/sdk/src/hook/EvmHookModule.ts index 6a7cfbbd3e..c544a845be 100644 --- a/typescript/sdk/src/hook/EvmHookModule.ts +++ b/typescript/sdk/src/hook/EvmHookModule.ts @@ -147,20 +147,23 @@ export class EvmHookModule extends HyperlaneModule< ); } - const unnormalizedCurrentConfig = await this.read(); - const currentConfig = normalizeConfig(unnormalizedCurrentConfig); - // Update the config this.args.config = targetConfig; + // We need to normalize the current and target configs to compare. + const normalizedCurrentConfig = normalizeConfig(await this.read()); + const normalizedTargetConfig = normalizeConfig(targetConfig); + // If configs match, no updates needed - if (deepEquals(currentConfig, targetConfig)) { + if (deepEquals(normalizedCurrentConfig, normalizedTargetConfig)) { return []; } - if (this.shouldDeployNewHook(currentConfig, targetConfig)) { + if ( + this.shouldDeployNewHook(normalizedCurrentConfig, normalizedTargetConfig) + ) { const contract = await this.deploy({ - config: targetConfig, + config: normalizedTargetConfig, }); this.args.addresses.deployedHook = contract.address; @@ -174,24 +177,24 @@ export class EvmHookModule extends HyperlaneModule< case HookType.INTERCHAIN_GAS_PAYMASTER: updateTxs.push( ...(await this.updateIgpHook({ - currentConfig, - targetConfig, + currentConfig: normalizedCurrentConfig, + targetConfig: normalizedTargetConfig, })), ); break; case HookType.PROTOCOL_FEE: updateTxs.push( ...(await this.updateProtocolFeeHook({ - currentConfig, - targetConfig, + currentConfig: normalizedCurrentConfig, + targetConfig: normalizedTargetConfig, })), ); break; case HookType.PAUSABLE: updateTxs.push( ...(await this.updatePausableHook({ - currentConfig, - targetConfig, + currentConfig: normalizedCurrentConfig, + targetConfig: normalizedTargetConfig, })), ); break; @@ -199,14 +202,16 @@ export class EvmHookModule extends HyperlaneModule< case HookType.FALLBACK_ROUTING: updateTxs.push( ...(await this.updateRoutingHook({ - currentConfig, - targetConfig, + currentConfig: normalizedCurrentConfig, + targetConfig: normalizedTargetConfig, })), ); break; default: // MERKLE_TREE, AGGREGATION and OP_STACK hooks should already be handled before the switch - throw new Error(`Unsupported hook type: ${targetConfig.type}`); + throw new Error( + `Unsupported hook type: ${normalizedTargetConfig.type}`, + ); } // Lastly, check if the resolved owner is different from the current owner @@ -216,14 +221,14 @@ export class EvmHookModule extends HyperlaneModule< ).owner(); // Return an ownership transfer transaction if required - if (!eqAddress(targetConfig.owner, owner)) { + if (!eqAddress(normalizedTargetConfig.owner, owner)) { updateTxs.push({ annotation: 'Transferring ownership of ownable Hook...', chainId: this.chainId, to: this.args.addresses.deployedHook, data: Ownable__factory.createInterface().encodeFunctionData( 'transferOwnership(address)', - [targetConfig.owner], + [normalizedTargetConfig.owner], ), }); }