Skip to content

Commit

Permalink
fix: HookModule update deploys due to unnormalized config (#5424)
Browse files Browse the repository at this point in the history
### Description
This PR fixes an issue with HookModule that causes HookModule to trigger
a new deployment due to unnormalized config

### Backward compatibility
Yes

### Testing
Unit Tests
  • Loading branch information
ltyu authored Feb 11, 2025
1 parent 8ce441c commit d6724c4
Show file tree
Hide file tree
Showing 3 changed files with 61 additions and 18 deletions.
5 changes: 5 additions & 0 deletions .changeset/twenty-jokes-relax.md
Original file line number Diff line number Diff line change
@@ -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
35 changes: 34 additions & 1 deletion typescript/sdk/src/hook/EvmHookModule.hardhat-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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<AggregationHookConfig>;

// expect 0 updates, but actually deploys a new hook
await expectTxsAndUpdate(hook, hookConfig, 0);
const deployedHookAfter =
(await hook.read()) as WithAddress<AggregationHookConfig>;

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))
Expand Down
39 changes: 22 additions & 17 deletions typescript/sdk/src/hook/EvmHookModule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -174,39 +177,41 @@ 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;
case HookType.ROUTING:
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
Expand All @@ -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],
),
});
}
Expand Down

0 comments on commit d6724c4

Please sign in to comment.