refactor: simplify warp route deployment and update logic #5358

mshojaei-txfusion · 2025-02-02T16:01:03Z

Description

Drive-by changes

Related issues

#5317

Backward compatibility

Testing

changeset-bot · 2025-02-02T16:01:06Z

⚠️ No Changeset found

Latest commit: 8241708

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

codecov · 2025-02-02T16:08:16Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.53%. Comparing base (7641389) to head (8241708).
Report is 4 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5358   +/-   ##
=======================================
  Coverage   77.53%   77.53%           
=======================================
  Files         103      103           
  Lines        2110     2110           
  Branches      190      190           
=======================================
  Hits         1636     1636           
  Misses        453      453           
  Partials       21       21

Components	Coverage Δ
core	`87.80% <ø> (ø)`
hooks	`79.39% <ø> (ø)`
isms	`83.68% <ø> (ø)`
token	`91.27% <ø> (ø)`
middlewares	`79.80% <ø> (ø)`

nambrot · 2025-02-05T00:40:49Z

typescript/cli/src/deploy/warp.ts

  if (transactions.length == 0)
    return logGreen(`Warp config is the same as target. No updates needed.`);

  await submitWarpApplyTransactions(params, groupBy(transactions, 'chainId'));
 }

-async function extendWarpRoute(
+async function deployNewWarpRoutes(


Why the rename here?

as I changed the functionality I renamed it, now that reverted that change no need to rename so reverted

nambrot · 2025-02-05T00:41:36Z

typescript/cli/src/deploy/warp.ts

  params: WarpApplyParams,
  apiKeys: ChainMap<string>,
  warpDeployConfig: WarpRouteDeployConfig,
  warpCoreConfigByChain: ChainMap<WarpCoreConfig['tokens'][number]>,
-) {
+): Promise<
+  | {


Would maybe be useful to add comments here for what the return values are?

removed return values

nambrot · 2025-02-05T00:50:19Z

typescript/cli/src/deploy/warp.ts

+    warpCoreConfigByChain,
+  );
+
+  if (deployResult) {


Why did you move this out of extendWarpRoute and into this function?

I just wanted to make it pure function, but based on current state of warp.ts it was not a good idea, moved it into extendWarpRoute again

nambrot · 2025-02-05T00:57:25Z

typescript/cli/src/deploy/warp.ts

+          destinationGas,
+        );
+
+        config.remoteRouters = remoteRouters;


Is this mutative behavior necessary? I was hoping that we could do something like

const expandedConfig = { remoteRouters, destinationGas, ...config }

That way you allow warp apply that explicitly specifies routers and destination gas to just use those values, but if they don't (which we expect most, if not all warp routes to do), they get the fully-enrolled behavior

In that case if you do warp read before your warp apply, apply wont fix unenrolled routes as read wrote the current state on warp-deployment file.
Is this ok?
so if you want to fix your broken routes you should not have any remoteRoutes and destinationGas properties for your route.

or maybe we can define --full-enrollment or something like this later on separated PR.
[to ignore warp deployment file and fully enroll and update destination gas on all routers]

Clarified this on the ticket now as well, but basically I want this behavior:

c) standardize the remoteRouters and destinationGas mapping behavior to:
i. if the passed in config to warp apply contains the remoteRouters and destinationGas mapping use that
ii. it eh passed in config does not contain those mappings, assume fully connected routers and use the gas value of each chain to construct the destinationGas mapping

Does that help clarify the intended behavior?

Also would like to see test cases for this behavior as well

nambrot · 2025-02-05T01:09:00Z

typescript/cli/src/deploy/warp.ts

@@ -627,17 +703,36 @@ async function updateExistingWarpRoute(
  );
  const transactions: AnnotatedEV5Transaction[] = [];

+  // Get all deployed router addresses


I generally view these kind of transforms as "config expansion", i.e. really WarpDeployConfig is kind of a subset of HypTokenRouterConfig and so things like expanding remoteRouters and destinationGas i would just put into a separate function (like populateDestinationGas already is. i.e. ideally something like

const expandedConfig = crossProduct({ remoteRouters: await getRouters(), destinationGas: await destinationGas()})

nambrot · 2025-02-05T01:12:06Z

typescript/cli/src/tests/warp/warp-apply.e2e-test.ts

@@ -371,4 +371,114 @@ describe('hyperlane warp apply e2e tests', async function () {
    expect(Object.keys(destinationGas_3)).to.include(chain2Id);
    expect(destinationGas_3[chain2Id]).to.equal('7777');
  });
+
+  it('should recover from manual router unenrollment', async () => {


Can you add a bit more context here on what you believe the test is doing? I'm actually not sure I agree with the premise of the test 😅

nambrot · 2025-02-05T01:13:14Z

typescript/cli/src/tests/warp/warp-apply.e2e-test.ts

@@ -371,4 +371,114 @@ describe('hyperlane warp apply e2e tests', async function () {
    expect(Object.keys(destinationGas_3)).to.include(chain2Id);
    expect(destinationGas_3[chain2Id]).to.equal('7777');
  });
+


A test I very much would like to add is the one from the issue. I.e. a warp route was extended, but the update txs were not applied, and then running warp apply with the same deploy config and updated warp core config results in the expected outcome

Yes this makes sense, I already tested it manually, will add e2e test for this. this will require to export extendWarpModule function

…ctions

refactor: simplify warp route deployment and update logic

99fee5c

mshojaei-txfusion requested a review from yorhodes February 2, 2025 16:01

mshojaei-txfusion requested review from jmrossy and ltyu as code owners February 2, 2025 16:01

refactor: extract remote router configuration logic

7f86337

mshojaei-txfusion force-pushed the refactor/consolidate-warp-route-updates branch from df66840 to 7f86337 Compare February 4, 2025 13:15

test: add warp route deployment idempotency and enroll recovery tests

6efd1b4

nambrot reviewed Feb 5, 2025

View reviewed changes

mshojaei-txfusion added 3 commits February 5, 2025 16:04

test: add TokenRouter unenrollment recovery test

2871ae0

refactor: simplify warp route extension deployment logic

d346051

refactor: extract remote router and destination gas configuration fun…

8241708

…ctions

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: simplify warp route deployment and update logic #5358

refactor: simplify warp route deployment and update logic #5358

mshojaei-txfusion commented Feb 2, 2025

changeset-bot bot commented Feb 2, 2025 •

edited

Loading

codecov bot commented Feb 2, 2025 •

edited

Loading

nambrot Feb 5, 2025

mshojaei-txfusion Feb 5, 2025

nambrot Feb 5, 2025

mshojaei-txfusion Feb 5, 2025

nambrot Feb 5, 2025

mshojaei-txfusion Feb 5, 2025

nambrot Feb 5, 2025

mshojaei-txfusion Feb 5, 2025

mshojaei-txfusion Feb 5, 2025 •

edited

Loading

nambrot Feb 5, 2025

nambrot Feb 5, 2025

nambrot Feb 5, 2025

mshojaei-txfusion Feb 5, 2025

nambrot Feb 5, 2025

nambrot Feb 5, 2025

mshojaei-txfusion Feb 5, 2025

refactor: simplify warp route deployment and update logic #5358

Are you sure you want to change the base?

refactor: simplify warp route deployment and update logic #5358

Conversation

mshojaei-txfusion commented Feb 2, 2025

Description

Drive-by changes

Related issues

Backward compatibility

Testing

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

⚠️ No Changeset found

codecov bot commented Feb 2, 2025 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshojaei-txfusion Feb 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

changeset-bot bot commented Feb 2, 2025 •

edited

Loading

codecov bot commented Feb 2, 2025 •

edited

Loading

mshojaei-txfusion Feb 5, 2025 •

edited

Loading