Repository navigation
feat(svm): publish stable production JSON IDL paths (ACP-233) - #1586
Conversation
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
27383c2 to
9b300fa
Compare
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
droplet-rl
left a comment
droplet-rl
left a comment
There was a problem hiding this comment.
The approach works. The allowlist is the only route into the stable paths. buildIdl.sh no longer looks at IS_TEST. The test-feature build lives in target/, and generateSvmAssets.ts reads all its inputs before replacing anything. Moving to prepack means npm pack and npm publish run the same build.
What I ran: I couldn't run Anchor here, so I rebuilt the inputs from the published 6.0.0-beta.3 tarball (base 11533867 is that version bump):
- production IDLs into
target/idl - the
.d.tsfiles astarget/types/*.ts - a one-contract
out/with onlyHubPool
Then I ran generateSvmAssets.ts and generate-svm-clients. yarn test-svm-idl-package passed 6/6, including the real npm pack and the external Codama consumer. I also checked the guard assumptions against the published IDLs:
- No instruction in any of the five IDLs has nested account groups.
- Every
event_authorityalready has the canonical__event_authorityseed. idls/message_transmitter_v2.jsonis byte-identical to the published copy.
So the strict checks shouldn't trip on today's inputs.
Nothing here blocks merging. Inline comments, roughly by importance:
TestIdlandTestAnchorare no longer exported.beta.3ships them, plusdist/src/svm/assets/idl/test.json. The new hardcoded IDL list drops them. Removing them is reasonable, and a code search found no users anywhere in the across-protocol org. But the description says existing IDL/type exports stay available, so please note the removal there or in the release notes.- The test-feature guard has a gap. It only catches
svm_spoke. A test-featuresponsored_cctp_src_peripheryIDL changes theStatelayout (addscurrent_time) but has notest_instruction, so the guard lets it through. I confirmed locally thatgenerateSvmAssets.tsaccepts it. - The cache key change has a cost. Adding
package.jsonto the shared hash also invalidates theverified-test-binariescache (the Dockersolana-verifybuilds) wheneverpackage.jsonchanges, e.g. version bumps. - Small nits: the fixture hardcodes
ix.accounts[2], andclean/clean-fastdon't remove the newsvm/output directory.
| import { readFileSync, mkdirSync, rmSync, writeFileSync } from "fs"; | ||
| import { readProductionIdls } from "./productionIdls"; | ||
|
|
||
| const idls = readProductionIdls("target/idl"); |
There was a problem hiding this comment.
Assets now come only from productionIdlNames. The old script copied everything in target/idl, so the test program's IDL is no longer picked up. Compared with the published 6.0.0-beta.3, this removes the root TestIdl and TestAnchor exports, plus dist/src/svm/assets/{idl/test.json,test.js,test.d.ts}.
I think dropping them is right, and a GitHub code search across the across-protocol org found no users. But the PR description says existing IDL/type exports stay available. Please mention this removal there or in the changelog, so nobody upgrading within 6.0.0-beta hits a missing export without warning.
There was a problem hiding this comment.
Documented in a1cd7e9f and the PR description. The README now explicitly lists the removal of TestIdl, TestAnchor, and dist/src/svm/assets/{idl/test.json,test.js,test.d.ts} relative to 6.0.0-beta.3, and directs tests to generate those interfaces from the matching source tag. The compatibility promise now refers to existing production exports.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖
| const idl: Idl = JSON.parse(contents); | ||
| assert.equal(idl.metadata.name, name, `Unexpected IDL at ${directory}/${name}.json`); | ||
| for (const instruction of idl.instructions) { | ||
| assert(!instruction.name.startsWith("test_"), `Test instruction in production IDL: ${instruction.name}`); |
There was a problem hiding this comment.
This guard only detects a test-feature build when the program adds a test_ instruction. That's true for svm_spoke, but not for sponsored_cctp_src_periphery. Under --features test, its State gains current_time: u64 (programs/sponsored-cctp-src-periphery/src/state.rs:14), and it has no test_* instruction.
I tested this locally: I added current_time to State in target/idl/sponsored_cctp_src_periphery.json, and generateSvmAssets.ts accepted it and copied it into src/svm/assets. From there build-ts would publish it to the stable path.
In practice, a whole-workspace IS_TEST=true yarn build-svm also emits a test svm_spoke.json, which does trip the guard. A program-scoped build or copy would not.
A cheap fix is a per-program check, e.g. assert that the sponsored_cctp_src_periphery State has no current_time field. Alternatively, reword the README/description so the guard claims to catch test instructions only, not test-feature IDLs in general.
There was a problem hiding this comment.
Fixed in a1cd7e9f. The shared production-IDL validator now rejects current_time in SponsoredCctpSrcPeriphery's State. Added a regression that leaves Spoke production-only and injects the sponsored test layout; both generateSvmAssets and exportIdl reject it. All seven package tests pass, including the external packed-package consumer; yarn typecheck-tests also passes.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖
| # conflicts with other caching actions that might have cleaned some of cached contents. | ||
| path: svm-${{ inputs.type }}.tar | ||
| key: svm-${{ inputs.type }}-${{ runner.os }}-node-${{ steps.resolved-node.outputs.version }}-${{ hashFiles('Cargo.toml', 'Anchor.toml', 'verified-build.json', '.github/actions/setup-solana-anchor/**', 'yarn.lock', 'Cargo.lock', 'programs/**/Cargo.toml', 'programs/**/Xargo.toml', 'programs/**/*.rs', 'idls/**/*.json', 'scripts/svm/buildHelpers/**') }} | ||
| key: svm-${{ inputs.type }}-${{ runner.os }}-node-${{ steps.resolved-node.outputs.version }}-${{ hashFiles('Cargo.toml', 'Anchor.toml', 'verified-build.json', '.github/actions/setup-solana-anchor/**', 'package.json', 'yarn.lock', 'Cargo.lock', 'programs/**/Cargo.toml', 'programs/**/Xargo.toml', 'programs/**/*.rs', 'idls/**/*.json', 'scripts/svm/buildHelpers/**') }} |
There was a problem hiding this comment.
package.json really is an input for type: artifacts, because productionIdls.ts derives the allowlist from files. But this hash is shared with type: verified-test-binaries (pr.yml:173), whose output doesn't depend on package.json. As written, any package.json change also invalidates the Docker solana-verify test-binary cache. That includes version bumps, script edits and dependency bumps, and package.json changed in most of the last several commits.
Two possible fixes:
- Keep the allowlist as a constant in
productionIdls.ts, whichscripts/svm/buildHelpers/**already covers. HaveIdlPackage.tsassert thatpackage.jsonfileslists exactly those paths. - Include
package.jsonin the key only wheninputs.type == 'artifacts'.
There was a problem hiding this comment.
Fixed in a1cd7e9f using the conditional-key option: package.json contributes only when inputs.type == 'artifacts'. The manifest remains the source of the public allowlist; package-only changes no longer invalidate the verified-test-binaries cache. The cache expressions pass isolated actionlint validation. Full PR workflow lint reports the same pre-existing old-action/missing-description diagnostics as the prior commit.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖
| programAddress: program, | ||
| seeds: [Buffer.from("__event_authority")], | ||
| }); | ||
| assert.equal(ix.accounts[2].address, authority); |
There was a problem hiding this comment.
Nit: this hardcodes the account position. To keep the test pinned to the IDL rather than to today's account order, you could derive the index: idl.instructions.find((i) => i.name === "adapter_execute_across_v5").accounts.findIndex((a) => a.name === "event_authority").
There was a problem hiding this comment.
Updated in a1cd7e9f: the fixture finds event_authority by name in the adapter instruction's IDL accounts and asserts it exists before checking the generated account address. The expected PDA is still independently derived from the canonical seed. The external packed-package fixture passes.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
|
Addressed the review feedback in All seven package tests, test typechecking, formatting and commit hooks pass. Cache expressions pass isolated actionlint validation; the full workflow's existing lint diagnostics are unchanged. The PR description now documents the removed test-program exports. Inline replies contain the individual fixes; leaving threads open for reviewer confirmation. Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖 |
droplet-rl
left a comment
droplet-rl
left a comment
There was a problem hiding this comment.
Re-reviewed at a1cd7e9f. a1cd7e9f fixes all four points from my first review, and I found nothing new.
TestIdl/TestAnchorremoval: the README and PR description now list the change from6.0.0-beta.3, and the compatibility promise is limited to production exports.- Sponsored test layout:
readProductionIdlsnow rejectscurrent_timein the sponsoredState, and fails loudly ifStateis missing or isn't a struct. Together with thetest_instruction check onsvm_spokeand the byte-for-byte comparison of the Circle snapshots, this catches every IDL difference thetestfeature currently causes in the five production programs. The new regression test injects only the sponsored layout while Spoke stays production, so the check is exercised on its own. - Cache key:
package.jsonnow counts only wheninputs.type == 'artifacts'. Theverified-test-binarieskey is back to its base form, so version bumps no longer force the Docker rebuild. - Fixture: it now looks up
event_authorityby name in the IDL and asserts it exists, while still deriving the PDA from the seed.
Bonus: the old clean-fast script was a shell syntax error (... & ; done makes sh fail with Syntax error: ";" unexpected). The new & done form runs.
Validation: I rebuilt the same toolchain-free inputs from the published beta.3 tarball and ran yarn test-svm-idl-package on a1cd7e9f: 7/7 pass, including the new sponsored-layout test and the packed external consumer. CI's Build TypeScript job, which runs the same suite, also passes on this SHA.
yarn typecheck-tests fails locally, with errors only about testCreateV5FillStatus. That's my setup: I can't run generate-svm-test-idls without Anchor. CI runs that step before typechecking (pr.yml:161-166), so the PR isn't affected.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Consumers currently depend on generated Solana clients or internal IDL paths, and asset generation can carry stale test/mock IDLs into the package. This adds three stable
@across-protocol/contracts/svm/idl/*.jsonpaths for downstream client generation:svm_spoke.json: the V5 Spoke interface, including Rust-derived input/JIT schemas and canonical event-authority PDA metadata.multicall_handler.jsonandsponsored_cctp_src_periphery.json: Across-owned legacy compatibility interfaces for existing quote construction and event indexing. They are not part of V5 flows.Circle-owned
message_transmitter_v2.jsonandtoken_messenger_minter_v2.jsonare excluded from the new stable paths. SDK's later client-generation migration will use reviewed, commit-pinned upstream Circle schemas. Existing production IDL/type/client exports, reviewed Circle snapshots, runtime helpers, legacy deep imports, EVM artifacts and shared exports remain available during migration.MulticallHandlerCoderis handwritten compilation logic and must move downstream separately; publishing an IDL cannot regenerate it.Compatibility change from
6.0.0-beta.3: the test program's rootTestIdlandTestAnchorexports anddist/src/svm/assets/{idl/test.json,test.js,test.d.ts}are removed. Tests needing those interfaces must generate them from the matching source tag.The build separates the public IDL allowlist from compatibility assets, removes stale JSON from both output directories, and rejects test-feature instructions, the sponsored bridge's test-only
State.current_timefield, or missing schema enrichment.npm packand publishing use the same build lifecycle. CI exercises the packed package from an external Codama consumer fixture. The README documents ownership, paths, version pinning and the separate GitHub Release binary channel.Based on
epic-v5/svmat11533867(#1580). Implements ACP-233; downstream ownership and migration are tracked in ACP-231. No protocol behavior or on-chain deployment changes.Validation:
yarn test-svm-idl-package: all seven tests pass, including a real offline npm pack, external generation/compilation of all three public clients, CommonJS/ESM JSON loading, V5 input/JIT codec round trips, account decoding and event-authority resolution.yarn typecheck-tests, focused Prettier checks andgit diff --checkpass.svm/idlwhile preserving adjacent files.IS_TEST=truepassed without leaking test/mock interfaces; this update changes packaging only.6.0.0-beta.3(local build, unpublished). Node22.22.0, Anchor CLI/core1.1.2, Rust1.97.1, Codama1.3.0, nodes-from-anchor1.2.2, renderers-js1.3.0, Solana Kit5.4.0, TypeScript5.6.2. No binary release was created or deployed.Downstream client migration and eventual runtime-export removal remain separate follow-up work.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖