Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add hook update into WarpModule.update() #4891

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

ltyu
Copy link
Contributor

@ltyu ltyu commented Nov 22, 2024

Description

This is a multi-part PR.

This adds createHookUpdateTxs() to WarpModule.update() such that it updates a warp route without a hook, and one with an existing hook.

Related issues

Backward compatibility

Yes

Testing

Unit Tests

Copy link

changeset-bot bot commented Nov 22, 2024

🦋 Changeset detected

Latest commit: abab8a9

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

@jmrossy jmrossy changed the title fea: Add warp apply for hook in WarpModule feat: Add warp apply for hook in WarpModule Nov 25, 2024
@ltyu ltyu marked this pull request as draft November 25, 2024 21:00
Copy link

codecov bot commented Nov 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.60%. Comparing base (4c74aff) to head (abab8a9).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4891   +/-   ##
=======================================
  Coverage   74.60%   74.60%           
=======================================
  Files         103      103           
  Lines        1516     1516           
  Branches      195      195           
=======================================
  Hits         1131     1131           
  Misses        364      364           
  Partials       21       21           
Components Coverage Δ
core 84.61% <ø> (ø)
hooks 77.77% <ø> (ø)
isms 79.02% <ø> (ø)
token 89.07% <ø> (ø)
middlewares 77.58% <ø> (ø)

@ltyu ltyu marked this pull request as ready for review November 27, 2024 21:21
@ltyu ltyu changed the title feat: Add warp apply for hook in WarpModule feat: Add hook update into WarpModule Nov 27, 2024
@ltyu ltyu changed the title feat: Add hook update into WarpModule feat: Add hook update into WarpModule.update() Nov 27, 2024
Copy link
Contributor

@jmrossy jmrossy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know this code well but the changes look fine to me :)

@@ -42,12 +46,14 @@ import { EvmERC20WarpRouteReader } from './EvmERC20WarpRouteReader.js';
import { HypERC20Deployer } from './deploy.js';
import { TokenRouterConfig, TokenRouterConfigSchema } from './schemas.js';

``;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo?

*
* @returns Object with deployedHook address, and update Transactions
*/
public async deployOrUpdateHook(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, public keyword is unnecessary

this.multiProvider,
{
chain: this.args.chain,
config: actualConfig.hook as HookConfig,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the casts here and below be avoided?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

2 participants