Codex/ctusd preview build - #1025
Open
nambrot wants to merge 18 commits into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
5 Skipped Deployments
|
📝 WalkthroughWalkthroughThis pull request upgrades Hyperlane SDK dependencies, hardcodes registry configuration values, introduces a warp route whitelist feature, and extends multi-collateral token routing logic throughout the transfer pipeline. The changes enable more sophisticated destination token selection when multiple collateralized tokens share connections on the same chains. Changes
Possibly related PRs
🚥 Pre-merge checks | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/features/transfer/TransferTokenForm.tsx (1)
386-438: Consider using TanStack Query for this async fetch, even in temp code.Look, I know this is marked as TEMP and will probably be yeeted out of the swamp eventually. But while it's here, usin'
useQuerywould give ya automatic caching, deduplication, and cleaner loading/error states without all this manual rigmarole.Also worth notin' - the dependency array uses
destinationToken?.chainName,?.addressOrDenom, and?.symbolindividually rather than the object itself. That's fine for avoidin' reference-change re-renders, but thedestinationTokenused on line 408 to constructTokenAmountis captured from closure at effect-creation time. If a different token object with identical properties somehow appears, you'd get the stale reference. Unlikely to cause real issues here, but somethin' to keep in mind.♻️ Optional: TanStack Query approach
import { useQuery } from '@tanstack/react-query'; function useDestinationRouterCollateralTooltip(destinationToken?: Token): string | undefined { const warpCore = useWarpCore(); const { data: collateralAmount, isLoading } = useQuery({ queryKey: ['destinationCollateral', destinationToken?.chainName, destinationToken?.addressOrDenom], queryFn: async () => { if (!destinationToken) return null; const amount = await warpCore.getTokenCollateral(destinationToken); return new TokenAmount(amount, destinationToken); }, enabled: !!destinationToken, }); return useMemo(() => { if (!destinationToken) return undefined; if (isLoading) return 'TEMP: Loading destination router collateral...'; if (!collateralAmount) return 'TEMP: Destination router collateral unavailable'; return `TEMP: Destination router collateral: ${collateralAmount .getDecimalFormattedAmount() .toFixed(4)} ${destinationToken.symbol}`; }, [destinationToken, isLoading, collateralAmount]); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/transfer/TransferTokenForm.tsx` around lines 386 - 438, The current useDestinationRouterCollateralTooltip hook manually manages async state (isLoading, isCancelled) and risks stale destinationToken closure; replace this with TanStack Query by using useQuery inside useDestinationRouterCollateralTooltip: use a queryKey like ['destinationCollateral', destinationToken?.chainName, destinationToken?.addressOrDenom], set enabled: !!destinationToken, and implement queryFn that reads the current destinationToken (return null if missing) and calls warpCore.getTokenCollateral(...) then wraps the result with new TokenAmount(amount, destinationToken); remove manual isLoading/isCancelled/catch/finally logic and rely on useQuery's isLoading/data/error, and keep the same useMemo return logic but depend on destinationToken, isLoading, and collateralAmount to avoid stale closures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CUSTOMIZE.md`:
- Around line 7-9: Update the registry paragraph in CUSTOMIZE.md to match the
current implementation in src/consts/config.ts: replace the reference to the
Hyperlane registry branch `nambrot/multi-collateral-deploy` with the pinned
`codex/nambrot-cross-collateral-deploy`, and explicitly state that
NEXT_PUBLIC_REGISTRY_URL and other NEXT_PUBLIC_REGISTRY_* overrides are disabled
by the code so they cannot be used; instead explain the supported alternatives
(manually define custom chains/warp routes or follow the repo’s pinned registry)
and point readers to src/consts/config.ts for the authoritative source.
In `@src/consts/warpRouteWhitelist.test.ts`:
- Around line 14-18: The test currently only validates
registry.getWarpRoutes()/snapshot and misses the new per-route recovery path;
update the test to either call assembleWarpCoreConfig() (so the missing-ID
repair runs) or stub GithubRegistry.getWarpRoute(routeId) to return the
per-route definition for a missing whitelist ID and then assert the resulting
whitelist includes that recovered route; target the GithubRegistry instance
constructed in the test and the assembleWarpCoreConfig function to ensure the
per-route recovery behavior is exercised.
In `@src/features/warpCore/warpCoreConfig.ts`:
- Around line 50-60: The current use of Promise.all when fetching per-route
configs (missingRouteIds.map(... -> registry.getWarpRoute(routeId))) means a
single rejection discards all successful fetches; replace Promise.all with
Promise.allSettled for the block that builds registryWarpRoutes from
missingRouteIds and handle only the fulfilled results to populate
registryWarpRoutes (use the settled result.value where status === 'fulfilled'
and ignore/log failures), and apply the same allSettled + per-result handling to
the other catch-path that builds fallbackRoutes (the second Promise.all usage
around registry.getWarpRoute) so each route fetch is isolated and partial
successes are preserved.
---
Nitpick comments:
In `@src/features/transfer/TransferTokenForm.tsx`:
- Around line 386-438: The current useDestinationRouterCollateralTooltip hook
manually manages async state (isLoading, isCancelled) and risks stale
destinationToken closure; replace this with TanStack Query by using useQuery
inside useDestinationRouterCollateralTooltip: use a queryKey like
['destinationCollateral', destinationToken?.chainName,
destinationToken?.addressOrDenom], set enabled: !!destinationToken, and
implement queryFn that reads the current destinationToken (return null if
missing) and calls warpCore.getTokenCollateral(...) then wraps the result with
new TokenAmount(amount, destinationToken); remove manual
isLoading/isCancelled/catch/finally logic and rely on useQuery's
isLoading/data/error, and keep the same useMemo return logic but depend on
destinationToken, isLoading, and collateralAmount to avoid stale closures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 89c868f7-0bac-4a1c-85ef-2adf6e3239dd
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
CUSTOMIZE.mdpackage.jsonsrc/consts/config.tssrc/consts/warpRouteWhitelist.test.tssrc/consts/warpRouteWhitelist.tssrc/consts/warpRoutes.yamlsrc/features/tokens/TokenSelectField.tsxsrc/features/tokens/utils.test.tssrc/features/tokens/utils.tssrc/features/transfer/TransferTokenForm.tsxsrc/features/transfer/fees.test.tssrc/features/transfer/fees.tssrc/features/transfer/maxAmount.tssrc/features/transfer/useFeeQuotes.test.tssrc/features/transfer/useFeeQuotes.tssrc/features/transfer/useTokenTransfer.tssrc/features/warpCore/warpCoreConfig.ts
Comment on lines
+7
to
9
| By default, this branch uses the Hyperlane GitHub registry at `https://github.com/hyperlane-xyz/hyperlane-registry` on branch `nambrot/multi-collateral-deploy`. | ||
|
|
||
| To use custom chains or custom warp routes, you can either configure a different registry using the `NEXT_PUBLIC_REGISTRY_URL` environment variable or define them manually (see the next two sections). |
There was a problem hiding this comment.
Doc drift in this registry section.
src/consts/config.ts now pins codex/nambrot-cross-collateral-deploy and disables NEXT_PUBLIC_REGISTRY_* overrides, but this paragraph still points folks at nambrot/multi-collateral-deploy and NEXT_PUBLIC_REGISTRY_URL. That’ll send customizers up the wrong hill.
📝 Suggested doc fix
-By default, this branch uses the Hyperlane GitHub registry at `https://github.com/hyperlane-xyz/hyperlane-registry` on branch `nambrot/multi-collateral-deploy`.
+By default, this branch uses the Hyperlane GitHub registry at `https://github.com/hyperlane-xyz/hyperlane-registry` on branch `codex/nambrot-cross-collateral-deploy`.
-To use custom chains or custom warp routes, you can either configure a different registry using the `NEXT_PUBLIC_REGISTRY_URL` environment variable or define them manually (see the next two sections).
+To use a different registry in this preview branch, update `src/consts/config.ts`, or define custom chains and custom warp routes manually (see the next two sections).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CUSTOMIZE.md` around lines 7 - 9, Update the registry paragraph in
CUSTOMIZE.md to match the current implementation in src/consts/config.ts:
replace the reference to the Hyperlane registry branch
`nambrot/multi-collateral-deploy` with the pinned
`codex/nambrot-cross-collateral-deploy`, and explicitly state that
NEXT_PUBLIC_REGISTRY_URL and other NEXT_PUBLIC_REGISTRY_* overrides are disabled
by the code so they cannot be used; instead explain the supported alternatives
(manually define custom chains/warp routes or follow the repo’s pinned registry)
and point readers to src/consts/config.ts for the authoritative source.
Comment on lines
+14
to
+18
| const registry = new GithubRegistry({ | ||
| uri: config.registryUrl, | ||
| branch: config.registryBranch, | ||
| proxyUrl: config.registryProxyUrl, | ||
| }); |
There was a problem hiding this comment.
This test still misses the new per-route recovery path.
assembleWarpCoreConfig() now repairs missing whitelist IDs with registry.getWarpRoute(routeId), but this test only checks getWarpRoutes() or the published snapshot. If a whitelisted route exists only as a per-route file, CI goes red even though the app would load it. I’d either assert through assembleWarpCoreConfig() or mirror the same missing-ID recovery here.
Also applies to: 21-29
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/consts/warpRouteWhitelist.test.ts` around lines 14 - 18, The test
currently only validates registry.getWarpRoutes()/snapshot and misses the new
per-route recovery path; update the test to either call assembleWarpCoreConfig()
(so the missing-ID repair runs) or stub GithubRegistry.getWarpRoute(routeId) to
return the per-route definition for a missing whitelist ID and then assert the
resulting whitelist includes that recovered route; target the GithubRegistry
instance constructed in the test and the assembleWarpCoreConfig function to
ensure the per-route recovery behavior is exercised.
Comment on lines
+50
to
+60
| if (missingRouteIds.length) { | ||
| const routeEntries = await Promise.all( | ||
| missingRouteIds.map( | ||
| async (routeId): Promise<[string, WarpCoreConfig | null]> => [ | ||
| routeId, | ||
| await registry.getWarpRoute(routeId), | ||
| ], | ||
| ), | ||
| ); | ||
| for (const [routeId, routeConfig] of routeEntries) { | ||
| if (routeConfig) registryWarpRoutes[routeId] = routeConfig; |
There was a problem hiding this comment.
Use per-route error isolation here.
Both recovery branches use Promise.all(). If any one registry.getWarpRoute(routeId) call rejects, we throw away the already-fetched registry payload and fall all the way back to published routes, so one flaky route can hide the rest of the whitelist.
🛠️ One way to keep partial successes
- const routeEntries = await Promise.all(
+ const routeEntries = await Promise.allSettled(
missingRouteIds.map(
async (routeId): Promise<[string, WarpCoreConfig | null]> => [
routeId,
await registry.getWarpRoute(routeId),
],
),
);
- for (const [routeId, routeConfig] of routeEntries) {
- if (routeConfig) registryWarpRoutes[routeId] = routeConfig;
+ for (const result of routeEntries) {
+ if (result.status !== 'fulfilled') {
+ logger.debug(
+ 'Failed to fetch whitelisted route from registry.getWarpRoute',
+ result.reason,
+ );
+ continue;
+ }
+ const [routeId, routeConfig] = result.value;
+ if (routeConfig) registryWarpRoutes[routeId] = routeConfig;
}Apply the same Promise.allSettled() pattern in the catch-path block that builds fallbackRoutes.
As per coding guidelines, "For expected issues (external systems, user input): use explicit error handling and try/catch at boundaries."
Also applies to: 77-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/warpCore/warpCoreConfig.ts` around lines 50 - 60, The current
use of Promise.all when fetching per-route configs (missingRouteIds.map(... ->
registry.getWarpRoute(routeId))) means a single rejection discards all
successful fetches; replace Promise.all with Promise.allSettled for the block
that builds registryWarpRoutes from missingRouteIds and handle only the
fulfilled results to populate registryWarpRoutes (use the settled result.value
where status === 'fulfilled' and ignore/log failures), and apply the same
allSettled + per-result handling to the other catch-path that builds
fallbackRoutes (the second Promise.all usage around registry.getWarpRoute) so
each route fetch is isolated and partial successes are preserved.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.