Conversation
Replace lying casts/guards in the SVM event pipeline with honest predicates (superstruct where structural). Behaviour changes: - findFillEvent: populate blockNumber/txnRef from the tx envelope (were undefined at runtime). - getEventName: exact match; unknown event names now throw at the query boundary. - findDeposit: skip malformed events rather than throwing mid-search. Groundwork for typed SVM event decoding (codama decoders follow separately).
Resolve each event by its IDL discriminator and decode it with the codama decoder generated from the SvmSpoke program, so decoded data now actually satisfies the generated EventData types (base58 Address strings, bigints, Uint8Array byte arrays, numeric enums) instead of the undescribed number[] and variant-object shapes produced by Anchor's BorshEventCoder. decodeEvent asserts that it was supplied the SvmSpoke IDL and that the event resolves to a known SVMEventNames member, so an unrecognised event now fails loudly rather than yielding data in an unexpected shape.
Fixes the two `SvmCpiEventsClient (integration)` failures on #1508. `decodeEvent` passed a `Buffer` to the codama decoders. They populate byte-array fields by slicing their input, and `Buffer.prototype.slice` returns a `Buffer` — so `depositId`, `inputAmount`, `messageHash` etc. came back as `Buffer`s rather than the `Uint8Array` the generated types promise. `Buffer.toString()` utf8-decodes instead of joining bytes, which is what the assertions tripped on: ``` - ^@^@^@ ... ^@ TM-oM-?M-= // Buffer + 0,0,0, ... ,0,9,84,144 // Uint8Array ``` Decode with `@solana/kit`'s base64 encoder (returns a plain `Uint8Array`) and skip the discriminator via the decoder's `offset` argument instead of slicing. Verified locally against a local validator: `Solana.SvmCpiEventsClient.Integration`, `Solana.EventData` and `Solana.SvmCpiEventsClient.ForgedEvent.unit` all pass, and the full suite is green. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: droplet-rl <284132418+droplet-rl@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
`name in SVMEventNames` also matches keys inherited from Object.prototype
("toString", "constructor", ...), so getEventName returned those as valid
event names instead of throwing. Use Object.hasOwn for an own-property check.
Adds a unit test covering the declared names, the inherited keys, and the
substring near-misses the previous `includes()` matcher accepted.
Co-Authored-By: Claude <noreply@anthropic.com>
|
But
IDL addresses for reference:
Switching from See also #1510 and #1513 — the same path breaks twice more further up the stack. |
|
This branch is cut from
Consequences at
Nothing is actually lost if the stack lands in order — I merged So it's not a correctness problem, just that #1508 through #1513 are being reviewed and CI'd against a tree that differs from the one that ships. A rebase of |
…1516) SvmCpiEventsClient is used downstream against the CCTP TokenMessengerMinter and MessageTransmitter programs, whose events were decoded via the generic Anchor coder into shapes no type describes. Add per-program decoder tables built from the codama clients generated in the contracts package, dispatched by IDL address, so CCTP event data now matches the generated event types. Each table is typed against a name-to-type map, its completeness against the IDL is pinned by a unit test, and decoder output is verified identical to the legacy Anchor+parseEventData pipeline for the events the relayer consumes (DepositForBurn, MintAndWithdraw, MessageReceived). Events without a registered decoder fall back to the generic Anchor path unchanged, and non-SvmSpoke decoding is now regression-tested end to end.
Remove the superstruct DepositIdEventData guard and the isByteArray/isUint8Array ArrayUtils split. These hardened consumers of untyped decode output; the typed codama decoders (next PR) fix the producer instead, and the final cleanup PR was deleting this machinery again. Dropping it keeps each rung of the stack touching this surface once. Also reverts an unrelated yarn.lock pin change.
Reshape each decoder-table entry to construct the name alongside the decoded payload, so the pairing is stated once per event and the follow-on typed-event change only needs to tighten the table's type annotation rather than rewrite its entries. Also adds the differential validation harness (scripts/validate-svm-event-decoding.ts) that the original decode commit referenced: it decodes recent mainnet SvmSpoke transactions through both the legacy Anchor pipeline and this path, and fails on any divergence.
| export function decodeEvent(idl: Idl, rawEvent: string): { data: unknown; name: string } { | ||
| // The generated decoders only apply to SvmSpoke events; any other program's events (e.g. the CCTP | ||
| // TokenMessengerMinter and MessageTransmitter programs) are decoded generically below. | ||
| if (idl.address === SvmSpokeIdl.address) { |
There was a problem hiding this comment.
Blocking: createFor accepts a caller-supplied IDL, but this selects the bundled SvmSpoke decoders from the address alone. A program address survives IDL upgrades, so an older or newer SvmSpoke IDL at this same address can be decoded with stale layouts. I reproduced this by cloning SvmSpokeIdl and adding a trailing u64 field to PausedDeposits: BorshEventCoder returned newField: 777n, while this path silently dropped it. Could this mirror the CCTP path and require idl === SvmSpokeIdl, falling back to generic Anchor decoding otherwise?
Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖
There was a problem hiding this comment.
Agreed on the finding, but I'd push back on the remedy — I think falling back to generic Anchor decoding here trades a narrow bug for a wider, quieter one.
The finding is real
The asymmetry is plain: this branch gates on idl.address === SvmSpokeIdl.address, while the CCTP branch ~20 lines down gates on idl === cctpProgram.idl and carries a six-line comment explaining precisely why address alone is insufficient. That rationale applies verbatim here. There's also a CCTP drift test (Solana.GenericEventDecoding.unit.test.ts, "decodes per the supplied IDL when a CCTP IDL differs from the bundled one") with no SvmSpoke counterpart. Reproduced your case on pxrl/svm2:
decodeEvent(drifted) -> {"isPaused":true} // new_field dropped
BorshEventCoder -> {is_paused, new_field} // present
But the generic fallback is not shape-preserving for SvmSpoke
I diffed what the two paths actually produce for real SvmSpoke events (generated decoder vs BorshEventCoder + parseEventData):
| event | field | generated | generic fallback |
|---|---|---|---|
| FundsDeposited | outputAmount, depositId |
Uint8Array(32) |
number[](32) |
| FilledRelay | inputAmount, depositId, messageHash |
Uint8Array(32) |
number[](32) |
| FilledRelay | relayExecutionInfo.fillType |
0 / 2 |
{"FastFill":{}} / {"SlowFill":{}} |
The enum is the problem. In #1514, fillFromEvent() assigns fillType: data.relayExecutionInfo.fillType straight into FillWithBlock, typed as a numeric FillType. A variant object there does not throw — it silently makes every fillType === FillType.SlowFill comparison false. SlowFill is 2, not 1, so there is no accidental truthiness to soften it. That is a worse failure mode than dropping an unknown trailing field, and on a fill-accounting path.
The number[] shift is currently absorbed by isUint8Array() duck-typing in unwrapEventData, but #1514 deletes isUint8Array outright. I checked BigNumber.from(number[]) and hexlify(number[]) and both still tolerate it, so that half survives; the enum does not.
Why the asymmetry is defensible in intent, if not in implementation
CCTP event data flows outward to consumers built against the legacy Anchor shape — which is why this PR's differential tests assert decodeEvent(CCTP) deep-equals legacyDecode(CCTP). The generic fallback is shape-preserving there, so it degrades safely. SvmSpoke data flows inward into SDK-typed code that requires the generated shape; eliminating number[] and variant-object shapes is the stated purpose of this PR. The fallback cannot be equivalent there by construction.
So I think the correct behaviour for a drifted SvmSpoke IDL is to reject, not to fall back. That also matches this branch's existing stance — it already asserts loudly on an unrecognised SvmSpoke event rather than yielding an unexpected shape.
On idl === SvmSpokeIdl specifically
Reference equality is fragile under npm dual-package resolution: a consumer whose @across-protocol/contracts dedupes to a different copy fails === even for byte-identical IDLs. For CCTP that degrades gracefully; for SvmSpoke, a throw would fire on a false positive. Comparing the event/type layout instead is both dual-package safe and strictly more precise. Prototyped and verified against five cases:
const layoutCache = new WeakMap<object, boolean>();
const bundledKey = JSON.stringify([SvmSpokeIdl.events ?? [], SvmSpokeIdl.types ?? []]);
function matchesBundledSpokeLayout(idl: Idl): boolean {
const cached = layoutCache.get(idl);
if (cached !== undefined) return cached;
const result = JSON.stringify([idl.events ?? [], idl.types ?? []]) === bundledKey;
layoutCache.set(idl, result);
return result;
}- accepts the bundled instance
- accepts a content-identical deep copy (which
===would reject) - rejects your
PausedDepositstrailing-field drift - rejects drift in a nested type (
RelayExecutionEventInfo, reached viaFilledRelay) — a per-event shallow comparison misses this, sinceFilledRelay's own type entry is untouched - rejects a drifted event discriminator
Memoised per IDL object, so the stringify cost is paid once per client rather than per event.
The trade-off, stated honestly
Rejecting means a consumer holding a newer SvmSpoke IDL is hard-blocked until the SDK bumps @across-protocol/contracts, where today they would keep running with a silently dropped field. For bridge accounting data I think loud-and-blocked is the right default, but it is a real cost and worth a deliberate decision rather than an implicit one.
Droplet, on behalf of @pxrl — analysis run against pxrl/svm2 @ 2a18278; all figures above reproduced locally.
| // Decode to a plain Uint8Array, not a Buffer: the generated decoders slice their input to populate byte-array | ||
| // fields, and slicing a Buffer yields a Buffer, whose toString() and JSON serialisation differ from the | ||
| // Uint8Array that the generated types promise. | ||
| const rawEventData = getBase64Encoder().encode(rawEvent); |
There was a problem hiding this comment.
Non-blocking follow-up candidate: processEventFromTx already has the event as bytes, converts it to a base64 string, and this immediately converts that string back to bytes. It may be cleaner to let decodeEvent accept string | Uint8Array, pass the original bytes through for generated decoders, and create base64 only for the Anchor fallback. Keeping string support would preserve the exported API; given the small payloads, I do not think this needs to delay this PR.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖
There was a problem hiding this comment.
Agreed, and agreed it shouldn't hold the PR.
Confirming the round-trip: processEventFromTx() has the event as bytes, does Buffer.from(ixData.slice(8)).toString("base64"), and both generated-decoder branches in decodeEvent() immediately undo it with getBase64Encoder().encode(rawEvent). Only the BorshEventCoder fallback genuinely needs the string.
One wrinkle worth noting if this is picked up: the base64 hop is currently what normalises Buffer to Uint8Array. The comment at the top of the SvmSpoke branch calls this out — the generated decoders slice their input to populate byte-array fields, and slicing a Buffer yields a Buffer, whose toString() and JSON serialisation differ from the Uint8Array the generated types promise. So passing the original bytes through would need Uint8Array.prototype.slice.call(...) or an explicit new Uint8Array(buf) at the boundary, otherwise the shape regression the comment warns about comes back through the side door. Cheap to handle, just not a pure deletion.
Droplet, on behalf of @pxrl
There was a problem hiding this comment.
Correcting myself — the wrinkle I raised above is wrong, and the refactor is cleaner than I implied.
I assumed bs58.decode() returned a Buffer. It doesn't: bs58 v6 (pinned ^6.0.0 here) returns a Uint8Array, and .slice() on it stays a Uint8Array. Verified against the installed version:
bs58.decode -> Uint8Array | isBuffer: false
.slice() -> Uint8Array | isBuffer: false
So ixData in processEventFromTx() is already the exact type the generated decoders want. The Buffer.from(...) there exists only to reach .toString("base64") — passing ixData.slice(8) straight through needs no normalisation shim, and the Buffer-vs-Uint8Array hazard the SvmSpoke branch comment warns about can't arise on that path. It only concerns how the base64 string is decoded back to bytes (getBase64Encoder() rather than Buffer.from(s, "base64")), which the fallback path would keep using.
Net: this is closer to a pure deletion than I suggested. Still agree it's a follow-up rather than a blocker.
Droplet, on behalf of @pxrl
Resolve each event by its IDL discriminator and decode it with the
codama decoder generated from the owning program, so decoded data now
actually satisfies the generated event types (base58 Address strings,
bigints, Uint8Array byte arrays, numeric enums) instead of the
undescribed number[] and variant-object shapes produced by Anchor's
BorshEventCoder. SvmSpoke events must resolve to a known SVMEventNames
member, so an unrecognised event fails loudly rather than yielding data
in an unexpected shape. CCTP (TokenMessengerMinter/MessageTransmitter)
events decode through per-program tables whose completeness against their
IDLs is pinned by unit tests; the bundled decoders apply only when the
caller supplied the exact bundled IDL instance (a program address
survives IDL upgrades), and any other IDL falls through to the generic
Anchor path, decoding per the supplied IDL. Each table entry constructs
its name/data pair, so the follow-on typed-event PR only tightens the
table's type annotation. Equivalence with the legacy pipeline is proven
by differential tests and by scripts/validate-svm-event-decoding.ts
against recent mainnet transactions.