feat(svm): add V5 fill-status payer float - #1537
Conversation
f35a0dd to
c74c70e
Compare
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
c74c70e to
e77b3f8
Compare
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
droplet-rl
left a comment
droplet-rl
left a comment
There was a problem hiding this comment.
Assessment
I read the full diff against reinis/acp-184-step-2-source-deposit, built with cargo check -p svm-spoke --features test (clean, pre-existing warnings only), and ran the unit tests.
The core mechanics look right. I specifically checked and could not break:
- The account-creation sequence correctly mirrors Anchor 0.31.1's
generate_create_account. Thecurrent_lamports == 0split is necessary — the System Program'screate_accountbails withAccountAlreadyInUsewhento.lamports() > 0— so the one-lamport prefunding DoS is genuinely handled, and an attacker can only transfer lamports to the fill-status PDA (allocate/assignneed the PDA signature), so there is no way to poison it into a non-empty state. V5_FILL_STATUS_SPACEis 45 bytes → rent-exempt minimum ≈ 1,204,080 lamports, comfortably above the 890,880 floor for the 0-data payer PDA, so permissionless close can never leave the float rent-paying.- The front-run vector I went looking for is closed:
request_slow_fillrejects V5-tagged messages (slow_fill.rs:52) and the relay hash binds the message, so nobody can squat the fill-status PDA ahead of a V5 fill. - Float scoping is sound —
create_v5_fill_statusrecords the same PDA it spends from, andWithdrawV5FillPayer's seed constraint binds withdrawal to the signing submitter (theConstraintSeedstest confirms).
What I'm asking for. My concerns are not exploitable bugs; they're about what the last two commits quietly removed and what the PR body claims.
13821579 refactor(svm): align fill payer with Anchor rent handling and 3594622c refactor(svm): simplify V5 fill-status creation deleted every Rust unit test this PR added, along with the RelayFilled replay error and the rent-remainder guard. Both are labelled as refactors, which is exactly where this kind of removal escapes review. Concretely, on HEAD:
$ cargo test -p svm-spoke --lib
test result: ok. 11 passed
11, not the 12 in the Validation section — and none of them are new. The only test-file change in the whole diff is the error-range assertion in v5.rs. So "add focused unit and validator coverage for signerless creation, replay rejection, …" is no longer accurate: unit coverage is zero, and the branchy creation helper is covered only by the two validator tests (happy path + replay). Please either restore the tests or correct the summary — right now the body over-states what was validated.
The individually actionable items are inline. The highest-value ones: restore the RelayFilled replay error (relayers need to distinguish "already filled, stop" from "malformed account, retry"), restore the layout-invariant assertion (it guards the exact property in the PR title), and cover the prefunded branch.
One item needing explicit sign-off: CloseFillPda relaxes Signer → UncheckedAccount on an instruction that is live on mainnet today. I believe it is safe — rent still lands on fill_status.relayer, and both legacy fills and slow-fill requests record a real beneficiary there — but it is an authorization relaxation plus an IDL signer-flag change to a deployed program, folded into a PR titled "add V5 fill-status payer float." It should be called out in the description and confirmed against downstream clients rather than discovered in the diff. Details inline.
The reserved Fill adapter branch is confirmed still fail-closed (v5_adapter.rs:45 → UnsupportedMode), and the test feature is properly segregated from the production artifact in buildSolanaVerify.sh / publish.yml.
| let (expected_payer, payer_bump) = Pubkey::find_program_address(&[V5_FILL_PAYER_SEED, submitter.as_ref()], &ID); | ||
| let (expected_fill_status, fill_status_bump) = Pubkey::find_program_address(&[FILL_STATUS_SEED, relay_hash], &ID); |
There was a problem hiding this comment.
Both derivations already exist in v5.rs and are re-derived inline here:
pub fn derive_v5_fill_payer(submitter: &Pubkey) -> (Pubkey, u8) // v5.rs:314
pub fn derive_fill_status(relay_hash: &[u8; 32]) -> (Pubkey, u8) // v5.rs:318That matters beyond DRY: pda_domains_match_golden_fixture (v5.rs:487) pins both of those helpers against the golden fixture, including /pdas/fillPayer and /pdas/fillStatus. This copy is outside that safety net, so a future seed-domain change would be caught in v5.rs and silently diverge here.
let (expected_payer, payer_bump) = derive_v5_fill_payer(submitter);
let (expected_fill_status, fill_status_bump) = derive_fill_status(relay_hash);Separately, worth considering for Step 4: two find_program_address calls cost up to ~25k CU in what will be the fill hot path inside an atomic tape. Taking both bumps as arguments and using create_program_address is safe here — a non-canonical bump derives a different address and fails the require_keys_eq! below — and would recover most of that budget.
There was a problem hiding this comment.
Fixed in ece3f332: the helper now calls derive_v5_fill_payer and derive_fill_status, so both derivations remain under the golden-fixture coverage. I left passing bumps / switching to create_program_address for Step 4, when the hot-path account ABI is finalized and its CU tradeoff can be measured.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖
| require_keys_eq!(*fill_status.owner, system_program::ID, V5Error::InvalidFillStatusAccount); | ||
| require!(fill_status.data_is_empty(), V5Error::InvalidFillStatusAccount); |
There was a problem hiding this comment.
3594622c dropped the dedicated replay signal from just above these lines:
- require_keys_neq!(*fill_status.owner, ID, CommonError::RelayFilled);The replay is still rejected — the owner == system_program check catches it — but it now surfaces as InvalidFillStatusAccount, which is the same error as "wrong PDA" and "foreign-owned account." Those need different off-chain handling: RelayFilled means stop retrying and drop the relay, whereas a malformed-account error means retry or alert. Legacy fill_relay returns CommonError::RelayFilled for exactly this case (fill.rs:121), so V5 fills would report a different error for identical semantics.
The doc comment above still says "an existing program-owned fill status is rejected as a replay," which no longer matches the error the caller actually observes. Please restore the require_keys_neq! so the classification stays consistent with the legacy path — the new TS assertion on InvalidFillStatusAccount would need updating to RelayFilled alongside it.
There was a problem hiding this comment.
Fixed in ece3f332, including the slow-fill distinction. A program-owned account is now deserialized: Filled returns CommonError::RelayFilled, RequestedSlowFill is accepted and replaced as ReplacedSlowFill, and Unfilled is accepted as FastFill. The validator test creates a real requested slow fill and confirms that replacement succeeds without debiting the V5 payer float.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖
| FillStatusAccount { status: FillStatus::Filled, relayer: expected_payer, fill_deadline } | ||
| .try_serialize(&mut &mut fill_status.try_borrow_mut_data()?[..]) |
There was a problem hiding this comment.
3594622c removed write_v5_fill_status and, with it, the only test asserting the layout invariant this PR is built on:
assert_eq!(V5_FILL_STATUS_SPACE, 8 + 1 + 32 + 4);
assert_eq!(FillStatusAccount::DISCRIMINATOR.len(), DISCRIMINATOR_SIZE);Inlining the struct literal is fine, but that assertion is the guard for "preserving the legacy account layout" in the PR title. V5_FILL_STATUS_SPACE is computed from INIT_SPACE, so it tracks the struct automatically and never trips — but a field added to FillStatus or FillStatusAccount would silently change the on-chain layout and the rent math on both the legacy and V5 paths, with nothing failing. It costs nothing to keep as a standalone #[test] against the constant; please restore it.
There was a problem hiding this comment.
I kept this test removed intentionally. The prior assertion only pinned the current type against a duplicated numeric size while round-tripping that same type; it did not prove compatibility with historical bytes. V5 now directly uses the shared FillStatusAccount, INIT_SPACE, discriminator, and serializer, so there is no separate V5 layout implementation to drift. A true legacy ABI invariant would be better added centrally as a golden-byte fixture rather than as a V5 helper test. I also removed the inaccurate unit-test claim from the PR description.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖
| if current_lamports == 0 { | ||
| invoke_signed( | ||
| &system_instruction::create_account( | ||
| payer.key, | ||
| fill_status.key, | ||
| required_lamports, | ||
| V5_FILL_STATUS_SPACE as u64, | ||
| &ID, | ||
| ), | ||
| &[payer.clone(), fill_status.clone(), system_program_info.clone()], | ||
| &[payer_seeds, fill_status_seeds], | ||
| )?; | ||
| } else { | ||
| if required_lamports > 0 { | ||
| invoke_signed( | ||
| &system_instruction::transfer(payer.key, fill_status.key, required_lamports), | ||
| &[payer.clone(), fill_status.clone(), system_program_info.clone()], | ||
| &[payer_seeds], | ||
| )?; | ||
| } | ||
| invoke_signed( | ||
| &system_instruction::allocate(fill_status.key, V5_FILL_STATUS_SPACE as u64), | ||
| &[fill_status.clone(), system_program_info.clone()], | ||
| &[fill_status_seeds], | ||
| )?; | ||
| invoke_signed( | ||
| &system_instruction::assign(fill_status.key, &ID), | ||
| &[fill_status.clone(), system_program_info.clone()], | ||
| &[fill_status_seeds], | ||
| )?; | ||
| } |
There was a problem hiding this comment.
The prefunded branch — the transfer / allocate / assign sequence that the summary calls out as the "one-lamport account-creation denial of service" mitigation — is never executed by any test. SvmSpoke.V5FillStatus.ts funds payer, never fillStatus, so every test run takes the current_lamports == 0 path and lines 64-82 are dead in CI.
I walked through the logic and believe it's correct, but this is the subtlest code in the PR (three chained invoke_signed calls with two different signer-seed sets) and it's the part a reader is least likely to verify by inspection. A one-line addition to the existing validator test — transfer 1 lamport to the fill-status PDA before calling testCreateV5FillStatus, then assert the account is created and the float is debited by rentExemption(45) - 1 — would cover it and directly demonstrate the DoS resistance the summary claims.
The InvalidFillPayer, InvalidAccountMutability, and MissingAccount branches (lines 35-41) are likewise untested.
There was a problem hiding this comment.
Covered in ece3f332. The validator test now exercises both the zero-balance create_account branch and the prefunded transfer / allocate / assign branch, checks the payer delta, and reclaims both accounts. Under Agave 4.1.2, transferring only one lamport to the absent PDA is itself rejected by the runtime rent rule, so the test uses the smallest valid prefund (minimum_balance(0)) and still verifies that creation correctly tops it up. The PR description no longer overclaims coverage for the other validation branches.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖
| let balance = ctx.accounts.payer.lamports(); | ||
| let amount = if amount == u64::MAX { balance } else { amount }; | ||
| let seeds: &[&[u8]] = &[V5_FILL_PAYER_SEED, submitter.as_ref(), &[ctx.bumps.payer]]; |
There was a problem hiding this comment.
13821579 removed the rent guard that used to sit here:
- require_fill_payer_spend(balance, amount, Rent::get()?.minimum_balance(0))?;and the matching spec sentence ("a nonzero remainder must be rent-exempt") was dropped from V5_ADAPTER_SPEC.md in this PR. The behaviour is still safe, but a partial withdrawal leaving a nonzero remainder below 890,880 lamports now fails at the runtime rent-state check with InsufficientFundsForRent — a bare transaction-level error with no program context — instead of the previous FillPayerRemainderNotRentExempt.
Note the new test never exercises this: it withdraws exactly rentMinimum from rentMinimum * 2, landing precisely on the rent-exempt floor, then drains via u64::MAX. Every value strictly between those two fails opaquely. Since this is a submitter-facing instruction and "withdraw most of my float" is the obvious usage, please either restore the guard or document the constraint in the spec and add a test pinning the failure mode.
Minor, same function: amount == 0 is accepted and emits a V5FillFloatWithdrawn with amount: 0, which will show up as noise for indexers.
There was a problem hiding this comment.
Addressed without restoring the custom guard. The program and V5 spec now explicitly document that partial withdrawals are subject to the runtime rent-state rule, keeping this path aligned with the System Program / Anchor behavior. 1dfa69f9 pins the failure atomically when a withdrawal would leave a nonzero rent-unsafe remainder; the same test covers an exact rent-floor remainder and the u64::MAX drain. I left zero withdrawals unchanged because the submitter can already emit events for its own float, so rejecting zero would not provide an integrity or anti-spam boundary.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖
| pub fn test_create_v5_fill_status( | ||
| ctx: Context<TestCreateV5FillStatus>, | ||
| submitter: Pubkey, | ||
| relay_hash: [u8; 32], | ||
| fill_deadline: u32, | ||
| ) -> Result<()> { |
There was a problem hiding this comment.
submitter is an unauthenticated instruction argument, so anyone holding this entrypoint can spend an arbitrary submitter's float and permanently mark any relay hash as Filled, blocking a legitimate fill.
The gating is correct — buildSolanaVerify.sh only adds --features test under IS_TEST=true, and publish.yml builds it as a separate svm-verified-test-binaries artifact, distinct from the production one. But this is a step up from the existing test-gated code in utils/testable_utils.rs, which only mocks time; this is the first one that can burn another account's lamports, and the test binaries are published as a public release artifact.
Cheap hardening that costs nothing in coverage: make submitter a Signer in TestCreateV5FillStatus and drop the argument. create_v5_fill_status still receives no signature for the payer PDA, so the signerless property under test is fully preserved — the wrapper just can't be pointed at a third party's float if a test build is ever deployed somewhere it shouldn't be. The TS test would only need to retain the generated keypair and add .signers([submitter]).
Also worth a # Safety-style line on create_v5_fill_status stating the precondition Step 4 must honour: the caller is responsible for supplying a Gateway-attested submitter, since the helper authenticates the derivation but not the value.
There was a problem hiding this comment.
Fixed in ece3f332: submitter is now a Signer account in the test-only wrapper, the unauthenticated instruction argument is gone, and the TS caller signs with that keypair. The helper also has a # Safety precondition stating that Step 4 must source submitter from Gateway-attested context.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖
| /// CHECK: The address constraint binds this non-signing account to the recorded rent recipient. | ||
| #[account(mut, address = fill_status.relayer @ SvmError::NotRelayer)] | ||
| pub signer: Signer<'info>, | ||
| pub signer: UncheckedAccount<'info>, |
There was a problem hiding this comment.
This relaxes an authorization check on an instruction that is live on mainnet today, and it's the one change here that reaches beyond V5.
I think it's safe: close = signer is still bound by address = fill_status.relayer, and every writer of that field records a real beneficiary — fill_relay the signing relayer, request_slow_fill the requester (slow_fill.rs:75), create_v5_fill_status the payer PDA. The deadline check still gates closure, fill_relay cannot re-fill past the deadline, so permissionless close opens no replay window. Rent cannot be redirected. I checked the in-repo callers (SvmSpoke.Fill.ts:371, SvmSpoke.SlowFill.AcrossPlus.ts:482, scripts/svm/closeRelayerPdas.ts) and they all still pass the relayer as a signer, which the runtime accepts as a redundant signature.
Two things I'd still want before this merges:
- Call it out in the PR description. "Legacy
close_fill_pdabecomes permissionless" is a security-relevant change to a deployed program and shouldn't be discovered by reading the diff of a PR titled "add V5 fill-status payer float." It also warrants a note for whoever signs off on the next deploy. - Confirm the IDL change downstream. The
signeraccount flips toisSigner: falsein the published IDL. Clients built against the old IDL keep working, but anything that introspects the IDL to decide which keypairs acloseFillPdatransaction needs — or that asserts on the account-meta shape — will see a different value. Worth a quick check against the relayer repo and a note in the release notes for@across-protocol/contracts.
Minor: the field is now named signer while explicitly not being one. Renaming would break the IDL account name, so keeping it is the right call, but the /// CHECK line and the updated lib.rs doc comment are now the only thing preventing a misreading — worth a brief inline note that the name is retained for IDL compatibility.
There was a problem hiding this comment.
Addressed in ece3f332 and the PR description. The description and V5 adapter spec now explicitly call out that existing close_fill_pda accounts become permissionless after expiry, that the immutable recorded relayer remains the destination, and that regenerated IDLs flip only the signer requirement while retaining the account name.
I also checked downstream usage: the SDK helper currently accepts a TransactionSigner and passes it to the generated close instruction, while the in-repo scripts/tests still sign; those old-IDL callers remain valid because the signature is merely redundant. I found no direct close caller in the relayer repositories checked locally. The compatibility note is now visible for release/deploy signoff.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖
There was a problem hiding this comment.
Follow-up after exercising the regenerated IDL in CI: the on-chain compatibility conclusion stands, but the client wording needed one distinction. Old-IDL clients keep sending the signer meta/signature and remain compatible. Anchor clients generated from the new IDL must omit the explicit .signers([relayer]); otherwise Web3 rejects it locally as an unknown signer because no instruction account is marked signing.
Fixed in b2474f10: the in-repository close callers now omit that explicit signer, verified-SVM CI regenerates the test-feature svm_spoke IDL, and the PR compatibility note documents both client cases.
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>
droplet-rl
left a comment
droplet-rl
left a comment
There was a problem hiding this comment.
Assessment
All seven points from my previous review are addressed, and the two follow-up commits are a genuine improvement rather than a minimal patch. Re-verified locally on 1dfa69f9:
$ cargo check -p svm-spoke --features test # clean, no new warnings
$ cargo test -p svm-spoke --lib
test result: ok. 11 passed
11 matches the corrected Validation section.
Point-by-point:
| Previous finding | Resolution |
|---|---|
| Duplicated PDA derivation | Now imports derive_v5_fill_payer / derive_fill_status from v5.rs — back under the golden-fixture test |
Replay surfaced as InvalidFillStatusAccount |
CommonError::RelayFilled restored (L56), TS asserts on it |
| Prefunded branch untested | Covered, and the assertions are stronger than I asked for — see below |
| Rent-remainder guard removed silently | Pinned by test ("insufficient funds for rent") and documented in both the spec and the lib.rs doc comment |
Test entrypoint took unauthenticated submitter |
Now Signer, argument dropped |
Missing caller precondition on submitter |
# Safety section added |
close_fill_pda relaxation undocumented |
Dedicated Compatibility note in the description + recorded in the spec for release coordination |
The prefunded-path test is worth calling out: rather than just asserting the account exists, it tracks the float across both creation modes and both closes, landing on initialFloat + prefundedLamports. That arithmetic only balances if the donated lamports are correctly absorbed as a top-up rather than double-paid, which is exactly the property the branch exists to guarantee. Good test.
On the new slow-fill replacement path. I checked whether overwriting relayer and fill_deadline when replacing a RequestedSlowFill lets a V5 submitter capture rent that the slow-fill requester paid. It does — but legacy fill_relay already does precisely the same thing (fill.rs:161-163), so this faithfully mirrors existing semantics rather than introducing new behaviour. Documenting it as "preserve standard fill-status semantics" is accurate. try_borrow_data on L50 is scoped to the if block and released before the try_borrow_mut_data on L107, so no borrow conflict.
Approving. Nothing here blocks. Three non-blocking items inline; the first is the one I'd least like to see slip, because it lands in Step 4 rather than here — relay_hash and fill_deadline are independent parameters with nothing binding them, and this helper is the first place in the program where the stored fill_deadline can diverge from the value committed in the relay hash. Worth closing off in the # Safety contract now while it's fresh.
| submitter: &Pubkey, | ||
| relay_hash: &[u8; 32], | ||
| fill_deadline: u32, |
There was a problem hiding this comment.
relay_hash and fill_deadline arrive as independent parameters and nothing checks that the deadline is the one committed inside the hash. Every other writer of this field binds them: fill_relay and request_slow_fill both carry is_relay_hash_valid(&relay_hash, &relay_data, &state), so the invariant "stored fill_deadline == the value committed in relay_hash" holds everywhere in the program today. This helper is the first place that can break it, and the new replacement path means it can now overwrite a deadline that request_slow_fill had already validated.
Why it matters downstream: the stored fill_deadline is the sole input to close_fill_pda's expiry check, and that instruction is permissionless as of this PR. A caller passing a too-small deadline writes a fill status that anyone can close immediately, which removes the replay guard and reopens the relay to a second fill.
Relatedly, the helper performs no expiry check of its own (it has no state access, so it can't). Legacy fill_relay rejects relay_data.fill_deadline < current_time. Without that check in the Step 4 caller, an expired-then-closed V5 fill status can simply be re-created for the same relay hash.
Neither is reachable today — the helper is #[allow(dead_code)] and the only caller is test-gated — so this is purely about not losing the constraint between PRs. Two options:
- Cheapest: extend the
# Safetyblock, which currently covers onlysubmitter, to state that the caller must sourcefill_deadlinefrom the sameRelayDatathat producedrelay_hash, and must reject deadlines already in the past. - Sturdier: take
&RelayDataplus the chain ID and derive the hash internally, mirroringis_relay_hash_valid, so the binding is structural instead of contractual.
Either is fine by me; I'd just rather it be decided here than rediscovered in Step 4 review.
There was a problem hiding this comment.
Fixed in b2474f10. The helper's # Safety contract now requires the caller to derive relay_hash from the same validated V5 RelayData that supplies fill_deadline, and to reject an expired deadline before calling the helper. Step 4 should enforce those conditions structurally in its caller.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖
| let fill_type = match existing.status { | ||
| FillStatus::Unfilled => FillType::FastFill, | ||
| FillStatus::RequestedSlowFill => FillType::ReplacedSlowFill, | ||
| FillStatus::Filled => return err!(CommonError::RelayFilled), | ||
| }; |
There was a problem hiding this comment.
Is the RequestedSlowFill arm reachable for a genuine V5 relay? As far as I can trace, no:
- A V5 relay's message is
V5_MAGIC_PREFIX || step_id, sois_v5_messagereturns true. request_slow_fillrejects V5-tagged messages withV5FillOnly(slow_fill.rs:52).- The relay hash commits to
message, so nobody can request a slow fill under alternate relay data that hashes to a V5 relay's hash.
So no V5 relay hash can ever carry a RequestedSlowFill status. The new test reaches this branch only by constructing a non-V5 relay (message: Buffer.alloc(0)) and then invoking the V5 helper on it — a combination Step 4 would never produce, since the adapter only handles V5-tagged relays.
The spec already acknowledges this ("although new V5-tagged slow-fill requests are disabled"), so I assume it's deliberate symmetry that becomes live if V5 slow fills are ever enabled. Fine by me if so — just flagging that it's currently unreachable surface whose only coverage exercises a shape production can't produce, in case that wasn't the intent.
Same question for the Unfilled arm: fill_relay and request_slow_fill both set a terminal status in the same instruction that initializes the account, so a persisted Unfilled account shouldn't exist either. Harmless as a defensive default, just noting it.
There was a problem hiding this comment.
@droplet-rl I checked this against the EVM V5 design before changing it. contracts-v5/docs/SPOKE_V5_FILLS.md explicitly quarantines V5-tagged deposits from slow-fill requests and executions, so RequestedSlowFill is not an intended V5 state; a persisted Unfilled state is likewise unreachable through the existing writers.
Fixed in b2474f10: I removed the speculative replacement behavior and the synthetic non-V5 test. The helper now accepts only an uninitialized PDA, returns RelayFilled for Filled, and rejects either remaining program-owned enum state as InvalidFillStatusAccount. The spec now records the same V5 slow-fill quarantine. This also matches the intended Solana lite-chain direction; removing the general SVM slow-fill instruction remains outside this PR. Please recheck this piece.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖
There was a problem hiding this comment.
Rechecked b2474f10. The quarantine change is correct — and I confirmed it independently of the EVM doc rather than taking it on faith:
request_slow_fillreturnsV5FillOnlyforis_v5_message(slow_fill.rs:52), andis_v5_messagematches on the first 32 bytes beingV5_MAGIC_PREFIX.- The relay hash commits to
message, andis_relay_hash_validbinds the two, so nobody can request a slow fill under alternate relay data that hashes to a V5 relay's hash.
So neither RequestedSlowFill nor Unfilled is reachable for a V5-tagged relay, and rejecting both as InvalidFillStatusAccount is strictly more conservative than replacing them. Fail-closed on a state that can't occur is the right call for this program. Dropping -> Result<FillType> follows correctly, since the only reachable V5 outcome was FastFill. Verified on b2474f10: cargo check -p svm-spoke --features test clean, cargo test -p svm-spoke --lib 11 passed.
One thing the refactor dropped that I don't think was intentional. Flattening the if/else removed both validations that used to guard the non-program-owned path:
require_keys_eq!(*fill_status.owner, system_program::ID, V5Error::InvalidFillStatusAccount);
require!(fill_status.data_is_empty(), V5Error::InvalidFillStatusAccount);Control now falls straight from the owner == &ID check into the creation sequence, so a fill_status owned by neither svm-spoke nor the System Program reaches create_account/allocate unvalidated.
Not exploitable, and I checked why: assigning or allocating that PDA requires its own signature, which only svm-spoke can produce via invoke_signed, so the owner is always svm-spoke or the System Program, and a system-owned one always has empty data. Even if it were reachable, the System Program's allocate returns AccountAlreadyInUse for a non-empty or non-system-owned account, so it fails safe. It's purely about keeping the validate-then-act shape and a program error instead of a bare System Program one:
if fill_status.owner == &ID {
// existing replay / invalid-state handling
} else {
require_keys_eq!(*fill_status.owner, system_program::ID, V5Error::InvalidFillStatusAccount);
require!(fill_status.data_is_empty(), V5Error::InvalidFillStatusAccount);
}Your call — I didn't push it, since you're mid-iteration on the branch and I'd rather not race your agent. Happy to if you want it.
Two smaller notes on the rest of the commit:
CI change is correct and was actually needed. The verified-test job deploys a binary built with IS_TEST=true but was regenerating only the mock_gateway IDL with --features test, so target/types/svm_spoke.ts wouldn't have carried testCreateV5FillStatus and the new test would have failed to build the instruction. Overwriting target/idl/svm_spoke.json is scoped to that job, so it doesn't leak into the published artifact.
The .signers([relayer]) removals. I checked whether the regenerated IDL breaks the compatibility note, since Anchor builds the account meta as isSigner: false and the old client still passes the keypair. On the pinned @solana/web3.js 1.98.2 it does not — signing with a key that isn't a required signer is tolerated with a deprecation warning, not an error:
Transaction references a signature that is unnecessary, only the fee payer and
instruction signer accounts should sign a transaction. This behavior is
deprecated and will throw an error in the next major version release.
So the compat note holds as written. Worth knowing that web3.js states it will throw in the next major, and that removing the signer from both SvmSpoke.Fill.ts and SvmSpoke.SlowFill.AcrossPlus.ts leaves the repo with no coverage of the old-client path. Keeping .signers([relayer]) at one of the two call sites and dropping it at the other would exercise both shapes for free.
Separately, the # Safety block now covers the relay_hash/fill_deadline binding and the expiry precondition — that fully addresses the point I flagged as the one I least wanted to see slip into Step 4. My approval stands.
| ID, | ||
| }; | ||
|
|
||
| pub const V5_FILL_STATUS_SPACE: usize = DISCRIMINATOR_SIZE + FillStatusAccount::INIT_SPACE; |
There was a problem hiding this comment.
Nit, carried over and downgraded: the layout assertion deleted in 3594622c is still gone.
assert_eq!(V5_FILL_STATUS_SPACE, 8 + 1 + 32 + 4);Correcting the Validation section to "11 passed" resolves my actual complaint — the description no longer claims coverage that isn't there — so this is no longer a blocker. But the invariant itself is still unguarded: V5_FILL_STATUS_SPACE derives from INIT_SPACE, so it silently tracks any field added to FillStatus or FillStatusAccount, and a change there would shift the on-chain layout and rent math on both the legacy and V5 paths with nothing failing. Given "without changing its account layout" is a bullet in this PR's summary, a three-line #[test] pinning the constant seems worth keeping around.
There was a problem hiding this comment.
Acknowledged. I am leaving the duplicate layout assertion out of this narrowly scoped change: the V5 path intentionally uses the shared FillStatusAccount type and its shared INIT_SPACE, so a V5-local assertion would not independently protect the legacy layout and could imply stronger coverage than it provides. If we add a golden byte-layout invariant, it should live centrally with the shared account definition and cover serialization, not only repeat the derived size expression.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2474f10e8
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ); | ||
|
|
||
| await program.methods | ||
| .testCreateV5FillStatus([...relayHash], fillDeadline) |
There was a problem hiding this comment.
When this test is run via yarn test-svm or yarn test-svm-solana-verify, IS_TEST=true applies only to the first build command in package.json; the subsequent yarn generate-svm-artifacts runs buildIdl.sh without that variable and regenerates svm_spoke without --features test. Consequently the generated client has no testCreateV5FillStatus method, so the repository's standard SVM test commands cannot execute this new suite. Export IS_TEST for the whole command chain or explicitly generate the test-feature IDL there as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b90ba45a. Both standard SVM test commands now generate normal production artifacts first, then call a shared generate-svm-test-idls helper before anchor test. Verified-SVM CI uses the same helper.
The helper writes only to target/idl and target/types; it never copies test-feature IDLs, mock-gateway artifacts, or generated clients into src/svm/assets or src/svm/clients. I verified that the production svm_spoke IDL excludes test_create_v5_fill_status, the target test IDL includes it, package assets remain unchanged, and yarn build-ts passes.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
2 participants
Summary
Filledstatus returnsRelayFilled, while any other program-owned fill status is invalidFillStatusAccount.relayerslot without changing its account layoutclose_fill_pdapermissionless while keeping its recordedrelayeras the immutable rent destination; only the V5 submitter may withdraw its payer floatinit_if_needed, relying on the System Program to enforce whether the account can be allocated and assignedtarget/The reserved V5
Filladapter branch remains fail-closed for Step 4.Part of ACP-184. This is Step 3 of the implementation plan and is stacked on #1536.
Compatibility note
This deliberately relaxes the live
close_fill_pdaauthorization rule for existing fill-status accounts. Old-IDL clients may continue supplying the recorded relayer signer meta and signature and remain compatible on-chain. Clients generated from the new IDL must stop explicitly passing that relayer to.signers(...), because the account is no longer marked as signing. The account name is retained to avoid needless client churn, and the in-repository callers have been updated. The V5 adapter spec records the authorization change for release coordination.Validation
cargo +nightly fmt --all -- --checkcargo check -p svm-spoke --features testcargo test -p svm-spoke --lib(11 passed)yarn lint-rust(passed with existing warnings)svm_spokeandmock_gateway; confirmedtest_create_v5_fill_statusis present only in the test IDL while production package assets remain unchangedyarn build-tsSvmSpoke.V5FillStatus.ts(2 passed: empty and prefunded creation/reclaim, plus withdrawal authorization and amounts)Sent from Reinis Martinsons's Codex CLI Agent using gpt-5.6-sol 🤖