Repository navigation
perf(svm): reuse validated Gateway vault for in-place fills - #1575
Reinis-FRP wants to merge 3 commits into
Conversation
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. |
0e5a1cc to
41f1ccc
Compare
droplet-rl
left a comment
droplet-rl
left a comment
There was a problem hiding this comment.
The change is correct, and the security properties are the same as before. Approving. My comments below are non-blocking.
Why the reuse is safe
- If
fill_input.recipient == GATEWAY_VAULT_AUTHORITY, the old code derivedATA(GATEWAY_VAULT_AUTHORITY, output_token, token_program.key). That is the same addressV5TokenAccounts::loadderives:token_program.keyistoken_program_idandmintisoutput_token. - Both paths called
find_v5_accountwith the same key, so they got the same first-matchAccountInfo. That is true even ifremaining_accountshas duplicate keys. - The writable check and
load_token_account(.., mint, GATEWAY_VAULT_AUTHORITY)had already run on that account, with no CPI in between. The second lookup and parse added nothing. - On the external path,
recipient != GATEWAY_VAULT_AUTHORITY. The ATA seeds are fixed-length (owner‖token_program‖mint), so its ATA can only match the vault through a PDA collision. Choosing delivery by recipient therefore gives the same result as the old key compare. - The live balance check, the delegate check, and
relay.recipient == fill_input.recipientin the handler are unchanged.
What I checked
- Ran
cargo test -p svm-spoke --lib v5_adapter::filllocally: both new tests pass. - External-recipient rejection cases (wrong recipient account, mint mismatch, reassigned ATA authority, missing approval) are still covered by
test/svm/SvmSpoke.V5Fill.ts. - Recomputed the README normalization from
baseline.json. The medians (84,688 / 73,837), normalized execution (78,688 / 69,337), the 9,351 normalized gap on every sample, sample 3's 6,351, and the 15,351 total-median gap all match.
Non-blocking
- The +28 CU on external fills could probably be removed.
deliveryis now decided by a second key compare that always matches on the in-place path and can never match on the external path. Choosing delivery inside the sameif/elsedrops that compare from both paths (inline suggestion). I checked that it compiles and that both new tests pass; I haven't re-run the CU benchmark. - Two small README wording points (inline).
- Optional:
baseline.jsonrecordshead: 24e07c9cplus a non-emptytrackedDiffSha256, so the snapshot points at the parent commit plus an uncommitted diff. Your binary-hash check (80510beb…) shows the numbers are identical. Still, if you regenerate the snapshot from a clean tree at the final commit, the file explains itself without the PR description.
| let recipient_info = if fill_input.recipient == GATEWAY_VAULT_AUTHORITY { | ||
| // The shared loader validated this writable vault, including its token authority. No CPI has intervened. | ||
| token_accounts.gateway_vault.clone() | ||
| } else { | ||
| let recipient = get_associated_token_address_with_program_id( | ||
| &fill_input.recipient, | ||
| &fill_input.output_token, | ||
| token_accounts.token_program.key, | ||
| ); | ||
| let recipient_info = find_v5_account(remaining_accounts, &recipient, true)?; | ||
| load_token_account( | ||
| recipient_info, | ||
| token_accounts.token_program.key, | ||
| &fill_input.output_token, | ||
| &fill_input.recipient, | ||
| )?; | ||
| recipient_info.clone() | ||
| }; | ||
|
|
||
| let delivery = if token_accounts.gateway_vault.key() == recipient_info.key() { |
There was a problem hiding this comment.
Nit / possible CU win: once the branch is on fill_input.recipient, gateway_vault.key() == recipient_info.key() on line 166 always matches on the in-place path and can only match on the external path through an ATA PDA collision. Deciding delivery inside the same branch removes one 32-byte compare from both paths, which may recover most of the +28 CU on external fills. Error order on both paths stays the same (recipient checks, then delegate; or the balance check):
let (recipient_info, delivery) = if fill_input.recipient == GATEWAY_VAULT_AUTHORITY {
// The shared loader validated this writable vault, including its token authority. No CPI has intervened.
// This check is not a debit. Builders must consume after one fill or enforce an aggregate floor covering
// every in-place fill recorded before full-balance consumption; step-root reuse alone is valid.
require!(token_accounts.source.amount >= output_amount, V5Error::InsufficientVaultBalance);
(token_accounts.gateway_vault.clone(), V5FillDelivery::InPlace)
} else {
let recipient = get_associated_token_address_with_program_id(
&fill_input.recipient,
&fill_input.output_token,
token_accounts.token_program.key,
);
let recipient_info = find_v5_account(remaining_accounts, &recipient, true)?;
load_token_account(
recipient_info,
token_accounts.token_program.key,
&fill_input.output_token,
&fill_input.recipient,
)?;
require!(token_accounts.source.delegate == COption::Some(V5_FILL_DELEGATE), V5Error::InvalidTokenAccount);
let delegate = find_v5_account(remaining_accounts, &V5_FILL_DELEGATE, false)?.clone();
(recipient_info.clone(), V5FillDelivery::Delegated(delegate))
};I checked that this compiles and that both new loader tests pass. I haven't re-run the CU benchmark, so the actual saving still needs measuring. If you'd rather keep the explicit key compare as defense-in-depth, that's fine too. In that case, a short comment saying it can't change the outcome would help.
There was a problem hiding this comment.
Agreed that the comparison is redundant. We’ll leave this cleanup out because #1574 removes V5FillDelivery and the comparison entirely, using one transfer path for both destinations. The validated-vault reuse in this PR still applies there; we’ll carry that over and refresh the combined CU measurements when integrating the two. No need to preserve the comparison as defense in depth.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖
| External minus in-place execution is 9,351 CU after normalization for every sample. Both fill variants have the same | ||
| number of Gateway-vault ATA derivations. Sample 3's in-place status bump is two steps lower (3,000 extra CU), leaving |
There was a problem hiding this comment.
Small wording point: the old paragraph singled out sample 3 because it was a reversal (in-place cost more than external). After this change sample 3 is no longer a reversal, and sample 4 now differs from the normalized gap by a similar amount in the other direction: measured gap 13,851, because the external status bump is 250 and the in-place one is 253, so +4,500. Either explain both samples, or replace the sample-3 sentence with a general one ("per-sample gaps differ only by status-bump differences").
There was a problem hiding this comment.
Updated in 7b0bf59: replaced the sample-3 example with the general explanation that per-sample execution gaps differ from the normalized gap only by status-bump differences. Checked the formula against all five fixtures, including sample 4’s 13,851 CU gap.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖
| slots and temporary paths in raw receipts naturally vary. Before committing an updated snapshot, run | ||
| `yarn prettier --write test/svm-gateway/cu/baseline.json`. | ||
|
|
||
| Validated Gateway vault reuse is tracked in [#1570](https://github.com/across-protocol/contracts/issues/1570). |
There was a problem hiding this comment.
Removed the tracking sentence in 7b0bf59. The README keeps the current baseline and interpretation; the implementation history stays in this PR.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖
7b0bf59 to
70d4348
Compare
70888c2 to
3d3178f
Compare
70d4348 to
1efdc02
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>
3d3178f to
a537c44
Compare
1efdc02 to
788437c
Compare
2 participants
When the committed fill recipient is
GATEWAY_VAULT_AUTHORITY, reuse the writable vault already authenticated by the shared token-account loader instead of deriving, finding and parsing the same ATA again. Reuse occurs before any CPI; external recipients retain their canonical ATA and serialized-authority checks.Restacked onto the current #1568, including #1564's self-transfer implementation. All fills still execute SPL
transfer_checked; balance, frozen-state and delegate/allowance checks remain in that CPI. The loader tests cover both token programs and reject missing, readonly, malformed, wrong-program, wrong-mint and wrong-authority vaults. Public IDLs and account ordering are unchanged. Fixes #1570.Measured execution CU medians against the same updated base:
Self-transfer savings range from 2,048 to 9,548 CU per fixture, with a median per-fixture saving of 3,548 CU. The difference between execution medians is 2,048 CU; differences of medians need not equal the median of differences. External fills cost 18 CU more in every fixture. Legacy flows and per-fixture approval/buffer costs are unchanged. These are consumed-CU measurements on the documented local runtime, not production budgets.
Validation after restacking: 18 Rust tests, all 25 CU fixtures, and all 14 fill validator tests passed. Coverage includes insufficient balance/allowance, frozen accounts, wrong account authority, PermanentDelegate rejection, rollback and rent reclaim. Guarded SBF builds passed; the validator-test Spoke binary matches the measured binary byte for byte. Formatting and diff checks passed. Full verified-build results are tracked separately in CI.
Targets #1568 (
reinis/svm-cu-benchmarks), independently of #1573.Per-fixture execution CU and provenance
Before snapshot and after snapshot use Node 22.22.0, Agave 4.1.2, cargo-build-sbf 4.1.0 and platform-tools 1.44/1.52/1.54 for legacy/Spoke/Gateway. Fixture, runtime, public IDL, legacy and Gateway hashes match. Approval and buffer costs are unchanged per fixture. Source revisions, tracked source-diff hashes and binary hashes are recorded in the snapshots.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖