fix: optimize hook module read in warp apply process by using provide… #5541

mshojaei-txfusion · 2025-02-21T12:39:22Z

Description

This PR simplifies the hook module read functionality by removing the EvmHookReader class and its associated complexity. The changes include:

Removal of the EvmHookReader import and instance in EvmHookModule
Simplification of the read() method to directly return the config
Reordering of config update in the update() method to ensure proper comparison

These changes make the code more maintainable and reduce unnecessary complexity while maintaining the same functionality.

Drive-by changes

Reordered the config update in the update() method to happen after normalization comparison

Related issues

Fixes HookModule.read() always derives when given a Hook Config #5449

Backward compatibility

Yes - These changes are backward compatible. The functionality remains the same, we're just simplifying the implementation. No infrastructure implications as this is a refactoring of the internal SDK code.

Testing

Manual - The changes have been tested manually to ensure the hook module read and update functionality works as expected. The changes are minimal and focused on internal implementation details.

changeset-bot · 2025-02-21T12:39:27Z

🦋 Changeset detected

Latest commit: 4f09729

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages

Name	Type
@hyperlane-xyz/sdk	Minor
@hyperlane-xyz/cli	Minor
@hyperlane-xyz/helloworld	Minor
@hyperlane-xyz/infra	Minor
@hyperlane-xyz/widgets	Minor
@hyperlane-xyz/ccip-server	Minor
@hyperlane-xyz/github-proxy	Minor
@hyperlane-xyz/utils	Minor
@hyperlane-xyz/core	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

…d config

…ule read method

…k config derivation

ltyu

good progress

ltyu · 2025-02-26T18:48:35Z

typescript/sdk/src/hook/EvmHookModule.ts

@@ -132,7 +129,7 @@ export class EvmHookModule extends HyperlaneModule<
  public async read(): Promise<HookConfig> {
    return typeof this.args.config === 'string'
      ? this.args.addresses.deployedHook
-      : this.reader.deriveHookConfig(this.args.addresses.deployedHook);
+      : { ...this.args.config };


we generally still want the Modules to derive onchain values. however, i do see your intention.

the challenge here is that we need some way to force update() to derive, but have another way to detect if this.args.config is an already derived config.

One way we can do this is by checking if the field address exists. This should mean that it's a DerivedHookConfig

I see, can you take another look and confirm I understood correctly?

Need changes

ltyu

lgtm

ltyu · 2025-02-28T17:19:03Z

.changeset/eighty-planets-tan.md

+'@hyperlane-xyz/sdk': minor
+---
+
+Optimize HookModule.read() by removing redundant hook config derivation


needs update?

nambrot · 2025-02-28T20:45:22Z

typescript/sdk/src/hook/EvmHookModule.ts

-    return typeof this.args.config === 'string'
-      ? this.args.addresses.deployedHook
-      : this.reader.deriveHookConfig(this.args.addresses.deployedHook);
+    const { config, addresses } = this.args;


I know you didn't create this, but can you add some comments to both the class, and the update and read function so that consumers know how to think about the semantics and API? It's not super obvious to me why you are able to just skip the hook derivation. IIUC, its that under only some circumstances we pass the actual config in, and the module can just assume that as the result of the read? If that is the case, imo the api has to be made pretty clear, when I call .read, i would expect that the module actually reads. Probably worth adding tests here if they dont exist cc @ltyu

@ltyu can you please first clarify this and then I write the comments?

The reason read() has this branching logic is because the Module was designed to allow

initialization through the constructor, which is used when we have an existing deployed Hook config.

initialization through create(), in this case, it is used to create a new Hook.

In both cases, we want to allow the caller to read(). The reason I suggested using the address is due to the first point. If a user initializes a config through the constructor, we can assume they have an existing deployed config i.e., a DerivedHookConfig. For the latter point, deployedHook address will be populated so we will derive from it.

alternatively, we can also populate this.args.config in the create() with a DerivedConfig to simplify read()

I'm not sure what the if (typeof config === 'string') is used for. perhaps @paulbalaji can clarify

Ok, so just talked IRL with @ltyu and @paulbalaji, here is our current assessment:

There is a few references for config in AbstractHyperlaneModule and its children (i.e. Core, Hook, ISM, ICA). As far as I can tell:

It's mainly a convenience compiler enforcement for the read and update function usage of the config

In module.read, when the this.args.config is a string (i.e a desired hook or ISM that is passed as the contract address) we use that as in indication to skip reading. Afaict, we do so because we expect that it's not derivable and the user "intentionally" set it as such (like when they have a bespoke hook/ISM they want to use)

We do generally pass the constructor-level config as "the expected config" for the module, however, we the update and create functions already expect the config as an explicit argument.

In all module update functions, we always explicitly call read which tries to derive the config

All these observations lead me to the following conclusions:

We practically don't actually use the TConfig of the stored args: HyperlaneModuleParams

We pretty much always read module config even though the caller/constructor of the module already has it (or they don't, but module.read wouldn't give that to them either.

We primarily use the fact that both hook and ism config can be specified as the contract address.

What I would like to suggest us to do:

Confirm the above understanding

Remove the storing of the config and replace it with its current only intended usage in form of indicating whether or not to attempt reading the config or not

Change the function signature of module.update to allow the caller to pass in the current config as an optional argument (that like warp module has already read for hook/ism). Alternatively, allow consumers to explicitly set the current config in an instance method (vs. implicitly as a generic constructor argument)

More generally:

Be much more clear about how we use config and avoid config by itself (i.e. prefer to use targetConfig, actualConfig, onchainConfig etc.)

@mshojaei-txfusion discussing with the team right now, so consider this issue/PR blocked until we confirm the recommendation

Filing #5610 as somewhat related

nambrot · 2025-02-28T20:46:15Z

typescript/sdk/src/hook/EvmHookModule.ts

    // We need to normalize the current and target configs to compare.
    const normalizedCurrentConfig = normalizeConfig(await this.read());
    const normalizedTargetConfig = normalizeConfig(targetConfig);

+    // Update the config


Why does moving this here make any difference? I feel like if we have logic that depends on the module state, then we should make that a lot more obvious

As now we use this.args.config on read as well, we can't change it before read.
What do you exactly mean by "we should make that a lot more obvious"?

We should have clear semantics as to when instance variables (like this.args.config are populated and readable). In this case, it sounds like this.args.config is expected to be not usable until read happens, and that is not obvious at all

mshojaei-txfusion requested review from yorhodes, ltyu and paulbalaji as code owners February 21, 2025 12:39

mshojaei-txfusion changed the base branch from refactor/consolidate-warp-route-updates to main February 21, 2025 14:18

mshojaei-txfusion changed the base branch from main to refactor/consolidate-warp-route-updates February 21, 2025 14:39

mshojaei-txfusion changed the base branch from refactor/consolidate-warp-route-updates to main February 21, 2025 15:08

mshojaei-txfusion changed the base branch from main to refactor/consolidate-warp-route-updates February 21, 2025 17:04

mshojaei-txfusion added 3 commits February 25, 2025 11:11

fix: optimize hook module read in warp apply process by using provide…

5c651b4

…d config

refactor: remove EvmHookReader on EvmHookModule and simplify hook mod…

8d15a33

…ule read method

docs(changeset): Optimize HookModule.read() by removing redundant hoo…

ad86c09

…k config derivation

mshojaei-txfusion force-pushed the fix/hook-module-read branch from 8100621 to ad86c09 Compare February 25, 2025 07:51

mshojaei-txfusion requested review from tkporter, Mo-Hussain and daniel-savu as code owners February 25, 2025 07:51

mshojaei-txfusion force-pushed the refactor/consolidate-warp-route-updates branch from 78aac8d to a777c6f Compare February 25, 2025 07:54

ltyu previously approved these changes Feb 26, 2025

View reviewed changes

ltyu self-requested a review February 26, 2025 18:51

feat: Enhance EvmHookModule read method with EvmHookReader

6be5204

mshojaei-txfusion changed the base branch from refactor/consolidate-warp-route-updates to main February 27, 2025 12:16

mshojaei-txfusion changed the base branch from main to refactor/consolidate-warp-route-updates February 27, 2025 15:08

ltyu reviewed Feb 28, 2025

View reviewed changes

nambrot reviewed Feb 28, 2025

View reviewed changes

docs: update changeset

4f09729

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: optimize hook module read in warp apply process by using provide… #5541

fix: optimize hook module read in warp apply process by using provide… #5541

mshojaei-txfusion commented Feb 21, 2025 •

edited

Loading

changeset-bot bot commented Feb 21, 2025 •

edited

Loading

ltyu left a comment

ltyu Feb 26, 2025

mshojaei-txfusion Feb 27, 2025

ltyu left a comment

ltyu Feb 28, 2025

mshojaei-txfusion Mar 4, 2025

nambrot Feb 28, 2025

mshojaei-txfusion Mar 4, 2025

ltyu Mar 4, 2025

nambrot Mar 5, 2025 •

edited

Loading

nambrot Mar 5, 2025

nambrot Feb 28, 2025

mshojaei-txfusion Mar 4, 2025

nambrot Mar 4, 2025

fix: optimize hook module read in warp apply process by using provide… #5541

Are you sure you want to change the base?

fix: optimize hook module read in warp apply process by using provide… #5541

Conversation

mshojaei-txfusion commented Feb 21, 2025 • edited Loading

Description

Drive-by changes

Related issues

Backward compatibility

Testing

changeset-bot bot commented Feb 21, 2025 • edited Loading

🦋 Changeset detected

ltyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nambrot Mar 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshojaei-txfusion commented Feb 21, 2025 •

edited

Loading

changeset-bot bot commented Feb 21, 2025 •

edited

Loading

nambrot Mar 5, 2025 •

edited

Loading