Skip to content

feat(svm): add V5 source deposits - #1536

Open
Reinis-FRP wants to merge 5 commits into
reinis/acp-184-step-1-foundationsfrom
reinis/acp-184-step-2-source-deposit
Open

feat(svm): add V5 source deposits#1536
Reinis-FRP wants to merge 5 commits into
reinis/acp-184-step-1-foundationsfrom
reinis/acp-184-step-2-source-deposit

Conversation

@Reinis-FRP

Copy link
Copy Markdown
Contributor

Summary

  • add the Gateway-authenticated adapter_execute_across_v5 entrypoint with source Deposit enabled and Fill reserved fail-closed
  • resolve literal or vault-balance input amounts, authenticate signed JIT changes, derive the V5 deposit ID, and reuse the shared deposit validation/transfer/event core
  • resolve and validate dynamic mint, token-program, Gateway-vault, SpokePool-vault, and source-delegate accounts with static Anchor-equivalent checks
  • add a test-only mock Gateway and source-deposit coverage for dynamic amounts, JIT domains, wrong accounts, insufficient allowance, malformed calls, events, and atomic rollback
  • update the V5 adapter specification for the enabled source-deposit behavior

Part of ACP-184. This is Step 2 of the implementation plan and is stacked on #1535.

Validation

  • cargo +nightly fmt --all -- --check
  • git diff --check 5424fd77..HEAD
  • cargo check -p svm-spoke
  • cargo test -p svm-spoke --lib (9 passed)
  • direct cargo build-sbf for svm-spoke
  • yarn prettier --check programs/svm-spoke/V5_ADAPTER_SPEC.md test/svm/SvmSpoke.V5Source.ts
  • repository pre-commit hook

The focused validator suite could not be started locally because Anchor CLI 0.31.1 generates --bind-address 0.0.0.0, which the installed Solana validator 4.1.2 rejects. The SBF build itself passed; CI should run the validator test with the repository-supported toolchain.


Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖

@linear

linear Bot commented Aug 28, 2026

Copy link
Copy Markdown

ACP-184

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
@Reinis-FRP
Reinis-FRP force-pushed the reinis/acp-184-step-2-source-deposit branch from 9b51cab to ea4f377 Compare August 28, 2026 13:18
@Reinis-FRP
Reinis-FRP requested a review from droplet-rl August 28, 2026 13:26

@droplet-rl droplet-rl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessment

The structure here is good. Refactoring _deposit to take a DepositAccounts struct + DepositId enum instead of a concrete Context<Deposit> is the right way to share the validation/transfer/event core, and it preserves the existing ordering (validate → transfer → assign id). Resolving every remaining account by derived key rather than position (find_v5_account) is the correct instinct for a Gateway-dispatched entrypoint, and the mint/token-program/vault/authority checks do faithfully mirror the static Anchor constraints on Deposit. Fail-closed Fill, paused_deposits, and the JIT domain binding all look right.

Verified locally on ea4f3770:

  • cargo check -p svm-spoke -p mock-gateway — clean
  • cargo test -p svm-spoke --lib — 9 passed, as claimed
  • I also confirmed the ABI concern is not a problem: svm_spoke::v5::gateway_adapter_discriminator() == svm_spoke::instruction::AdapterExecuteAcrossV5::DISCRIMINATOR. It holds today, but nothing pins it — see the inline note.

Blocking

  1. The source-delegate allowance is never bound in-program. require_v5_delegate_allowance and V5Error::InsufficientDelegateAllowance shipped in Step 1 and are now unreachable — the only enforcement is SPL's own insufficient funds. Because the source delegate is a static program-wide PDA (unlike the per-deposit ["delegate", seed_hash] PDA everywhere else in this program), a residual allowance left on a shared Gateway vault by a partially-settled tape is directly spendable by the next permissionless Gateway path — with an attacker-chosen depositor/recipient, since neither is authenticated here. source is already deserialized, so binding delegate/delegated_amount costs almost nothing. Details inline.

  2. The Token-2022 gate is a two-entry denylist in a component whose stated design is fail-closed. PermanentDelegate (mint admin can drain the SpokePool vault post-deposit) and any extension Token-2022 adds later pass silently.

  3. The new branches have no validator coverage. UnsupportedTokenExtension — a brand-new error and code path — is never exercised; nor are DepositsArePaused, ResolvedInputAmountBelowCommitted, or load_token_account's authority check on this path. The one allowance test asserts on an SPL log substring.

Since the validator suite didn't run locally, CI green is a hard prerequisite here — and note buildIdl.sh/buildSolanaVerify.sh both iterate programs/* unconditionally, so mock-gateway is now in the IDL-generation and verified-build paths too. That's the first thing to confirm.

Non-blocking

Nits on redundant checks, the seed right-alignment trick, the missing ### Required Accounts: doc block, and the published-artifact side effects of mock-gateway are inline. The V5_ADAPTER_SPEC.md claim that a residual allowance "is not privileged" is the one doc statement I'd push back on.

let source_delegate_info = find_v5_account(remaining_accounts, &source_delegate, false)?;

// Together with the canonical addresses above, these checks mirror the corresponding static mint and
// associated-token constraints. Delegate authorization and allowance are enforced by transfer_checked.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking. "Delegate authorization and allowance are enforced by transfer_checked" is true but leaves require_v5_delegate_allowance and V5Error::InsufficientDelegateAllowance (both added in #1535) as dead code, and it gives up a cheap invariant.

The concern is the static delegate. Everywhere else in this program the delegate PDA is ["delegate", derive_seed_hash(all deposit params)], so an approval is cryptographically bound to exactly one deposit's parameters. V5_SOURCE_DELEGATE_SEED is a single constant seed, so any allowance standing on a Gateway vault for this PDA is spendable by any adapter_execute_across_v5 call — and params.depositor / params.recipient are unauthenticated wire fields.

Concretely: tape A approves 1,000,000 and spends 500,000, leaving 500,000 of allowance and 500,000 of balance on the shared per-mint vault. An attacker submits a new Gateway path with Literal mode, input_amount = 500_000, depositor/recipient = themselves. No APPROVE needed — the residual covers it. They get a valid FundsDeposited with the V5 magic prefix, funded by someone else's residual.

V5_ADAPTER_SPEC.md puts the burden on Gateway ("the continuing tape must leave no residual balance… Gateway does not currently enforce this"), but SpokePool can close it locally for ~free — source is already deserialized on the line below:

let source = load_token_account(gateway_vault_info, &token_program_id, &input_token, &gateway_vault_authority)?;
require_keys_eq!(
    source.delegate.ok_or_else(|| error!(V5Error::InsufficientDelegateAllowance))?,
    source_delegate,
    V5Error::InvalidTokenAccount
);

and then in execute_v5_deposit, after resolve_v5_input_amount:

require_v5_delegate_allowance(source.delegated_amount, input_amount)?;

That alone gets you the dedicated error and a non-brittle test. To actually kill the residual-allowance path you'd want exact consumption (delegated_amount == input_amount), which makes each Gateway APPROVE single-use from the SpokePool's side. Is there a reason the Gateway tape can't approve exactly the resolved amount? If exact-match is too strict for real tapes, please say so in the spec rather than asserting the residual is harmless.

(load_v5_deposit_accounts would need input_amount threaded in, or return source.delegated_amount alongside source.amount.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 669bd80. The loader now binds source.delegate to v5_source_delegate, and execution requires delegated_amount >= input_amount with the dedicated InsufficientDelegateAllowance error.

I intentionally did not require exact equality. The EVM adapter ultimately relies on ERC20 transferFrom, which accepts sufficient or maximum allowance while pulling exactly the requested amount; this path now preserves that behavior. Exact equality would reject valid maximum approvals without securing residual Gateway custody, because any balance left in the shared Gateway vault is already movable by a later committed Gateway TRANSFER. The spec now states that invariant explicitly. Validator coverage includes a 900k approval for a 750k pull, literal under-approval, and a bips-resolved allowance overshoot.


Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖

.get_extension_types()
.map_err(|_| error!(V5Error::InvalidTokenAccount))?;
require!(
!extensions.contains(&ExtensionType::TransferFeeConfig) && !extensions.contains(&ExtensionType::TransferHook),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking. A two-entry denylist is the wrong polarity for a gate in a module whose whole stated design is fail-closed. Extensions that pass today but arguably shouldn't:

  • PermanentDelegate — the mint's permanent delegate can move tokens straight back out of the SpokePool vault after the deposit event is emitted. That's the same class of loss as a transfer fee, but silent.
  • DefaultAccountState (frozen) / Pausable — fail at transfer today, so no accounting break, but they can strand the SpokePool vault later.
  • Anything Token-2022 adds after this ships is accepted by default. That's the part that rots.

Suggest inverting to an allowlist of extensions known to be inert for a vault-to-vault transfer_checked (metadata, metadata pointer, close authority, …) and rejecting the rest with UnsupportedTokenExtension. Same line count, and new extensions then fail closed instead of open.

I'll note the V3 deposit path is looser than this (it accepts transfer-fee mints), so this is already a net improvement — but since you're writing the gate anyway, an allowlist is strictly better and the spec section ("Transfer-fee mints are excluded… Transfer hooks remain disabled") reads as if it already is one.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 669bd80 by inverting the Token-2022 check to a fail-closed allowlist. Only mint-close authority and metadata/group pointer or data extensions are accepted; transfer fees, transfer hooks, permanent delegates, default account state, and unknown future extensions fail with UnsupportedTokenExtension. Unit coverage pins both accepted and rejected examples, and validator coverage exercises transfer-fee and transfer-hook rejection plus a plain Token-2022 happy path.


Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖

Comment thread test/svm/SvmSpoke.V5Source.ts Outdated
execute(input, Buffer.alloc(0), 1_000_000n, false, sourceDelegate, gatewayVault),
"MissingAccount"
);
await expectError(execute(input, Buffer.alloc(0), deposit.inputAmount - 1n), "insufficient funds");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking (coverage). Two things:

  1. Asserting on "insufficient funds" couples the test to spl-token's log string. If you add the explicit require_v5_delegate_allowance call from my other comment, this becomes "InsufficientDelegateAllowance" and stops being brittle.
  2. Branches added in this PR with zero validator coverage:
    • UnsupportedTokenExtension — brand-new error, brand-new reject_unsupported_mint_extensions path, never exercised. Needs a Token-2022 mint created with TransferFeeConfig (and ideally one with TransferHook) plus a plain Token-2022 mint proving the happy path still works. This is the gap I'd most want closed before merge — the function is currently unreachable-in-tests code guarding fund safety.
    • DepositsArePaused via the V5 entrypoint (the check moved from an Anchor constraint to a hand-rolled require!, so it's no longer structurally guaranteed).
    • ResolvedInputAmountBelowCommitted on-chain — unit-tested in v5.rs, but the wiring of source.amount into resolve_v5_input_amount is not.
    • load_token_account's account.owner == authority check. Both current wrong-account cases short-circuit at MissingAccount, so the authority assertion never fires. Passing a same-mint ATA owned by an unrelated authority at the derived spoke-vault key isn't constructible — but a wrong-state ATA is, if you can reach a second initialized state.

Also worth a case for bips mode where the resolved amount exceeds the granted allowance, since that's the exact overshoot the shared-vault design makes possible.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 669bd80. The allowance assertions now use InsufficientDelegateAllowance; coverage was added for bips overshoot, paused deposits, the committed dynamic-amount floor, transfer-fee and transfer-hook rejection, and a successful plain Token-2022 transfer. The existing over-approval case also proves the adapter pulls only the resolved amount.

I kept the token-account authority check as defense in depth. Because the account key must first equal the canonical ATA derived from that same authority, a supplied wrong-state or wrong-authority ATA necessarily has a different key and fails MissingAccount; reaching the authority branch with a live same-key account would require the owning PDA to authorize an authority mutation. I avoided adding an unsafe mock-only mutation just to manufacture that otherwise-unconstructible state.


Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖

require_gateway_dispatch_authority(&ctx.accounts.dispatch_authority)?;
match decode_v5_adapter_input(&input)?.mode {
V5AdapterMode::Deposit(deposit) => execute_v5_deposit(ctx, ctx_values, deposit, &jit_data),
V5AdapterMode::Fill(_) => return err!(V5Error::UnsupportedMode),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return is redundant — this match is the function's tail expression, and the sibling arm returns the value directly. V5AdapterMode::Fill(_) => err!(V5Error::UnsupportedMode), reads consistently with the Deposit arm.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 669bd80: removed the redundant return, so the Fill arm is now the match tail expression.


Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖

V5Error::InvalidTokenAccount
);
let token_program = find_v5_account(remaining_accounts, &token_program_id, false)?;
require!(token_program.executable, V5Error::InvalidTokenAccount);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two redundancies here, both cheap to drop (repo style leans hard on succinctness):

  • token_program.executable — you already required token_program_id to be anchor_spl::token::ID or token_2022::ID on the line above. Both are executable by construction; the account at that address can't be anything else.
  • load_mint re-checks *info.owner == *token_program (line 144), but token_program_id was derived from mint_info.owner on line 105. That comparison can never fail.

The load_token_account owner check is the one that earns its keep — keep that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 669bd80: removed both the redundant executable check and the mint-owner comparison while retaining the token-account owner, mint, and authority validation.


Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖

}

/// Executes one Gateway-authenticated Across V5 adapter branch. Wire version 1 enables source deposits; the
/// reserved Fill discriminant fails closed until its destination behavior is enabled.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every other handler in this #[program] block carries a ### Required Accounts: / ### Parameters: doc block. This one is the strongest candidate for it in the whole program: the accounts aren't in the Accounts struct at all, they're key-resolved out of remaining_accounts, so the doc comment is the only place an integrator can learn what to pass.

Worth listing: dispatch_authority (signer, Gateway PDA), state, then the remaining set — input mint, its token program, Gateway vault ATA (writable), pre-created SpokePool vault ATA (writable), ["v5_source_delegate"]. The spec has this; the header doesn't, and CLAUDE.md asks for docs in the same change.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 669bd80. The entrypoint now documents all static accounts, the complete unordered remaining-account set and writability, PDA derivations, and each wire/JIT parameter.


Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖

transfer_checked(cpi_context, amount, mint.decimals)
let bump_seed = [bump];
// Reuse the same offset after appending the bump, without allocating a seed vector.
let signer_seeds = [delegate_seeds[0], delegate_seeds[1], bump_seed.as_ref()];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The right-alignment trick is correct — I traced both arms (start=0[b"delegate", hash, bump], start=1[seed, bump]) and they match find_program_address — but it costs a fair bit of readability to avoid one small Vec, and the [&[], *seed] placeholder is the kind of thing that invites a wrong-index edit later.

More practically: Step 3 needs derive_v5_fill_payer(submitter), which is a two-seed PDA with a runtime (non-'static) component. Neither FunctionSeed(&'static [u8]) nor the fixed-2 array accommodates that, so this shape gets reworked one PR from now. Might be worth landing the general form here instead — e.g. take seeds: &[&[u8]] and build the signer array once — so Step 3 doesn't have to re-touch a fund-moving helper.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kept this unchanged for now to avoid broadening the deposit-focused diff. The payer PDA is not a token-transfer delegate and will not use this helper; the next relevant token delegate is the static fill function seed, which still fits FunctionSeed. When fill is refactored in the follow-up step, both signed transfer call sites can be collapsed around the final shared shape instead of introducing an intermediate generalization here.


Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖

Comment thread programs/svm-spoke/V5_ADAPTER_SPEC.md Outdated
against shared live state, and the continuing tape must leave no residual balance or stale approval that a later
permissionless execution could consume. Gateway does not currently enforce this net-zero settlement invariant.
against shared live state, and the continuing tape must leave no residual balance. Gateway does not currently enforce
this net-zero settlement invariant. A residual delegate allowance is not privileged—the next permissionless

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A residual delegate allowance is not privileged—the next permissionless APPROVE may replace it—but the SpokePool never delegates its own vault.

I'd push back on this. "May be replaced" isn't a safety property — nothing requires it to be replaced, and until it is, the residual is spendable by any Gateway path that routes into this adapter with attacker-chosen depositor/recipient (see my comment on v5_adapter.rs). Combined with the residual balance that the paragraph above concedes Gateway doesn't enforce away, the two together are a live path, not a theoretical one.

The second clause is accurate and worth keeping — the SpokePool vault is never delegated, so the exposure is bounded to whatever is sitting in the shared Gateway vault. But the framing should be "this depends on a Gateway invariant that is currently unenforced," not "not privileged." If you add the explicit allowance binding, this paragraph gets to make a much stronger claim instead.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 669bd80. The spec no longer calls residual allowance harmless: it documents the explicit delegate binding and sufficient-allowance check, explains why exact equality is intentionally not required for EVM transferFrom parity, and states that residual shared-vault balance remains a Gateway custody invariant because a later committed Gateway TRANSFER can move it. It also retains the distinction that the SpokePool vault itself is never delegated.


Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖

use anchor_spl::token_interface::{self, ApproveChecked, Mint, TokenAccount, TokenInterface};
use svm_spoke::program::SvmSpoke;

declare_id!("34trBszXuqhRjWaMxXWsunJNmyUsBvDNPxAwTzbPTm4p");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth a second look at the blast radius of putting this under programs/, because two build scripts iterate programs/* unconditionally — not gated on IS_TEST:

  • scripts/svm/buildHelpers/buildSolanaVerify.sh will now run a full solana-verify build for mock_gateway on yarn build-verified, i.e. a reproducible-build artifact for a mock, plus the CI time.
  • buildIdl.sh + generateSvmAssets.sh copy every target/idl/*.json into src/svm/assets/idl/ and auto-generate the barrel, so mockGatewayIdl and MockGatewayAnchor become public exports of the published @across-protocol/contracts package.

programs/test already does this, so it's a precedent — but this one declares 34trBszXuqhRjWaMxXWsunJNmyUsBvDNPxAwTzbPTm4p, the real Gateway program ID. Shipping a mock IDL keyed to a live mainnet address is a meaningfully worse footgun than shipping testIdl: a consumer wiring mockGatewayIdl against that address gets an execute_adapter entrypoint that doesn't exist there. Suggest an exclusion list in buildIdl.sh/generateSvmAssets.sh (or drop the ID collision, though I assume it's load-bearing for the dispatch_authority derivation — which it is).

Since the validator suite didn't run locally, the IDL-generation job is also the most likely place this PR breaks CI: anchor idl build --program-name mock_gateway on a program crate that depends on another Anchor program crate is a known-fussy combination. Please confirm upload-svm-artifacts is green (it only runs on cache miss, and a new program should miss).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 669bd80. Production builds now exclude mock_gateway from verified artifacts, generated IDLs, generated TypeScript types, and published SVM assets; IS_TEST=true still includes it for validator execution, and stale mock artifacts are removed before production asset generation.

The fresh CI run on this head confirms both relevant paths: Upload SVM artifacts passed, including production SVM artifact generation/upload, and Lint and Check Generated Files passed with no generated-file drift.


Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖

AccountMeta::new_readonly(ctx.accounts.token_program.key(), false),
AccountMeta::new_readonly(ctx.accounts.source_delegate.key(), false),
],
data: svm_spoke::instruction::AdapterExecuteAcrossV5 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice — using svm_spoke::instruction::AdapterExecuteAcrossV5 { .. }.data() means the mock encodes via Anchor's own discriminator rather than hand-rolling it.

That does leave one thing unpinned, though: the mock proves the Anchor discriminator round-trips, and the v5.rs unit test proves gateway_adapter_discriminator() matches the golden fixture, but nothing asserts the two are the same value. They are today — I checked by adding this temporarily and it passes:

assert_eq!(
    svm_spoke::v5::gateway_adapter_discriminator(),
    svm_spoke::instruction::AdapterExecuteAcrossV5::DISCRIMINATOR,
);

The coupling is a string: GATEWAY_ADAPTER_EXECUTE_V5_PREIMAGE = b"global:adapter_execute_across_v5" has to track the Rust fn name. Rename adapter_execute_across_v5 and the real Gateway silently dispatches to a discriminator this program no longer answers to — with the fixture test still green, because the fixture pins the constant, not the entrypoint. Now that the entrypoint exists, that two-line assertion closes the loop cheaply; mock-gateway is the natural home since it already depends on both.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 669bd80. mock-gateway now asserts that gateway_adapter_discriminator() equals Anchor's AdapterExecuteAcrossV5::DISCRIMINATOR; cargo test -p mock-gateway --lib passes with both tests.


Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖

@droplet-rl

Copy link
Copy Markdown
Contributor

🔎 View trace

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
@Reinis-FRP
Reinis-FRP requested a review from droplet-rl August 28, 2026 14:10

@droplet-rl droplet-rl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review of 669bd801 ("fix(svm): address V5 source review")

All three blocking items from my previous review are resolved, and one of them with a better answer than I proposed. Approving.

Verified locally on 669bd801 (base still 5424fd77):

  • cargo test -p svm-spoke --lib — 10 passed (up from 9; the new allowlist test is in the CI-covered crate)
  • cargo test -p mock-gateway --lib — 2 passed
  • rustfmt.toml sets only reorder_imports, so the split use anchor_lang::{prelude::*} / use anchor_lang::solana_program::... statements are already in canonical order — no cargo +nightly fmt --check failure
  • Traced the new IS_TEST guards against .github/workflows/pr.yml: test-svm-verified sets IS_TEST: true on the "Create verified build" step, so target/deploy/mock_gateway.so still exists for anchor test --skip-build. upload-svm-artifacts does not set it, so the mock IDL is correctly excluded from published assets. The guards land on the right side of both paths.

Item-by-item

1. Delegate allowance (was blocking) — resolved, and I accept the pushback. require_v5_delegate_allowance(source.delegated_amount, input_amount) plus the source.delegate == Some(v5_source_delegate) binding make the dedicated error reachable and the test now asserts on it instead of an spl-token log string.

You declined exact-consumption, and the spec rationale is the right call: "Any residual Gateway-vault balance is already movable by a later committed Gateway TRANSFER; exact allowance would not replace that custody invariant." That's correct and it's a better framing than mine — Gateway holds the vault authority, so residual balance is Gateway-custodied regardless of what the SpokePool asserts about the allowance. Requiring equality would close one route to a residual that remains reachable by several others, at the cost of breaking max-approval tapes. The net-zero invariant is genuinely Gateway's to enforce, and the spec now says so plainly instead of calling the residual "not privileged" — that was my actual ask.

2. Token-2022 gate (was blocking) — resolved. Inverting to is_supported_v5_mint_extension is exactly right, and the allowlist choices hold up: ScaledUiAmount, InterestBearingConfig, ConfidentialTransferMint, and PermanentDelegate all now fail closed, which the old two-entry denylist let through. MintCloseAuthority is safe to permit — close requires zero supply, so there's nothing to lose when it's exercisable. Nice touch adding the plain-Token-2022 happy-path assertion; that's what stops an allowlist from silently over-rejecting.

3. Test coverage (was blocking) — resolved. UnsupportedTokenExtension for both TransferFeeConfig and TransferHook, DepositsArePaused, ResolvedInputAmountBelowCommitted, and the bips-overshoot allowance case are all now exercised through the mock. Building the extended mints by hand with getMintLen is the right way to do it.

4–7. Nits — all resolved. Redundant executable / load_mint owner checks dropped (both were provably unreachable, since token_program_id derives from mint_info.owner), return err! cleaned up, the ### Required Accounts: block added and accurate about order-independence, build scripts gated.

One follow-up (non-blocking, one line)

The discriminator-pin test landed in mock-gateway, but CI's "Test rust" step runs cargo test -p svm-spoke --lib — package-scoped, so that assertion never executes in CI. Details and a verified one-line relocation inline.

I did not re-run the validator suite (no Anchor CLI here), so test-svm-verified remains the gate on the eight TS cases.

Comment thread programs/mock-gateway/src/lib.rs Outdated
use super::*;

#[test]
fn adapter_discriminator_matches_gateway_abi() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the assertion I asked for and it passes (cargo test -p mock-gateway --lib → 2 passed). One problem with the placement, though: CI's "Test rust" step is

- name: Test rust
  run: cargo test -p svm-spoke --lib

-p svm-spoke is package-scoped, so this test never runs in CI. The guard is inert exactly where it needs to be live — the failure mode it protects against (renaming adapter_execute_across_v5 and silently desyncing GATEWAY_ADAPTER_EXECUTE_V5_PREIMAGE from the real Gateway's dispatch) would land green.

Cheapest fix is to move it into svm-spoke's own test module, where CI already covers it. I verified this compiles and passes from inside v5.rs::tests:

#[test]
fn adapter_discriminator_matches_gateway_abi() {
    use anchor_lang::Discriminator;
    assert_eq!(
        gateway_adapter_discriminator(),
        crate::instruction::AdapterExecuteAcrossV5::DISCRIMINATOR,
    );
}

(crate::instruction:: resolves — Anchor emits pub mod instruction at the crate root, beside the #[program] module.) That also drops the anchor_lang::Discriminator import from the mock entirely.

Alternative is widening CI to cargo test --workspace --lib, which would pick up mock-gateway's test_id too and guard future mock-side tests — but that's a CI change for one assertion, so relocating seems better. Either way, worth doing before merge since it's the test that keeps the cross-repo ABI honest.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in ef66daf. The assertion now lives in svm-spoke::v5::tests, imports Discriminator there, and compares the computed Gateway discriminator with crate::instruction::AdapterExecuteAcrossV5::DISCRIMINATOR. The duplicate mock-side test was removed. cargo test -p svm-spoke --lib passes, so the guard now runs in the existing CI Test rust command.


Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖

let mint = load_mint(mint_info)?;
let source = load_token_account(gateway_vault_info, &token_program_id, &input_token, &gateway_vault_authority)?;
load_token_account(spoke_vault_info, &token_program_id, &input_token, &state)?;
require!(source.delegate == COption::Some(source_delegate), V5Error::InvalidTokenAccount);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good defensive addition, and the COption comparison is the correct way to express it (spl-token clears delegated_amount to 0 on Revoke, so there's no stale-amount path when delegate is None).

Worth knowing it's currently unreachable in tests: mock_gateway::execute_adapter always approve_checkeds to source_delegate before the CPI, so source.delegate is always Some(source_delegate) by the time the adapter reads it — even at approval_amount = 0, which sets the delegate and trips the allowance check on the next line instead. The delegate = owner case in the wrong-accounts test short-circuits at find_v5_account with MissingAccount, well before this line.

Not worth contorting the mock for on its own, but if Step 3 adds a mock flag anyway (e.g. approve-to-arbitrary-delegate, or skip-approve so a prior standalone Approve instruction sets a different one), this check is a free extra case at that point. Purely defense-in-depth as things stand — the allowance check covers every practically reachable input.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I kept this as defense in depth and did not add a mock-only flag solely to manufacture the otherwise unreachable branch. The current validator cases cover the practically reachable allowance failures; when the Step 3 mock gains another approval mode, I will add the wrong-active-delegate case there at essentially no additional mock complexity.


Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖

fn is_supported_v5_mint_extension(extension: &ExtensionType) -> bool {
matches!(
extension,
ExtensionType::MintCloseAuthority

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allowlist contents check out. Walking the Token-2022 mint extensions that are now rejected and weren't before: PermanentDelegate (mint admin could move tokens back out of the SpokePool vault after the event was emitted — the one I was most concerned about), ScaledUiAmount and InterestBearingConfig (both reinterpret amounts against the transfer_checked decimals assertion), ConfidentialTransferMint, DefaultAccountState, Pausable, NonTransferable. That's the right set.

MintCloseAuthority is fine to permit — CloseAccount on a mint requires zero supply, so it's only exercisable when there's nothing in any vault to lose.

One note for whoever revisits this: the allowlist covers mint extensions only, which is correct for what get_extension_types returns here. The account-side extensions that could matter on the two vaults (CpiGuard on the source blocking delegate-signed CPI transfers, MemoTransfer on the destination) all fail the transfer rather than corrupting accounting, so they're safe-by-default — but that's an implicit property, not something this function establishes. Might be worth a sentence in V5_ADAPTER_SPEC.md so a future reader doesn't assume this gate covers account extensions too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in ef66daf. The spec now states explicitly that the allowlist covers mint extensions only and records the account-side behavior relevant to this path: source CPI guard and destination memo requirements cause the token transfer to fail rather than changing deposit accounting.


Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖

@droplet-rl

Copy link
Copy Markdown
Contributor

🔎 View trace

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
@Reinis-FRP
Reinis-FRP marked this pull request as ready for review August 28, 2026 14:34
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants