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 warp deploy configs reader functions #521

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

Conversation

ltyu
Copy link
Contributor

@ltyu ltyu commented Jan 24, 2025

Description

Add Warp Deploy Config reader functions getWarpDeployConfigs() and getWarpDeployConfig() as well DRY out logic that is shared with getWarpRoute()

Backward compatibility

Yes

Testing

Unit tests

Fixes: hyperlane-xyz/hyperlane-monorepo#5261

Copy link

changeset-bot bot commented Jan 24, 2025

🦋 Changeset detected

Latest commit: ef4d7dc

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

This PR includes changesets to release 1 package
Name Type
@hyperlane-xyz/registry Minor

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

@@ -72,9 +75,21 @@ export abstract class SynchronousRegistry extends BaseRegistry implements IRegis
return Object.fromEntries(idsWithConfigs);
}

/**
* Retrieves a map of all the warp routes deployment configs
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if a filter parameter is needed yet

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe not but I suggest including it for consistency. If you don't do it now, it'll probably be 6 months before this is touched again

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

Copy link
Collaborator

@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.

Seems like you're on the right track

warpDeploys[routeId] = filePath;
}

return (this.listContentCache = { chains, deployments: { warpRoutes, warpDeploys } });
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest renaming warpDeploys, it's not clear how it's different from wrapRoutes

return this.readConfigsForIds(ids, warpRoutes);
}

protected getWarpDeploysForIds(ids: WarpRouteId[]): WarpRouteDeployConfig[] {
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto throughout

warpRoutes: Record<WarpRouteId, string>;

// Warp route ID to warp config URI
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// Warp route ID to warp config URI
// Warp route ID to warp deploy config URI

@@ -58,6 +58,7 @@ export class PartialRegistry extends SynchronousRegistry implements IRegistry {
chains,
deployments: {
warpRoutes,
warpDeploys: warpRoutes
Copy link
Collaborator

Choose a reason for hiding this comment

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

intentional? Doesn't look right

@@ -87,6 +88,10 @@ export class PartialRegistry extends SynchronousRegistry implements IRegistry {
}) as WarpCoreConfig[];
}

protected getWarpDeploysForIds(_ids: WarpRouteId[]): WarpRouteDeployConfig[] {
throw new Error('Method not implemented.');
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd expect this to filter and return some member var state like what getWarpRoutesForIds does

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is kind of hard to implement because the warpRouteId includes the symbol name (USDC/base-ethereum), which WarpDeployConfig does not have. We will need to derive from the config.token.

Is it worth implementing now as it seems like we may merge both configs soon

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, k nvm

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please leave a comment about this though

@@ -64,6 +64,9 @@ export abstract class SynchronousRegistry extends BaseRegistry implements IRegis
return this.getWarpRoutesForIds([routeId])[0] || null;
}

/**
* Retrieves a filtered map of the warp routes artifacts
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's try to be consistent with our nomenclature. I don't think I call these artifacts anywhere in the registry code but I could be wrong. The problem with 'artifact' is that while it is the output of one tool, it's also the input to another.

@@ -72,9 +75,21 @@ export abstract class SynchronousRegistry extends BaseRegistry implements IRegis
return Object.fromEntries(idsWithConfigs);
}

/**
* Retrieves a map of all the warp routes deployment configs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe not but I suggest including it for consistency. If you don't do it now, it'll probably be 6 months before this is touched again

@@ -19,15 +19,15 @@ describe('Warp Route Configs', () => {
const routes = localRegistry.getWarpRoutes();

for (const id of Object.keys(routes)) {
it(`Route ${id} is valid`, async () => {
it(`Core Config ${id} is valid`, async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Replace Core with WarpCore. Core is too ambiguous with hyperlane core config IMO

Comment on lines 125 to 144
const warpDeploys = localRegistry.getWarpDeploys();

// These Ids do not validate due to owner
// Remove after https://github.com/hyperlane-xyz/hyperlane-monorepo/issues/5292
const excludeIds = [
'ECLIP/arbitrum-neutron',
'INJ/inevm-injective',
'TIA/arbitrum-neutron',
'INJ/inevm-injective',
'TIA/arbitrum-neutron',
'TIA/eclipsemainnet-stride',
'TIA/mantapacific-neutron',
'stTIA/eclipsemainnet-stride'
];
const configs = Object.keys(warpDeploys).filter(id =>!excludeIds.includes(id));
for (const id of configs) {
it(`Deploy config ${id} is valid`, async () => {
WarpRouteDeployConfigSchema.parse(warpDeploys[id]);
});
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please put this section in a separate describe block

@ltyu ltyu changed the title feat: warp speed warp configs reader feat: Add warp deploy configs reader functions Jan 24, 2025
warpDeployConfigURIs[routeId] = filePath;
}

return (this.listContentCache = { chains, deployments: { warpRoutes, warpDeployConfigURIs } });
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: wondering why the warpDeployConfigURIs name feels inconsistent with the other exports here. Would warpDeployConfigs be better?

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

Successfully merging this pull request may close these issues.

Warp Deploy Config fetching via IRegistry
2 participants