test(svm): add real Gateway V5 conformance (ACP-184 Step 5) - #1544
test(svm): add real Gateway V5 conformance (ACP-184 Step 5)#1544Reinis-FRP wants to merge 6 commits into
Conversation
droplet-rl
left a comment
There was a problem hiding this comment.
Summary
Reviewed at 2d4155ab against base reinis/acp-184-step-4-destination-fill (13 files, +1211/-0). This is genuinely test/docs/CI-only: the programs/svm-spoke/src/v5.rs addition sits inside the #[cfg(test)] mod tests block at line 341, package.json only gains a script, and no packaged IDL/client is touched. Scope matches the description.
The suite is strong. What I like in particular:
- It derives destination relays from actual origin
FundsDepositedevents rather than hand-builtRelayData, so the deposit-ID derivation, theV5_MAGIC_PREFIX || stepIdwitness, andget_relay_hashare exercised end-to-end against the real Gateway instead of being restated in the test. - The account-authentication cases are meaningful, not tautological.
relayJit(relay, wrong)/relayJit(relay, status, wrong)fail withMissingAccountbecauseload_v5_fill_accountsre-derivesV5FillStatusPdasand the real PDA is not in the forwarded set — which is precisely the property that makes the outer remaining-account pool safe. Addingwrongto the pool viaextraand still getting a rejection is the right way to pin "pool membership does not equal forwarding." - Deliberately keeping the unsafe primitives (short consumption, two fills over one balance, floor-of-committed-minima with larger JIT outputs) as passing counterexamples rather than pretending the program enforces delivery is the honest framing, and the spec/README text around it is accurate w.r.t.
v5_adapter.rs:196(source.amount >= output_amountis an assertion, not a debit). - Good separation from the mock lane:
InsufficientVaultBalance, allowance, pause, Token-2022 extension and exclusivity cases already live intest/svm/SvmSpoke.V5Fill.ts/.V5Source.ts, so this lane doesn't duplicate them.
Independent verification of the new fixture
I re-derived programs/svm-spoke/fixtures/v5_gateway_path.json from scratch (pure-Python Keccak-256, no repo code):
pathId,siblingPathId,stepRoot,witness— all four match.executorbase58-encodes to34trBszXuqhRjWaMxXWsunJNmyUsBvDNPxAwTzbPTm4p, i.e.constants::GATEWAY_PROGRAM_ID. Sincereference.ts::pathIdhardcodesGATEWAY.toBuffer(), the TS assertion transitively pins that too.V5_MAGIC_PREFIX=keccak256("AcrossV5MessagePrefix.V1"), matching the comment atconstants.rs:27.- The
messageblob decodes byte-for-byte as the Borsh tape[BALANCE_REQ(mint, 500000), TRANSFER(mint, recipient, u64::MAX, 10000bps)]— op vector02000000 0011, then two length-prefixed inputs of0x28and0x4abytes.
The fixture is self-consistent and correctly cross-checked in all three languages.
Main thing I'd like addressed
Nothing here runs in CI, and one piece of it easily could. The lane is workflow_dispatch-only and blocked on an environment/secret that doesn't exist yet (correctly disclosed in the PR body). But PathVectors.ts needs no validator and no private dependency — it's pure hashing — while its Rust and Solidity counterparts do run on every PR. That leaves reference.ts, the encoder most likely to be reused downstream by ACB-637/ENG-320, as the only unpinned side of the vector. Note also that pr.yml's paths-filter matches test/svm/**, which does not match test/svm-gateway/**, so a future PR touching only this directory triggers no SVM job at all. Details inline.
Everything else below is non-blocking: CI hygiene and a few assertions that could be tightened. Not requesting changes — this is test infrastructure with no production surface, and the substance is sound.
Caveat on validation: I could not execute the lane (private solana-v5 + Solana toolchain unavailable here), so I'm relying on your reported 14/14 · 16/16 · 5/5. My review is static analysis plus the independent fixture re-derivation above.
droplet-rl
left a comment
There was a problem hiding this comment.
Approving
Re-reviewed the delta 2d4155ab..1176c013 (3 commits, 10 files, +109/−108) and re-checked the full PR against base. All ten comments from my previous review are addressed, several more thoroughly than I asked. LGTM.
Fixes verified
| Previous comment | Resolution |
|---|---|
PathVectors.ts never runs in CI |
New test-svm-gateway-vectors script wired into lint-and-check-generated (pr.yml:104) |
paths-filter misses test/svm-gateway/** |
Added at pr.yml:147 |
| Hardcoded toolchain versions | Workflow deleted |
| Unverified binary download | Workflow deleted |
| Params-buffer leak on setup failure | Setup moved inside try — plus a new regression test |
FilledRelay event in failed tx not asserted |
Now decoded from the failed receipt |
assert.throws on a local helper |
assert.equal(delivered, consumed) added |
| Missing status-PDA assertions after JIT rejections | Added at both sites |
| Hardcoded spoke program ID | Read from target/idl/svm_spoke.json |
Glob picked up reference.ts/provider.ts |
Explicit spec file list |
Two fixes that went beyond the ask
The failed-receipt event decoding (RealGateway.ts:558-560) is now the real thing. Rather than grepping logs, it calls processEventFromTx(receipt, [spoke]) — which walks meta.innerInstructions and decodes the emit_cpi! self-invoke — then asserts exactly one attempted filledRelay and matches its depositId against the relay. That is precisely the hazard V5_ADAPTER_SPEC.md warns about ("Failed transaction logs may contain attempted fill events"), and the test is now the evidence for it rather than a proxy. I confirmed the signature at solanaProgramUtils.ts:55 accepts a VersionedTransactionResponse, which is what getTransaction(..., {maxSupportedTransactionVersion: 0}) returns.
The new retry test (RealGateway.ts:405-427) is self-verifying, which is the part I like. It patches provider.sendAndConfirm to let the setup instruction land on-chain and then throw, so the buffer genuinely exists when finally runs. Because it wraps expectFailure, a patch that silently failed to match would surface as expected injected setup confirmation failure rather than a false pass. The close instruction correctly escapes the injection (its discriminator doesn't match failAfter), so cleanup still runs inside the patched window, and path([floor(mint, 0n)]) encodes to 177 bytes — one fragment — so both loop branches are reachable. Restoration is in a finally.
Checks I ran
- OZ
Hashes.commutativeKeccak256—@openzeppelin/contractsis pinned at5.5.0andHashes.sollanded in 5.1.0, so the new import resolves. Swapping the hand-rolled ternary for the canonical helper also documents that the Gateway's sorted-pair rule is the standard commutative hash, and deriving the prefix viakeccak256("AcrossV5MessagePrefix.V1")instead of the literal means the Solidity vector now catches constant drift the way the Rust one already did. Both orders asserted in Rust (v5.rs:405-412) and Solidity. - The new CI step works on a fresh checkout.
lint-and-check-generateddoes not download SVM artifacts, so I traced the import graph:PathVectors.ts→reference.ts→src/types/svm.ts, which imports only@coral-xyz/anchor,@solana/web3.jsandethers. Notarget/or generated-client dependency, andresolveJsonModuleis already set. It will run green. - No dangling references. Grepped the tree for
SVM_GATEWAY_READ_TOKEN,svm-gateway-integration,svm-gateway.ymlandlinear.app— all clean. Removing the private Linear links from a public repo is the right call; the replacement prose keeps the policy without the unresolvable pointers. v5_gateway_path.jsonandreference.tsare byte-identical to what I reviewed last round, so my independent Keccak re-derivation ofpathId/siblingPathId/stepRoot/witnessstill holds.
Carrying forward
The toolchain-pinning and binary-integrity concerns I raised were resolved by deleting the workflow rather than by fixing it. They'll be live again when solana-v5 goes public and CI returns — flagged inline on the README so they aren't lost with the file.
Same caveat as last time: I could not execute the real-Gateway lane here (private solana-v5, no Solana toolchain), so the reported 15/15 · 1/1 · 12/12 · 16/16 · 5/5 is taken from the PR description. My verification is static analysis plus the independent fixture derivation.
| fixture, but do not run the real-Gateway lane. Changes in `test/svm-gateway` also trigger the ordinary SVM tests. | ||
|
|
||
| After `solana-v5` is public, add CI coverage using a public checkout at the immutable `GATEWAY_COMMIT`, without a | ||
| cross-repository token or approval environment. This is a temporary CI limitation, not a protocol blocker. |
There was a problem hiding this comment.
Worth capturing the specifics here so they survive to the follow-up, since the deleted workflow resolved two of my earlier comments by removal rather than by fix. When this CI comes back:
- Prefer the existing
.github/actions/setup-solana-anchorcomposite over hardcodinganchor_version/solana_version/node-version. It callsextract-versionsto derive them from the repo, so the lane won't silently drift whenanchor-langis bumped inprograms/svm-spoke/Cargo.toml.pr.ymlalso centralises Node asenv.NODE_VERSION. - If the Gateway's Anchor CLI (otter-sec 1.1.2) still has to be fetched as a release binary, add a
sha256sum -c. Release assets are mutable even when the URL is version-pinned.
Neither applies today — purely a note for whoever writes the public-checkout workflow.
There was a problem hiding this comment.
Documented in 935b2709: reuse the shared toolchain setup and Node version, and verify any downloaded Gateway Anchor binary against a reviewed, pinned SHA-256 checksum before execution. CI remains deferred until publication.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖
| } | ||
| const signature = await send(ComputeBudgetProgram.setComputeUnitLimit({ units: 1_400_000 }), instruction); | ||
| assert.isNull(await connection.getAccountInfo(buffer), "successful execution closes parameter buffer"); | ||
| return { signature, logs: [] as string[], attemptedEvents: [] }; |
There was a problem hiding this comment.
Optional nit: attemptedEvents: [] (and logs: []) on the success path are stubs that exist only to keep the return shape uniform, but the name doesn't signal that. A future test author who calls execute(...) on a successful path and checks result.attemptedEvents gets an empty array and could reasonably read that as "no events were emitted" — when in fact successful-path events are read via readEventsUntilFound, as origin() and the first test do.
Either naming it for its actual scope or returning it only under opts.failed would remove the ambiguity. Not worth holding the PR for.
There was a problem hiding this comment.
Fixed in 935b2709. Success returns only signature; logs and attempted events exist only under failedReceipt for a confirmed failed transaction. Updated both success-path and rollback assertions. Full Gateway suite: 15/15; focused typecheck and formatting pass.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖
79c0b5d to
deb977f
Compare
935b270 to
638afe1
Compare
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
deb977f to
7d7daaa
Compare
638afe1 to
0c25db4
Compare
Scope
ACP-184 Step 5, stacked on #1538: real-Gateway integration tests, cross-VM fixtures, and docs. No production contract or packaged IDL/client changes.
solana-v5@457cf693and run an isolated validator.Follow-ups
yarn test-svm-gatewaylocally whilesolana-v5is private. The secret-dependent workflow has been removed; no cross-repository token or approval environment is needed.test/svm-gateway/**in the SVM path filter. It does not run the real-Gateway lane.solana-v5is public, add real-Gateway CI using a public checkout at the pinned commit. This is a temporary CI limitation, not a protocol blocker.Validation
At 1ea896d5: real-Gateway suite 15/15; standalone TypeScript vector 1/1; Rust 12/12 (default) and 16/16 (
testfeature); Foundry vectors 5/5. Focused TypeScript, formatting, workflow-structure, and diff checks pass.Review fixes cover setup-error cleanup/retry, attempted fill-event decoding from failed receipts, exact underdelivery and rejected-JIT assertions, IDL-derived program identity, explicit spec files, and both sibling-hash orders.
Limitations: the local compiler reports stack-size diagnostics in unchanged legacy fill handlers; this V5 lane does not certify them. The Step 4 base also has three unrelated EVM setup failures (CI run). Verified-build checks remain separate.