feat: add ICA message decoding and visualization#259
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Add support for decoding and displaying Interchain Account (ICA) messages: - Decode all 3 ICA message types: CALLS, COMMITMENT, REVEAL - Detect ICA messages by checking sender/recipient against known ICA routers - Compute derived ICA addresses using the SDK - Fetch CCIP Read ISM URLs for REVEAL messages - Link related COMMITMENT and REVEAL messages bidirectionally - Add IcaDetailsCard component with: - Status sections (delivered/pending) with commitment hash - Owner, Account, ISM, Salt, User fields - CCIP Read Gateway info for pending REVEAL messages - Call details with execution status - Update debugger to use new ICA decoding API
yorhodes
force-pushed
the
feat/ica-decoding
branch
from
eb7f097 to
c83c7ec
Compare
📝 WalkthroughWalkthroughThis PR substantially overhauls Interchain Account (ICA) handling across the debugger and message details layers, introducing a comprehensive ICA decoding and routing API, enhanced debugging diagnostics, and data-driven UI rendering for ICA messages with support for multiple message types (CALLS, COMMITMENT, REVEAL). Changes
Sequence Diagram(s)sequenceDiagram
participant Debugger as Debugger
participant ICALogic as ICA Logic
participant Router as ICA Router Contract
participant Provider as Destination Provider
Debugger->>ICALogic: tryDebugIcaMsg(message)
ICALogic->>ICALogic: Check if CALLS message
alt Non-CALLS message
ICALogic-->>Debugger: Return null (early exit)
end
ICALogic->>Router: computeIcaAddress(owner, ism, salt, ...)
Router-->>ICALogic: icaAddress
ICALogic->>Provider: Iterate calls with logging
loop For each call in message
ICALogic->>ICALogic: tryCheckIcaCall(call, callValue)
alt Call succeeds
ICALogic->>Provider: estimateGas(to, data, value)
Provider-->>ICALogic: Gas estimate OK
else Call fails
ICALogic->>ICALogic: Record failedCallIndex & errorReason
ICALogic-->>Debugger: Return IcaDebugResult with failure details
end
end
alt All calls validated
ICALogic-->>Debugger: Return null (success)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 6
🤖 Fix all issues with AI agents
In `@src/features/debugger/debugMessage.ts`:
- Around line 418-426: The loop in debugMessage.ts calls tryCheckIcaCall using
recipient (the ICA router) which leads to misleading gas estimates; instead,
skip ICA call checks until the actual derived ICA address is available—update
the loop that iterates over calls to detect ICA-targeted calls (using recipient
and call.to context) and bypass calling tryCheckIcaCall for those, emitting a
clear debug/info message like "Skipping ICA call check until derived ICA address
is implemented" rather than performing estimateGas with the router address; keep
tryCheckIcaCall intact for future use once proper derivation is added.
In `@src/features/messages/cards/IcaDetailsCard.tsx`:
- Around line 612-613: Wrap the BigNumber conversion for call.value in defensive
handling: validate or try-catch around BigNumber.from(call.value) used to
compute hasValue and formattedValue, and fall back to safe defaults (hasValue =
false and formattedValue = '0') when parsing fails; update the logic in the
block that defines hasValue and formattedValue (referencing BigNumber.from,
call.value, hasValue, formattedValue, and fromWei) so malformed inputs don't
throw and the component can render safely.
In `@src/features/messages/ica.ts`:
- Around line 559-566: The current fallback in the message-matching logic
decodes reveal metadata from processCalls[0] and returns revealData.calls, which
can return data for the wrong message when multiple messages are batched; change
the fallback to return null instead of decoding/returning processCalls[0] so
callers receive a clear "unavailable" signal (update the code path that
references decodeRevealMetadata, processCalls, and revealData to return null
here and ensure callers handle a null result appropriately).
- Around line 136-138: The empty-body/malformed input check in decodeIcaBody is
unsafe because BigNumber.from(body) will throw on non-numeric or non-hex
strings; wrap the conversion in a safe validation or try/catch so malformed
input returns null instead of crashing. Specifically, in decodeIcaBody validate
that body is a non-empty numeric/hex string (or call ethers/utils.isHexString or
similar) before calling BigNumber.from(body), or catch errors from
BigNumber.from and return null; update the logic around BigNumber.from(body) to
perform this safe check so decodeIcaBody never throws on malformed body values.
In `@src/store.ts`:
- Around line 44-45: The store exposes icaRouterAddressMap and
setIcaRouterAddressMap but they are never populated or read; remove the unused
slice to avoid dead state. Delete the icaRouterAddressMap property and
setIcaRouterAddressMap setter from the store type and initialization (references
to icaRouterAddressMap and setIcaRouterAddressMap in store creation), and remove
any tests/consumers that reference them; alternatively, if you prefer
centralizing state, refactor callers that currently read the module-level
ICA_ROUTER_MAP (from buildIcaRouterAddressMap / ICA_ROUTER_MAP) to instead read
from and update the store via setIcaRouterAddressMap and
icaRouterAddressMap—pick one approach and apply consistently so only one source
of truth remains.
In `@src/types.ts`:
- Around line 75-91: Remove the unused IcaMessageDetails type declaration and
its export: delete the IcaMessageDetails interface (keeping IcaCall and
IcaRouterAddressMap intact), then search for any lingering imports/usages and
replace them with the actual DecodedIcaMessage type returned by
decodeIcaBody/parseIcaMessageDetails where appropriate; ensure no other modules
reference IcaMessageDetails and run the typecheck/build to confirm no remaining
references.
🧹 Nitpick comments (3)
src/features/messages/cards/IcaDetailsCard.tsx (2)
143-210: Async explorer URL fetching could be simplifiedThis callback recreates on every render when dependencies change, and the effect triggers the fetch. It's like walkin' through the swamp the long way. Consider using
useQueryfrom tanstack (already imported in ica.ts) for consistency with other data fetching in this PR, or at minimum add cleanup to prevent setting state on unmounted components.♻️ Consider adding cleanup for unmount safety
useEffect(() => { - getExplorerUrls().catch(() => setExplorerUrls({})); - }, [getExplorerUrls]); + let cancelled = false; + getExplorerUrls() + .then((urls) => { + if (!cancelled) setExplorerUrls(urls || {}); + }) + .catch(() => { + if (!cancelled) setExplorerUrls({}); + }); + return () => { + cancelled = true; + }; + }, [getExplorerUrls]);This would require modifying
getExplorerUrlsto return the urls object instead of setting state directly.
513-527: IIFE pattern works but extraction would be cleanerThis works fine, but usin' an IIFE inside JSX is a bit like buildin' a house in a swamp - technically possible but makes folks scratch their heads. Could extract to a tiny component or move the logic above the return.
src/features/messages/ica.ts (1)
494-499: Avoidanytype for multiProviderUsin'
anyhere is like leavin' the door to my swamp wide open - anything could wander in. TheMultiProtocolProvidertype is already imported in the store and used elsewhere.♻️ Proposed type fix
+import { MultiProtocolProvider } from '@hyperlane-xyz/sdk'; + export async function fetchRevealCalls( destinationChainName: string, processTxHash: string, messageId: string, - multiProvider: any, + multiProvider: MultiProtocolProvider, ): Promise<IcaCall[] | null> {
- Fix P1: Wrap BigNumber.from() in try/catch to prevent crashes on invalid data - Fix P2: Remove risky fallback to first process call, return null instead - Cleanup: Remove unused IcaMessageDetails type and icaRouterAddressMap store - UI: Fix link icon alignment in KeyValueRow - Add computeIcaAddress() to get real ICA address for accurate debugging - Include call.value in gas estimation for ICA calls - Add failed call indicator (red/amber) in IcaDetailsCard UI - Return structured IcaDebugResult with failedCallIndex from debugger
| <div className="text-sm font-medium text-green-800">Commitment Revealed</div> | ||
| <div className="mt-2 flex items-start gap-2"> |
There was a problem hiding this comment.
imo this can be collapsible, the section is already very very green
There was a problem hiding this comment.
nah i take this back, I think this bit is fine and the bit below can be less in-your-face
There was a problem hiding this comment.
is it just me eyes or are there 4 different greens in use?
There was a problem hiding this comment.
I think ideally we want this section to be collapsible, imo normal users wouldn't really care about the ICA stuff
| <label | ||
| className={`text-sm font-medium ${isDelivered ? 'text-green-600' : 'text-amber-600'}`} | ||
| > | ||
| {isDelivered ? 'Calls executed:' : 'Calls to execute:'} |
There was a problem hiding this comment.
imo this whole calls executed bit could be the regular grey bgs/borders/text? if there's too much green/amber on a page it then loses the value of being an indicator imo
There was a problem hiding this comment.
also seems like we're using different colours here?
| showCopy={true} | ||
| blurValue={blur} | ||
| /> | ||
| {/* Message type info */} |
There was a problem hiding this comment.
this entire section could use a bit of refactoring, I see a lot of multiple jsx with repeated classes, for example the commitment and reveal section has almost the same structure
| useEffect(() => { | ||
| getExplorerUrls().catch(() => setExplorerUrls({})); | ||
| }, [getExplorerUrls]); |
There was a problem hiding this comment.
nit: prefer useEffect to be at the end (right before the return)
| for (let i = 0; i < displayCalls.length; i++) { | ||
| const call = displayCalls[i]; | ||
| urls[`call-${i}`] = await tryGetBlockExplorerAddressUrl( | ||
| multiProvider, | ||
| destinationDomainId, | ||
| call.to, | ||
| ); | ||
| } | ||
|
|
||
| // Get URL for owner on origin chain (use derived value for REVEAL messages) | ||
| if (displayOwner) { | ||
| urls['owner'] = await tryGetBlockExplorerAddressUrl( | ||
| multiProvider, | ||
| originDomainId, | ||
| displayOwner, | ||
| ); | ||
| } | ||
|
|
||
| // Get URL for ICA address on destination chain | ||
| if (icaAddress) { | ||
| urls['ica'] = await tryGetBlockExplorerAddressUrl( | ||
| multiProvider, | ||
| destinationDomainId, | ||
| icaAddress, | ||
| ); | ||
| } | ||
|
|
||
| // Get URL for salt address on origin chain (use derived value for REVEAL messages) | ||
| const saltAddress = extractAddressFromSalt(displaySalt); | ||
| if (saltAddress) { | ||
| urls['saltAddress'] = await tryGetBlockExplorerAddressUrl( | ||
| multiProvider, | ||
| originDomainId, | ||
| saltAddress, | ||
| ); | ||
| } | ||
|
|
||
| // Get URL for ISM address on destination chain (use derived value for REVEAL messages) | ||
| if (displayIsm && displayIsm !== '0x0000000000000000000000000000000000000000') { | ||
| urls['ism'] = await tryGetBlockExplorerAddressUrl( | ||
| multiProvider, | ||
| destinationDomainId, | ||
| displayIsm, | ||
| ); | ||
| } | ||
|
|
||
| setExplorerUrls(urls); |
There was a problem hiding this comment.
could we maybe do Promise.all for these?
| useEffect(() => { | ||
| getExplorerUrls().catch(() => setExplorerUrls({})); | ||
| }, [getExplorerUrls]); |
There was a problem hiding this comment.
to prevent status update if the component unmounts, you should cleanup the component
useEffect(() => {
let cancelled = false;
getExplorerUrls()
.then(urls => {
if (!cancelled) {
setExplorerUrls(urls);
}
})
.catch(() => {
if (!cancelled) {
setExplorerUrls({});
}
});
return () => {
cancelled = true;
};
}, [getExplorerUrls]);
| {displayOwner && ( | ||
| <KeyValueRow | ||
| label="Owner:" | ||
| labelWidth="w-28 sm:w-36" | ||
| display={displayOwner} | ||
| displayWidth="flex-1 min-w-0" | ||
| link={explorerUrls['owner'] || undefined} | ||
| showCopy={true} | ||
| blurValue={blur} | ||
| /> | ||
| )} | ||
|
|
||
| {/* Show ICA address when we have owner data */} | ||
| {displayOwner && ( | ||
| <KeyValueRow | ||
| label="Account:" | ||
| labelWidth="w-28 sm:w-36" | ||
| display={ | ||
| icaAddress | ||
| ? icaAddress | ||
| : isIcaFetching | ||
| ? 'Computing...' | ||
| : isIcaError | ||
| ? 'Error computing' | ||
| : 'Unknown' | ||
| } | ||
| displayWidth="flex-1 min-w-0" | ||
| link={icaAddress ? explorerUrls['ica'] || undefined : undefined} | ||
| showCopy={!!icaAddress} | ||
| blurValue={blur} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
can be combined and use a react fragment
| let hasValue = false; | ||
| let formattedValue = '0'; | ||
| try { | ||
| hasValue = BigNumber.from(call.value).gt(0); | ||
| formattedValue = hasValue ? fromWei(call.value, 18) : '0'; | ||
| } catch { | ||
| // Malformed value, use defaults | ||
| } |
There was a problem hiding this comment.
you should not just call try catch inside a component, put the try catch into its own separate function
There was a problem hiding this comment.
my recommendation is to use a memo hook and return hasValue and formattedValue there
const {hasValue, formattedValue } = useMemo( () => { try { } catch {} } , [])
| let borderClass: string; | ||
| let labelClass: string; | ||
| let statusSuffix = ''; | ||
|
|
||
| if (isDelivered) { | ||
| // All calls succeeded | ||
| borderClass = 'border-green-200 bg-green-50'; | ||
| labelClass = 'text-green-600'; | ||
| } else if (isFailed) { | ||
| // This specific call failed | ||
| borderClass = 'border-red-200 bg-red-50'; | ||
| labelClass = 'text-red-600'; | ||
| statusSuffix = ' — Failed'; | ||
| } else { | ||
| // Pending (either not checked yet, or after a failed call) | ||
| borderClass = 'border-amber-200 bg-amber-50'; | ||
| labelClass = 'text-amber-600'; | ||
| } |
There was a problem hiding this comment.
same here, this could also be a function outside the component itself
| const { Mailbox__factory } = await import('@hyperlane-xyz/core'); | ||
| // eslint-disable-next-line camelcase | ||
| const mailboxInterface = Mailbox__factory.createInterface(); |
There was a problem hiding this comment.
why lazy load here but not for factory? probably best to keep consistent
| return null; | ||
| } | ||
|
|
||
| const provider = multiProvider.getEthersV5Provider(destinationChainName); |
There was a problem hiding this comment.
does this only works for EVM chains? Maybe should return early in case the chain is not a EVM chain before doing this
| try { | ||
| const provider = multiProvider.getEthersV5Provider(originDomainId); | ||
| return tryFetchIcaAddress(originDomainId, sender, provider); | ||
| const provider = multiProvider.getEthersV5Provider(destinationChainName); |
3 participants
Summary
Add support for decoding and displaying Interchain Account (ICA) messages in the explorer.
Core Logic (
src/features/messages/ica.ts)InterchainAccountMessage.sol:getLocalInterchainAccount()with salt parameterroute()on destination ICA routerUI Component (
src/features/messages/cards/IcaDetailsCard.tsx)Other Changes
src/types.tsTest Messages
0xe89b32c5f63f0f62ffea080f3050d4a2acb9e5e39ac21b2ed3573a1c0c41ec1dSummary by CodeRabbit
New Features
UI/UX Improvements
✏️ Tip: You can customize this high-level summary in your review settings.