Skip to content

refactor(svm): collapse deposit and fill execution to V5 - #1564

Open
Reinis-FRP wants to merge 49 commits into
epic-v5/svmfrom
reinis/svm-v5-core-simplification
Open

Reinis-FRP wants to merge 49 commits into
epic-v5/svmfrom
reinis/svm-v5-core-simplification

Conversation

@Reinis-FRP

@Reinis-FRP Reinis-FRP commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

After #1562 retired the V4 deposit/fill entrypoints, collapse the remaining shared execution code into a V5-only adapter. Deposit and fill execution live in separate private modules, with shared mint/token-program and canonical Gateway-vault validation. Fill completion is inline; relay hashing and event emission reuse the validated message hash. Remove unreachable legacy branches and organize fill-status lifecycle, historical cleanup, account lookup and test support by responsibility.

Every fill now uses SPL transfer_checked, including self-transfers into the Gateway vault (merged from #1574). This checks balance, frozen state and delegate authority/allowance. A self-transfer does not debit funds, consume allowance or prove delivery: the committed tape must still enforce the total actual fill obligations and full terminal consumption. Deposits and fills both rely on SPL for delegate validation. The mint-extension allowlist continues to reject PermanentDelegate; new external/self-transfer fill regressions exercise that boundary with the fill PDA as permanent delegate and no ordinary approval.

Builder and compatibility changes:

  • Use v5_deposit_delegate for deposit approvals and the supplied delegate account: 8DWnJFMBTSDYWsUUSqna9tx9LJbU1yUfq7jTiPJDf8sX, bump 252.
  • Every fill, including a self-transfer, needs the v5_fill_delegate account and sufficient approval: D27f3mVXRL6N3bgja49UWLQu7kt57sy1aZYy7ZEwdxn1, bump 252. Missing/wrong approvals fail with SPL OwnerMismatch (0x4); insufficient allowance fails with InsufficientFunds (0x1). Deposits now report these SPL errors too.
  • V5 errors occupy 8000–8014 and calldata errors 9000–9006. InsufficientVaultBalance (8015) was removed; existing CommonError/SvmError assignments are retained. The spec documents Anchor 0.31.1's error-export limitations.
  • Rename CloseFillPda.signer and FillStatusAccount.relayer to rent_recipient (rentRecipient in TypeScript). Instruction encoding, account order/privileges, serialized layouts, event schemas and payload codecs remain unchanged. Old-IDL clients remain compatible with the renames; consumers adopting the new IDL must update property names. NotRelayer, its message and code 7002 are unchanged. Deposit identities, replay protection, payer/rent behavior and historical cleanup are preserved.

Release requirement: publish the updated IDL and generated clients with the audited V5 release under a new package version in the planned v6 major release. Any intervening prerelease publication must also use a new prerelease version. The rentRecipient renames require consumer source updates when adopting the new clients; existing clients remain binary-compatible with these renames.

Targets epic-v5/svm, which includes #1562. Constant-PDA transfer optimization remains in #1573.

Cleanup follow-up at c44014a7: the reference script scans fill-status accounts by rent recipient, using --submitter for V5 or --relayer for legacy relayers/requesters. Comments now cover slow-fill requesters, and the docs distinguish existing legacy compatibility from future backend V5 integration.

Validation for this follow-up: 9 focused validator tests passed against the existing verified test binary, including actual script-function discovery/closure for V5 and the historical requested-account fixture. TypeScript, CLI argument checks and commit hooks passed. The successful CLI entrypoint and concurrent-close/error-recovery branches were not exercised end-to-end. On-chain changes in this follow-up are comments only.

Earlier validation at e0366862:

  • Guarded deterministic solana-verify test-feature builds for Spoke and Mock Gateway passed using the Solana 2.2.1 image, with no stack-overflow diagnostics.
  • Rust unit tests: 16/16 with the test feature.
  • Focused validator tests: 8/8, covering payer lifecycle, historical requested-status decoding/reclaim, external fill/event/reclaim and downstream rollback.
  • Compatibility comparison confirmed that only the intended IDL names/docs changed: old/new Anchor and new Codama close instructions encode identically, historical account bytes round-trip, and errors/discriminators/account privileges are unchanged.
  • Production IDLs/clients and test IDLs regenerated; test TypeScript checking, a separate typecheck of the close-PDA script, formatting and diff checks passed under Node 22. Commit hooks passed.
  • The full real-Gateway suite was not rerun for this rename. Earlier validation at 7dbc281f passed all 25 mock-Gateway deposit/fill tests, 3 focused real-Gateway approval tests, and 16 Rust tests in each production/test configuration. New-head CI is tracked in the PR checks.

Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖

@Reinis-FRP
Reinis-FRP force-pushed the reinis/svm-v5-core-simplification branch 2 times, most recently from 3edfabe to 4d04090 Compare September 25, 2026 10:18
@Reinis-FRP
Reinis-FRP force-pushed the reinis/svm-v4-entrypoint-retirement branch from c41783c to 168eeac Compare September 25, 2026 10:20
@Reinis-FRP
Reinis-FRP force-pushed the reinis/svm-v5-core-simplification branch 2 times, most recently from dfceebf to 9835367 Compare September 25, 2026 10:44

@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.

Reviewed the full diff against reinis/svm-v4-entrypoint-retirement and verified locally. This is a clean, behavior-preserving collapse — approving with a few non-blocking notes.

What I verified

  • Builds and tests: cargo test -p svm-spoke --lib --features test → 16/16 pass, and cargo check produces no new unused/dead_code warnings (only pre-existing anchor-debug cfg noise).
  • Deposit delegate PDA: hardcoded_v5_authorities_match_canonical_pdas and pda_domains_match_golden_fixture both pass, so 8DWnJFMBTSDYWsUUSqna9tx9LJbU1yUfq7jTiPJDf8sX / bump 252 for ["v5_deposit_delegate"] is confirmed against both the live derivation and the updated v5_adapter_v1.json. The rename is fully applied — no v5_source_delegate / sourceDelegate / V5_SOURCE_DELEGATE references remain anywhere in Rust, TS, or docs. The Solidity vector test (SvmSpokeV5Vectors.t.sol) doesn't read /pdas/, so the fixture edit is safe cross-language.
  • Ordering preserved: deposit is still pause → JIT resolve → account load → amount resolve → output-token/quote/deadline/exclusivity → transfer → event. Fill is still pause → JIT decode → commitment/min-output → relay hash → account load → exclusivity → deadline → fill-status create → delivery → write_filled → event. Matches the pre-collapse _deposit/_fill sequence exactly, including that account loading already ran before the shared core.
  • number_of_deposits: V5 already used DepositId::Fixed, so dropping the Next variant is a no-op for state.
  • Message-hash reuse (1fc2179b): keccak::hash(&relay.message) is computed only after relay.message.len() == 64 is enforced, so it's identical to the old hash_non_empty_message result and saves a keccak. Correct.
  • Error-range swap: ERROR_CODES.md, V5_ADAPTER_SPEC.md, and compatibility.rs are all consistent at V5=8000–8015 / CallData=9000–9006, and there are no numeric error maps in TS to update.
  • Removed test coverage is genuinely redundant: FillsArePaused, NotExclusiveRelayer, ExpiredFillDeadline, RelayFilled, and in-place InvalidTokenAccount are all still pinned by SvmSpoke.V5Fill.ts / RealGateway.ts. The new in-place event assertions mirror the existing external-fill block, so the TS types are fine.
  • Stack frame: the #[inline(never)] guard on _fill is gone. I ran cargo build-sbf --features test and it emits no Stack offset ... exceeded max offset warnings, so this isn't a problem at current size (see inline note).

Notes

Nothing blocking. Three small things inline, plus one the diff view can't reach: programs/mock-gateway/src/lib.rs:226 still names the field source_delegate with the doc comment "The svm-spoke adapter authenticates the static source-delegate key." It's the one spot the rename missed — harmless (the test supplies the key), but worth renaming for consistency with the rest of the sweep.

Worth flagging for coordination rather than for this PR: the deposit delegate seed change is builder-breaking, so any Gateway route-builder or published integration material outside this repo needs the new approve target before enablement. The V5_ADAPTER_SPEC.md addition covers it well on the docs side.

use crate::{common::RelayData, error::CommonError};

pub fn get_relay_hash(relay_data: &RelayData, chain_id: u64) -> [u8; 32] {
pub fn get_relay_hash(relay_data: &RelayData, chain_id: u64, message_hash: &[u8; 32]) -> [u8; 32] {

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.

get_relay_hash used to own the empty-message convention internally (hash_non_empty_message → zeros for an empty message); now it splices in whatever the caller passes. Today that's exactly one call site and V5 enforces a 64-byte message first, so the result is identical — no issue as shipped.

But this is pub and glob-re-exported through utils::*, and the value it produces derives the fill-status PDA. A future caller that reaches for the obvious keccak::hash(&msg) on a possibly-empty message would silently produce a relay hash diverging from the EVM/legacy convention, with no compile-time signal. A one-line doc comment pinning the invariant ("message_hash must be keccak(message), or zeros when message is empty") would keep that cheap.

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.

Documented in 959567a: callers must supply keccak256(message), or zeros for an empty message, matching EVM's _hashNonEmptyMessage. The helper explicitly leaves this unchecked so the fill can reuse its already validated hash.

The same commit addresses the mock naming note: Mock Gateway now uses deposit_delegate consistently, including its generated test IDL. Account ordering is unchanged. The full verified test build and all 11 source-deposit validator tests passed.


Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖

Ok(mint.base.decimals)
}

fn is_supported_v5_mint_extension(extension: &ExtensionType) -> bool {

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.

Dropping the predicate unit test leaves this allowlist less covered than the PR description implies. The validator tests only exercise TransferFeeConfig and TransferHook, and only on the deposit path (SvmSpoke.V5Source.ts:464). After this change nothing pins:

  • rejection of PermanentDelegate and DefaultAccountState
  • acceptance of MintCloseAuthority / MetadataPointer (a typo narrowing the arm would fail closed silently, not loudly)
  • any extension behavior on the fill path's validate_v5_mint

This is a fail-closed security boundary and the test was six lines with no fixture cost. I'd keep token_2022_mint_extension_allowlist_is_fail_closed here rather than rely on the two validator cases.

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.

Keeping this predicate test removed. It has some regression value for the sampled variants, but it repeats selected arms of the allowlist without exercising mint decoding, extension enumeration, or whether deposit/fill invokes the validation. It also does not establish rejection of the full complement or future variants; that comes from the predicate's default rejection.

You're right that the existing validator coverage is narrower: it rejects TransferFeeConfig and TransferHook on deposits, and does not cover allowed extensions or extension behavior on fills. Those gaps are better addressed with real extended-mint tests through both paths than by restoring a second copy of the predicate's selected cases.


Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖

AdapterExecuteAcrossV5,
};

pub(super) fn execute_v5_fill<'info>(

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 #[inline(never)] on _fill came with an explicit rationale — "Preserve a separate SBF frame; inlining event construction can push stack-heavy fill handlers past the 4 KiB limit" — and inlining the core here removes both the attribute and the comment. execute_v5_fill is now a single-call-site pub(super) fn, so LLVM will very likely fold it into adapter_execute_across_v5, merging the JIT decode, the 10-field V5FillAccounts, and the ~400-byte FilledRelay into one frame.

I checked this rather than guess: cargo build-sbf --manifest-path programs/svm-spoke/Cargo.toml -- --features test produces zero Stack offset ... exceeded max offset warnings, so there's real headroom now that the V4 handlers and their large Anchor Accounts structs are gone. Not a bug.

The ask is just that the constraint stay written down somewhere — the SBF build only warns on overflow, so if a later change pushes this frame over, CI stays green and the failure shows up at runtime. A one-line comment noting the 4 KiB frame budget would preserve what the old attribute was documenting.

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 the shared build tooling on the base PR #1562, commit 097fcc1, and inherited here and by #1565. Local Anchor builds, each production/test solana-verify build, and the real-Gateway SBF builds now fail when compiler output contains stack-offset, stack-frame-size, or frame-overwrite diagnostics, even when the compiler exits successfully. The README documents this, and the build-helper cache key invalidates previously cached builds.

The guard has 10 passing regression cases covering stdout/stderr, split writes, negative offsets, alternate diagnostics, normal output and compiler failures; CI runs them. Full verified test builds passed for all five programs on both #1562 and this branch without stack-overflow diagnostics. The inlined fill remains unchanged.


Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖

@droplet-rl

Copy link
Copy Markdown
Contributor

🔎 View trace

@Reinis-FRP
Reinis-FRP force-pushed the reinis/svm-v5-core-simplification branch 2 times, most recently from 8c8812d to 959567a Compare September 25, 2026 15:05
@droplet-rl

Copy link
Copy Markdown
Contributor

🔎 View trace

@Reinis-FRP

Copy link
Copy Markdown
Contributor Author

@droplet-rl please recheck this PR with the latest review responses

@droplet-rl

Copy link
Copy Markdown
Contributor

Rechecking now. The rebase left all 11 previously reviewed commits unchanged (range-diff is identical), so I'm reviewing the new commit 959567a and the stack guard in base commit 097fcc1, then re-running the tests and the SBF build. Full results shortly.

@droplet-rl

Copy link
Copy Markdown
Contributor

I rechecked the PR at 959567ac. My earlier approval still stands, and the one thread still open isn't blocking.

The rebase onto the updated #1562 didn't touch the 11 commits I already reviewed: git range-diff shows them all unchanged. The only new commit is 959567ac.

Review threads

  • get_relay_hash invariant: resolved. The new doc comment states what callers must pass: keccak256(message), or [0; 32] when the message is empty, matching EVM _hashNonEmptyMessage. It also says why the function doesn't check this itself.
  • Mock Gateway naming: resolved. deposit_delegate is now used everywhere and account order is unchanged. No source_delegate, sourceDelegate, SOURCE_DELEGATE or "source-delegate" references remain in any .rs, .ts, .md or .json file.
  • Stack-frame guard: resolved by 097fcc1b on the base branch, which is better than the comment I asked for. I checked it directly:
    • With a fake stack-offset diagnostic and exit 0, runSbfBuild.sh exits 1.
    • Clean output exits 0.
    • A compiler failure keeps its own exit code (3), thanks to pipefail.
    • CI now runs the guard's own tests via yarn test-svm-build-guard.
  • Token-2022 predicate test: I accept keeping it removed. You're right that a test repeating a few allowlist entries doesn't check mint decoding or whether validation is actually called, and anything not on the list is rejected by default. The gap we agreed on is still there, though: nothing tests an allowed extension, and the fill path has no extension coverage at all. Could the real extended-mint tests on both paths go on a tracked follow-up so they don't get lost? That shouldn't hold up this PR.

Re-verified at 959567ac

  • cargo test -p svm-spoke --lib --features test: 16/16 pass.
  • Built svm-spoke and mock-gateway with cargo build-sbf --features test through the new guard: both exit 0 with no stack-offset diagnostics. The mock-gateway build confirms the rename compiles.

No code changes from me, so nothing was pushed.

@droplet-rl

Copy link
Copy Markdown
Contributor

🔎 View trace

@Reinis-FRP
Reinis-FRP marked this pull request as ready for review September 28, 2026 10:13
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-28T10:16:06.892090Z 959567a Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@Reinis-FRP
Reinis-FRP force-pushed the reinis/svm-v5-core-simplification branch from 959567a to c0c1ebd Compare September 28, 2026 11:04
@Reinis-FRP
Reinis-FRP force-pushed the reinis/svm-v4-entrypoint-retirement branch from 097fcc1 to c0bccd8 Compare September 28, 2026 11:04
@Reinis-FRP
Reinis-FRP force-pushed the reinis/svm-v5-core-simplification branch from c0c1ebd to 982580e Compare September 28, 2026 13:09
@Reinis-FRP
Reinis-FRP force-pushed the reinis/svm-v4-entrypoint-retirement branch from c0bccd8 to 5a788e3 Compare September 28, 2026 13:09
@Reinis-FRP
Reinis-FRP force-pushed the reinis/svm-v5-core-simplification branch from 0a6b5ab to bb37ba1 Compare October 5, 2026 07:48
Base automatically changed from reinis/svm-v4-entrypoint-retirement to epic-v5/svm October 5, 2026 08:02
Reinis-FRP and others added 2 commits October 5, 2026 08:07
The squashed #1562 base has the same tree as e7b52c9, the original parent of this branch. Preserve the V5 simplification tree while recording the new base ancestry, avoiding rewrites of dependent PRs.

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
* replace InPlace with self-transfer

Signed-off-by: Ihor Farion <ihor@umaproject.org>

* docs(svm): clarify self-transfer approval and delivery requirements

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* test(svm): tighten fill error checks and wrong-delegate coverage

Match exact token-program error codes, verify wrong-delegate rejection and atomic rollback through the real Gateway, and remove the unused InsufficientVaultBalance error. Update the delegate and self-transfer documentation.

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

* docs(svm): distinguish fill delegate and allowance errors

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

---------

Signed-off-by: Ihor Farion <ihor@umaproject.org>
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
Co-authored-by: Reinis Martinsons <reinis@umaproject.org>
@Reinis-FRP
Reinis-FRP force-pushed the reinis/svm-v5-core-simplification branch from 90efcbf to 0436c8b Compare October 5, 2026 09:49
@Reinis-FRP

Copy link
Copy Markdown
Contributor Author

@droplet-rl , please recheck now that #1574 simplification has been merged here

@droplet-rl

Copy link
Copy Markdown
Contributor

Rechecking at 0436c8b8. So far, the base-ancestry merge 095c442a keeps the reviewed tree byte-for-byte, the 14 commits I reviewed earlier are unchanged apart from rebase fixups in the docs, and the two new commits ahead of #1574 are documentation only. Next I'm reviewing #1574's self-transfer change in detail and rerunning the tests and builds.

@droplet-rl

Copy link
Copy Markdown
Contributor

I've finished reviewing #1574's code and tests. I'm now checking the SPL Token and Token-2022 self-transfer code paths against source, then rerunning the Rust tests and the guarded SBF builds.

@droplet-rl

Copy link
Copy Markdown
Contributor

The SPL Token 7.0.0 and Token-2022 6.0.0 sources match the self-transfer claims, and the Rust tests pass (16/16). I'm doing the guarded SBF builds and a last check of the documentation, then I'll post the full assessment.

@droplet-rl

Copy link
Copy Markdown
Contributor

I rechecked the PR at 0436c8b8, now that #1574 is merged in. #1574 makes the in-place fill strictly safer and leaves the delivery boundary unchanged, so my approval stands. One thing needs fixing before merge: the PR description is out of date. I also have two optional suggestions. I made no code changes.

Branch history

  • The ancestry merge 095c442a has the same tree as bb37ba1e, and the epic-v5/svm tip has the same tree as e7b52c96. So the merge records the new base without changing any code.
  • The 14 commits I reviewed earlier are unchanged apart from rebase fixups in the docs in bc4e8458.
  • 4fc564ff and bb37ba1e change documentation only. Both are accurate: derive_v5_deposit_id does not consume IDs, and number_of_deposits is frozen.

#1574: in-place delivery replaced by a self-transfer

  • One code path: Every fill now calls transfer_from for the exact output amount, from the authenticated Gateway vault to the authenticated recipient token account. The authority is v5_fill_delegate, looked up by its address in the loader and checked again against the derived PDA in transfer_from. V5FillAccounts is still built only inside load.
  • Self-transfer behavior matches the source. I checked process_transfer in SPL Token 7.0.0 and Token-2022 6.0.0:
    • A frozen account fails with AccountFrozen.
    • A balance below the amount fails with InsufficientFunds.
    • The authority must be the vault's delegate. Otherwise the program checks it against the vault owner (GATEWAY_VAULT_AUTHORITY) and fails with OwnerMismatch.
    • The allowance must cover the amount, but it isn't reduced when source and destination are the same account.
    • Both programs return Ok(()) before touching any balances.
  • Net effect: The in-place fill keeps the same balance assertion and now also rejects frozen vaults and requires an approval. That settles Faisal's frozen-account thread on-chain rather than by argument. It still doesn't debit or reserve anything, so the tape must still floor and consume the total owed across fills, and the spec says so.
  • Tests: Both token programs are covered:
    • no approval → 0x4
    • allowance too low → 0x1
    • balance too low with enough allowance → 0x1
    • frozen vault → 0x11
    • after a successful fill, the balance and allowance are unchanged
    • failures leave no fill-status account and charge no payer rent
  • Error removal: InsufficientVaultBalance was the last variant (8015), so no other code shifts. The docs and the compatibility test are updated.

Something the code now depends on: The explicit source.delegate == Some(V5_FILL_DELEGATE) check is gone from external fills too, so rejections now come from the token program. That's safe because the vault authority is checked. However, Token-2022 checks a mint's permanent delegate before the regular delegate, and that branch skips the allowance check. It stays unreachable only because token.rs rejects the PermanentDelegate extension. The old check only partly covered this anyway (it checked who the delegate was, not the allowance), but the allowlist is now the only protection. I'd add a fill-path PermanentDelegate rejection test to the extended-mint follow-up.

Needs fixing: the PR description

  • Paragraphs 3 and 6 still describe removing the InPlace equality check and preserving "authenticated InPlace delivery".
  • It says V5 errors are 8000–8015; they are now 8000–8014.
  • It lists the deposit delegate as the only builder-facing change. Every in-place fill now also needs the v5_fill_delegate account and an approval of at least the output amount. Tapes without that approval now fail with OwnerMismatch.
  • The validation section still refers to 13adc080.

Optional suggestions

  1. Compute cost: transfer_from calls find_program_address on every use. At 1,500 CU per bump attempt, bump 252 takes 4 attempts (about 6,000 CU), where bump 255 took 1. That means:

    • the v5_source_delegate (bump 255) → v5_deposit_delegate (bump 252) rename adds about 4,500 CU per deposit;
    • in-place fills, which used to make no transfer, now pay about 6,000 CU for the derivation plus the token CPI.

    The address and bump constants already exist and are checked by hardcoded_v5_authorities_match_canonical_pdas. Passing them in removes the derivation, and a wrong constant would fail safely at the token program's signer check. This matters most on the destination swap path. These numbers come from the cost schedule; I didn't measure them. I can push this if you want it.

  2. Error consistency: Deposits still run the explicit delegate check and return InvalidTokenAccount (deposit.rs:114), while fills now return the token program's OwnerMismatch. Both are safe; you may want the two paths to report the same way.

Re-verified at 0436c8b8

  • cargo test -p svm-spoke --lib --features test: 16/16 pass, with no new compiler warnings.
  • The guarded cargo build-sbf --features test for svm-spoke and mock-gateway: both exit 0 with no stack diagnostics.
  • CI is all green, including "Test verified SVM build". I didn't run the TypeScript suites locally.

@droplet-rl

Copy link
Copy Markdown
Contributor

🔎 View trace

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
@Reinis-FRP

Copy link
Copy Markdown
Contributor Author

@droplet-rl addressed the follow-ups from your recheck in 7dbc281:

  • Removed the deposit-only delegate pre-check. Deposits and fills now both rely on SPL transfer_checked for authority/allowance validation. Real-Gateway regressions supply the correct delegate account while omitting approval or approving the wrong delegate, assert SPL OwnerMismatch, and verify rollback. Source allowance assertions now match the exact token program/error code.
  • Added external and self-transfer fill regressions using a Token-2022 mint whose permanent delegate is the fill PDA, with no ordinary approval. Both reject with UnsupportedTokenExtension, leaving balances, fill status and payer unchanged. The predicate test stays removed; these tests exercise actual mint decoding and fill execution.
  • Rewrote the PR description for the final self-transfer implementation, required builder updates, error range 8000–8014, current base and fresh validation.

The constant-PDA optimization is already tracked in #1573, so I left it there.

Validation: 16 Rust tests passed in each configuration, all 25 mock-Gateway deposit/fill tests and 3 focused real-Gateway approval tests passed, as did guarded SBF builds and TypeScript test checking after client/IDL generation. New-head CI, including the verified build, is pending. Please recheck when ready.


Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖

@droplet-rl

Copy link
Copy Markdown
Contributor

Rechecking at 7dbc281f. So far I've reviewed the deposit pre-check removal and the new permanent-delegate and approval regression tests. Next I'm checking the rewritten PR description and rerunning the tests and guarded builds.

@droplet-rl

Copy link
Copy Markdown
Contributor

I rechecked the PR at 7dbc281f. All three follow-ups are handled correctly, and I found no regressions, so my approval stands. I made no code changes.

Deposit delegate pre-check removed: This is safe, and deposits and fills now validate the same way.

  • The Gateway vault's authority is still checked to be GATEWAY_VAULT_AUTHORITY.
  • The transfer authority is still found by the V5_DEPOSIT_DELEGATE address and checked again against the derived PDA in transfer_from.
  • The token program then requires that PDA to be the vault's delegate with enough allowance. If not, it fails with OwnerMismatch or InsufficientFunds.
  • The permanent-delegate case is covered by the shared V5TokenAccounts::load.
  • One side effect: a missing or wrong deposit approval is now reported at the transfer, after the quote, deadline and exclusivity checks, rather than when accounts are loaded. The same transactions are still accepted and rejected; only which error appears changes when there are several problems at once.
  • The new real-Gateway tests supply the correct delegate account with no approval or with an approval to the wrong delegate. They check for 0x4 from the exact token program and that the user account, Gateway vault, spoke vault and state are unchanged afterwards.

Permanent-delegate tests: These cover the gap I raised. A Token-2022 mint names v5_fill_delegate as its permanent delegate, with no ordinary approval (delegate is null and delegatedAmount is 0). Both external and self-transfer fills are rejected with UnsupportedTokenExtension. All watched accounts, including the fill-status PDA and payer, are byte-identical afterwards. Unlike the removed predicate test, these decode a real mint and run the fill end to end. Deposits use the same loader, so they get the same check without a separate test.

PR description: It's accurate now. It covers the self-transfer behavior and its limits, both builder-facing delegate requirements with their addresses and bumps, the SPL error codes, the 8000–8014 range, the current base, and validation at 7dbc281f. Leaving the constant-PDA work to #1573 (perf(svm): reuse constant V5 transfer delegates) is fine.

One optional wording fix: V5_ADAPTER_SPEC.md:69-70 still says "The adapter binds the vault's delegate to v5_deposit_delegate." The adapter no longer reads the delegate field. Something like "The adapter signs the transfer as v5_deposit_delegate; the token program requires that PDA to be the vault's delegate and accepts sufficient or maximum approvals…" would match the code. Line 218 is already accurate.

Re-verified at 7dbc281f

  • cargo test -p svm-spoke --lib: 16/16 pass with and without --features test, with no new compiler warnings.
  • The guarded cargo build-sbf --features test for svm-spoke and mock-gateway: both exit 0 with no stack diagnostics.
  • In CI, "Test verified SVM build" is still pending; every other check passes. I didn't run the TypeScript suites locally.

@droplet-rl

Copy link
Copy Markdown
Contributor

🔎 View trace

Allow 500k CU for the 12-account trace-limit test: random ATA PDA searches can exceed the previous 400k budget. Confirm the transaction before reading the created accounts.

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

@fusmanii fusmanii 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.

the changes make sense, just one comment

/// The name `signer` is retained for client/IDL compatibility only. For V5 fills, supply the submitter's
/// `["v5_fill_payer", submitter]` PDA recorded in `fill_status.relayer`; legacy fills retain the relayer address.
#[account(mut, address = fill_status.relayer @ SvmError::NotRelayer)]
pub signer: UncheckedAccount<'info>,

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.

signer is misleading here since this account actually the fill_payer PDA, I would just call it fill_payer here or just payer

@Reinis-FRP Reinis-FRP Oct 6, 2026 •

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 e036686: renamed both CloseFillPda.signer and FillStatusAccount.relayer to rent_recipient (rentRecipient in TypeScript). This covers both the V5 payer PDA and the historical relayer that receives rent. Also corrected the stale state (Writable) Rustdoc to Readonly and updated the cleanup script to read the recorded recipient.

The renames preserve account order, privileges, discriminators and serialized layout; old-IDL clients still encode the same close instruction. We kept NotRelayer, its message and code 7002 unchanged because clients can consume runtime error names/messages. The spec and PR body now record the package-version requirement and source migration for consumers adopting the new IDL.


Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖

Rename the close account and stored recipient field without changing wire layouts or NotRelayer. Update callers and document the client migration and release version requirement.

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

@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.

Reviewed e036686 (rent_recipient renames). Nothing blocking.

Verified locally:

  • Generated the Anchor IDL (__anchor_private_print_idl) at e0366862^ and e0366862 and diffed them. The only changes are the close_fill_pda account signer → rent_recipient, the FillStatusAccount field relayer → rent_recipient, and docs. Writable/signer flags, account order, discriminators and error codes are unchanged; NotRelayer stays 7002.
  • cargo test -p svm-spoke --features test --lib: 16/16 pass.
  • Nothing in programs/, scripts/, test/ or src/svm still uses signer: / .relayer for the close or the fill status.
  • Reading fillStatus.rentRecipient in closeRelayerPdas.ts also fixes V5 closes, which failed with NotRelayer on the base branch because the script passed the wallet.

Three non-blocking notes inline.

#[derive(InitSpace)]
pub struct FillStatusAccount {
pub status: FillStatus, // Tracks fill completion to prevent replay.
pub rent_recipient: Pubkey, // Rent recipient for closing this PDA; legacy fills store the submitting relayer.

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.

Nit: legacy RequestedSlowFill accounts store the slow-fill requester, not a relayer. The old request_slow_fill set fill_status_account.relayer = ctx.accounts.signer.key() (slow_fill.rs:79 before #1550), and SvmSpoke.SlowFillRetirement.ts checks rentRecipient == legacyRequester. Suggest "legacy accounts store the submitting relayer or slow-fill requester". The same wording is used in close_fill_pda.rs:13 ("legacy fills retain the relayer address") and lib.rs:221 ("historical relayer").

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.

Fixed in c44014a. Updated the field comment, close-account comment, instruction Rustdoc and spec to include historical slow-fill requesters as well as relayers.


Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖

Comment thread scripts/svm/closeRelayerPdas.ts Outdated
.accountsPartial({
state: statePda,
signer: provider.wallet.publicKey,
rentRecipient: fillStatus.rentRecipient,

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.

Non-blocking, pre-existing: closing is now correct, but discovery (L34-36) still compares --relayer with FilledRelay.relayer, which for V5 is jit.repayment_address (v5_adapter/fill.rs:92). That isn't the submitter whose ["v5_fill_payer", submitter] PDA gets the rent. A submitter repaid on an EVM chain would have to pass its bytes32 repayment address as base58 to find its own fills. Funds are safe because rent can only go to the recorded recipient.

A simpler approach that skips event replay: getProgramAccounts with dataSize: 45 and memcmp at offset 9 (8-byte discriminator + 1-byte status) set to v5_fill_payer(submitter) (or the legacy relayer). Then close the accounts whose fill_deadline (u32 LE at offset 41) has passed. This also finds PDAs whose events fall outside the readProgramEvents window. Fine as a follow-up.

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 c44014a. The reference script now scans live fill-status accounts using Anchor's discriminator filter plus the 45-byte size and recipient filter. --submitter derives the V5 payer PDA; --relayer selects a legacy relayer/requester. It checks expiry against confirmed block time and no longer depends on fill events.

The cleanup function passed real validator tests covering discovery without events, exact-deadline/future skips, another submitter's accounts, rent returned to the payer PDA, repeat runs and the frozen historical RequestedSlowFill fixture. The focused suite passed 9 tests, plus TypeScript and CLI argument checks. The successful CLI main path and concurrent-close/error-recovery branches were not exercised end-to-end.

Small clarification: readProgramEvents already paginates; its 1,000 limit is a page size. Scanning still avoids transaction-history availability and includes requests that never emitted a fill.


Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖

5. Deploy with both pause flags still set and verify legacy deposit/fill and slow-fill selectors are absent from
5. Publish the updated IDL and generated clients with the audited V5 release under a new package version in the
planned v6 major release. Any intervening prerelease publication must also use a new prerelease version.
The `rentRecipient` renames require consumer source updates when adopting the new clients; existing clients

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.

Heads-up for other repos: consumers need more than a property rename. The SDK's createCloseFillPdaInstruction (across-protocol/sdk src/arch/svm/SpokeUtils.ts:933) passes the caller's wallet TransactionSigner as signer, and relayer Monitor.closePDAs calls it. For V5 fills that fails with NotRelayer whichever IDL is used, because the recorded recipient is the payer PDA. This came from the earlier payer-PDA change, not this rename. When consumers adopt the new clients they also need to read rentRecipient from the fill-status account (as closeRelayerPdas.ts now does) rather than pass the wallet. Might be worth stating that here so the SDK/relayer migration isn't treated as a rename only.

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.

Clarified in c44014a: future backend V5 cleanup must supply the recorded payer PDA, while the wallet remains the transaction fee payer. Reading the status account provides that recipient. This is part of the upcoming V5 SDK/relayer integration, not a current production regression or a prerequisite for legacy rent cleanup.

Existing legacy cleanup remains compatible when the wallet matches the recorded recipient. The updated local script is explicitly a reference implementation; it does not update the production runner.


Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖

Scan live fill-status accounts for V5 payer PDAs or legacy recipients instead of replaying fill events. Cover expiry, recipient isolation and historical requested-account cleanup, and clarify legacy compatibility and future V5 client requirements.

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
@Reinis-FRP
Reinis-FRP requested a review from droplet-rl October 6, 2026 09:27

@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.

I re-reviewed at c44014a7. There are three new commits since my last recheck at 7dbc281f: 925c77c7, e0366862 and c44014a7. My approval stands; the two comments below don't block merge.

e0366862: renaming to rent_recipient

  • Nothing on-chain changes. FillStatusAccount keeps its struct name, so its account discriminator is the same. Fields stay in the same order with the same types (status: u8 enum, then Pubkey, then u32), so the Borsh layout and the 45-byte size are unchanged.
  • close_fill_pda is unchanged on the wire. The renamed account is still first, still mut, and still not a signer. Only the instruction's IDL account name changes, and the address = fill_status.rent_recipient @ NotRelayer check is the same.
  • The historical-decoding test in compatibility.rs still round-trips the legacy bytes. Adding a pin for NotRelayer = 7002 is a good touch. The state (Readonly) doc fix in lib.rs is also right, since CloseFillPda.state isn't mut.
  • No old names remain in this repo. No src/, scripts/ or test/ code still uses .relayer on a fill-status account or signer for closeFillPda.

c44014a7: cleanup discovery by rent recipient

  • The filter offsets are correct. Data is 45 bytes (8 discriminator + 1 status + 32 + 4), and the recipient sits at offset 9 behind the discriminator and the 1-byte enum tag. Anchor's .all() adds the discriminator filter itself.
  • V5 rent goes to the right place. The script derives the rent recipient from ["v5_fill_payer", submitter]. That's more correct than the old script, which matched FilledRelay.relayer (the repayment address) and always closed to the wallet.
  • Expiry matches the program. currentTime <= fillDeadline is the same comparison the program makes on-chain.
  • Errors are handled sensibly. If another caller closes an account concurrently, the script skips it instead of failing. Any remaining failures make the run exit nonzero after it has processed every account.
  • The tests cover the edge cases: deadlines at current time − 1, current time, and + 1; isolation from another submitter; idempotent re-runs; and the historical requested-slow-fill account, which has no FilledRelay event.

925c77c7

Raising the CU limit to 500k and switching to sendAndConfirmTransaction instead of a 1 s sleep is a sensible fix for the flaky batched-ATA test.

Verified at c44014a7

  • cargo test -p svm-spoke --lib: 16/16 pass with and without --features test, with no new compiler warnings.
  • The guarded cargo build-sbf --features test for svm-spoke and mock-gateway: both exit 0 with no stack diagnostics.
  • CI: "Test verified SVM build" and "EVM Tests" were still pending when I submitted; every other check passes. I didn't run the TypeScript suites locally.

Downstream impact (outside this repo)

I checked who consumes the renamed client, so the coordination is concrete.

  • across-protocol/sdk src/arch/svm/SpokeUtils.ts has createCloseFillPdaInstruction, which calls getCloseFillPdaInstruction({ signer, state, fillStatus }). That stops compiling once the SDK adopts the new clients. It's a loud failure, not a silent one, and a major version is already planned.
  • decodeFillStatusAccount(...).data.status in the same file isn't affected.
  • The more important caller is the relayer's Monitor.closePDAs; see the inline comment.

Comment on lines +213 to +216
property names. Existing legacy cleanup remains compatible when the supplied wallet matches the recorded rent
recipient. Future V5 cleanup clients must supply the recorded payer PDA instead of assuming the recipient is their
wallet; reading the fill-status account provides that address. The transaction fee payer remains a separate signer.
This is part of future backend V5 integration, not a prerequisite for reclaiming legacy rent. The `NotRelayer`

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.

"Not a prerequisite for reclaiming legacy rent" is true. However, the existing cleanup client will start failing on V5 accounts as soon as V5 fills are unpaused, unless it changes first.

across-protocol/relayer src/monitor/Monitor.ts closePDAs works like this:

  • It collects fills where FilledRelay.relayer is a monitored relayer. For V5 fills that field is jit.repayment_address, so any V5 fill repaid to a monitored SVM address matches.
  • It checks that the fill status is Filled and past its deadline.
  • It calls the SDK's createCloseFillPdaInstruction(signer, …), which supplies the monitor's own wallet as the rent recipient.

For a V5 account the recorded recipient is the submitter's payer PDA, so every attempt reverts with NotRelayer. In simulate mode that's a warning each run; otherwise it's a failed send.

The SDK already has to touch this exact call for the signer → rentRecipient rename. If createCloseFillPdaInstruction read the fill-status account and passed its recorded rentRecipient, the existing monitor would close legacy and V5 accounts correctly with no other changes, because the close is permissionless and rent goes to the recorded address. I'd either say that here, or add "cleanup clients pass the recorded rent recipient" to the pre-unpause checklist in deployment step 6 ("Validate replacement V5 route-building and relayer execution support…").

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.

Added to deployment step 6 in 47f44f0: before enabling V5 routes, any enabled cleanup client must pass the recorded rent recipient (the submitter's payer PDA for V5), keeping the transaction fee payer separate. The text explicitly scopes this to V5 backend readiness; existing legacy rent cleanup remains independent of that integration.


Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖

Comment thread programs/svm-spoke/V5_ADAPTER_SPEC.md Outdated
@@ -68,7 +68,7 @@ Amount resolution rejects `bips` greater than 10,000; the wire decoder does not.
mint rather than isolated per execution. `InputVaultBalance` therefore resolves 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. The adapter binds the vault's delegate to

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.

Optional, carried over from my last recheck: since 7dbc281f dropped the deposit delegate pre-check, the adapter no longer reads or binds the vault's delegate field. It signs the transfer as v5_deposit_delegate, and the token program enforces the delegate. Suggested wording: "The adapter signs the transfer as v5_deposit_delegate; the token program requires that PDA to be the vault's delegate and accepts sufficient or maximum approvals rather than requiring equality, …". Line 218 already describes this correctly.

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.

Updated in 47f44f0: the adapter signs as v5_deposit_delegate, while the token program enforces delegation and sufficient/maximum allowance. This now matches the removal of the adapter-side delegate pre-check.


Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖

@droplet-rl

Copy link
Copy Markdown
Contributor

🔎 View trace

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.

5 participants