feat(svm): add V5 adapter foundations - #1535
Conversation
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
1c58662 to
a61908f
Compare
droplet-rl
left a comment
There was a problem hiding this comment.
Summary
Solid foundations PR. I reproduced every test claim locally and independently verified the frozen ABI constants against their upstream definitions rather than just against the fixture.
Verified green:
cargo test -p svm-spoke v5::tests --lib— 7 passedyarn ts-mocha -p ./tsconfig.json -t 1000000 test/svm/SvmSpoke.V5Foundations.ts— 3 passedyarn test-evm-foundry -- --match-contract SvmSpokeV5VectorsTest— 4 passed
(cargo +nightly fmt and cargo clippy I could not run — neither component is installed in this environment.)
Cross-repo conformance I checked by hand against across-protocol/solana-v5@programs/gateway:
| Frozen constant | Upstream | Match |
|---|---|---|
GATEWAY_PROGRAM_ID |
gateway/src/lib.rs declare_id! |
✅ |
GATEWAY_ADAPTER_EXECUTE_V5_PREIMAGE |
ADAPTER_EXECUTE_ACROSS_V5_PREIMAGE |
✅ |
GATEWAY_DISPATCH_AUTHORITY_SEED + [seed, callee_program_id] |
DISPATCH_AUTHORITY_SEED, execute_via_adapter |
✅ |
GATEWAY_VAULT_AUTHORITY_SEED / ["vault_authority"] |
VAULT_AUTHORITY_SEED, execute.rs |
✅ |
V5GatewayContext field order/widths |
dispatch::CtxValues |
✅ |
dispatch layout disc(8) ‖ ctx ‖ len(4) ‖ input ‖ len(4) ‖ jit |
encode_dispatch_data |
✅ |
And against across-protocol/contracts-v5@src/adapters/AcrossDepositDelegateAdapter.sol: AcrossDepositParams / AcrossDepositInput / AcrossDepositJitParams field order matches, the paramModificationDigest layout matches, the improvement-only + ignore-unpermitted semantics match, and the low-s / v∈{27,28} rules match what OZ ECDSA.recover enforces. find_v5_account's search-by-derived-key contract also lines up with the Gateway's forwarding loop (dedups keys, preserves committed writability), so the design holds.
Nice work — the wire encoding is right, the strictness is right, and the fixture is genuinely useful.
The one thing I'd like an answer on before this is "frozen"
V5DepositModificationRules::validate() rejects authority == 0 && any permission set. On EVM that is a supported configuration: adapterDelegateExecuteAcrossV5 decodes JIT whenever the whole paramModificationRules word is nonzero, and _applyParamModifications only verifies a signature if (authority != address(0)) — the struct comment even reads "the signing authority if one is required". So EVM supports permissionless improvement-only JIT; this SVM surface rejects it. It fails closed, so it isn't a vulnerability, but it means a cross-VM path builder can emit rules that are valid on EVM and revert on SVM. If intentional, please say so explicitly in V5_ADAPTER_SPEC.md and name the EVM behavior being deviated from. Details inline.
Other themes
- The Rust golden-vector tests never run in CI.
pr.ymlrunsyarn lint-rustbut nocargo test. The TS and Solidity halves run; the Rust half — the primary guard on this surface — does not. - The load-bearing invariant behind
InputVaultBalanceis unstated. The EVM analog resolves against a per-execution context; the SVM one resolves against a vault thatsolana-v5/AGENTS.mddocuments as "shared per mint rather than isolated per execution ... the gateway does not yet enforce net-zero settlement." - The three-language verification proves less than the PR description implies — all three tests re-derive the same formulas from the same fixture.
- CU on the hot path — several
find_program_addressgrinds that the in-repo Gateway adapters avoid by persisting bumps at init.
Question not covered inline
EVM's inputAmountParam carries an INPUT_PARAM_SET_CALL_VALUE flag for native deposits. V5InputAmountMode has no analog, and this repo does ship a nativeDeposit path. Is native-SOL V5 deposit deliberately out of wire v1? Adding a Borsh variant later is additive, but it would sit awkwardly against a version: u8 = 1 freeze — worth one line in the spec either way.
Not blocking — nothing here is callable yet — but the divergence and the CI gap are cheapest to fix now, before the behavior steps build on top.
| pub fn validate(&self) -> Result<()> { | ||
| let has_authority = self.authority != [0u8; 20]; | ||
| let has_permission = self.allow_output_amount || self.allow_exclusive_relayer; | ||
| require!(has_authority == has_permission, V5Error::InvalidParamModificationRules); |
There was a problem hiding this comment.
Deliberate divergence from the EVM adapter — please document it here and in the spec.
This rejects authority == 0 with a permission flag set. contracts-v5/src/adapters/AcrossDepositDelegateAdapter.sol accepts exactly that shape:
if (uint256(params.paramModificationRules) != 0) { // whole word, not just the authority
_applyParamModifications(...);
}
// ...and inside:
if (authority != address(0)) { /* verify signature */ } // skipped entirely when authority == 0
if (allowAmountOut) { /* improvement-only bump, no signature required */ }The EVM struct comment says "the signing authority if one is required. Zero disables JIT entirely" — zero word, not zero authority. So permissionless improvement-only outputAmount JIT is a valid EVM path shape, and this rejects it with InvalidParamModificationRules.
Failing closed is the right default and I'd keep the stricter rule. But V5_ADAPTER_SPEC.md:54 states the SVM rule ("it never means permissionless modification") as if it were the shared rule, so a path builder reading the spec has no way to learn EVM differs. Since this PR is the freeze, please add an explicit "diverges from AcrossDepositDelegateAdapter.sol, which permits authority-less JIT" note — and confirm the integrator side never emits that shape for SVM.
There was a problem hiding this comment.
Documented the intentional divergence in both V5DepositModificationRules::validate() and V5_ADAPTER_SPEC.md: zero authority disables JIT on SVM v1, while AcrossDepositDelegateAdapter permits authority-less JIT when a permission flag is set. The spec now states that SVM route builders must not emit that EVM-only shape. Step 1 does not include a live SVM route builder, so this is recorded as an explicit integration requirement rather than claiming an enforcement point that is not in this PR.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖
| } | ||
|
|
||
| /// Literal uses the committed `input_amount`. Balance-relative mode resolves `bips` of the canonical Gateway input | ||
| /// vault's live token amount, rounded down, and later enforces the committed amount as a floor. |
There was a problem hiding this comment.
This doc reads as execution-scoped; the account it resolves against is not.
The EVM analog resolves BalanceSub against address(this) — under ADAPTER_DELEGATECALL that's the Executor, a per-execution context. Here "the canonical Gateway input vault" is ATA(PDA(["vault_authority"], gateway), mint), which solana-v5/AGENTS.md describes as:
Vaults are shared per mint rather than isolated per execution. Any residual balance or stale approval left between executions may be consumed by a later permissionless tape. Executions must therefore leave no residual vault balance; the gateway does not yet enforce net-zero settlement.
So bips of the live balance is only equivalent to the EVM semantics under an invariant the Gateway explicitly does not enforce. Combined with the spec's ["v5_source_delegate"] allowance rule ("may grant any allowance at least the resolved amount, including u64::MAX"), bips = 10_000 over a vault with residual balance sweeps whatever is sitting there.
The math here is correct and I'm not asking for a code change — but this is the most load-bearing unstated assumption in the frozen surface. Please cross-reference the Gateway's net-zero-settlement invariant in both this doc comment and V5_ADAPTER_SPEC.md:85, so Step 2 doesn't have to rediscover it.
There was a problem hiding this comment.
Documented this invariant at both sites. The enum comment and spec now state that Gateway vaults are shared per mint, InputVaultBalance resolves against shared live state, the continuing tape must clear residual state, and Gateway does not currently enforce net-zero settlement. The spec also calls out stale approval explicitly.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖
|
|
||
| pub fn decode_v5_adapter_input(data: &[u8]) -> Result<V5AdapterInput> { | ||
| let input: V5AdapterInput = decode_strict(data)?; | ||
| require_eq!(input.version, V5_ADAPTER_WIRE_VERSION, V5Error::UnsupportedVersion); |
There was a problem hiding this comment.
Version is checked after the whole body is deserialized, which inverts the error for the case this exists to handle.
decode_strict::<V5AdapterInput> parses everything first, so a future v2 payload with a changed layout almost certainly fails inside Borsh and surfaces InvalidWireFormat — never reaching this require_eq!. UnsupportedVersion will realistically only fire for a v2 payload that happens to be layout-compatible with v1, which is the least interesting case.
For a versioned boundary whose job is graceful rejection of unknown versions, peel the leading byte first:
let (&version, body) = data.split_first().ok_or_else(|| error!(V5Error::InvalidWireFormat))?;
require_eq!(version, V5_ADAPTER_WIRE_VERSION, V5Error::UnsupportedVersion);The on-wire bytes are unchanged, so this costs nothing at the freeze — it just makes the error honest.
There was a problem hiding this comment.
Fixed. The decoder now peels and validates the leading version byte before Borsh-decoding the mode body. Exact-error tests cover a full wrong-version payload and a one-byte future-version payload returning UnsupportedVersion, while empty input returns InvalidWireFormat.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖
| } | ||
|
|
||
| pub fn require_gateway_dispatch_authority(account: &AccountInfo) -> Result<()> { | ||
| let (expected, _) = derive_gateway_dispatch_authority(); |
There was a problem hiding this comment.
find_program_address on every dispatch, plus a misleading error.
Two things:
-
This grinds the full derivation each call (~1.5k–15k CU depending on bump). The Gateway ships the callee-side check as
gateway::dispatch::is_dispatch_authority, which takes a stored canonical bump and usescreate_program_address— and its own in-repo adapters (prefunded-adapter,cctp-mint-adapter,cctp-burn-adapter) all persistdispatch_authority_bumpin config at init for exactly this reason. Hand-rolling the check is fine and sanctioned here (mock-executor/mock-plannerdo the same, and the "no cross-repo Rust dependency" comment inconstants.rsjustifies it), but the bump caching is worth carrying over. Same applies toderive_v5_source_delegate/derive_v5_fill_delegate/derive_v5_fill_payer, which will all run per deposit/fill. -
V5Error::MissingAccount("Missing required Across V5 account!") also fires when the account is present but isn't a signer or has the wrong key. That's the more likely failure in practice and the message points debugging in the wrong direction. Worth a distinct variant.
There was a problem hiding this comment.
Added a dedicated InvalidDispatchAuthority error for the wrong-key/non-signer cases.
I have not added bump persistence in this foundations step: it exposes no callable V5 entrypoint and has no V5 config account in which to store the fixed dispatch bump. The submitter- and relay-derived PDAs also cannot share one cached bump. The derivation helpers continue to return their bumps so the behavior step can choose persisted or caller-provided bumps once the actual account topology and hot paths exist.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖
| solana-security-txt = "1.1.1" No newline at end of file | ||
| solana-security-txt = "1.1.1" | ||
|
|
||
| [dev-dependencies] |
There was a problem hiding this comment.
These dev-deps back 7 tests that CI will never run.
.github/workflows/pr.yml has lint-rust (cargo +nightly fmt --all -- --check && cargo clippy) but no cargo test anywhere in the file. The TS vectors run via anchor test and the Solidity vectors via test-evm, so two of the three languages are covered — but the Rust half, which is the one that actually exercises decode_v5_adapter_input, resolve_v5_deposit_modifications, and the PDA derivations, is unguarded.
Given the whole point of this PR is a frozen surface protected by golden vectors, that's the gap most likely to let drift through. A step in the existing lint-and-check-generated job (it already installs a Rust toolchain and warms the cargo cache) would cover it:
- name: Test rust
run: cargo test -p svm-spoke --libThere was a problem hiding this comment.
Added cargo test -p svm-spoke --lib to the existing lint-and-check-generated job, immediately after Rust lint. The expanded local suite now passes 9 tests.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖
| import { ECDSA } from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; | ||
|
|
||
| /// @notice EVM-side conformance checks for the SVM V5 adapter's cross-VM hash and signature fixtures. | ||
| contract SvmSpokeV5VectorsTest is Test { |
There was a problem hiding this comment.
Worth being precise about what this proves.
This re-implements the SVM formulas in Solidity against the shared fixture. It doesn't call contracts-v5's AcrossDepositDelegateAdapter.paramModificationDigest, and testDepositIdVector doesn't mirror this repo's own SpokePool.getUnsafeDepositId (contracts/spoke-pools/SpokePool.sol:1289) — which packs a 20-byte msg.sender, not a 32-byte program id. So a green run means "keccak and sha256 agree across three languages," not "the SVM surface matches the EVM adapter."
That's still worth having — it catches endianness and packing slips in the fixture. But the PR description's "golden vectors verified from Rust, TypeScript, and Solidity" reads like cross-VM conformance, and someone will later treat green CI as exactly that.
For what it's worth I did the comparison by hand and the digest layout does match EVM (abi.encodePacked(domain, pathId, uint256 depositNonce, uint256 newOutputAmount, bytes32 newExclusiveRelayer)); the depositId preimage necessarily differs because of the address widths. Two suggestions, either is fine: note in V5_ADAPTER_SPEC.md that these are self-consistency vectors rather than EVM conformance vectors, or record the corresponding EVM-side values in the fixture so the divergence is visible as data.
There was a problem hiding this comment.
Clarified this in V5_ADAPTER_SPEC.md: these are cross-language self-consistency vectors, not an invocation of the EVM adapter. The spec now records that the JIT digest layout matches AcrossDepositDelegateAdapter, while the deposit-ID preimage necessarily differs because SVM commits a 32-byte executor program ID and EVM commits a 20-byte caller address.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖
| } | ||
|
|
||
| // Across V5 adapter specific errors. | ||
| #[error_code] |
There was a problem hiding this comment.
Cheap moment to give this enum its own offset.
Anchor defaults every #[error_code] enum to 6000, so V5Error::InvalidWireFormat == 6000 == CommonError::InvalidQuoteTimestamp == SvmError::NotOwner == CallDataError::InvalidSelector. Clients decoding by numeric code can't tell them apart, and the generated IDL will carry duplicate codes.
That's a pre-existing pattern in this file, so not something this PR broke — but V5Error is brand new with no clients depending on its codes yet, which makes it the one enum here that can still be fixed for free:
#[error_code(offset = 1000)] // 7000..
pub enum V5Error {Worth doing before Step 2 starts surfacing these errors to the Gateway and off-chain tooling.
There was a problem hiding this comment.
Added a dedicated V5 error range and a regression assertion that InvalidWireFormat == 7000. I used #[error_code(offset = 7000)]: in Anchor 0.31 an explicit offset replaces the default 6000 base rather than being added to it, so offset = 1000 would generate code 1000.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖
| pub const V5_SOURCE_DELEGATE_SEED: &[u8] = b"v5_source_delegate"; | ||
| pub const V5_FILL_DELEGATE_SEED: &[u8] = b"v5_fill_delegate"; | ||
| pub const V5_FILL_PAYER_SEED: &[u8] = b"v5_fill_payer"; | ||
| pub const FILL_STATUS_SEED: &[u8] = b"fills"; |
There was a problem hiding this comment.
The constant is introduced but the three existing call sites still use the bare literal — instructions/fill.rs:70, instructions/slow_fill.rs:36, instructions/slow_fill.rs:167.
Right now there are two sources of truth for the same replay namespace, which is the opposite of what extracting the constant was for. Anchor's seeds = [...] accepts a const, so they can all take FILL_STATUS_SEED in this PR.
There was a problem hiding this comment.
Fixed all three existing account constraints to use FILL_STATUS_SEED; there are no remaining b"fills" seed literals in those fill/slow-fill call sites.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖
| assert!(recover_v5_authority(&digest, &array(&fixture, "/jit/highSSignature")).is_err()); | ||
| let mut invalid_v = signature; | ||
| invalid_v[64] = 0; | ||
| assert!(recover_v5_authority(&digest, &invalid_v).is_err()); |
There was a problem hiding this comment.
Missing the negative test that matters most for the JIT boundary.
Current coverage is high-s rejection and v = 0 rejection — both malleability/encoding checks. What isn't covered is domain separation, which is the actual security property of the digest:
- a signature valid for
path_idA rejected underpath_idB - a signature valid for
deposit_nonceN rejected under N+1 - a well-formed signature from a different signer rejected against the committed authority
All three are a couple of lines each given libsecp256k1 is already a dev-dep (the test at line 559 shows the pattern). The first is the one that would actually catch a regression if someone later reorders or drops a field in v5_param_modification_digest — a field-order swap would keep every existing test green.
There was a problem hiding this comment.
Added all three domain-separation negatives: the golden signature is rejected for a changed path ID, nonce + 1, and a different committed authority.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖
| deposit.deposit_params.exclusive_relayer | ||
| ); | ||
|
|
||
| deposit.modification_rules.allow_output_amount = true; |
There was a problem hiding this comment.
This assignment is a no-op — the fixture already commits allow_output_amount = true (wire.depositInput ends ...0101), and nothing between here and line 541 clears it.
Harmless, but it reads as "now enable output-amount modification," which makes the improvement-rule assertion below look like it's testing a state transition it isn't. Either drop the line, or set it to false first so the flip is real.
There was a problem hiding this comment.
Removed the no-op assignment so the improvement-rule assertion no longer suggests a permission transition that did not occur.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
droplet-rl
left a comment
There was a problem hiding this comment.
Re-review of 2414952b — approving
All ten threads from the previous round are addressed: nine fixed, one deferred with a rationale I agree with. I re-verified the delta rather than taking the replies at face value.
Reproduced: cargo test -p svm-spoke --lib → 9 passed, matching the PR description (7 prior v5 tests + v5_errors_use_dedicated_range + the pre-existing test_id).
Verification of each fix
| Item | Status | How I checked |
|---|---|---|
| Permissionless-JIT divergence | ✅ | Documented at validate() and in the spec, with the explicit "route builders must not emit that EVM-only rule shape" requirement |
| Shared-vault invariant | ✅ | Both sites now state it; matches solana-v5/AGENTS.md verbatim, incl. stale approvals |
| Version-before-decode | ✅ | See below — wire-identical, and proven by the surviving round-trip assertion |
InvalidDispatchAuthority |
✅ | Distinct variant; bump caching deferred (see below) |
cargo test in CI |
✅ | Added to lint-and-check-generated, which already compiles the crate via cargo clippy and resolves declare_program! from the committed idls/ — so it will work without downloading SVM artifacts |
| Vector-scope wording | ✅ | Spec now says "cross-language self-consistency vectors, not an invocation of the EVM adapter" and records the 32-byte-vs-20-byte deposit-ID divergence |
| Error offset | ✅ | offset = 7000 + regression test |
FILL_STATUS_SEED |
✅ | All three sites; IDL-safe, see below |
| Domain-separation tests | ✅ | path_id, nonce+1, and wrong-authority negatives all present |
| No-op test line | ✅ | Removed; allow_output_amount is still true from the fixture at that point, so the improvement assertion is unchanged |
You were right and I was wrong on the error offset. My suggestion of #[error_code(offset = 1000)] // 7000.. assumed the offset was added to the 6000 base. It replaces it — anchor-attribute-error substitutes the supplied value for ERROR_CODE_OFFSET outright. offset = 7000 is correct, and the regression assertion is a good way to pin it.
The new decoder is wire-identical. Borsh lays V5AdapterInput out as version: u8 immediately followed by mode, so peeling data[0] and decoding the remainder as V5AdapterMode consumes exactly the same bytes. The golden assert_eq!(serialize(&input), input_bytes) round-trip still passing is the proof — a framing slip would have broken it. Empty input still maps to InvalidWireFormat, and both the truncated and full future-version payloads now report UnsupportedVersion.
The FILL_STATUS_SEED refactor does not perturb the IDL — worth stating explicitly, since these are the only three seeds = [CONST, ..] sites in the entire program (every other seed in svm-spoke is a bare byte-string literal), so there was no in-repo precedent to lean on. In anchor-syn-0.31.1/src/idl/accounts.rs::parse_seed, a syn::Expr::Path matching neither an instruction arg nor an account field falls through to IdlSeed::Const { value: AsRef::<[u8]>::as_ref(&FILL_STATUS_SEED).into() }, evaluated at IDL-build time. That yields the same five bytes the previous Expr::Lit arm produced, so the generated IDL, the Codama clients, and any PDA derivation downstream are unchanged.
Deferring the dispatch-authority bump caching is the right call. There is no V5 config account in Step 1 to persist a bump into, and inventing one before the account topology exists would be speculative. The derivation helpers returning their bumps keeps the option open, and derive_v5_fill_payer / derive_fill_status genuinely cannot share a cached bump anyway. Worth revisiting when the behavior step wires up a hot path.
Not re-run
The fixture, SvmSpoke.V5Foundations.ts, and SvmSpokeV5Vectors.t.sol are untouched in this delta, so their previously verified results (3 and 4 passing) still hold. I could not run cargo +nightly fmt, cargo clippy, or prettier — none of the three are installed in this environment — so those checks rest on CI.
One optional nit inline about the scope of the new error-code assertion. Nothing blocking. Nice turnaround.
|
|
||
| #[test] | ||
| fn v5_errors_use_dedicated_range() { | ||
| assert_eq!(u32::from(V5Error::InvalidWireFormat), 7_000); |
There was a problem hiding this comment.
Optional: this pins the base offset but not the variant ordering, and the ordering is the part that just moved.
InvalidDispatchAuthority was inserted between MissingAccount and InvalidAccountMutability in this very commit, which shifted the four variants after it by +1 (InvalidAccountMutability 7005→7006, ... ParamModificationNotAnImprovement 7008→7009). Harmless now — V5Error is unreleased and nothing decodes these yet — but once Step 2 ships and off-chain tooling starts matching on numeric codes, the same mid-enum insertion becomes a silent breaking change that this assertion would not catch.
Pinning both ends costs one line and turns any future reordering into a failing test rather than a client-side mystery:
assert_eq!(u32::from(V5Error::InvalidWireFormat), 7_000);
assert_eq!(u32::from(V5Error::ParamModificationNotAnImprovement), 7_009);There was a problem hiding this comment.
Fixed in 176cf8d4. The regression test now pins both ends of the initial V5 error range: InvalidWireFormat == 7000 and ParamModificationNotAnImprovement == 7009. The full svm-spoke library suite remains green at 9/9.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
15eb20d to
5424fd7
Compare
Summary
Linear: ACP-184
Stacked on #1495.
Testing
cargo +nightly fmt --all -- --checkcargo test -p svm-spoke --lib(9 passed)yarn prettier --check programs/svm-spoke/V5_ADAPTER_SPEC.md programs/svm-spoke/fixtures/v5_adapter_v1.json test/evm/foundry/local/SvmSpokeV5Vectors.t.sol test/svm/SvmSpoke.V5Foundations.tsyarn ts-mocha -p ./tsconfig.json -t 1000000 test/svm/SvmSpoke.V5Foundations.ts(3 passed)yarn test-evm-foundry -- --match-contract SvmSpokeV5VectorsTest(4 passed)Scope
This PR establishes Step 1 foundations only. It does not expose a live deposit or fill entrypoint.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖