Repository navigation
improve: replace InPlace with self-transfer - #1574
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. |
![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: ddc93cb2e7
ℹ️ 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".
| require!(token_accounts.source.delegate == COption::Some(V5_FILL_DELEGATE), V5Error::InvalidTokenAccount); | ||
| V5FillDelivery::Delegated(find_v5_account(remaining_accounts, &V5_FILL_DELEGATE, false)?.clone()) | ||
| }; | ||
| let delegate = find_v5_account(remaining_accounts, &V5_FILL_DELEGATE, false)?; |
There was a problem hiding this comment.
When a client constructs an in-place fill, this unconditional lookup now requires v5_fill_delegate, but the public adapter_execute_across_v5 Required Accounts documentation in programs/svm-spoke/src/lib.rs still says that only external delivery requires it. A builder following that API documentation will omit the account and receive MissingAccount; update the Rustdoc alongside the changed account contract.
AGENTS.md reference: AGENTS.md:L11-L16
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Concrete replacement for the existing documentation issue in programs/svm-spoke/src/lib.rs (the file is outside this PR diff, so this is a patch rather than an applyable inline suggestion):
- /// submitter-scoped ["v5_fill_payer"], relay-scoped fill-status PDA, and System Program; external delivery also
- /// requires the recipient ATA and ["v5_fill_delegate"]. Account order is unrestricted because each account is
- /// resolved by its authenticated expected key.
+ /// submitter-scoped ["v5_fill_payer"], relay-scoped fill-status PDA, System Program, recipient ATA, and
+ /// ["v5_fill_delegate"], including when the recipient ATA is the Gateway vault itself. Account order is
+ /// unrestricted because each account is resolved by its authenticated expected key.Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖
There was a problem hiding this comment.
Fixed in fbe350f. The required-account Rustdoc now includes the recipient ATA and v5_fill_delegate for every fill, including self-transfers into the Gateway vault.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖
Reinis-FRP
left a comment
Reinis-FRP
left a comment
There was a problem hiding this comment.
The self-transfer simplification looks sound under the existing supported-token and atomic Gateway execution model. These suggestions clarify the remaining delivery requirements and avoid presenting zero approval as a security requirement.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖
| // Self-transfers validate balance and authority without debiting funds or allowance. | ||
| // The committed Gateway tape must still consume the funds and clear any remaining approval. |
There was a problem hiding this comment.
Zeroing the allowance is optional cleanup under the current permissionless Gateway model: a later caller can already execute a fresh APPROVE or an owner-authorized TRANSFER. It does not add a fund-security boundary. Keep the actual balance/consumption requirement explicit here, including multiple fills observing the same balance.
| // Self-transfers validate balance and authority without debiting funds or allowance. | |
| // The committed Gateway tape must still consume the funds and clear any remaining approval. | |
| // Self-transfers validate balance, frozen state, and authority without debiting funds or allowance. | |
| // The committed Gateway tape must enforce balance checks covering all fill obligations and consume the funds. |
Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖
There was a problem hiding this comment.
Applied in fbe350f. The comment now keeps balance checks covering all fill obligations and consumption explicit, without requiring approval cleanup.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖
| delegate authority/allowance without debiting funds or consuming allowance. The tape must clear the remaining fill | ||
| allowance with `APPROVE(..., 0)` or replace it with the next spender's approval and consume that approval. A step root | ||
| may be reused across source deposits, but canonical builders must either allow at most one in-place fill before a post-fill floor and | ||
| full-balance terminal consumption, or enforce a cumulative floor covering every in-place fill recorded before that | ||
| consumption. The committed terminal outcome must be acceptable to every deposit matching the root. A fixed minimum | ||
| for one fill does not prove aggregate delivery. |
There was a problem hiding this comment.
Suggest distinguishing optional allowance cleanup from the required delivery invariant. The reference tapes may retain the zero approval, but builders should not read it as necessary to protect the shared vault.
| delegate authority/allowance without debiting funds or consuming allowance. The tape must clear the remaining fill | |
| allowance with `APPROVE(..., 0)` or replace it with the next spender's approval and consume that approval. A step root | |
| may be reused across source deposits, but canonical builders must either allow at most one in-place fill before a post-fill floor and | |
| full-balance terminal consumption, or enforce a cumulative floor covering every in-place fill recorded before that | |
| consumption. The committed terminal outcome must be acceptable to every deposit matching the root. A fixed minimum | |
| for one fill does not prove aggregate delivery. | |
| delegate authority/allowance without debiting funds or consuming allowance. Clearing the remaining allowance with | |
| `APPROVE(..., 0)` is optional cleanup: permissionless Gateway execution already permits fresh approvals and | |
| owner-authorized transfers, so clearing it does not protect funds left in the shared vault. A step root may be reused | |
| across source deposits, but canonical builders must either allow at most one in-place fill before a post-fill floor | |
| and full-balance terminal consumption, or enforce a cumulative floor covering every in-place fill recorded before | |
| that consumption. The committed terminal outcome must be acceptable to every deposit matching the root. A fixed | |
| minimum for one fill does not prove aggregate delivery. |
Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖
There was a problem hiding this comment.
Applied in fbe350f. The spec now explains why zero approval is optional cleanup under permissionless Gateway execution, while preserving the aggregate balance and terminal-consumption requirements.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖
| self-transfer. The latter still depends on the proportional or aggregate continuing-path rule above, and its | ||
| unconsumed allowance must be cleared. Any later failure rolls back token, fill-status, and payer-float changes together. |
There was a problem hiding this comment.
Same clarification in the execution summary: consumption remains required; allowance cleanup is optional.
| self-transfer. The latter still depends on the proportional or aggregate continuing-path rule above, and its | |
| unconsumed allowance must be cleared. Any later failure rolls back token, fill-status, and payer-float changes together. | |
| self-transfer. The latter still depends on the proportional or aggregate continuing-path rule above; clearing its | |
| unconsumed allowance is optional cleanup. Any later failure rolls back token, fill-status, and payer-float changes together. |
Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖
There was a problem hiding this comment.
Applied in fbe350f. The execution summary now describes allowance cleanup as optional and retains the continuing-path and atomic rollback requirements.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖
Signed-off-by: Ihor Farion <ihor@umaproject.org>
0a6b5ab to
bb37ba1
Compare
ddc93cb to
bcc69e8
Compare
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.
The program change is correct. I found no blockers in fill.rs. One test assertion is weak and should be tightened, and a few docs outside the diff are out of date.
What I checked
-
Self-transfer behaviour. I read the transfer processor in spl-token 7.0.0 and spl-token-2022 6.0.0. When
source == destination,transfer_checkedstill checks:- frozen state
amount <= balance- mint and decimals
- delegate authority, with
delegated_amount >= amount
It then returns before it changes the balance or the allowance.
-
In-place fills are now stricter than the old
InPlacebranch. The old branch only checked the balance. Now an approval is also required, and the vault must not be frozen. -
External fills keep the same guarantee. The explicit
delegate == V5_FILL_DELEGATEcheck is gone. SPL still rejects the transfer: if the vault's delegate is a different key, it falls through to owner validation and fails withOwnerMismatch. -
No Token-2022 side effects. The mint-extension allowlist already excludes permanent delegates, transfer hooks and transfer fees.
-
Account resolution and rollback behaviour are unchanged.
-
Build and tests.
cargo check -p svm-spokeis clean, with no unused-import or dead-code warnings.cargo test -p svm-spokepasses 16/16. I couldn't run the anchor TS suites locally, and "Test verified SVM build" is still pending in CI.
Also good: the expectError fix in SvmSpoke.V5Fill.ts. The old helper called assert.fail("Expected X") inside its own try, and its catch then found X in that message. So every expected-error assertion in that file passed even when the transaction succeeded. It now matches the helpers in V5Source and V5FillStatus.
Should fix (tests). "custom program error: 0x1" is a prefix of almost every svm_spoke error code: CommonError 0x1770+, SvmError 0x1b58+, V5Error 0x1f40–0x1f4f. It also prefixes SPL 0x10–0x1f. So the new in-place assertions pass on unrelated failures. That includes the assertion that used to check InsufficientVaultBalance. See the inline comment.
Non-blocking
- Dead error variant. Nothing returns
V5Error::InsufficientVaultBalance(8015) any more. It is the last V5 variant, and V5 hasn't shipped (ERROR_CODES.mdlists it with no pre-release code). So you can delete it, together with itsERROR_CODES.mdrow and thetests/compatibility.rsassertion, without shifting any other code. If you'd rather keep it, mark it as reserved. - Out-of-date docs outside the diff:
AGENTS.md:60-61still says "External fills transfer tokens; in-place fills check the shared vault".V5_ADAPTER_SPEC.md:343-344describes the swap flow as "a single in-place Across fill followed by GatewayAPPROVE(inputMint, executor_authority, full balance)…". It omits theAPPROVE(fill delegate)that the tape now needs before the fill.
| await expectError(execute(input, undefined, { ...options, approval: null }), "custom program error: 0x4"); | ||
| await expectError( | ||
| execute(input, undefined, { ...options, approval: outputAmount - 1n }), | ||
| "custom program error: 0x1" |
There was a problem hiding this comment.
expectError does a substring match. "custom program error: 0x1" therefore also matches:
0x1f49(InvalidTokenAccount)0x1f4d(FillCommitmentMismatch)- all of
CommonError(0x1770+) 0x11(AccountFrozen)
So this assertion, the ones at 385 and 409, and the existing one at 254 would still pass if the spoke rejected the fill for an unrelated reason. Before this PR, line 385 checked for InsufficientVaultBalance specifically.
You could let expectError accept a RegExp and match the whole code:
/custom program error: 0x1\b/ // InsufficientFunds only, not 0x11 / 0x1f49 / 0x1770…Alternatively, match the SPL log line ("Error: insufficient funds", "Error: owner does not match", "Error: Account is frozen"). This only works if the token program on the test validator writes those messages to its logs.
There was a problem hiding this comment.
Fixed in 3a028ee. The SPL assertions now match the failing token program and the complete hexadecimal code with a word boundary, so 0x1 cannot accept 0x11, 0x1f49, or a failure from another program. I also removed the unused trailing InsufficientVaultBalance variant and updated the error table/range test without shifting the remaining codes. All 12 V5 fill tests pass against the rebuilt programs, including SPL Token and Token-2022 cases.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖
|
|
||
| await expectError(execute(undefined, undefined, { approval: outputAmount - 1n }), "custom program error: 0x1"); | ||
| await expectError(execute(undefined, undefined, { delegate: Keypair.generate().publicKey }), "InvalidTokenAccount"); | ||
| await expectError(execute(undefined, undefined, { delegate: Keypair.generate().publicKey }), "MissingAccount"); |
There was a problem hiding this comment.
This case now fails earlier, when the loader looks up V5_FILL_DELEGATE. So the file no longer tests a vault that is approved to a different delegate while the real fill-delegate account is supplied. The removed explicit InvalidTokenAccount check used to cover that case.
SPL still rejects it with OwnerMismatch (0x4), because it falls through to owner validation. Since the program no longer has its own guard, one assertion would be useful here. For example, a RealGateway path that approves some other key before an external fill should fail with 0x4. Non-blocking.
There was a problem hiding this comment.
Added in 3a028ee. The real-Gateway regression approves the deposit delegate but supplies the correct fill-delegate account to an external fill. It submits an actual failing transaction, checks the exact SPL OwnerMismatch (0x4) log, and verifies rollback of funding, recipient balance, fill status, payer rent, and approval. The focused real-Gateway test passes.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖
| transfer_from( | ||
| accounts.transfer, | ||
| accounts.token_program, | ||
| relay.output_amount, | ||
| accounts.mint_decimals, | ||
| V5_FILL_DELEGATE_SEED, | ||
| )?; |
There was a problem hiding this comment.
Non-blocking. The delegate, allowance and frozen checks now fail inside the token CPI with raw SPL codes. They also run after the fill-status account has been created. That is rolled back, so it isn't a safety issue. The main cost is clarity: a missing APPROVE shows up as OwnerMismatch (0x4), which is a confusing signal for someone debugging a tape. If you want a typed error back, one pre-check in load keeps the single transfer path:
require!(token_accounts.source.delegate == COption::Some(V5_FILL_DELEGATE), V5Error::InvalidTokenAccount);Possible follow-up: transfer_from re-derives the delegate PDA with find_program_address on every call, even though V5_FILL_DELEGATE_BUMP is a constant. With bump 252 that is 4 create_program_address attempts, about 6k CU. In-place fills now pay that plus the token CPI, inside the same CU budget as the swap legs.
There was a problem hiding this comment.
Keeping the single transfer path and token-program validation here. 3a028ee documents OwnerMismatch (0x4) for missing/wrong approval instead of restoring the redundant pre-check. The constant address/seed/bump optimization is already implemented separately in #1573.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖
| `8DWnJFMBTSDYWsUUSqna9tx9LJbU1yUfq7jTiPJDf8sX` with bump 252. Builders must use this PDA as both the approval | ||
| target and the supplied deposit delegate account. |
There was a problem hiding this comment.
Every fill path, including in-place, now needs an APPROVE to the fill delegate. Consider pinning its address here the same way as the deposit delegate: v5_fill_delegate derives D27f3mVXRL6N3bgja49UWLQu7kt57sy1aZYy7ZEwdxn1 with bump 252. That matches constants.rs and the /pdas/fillDelegate fixture. Builders must use this PDA as both the approval target and the fill-delegate account they pass in.
There was a problem hiding this comment.
Added in 3a028ee: the fill-delegate address and bump, its use as both approval target and supplied account, and the requirement for sufficient approval when the fill executes. Also updated the AGENTS overview and destination-swap sequence to reflect self-transfers and the initial fill approval.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖
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>
droplet-rl
left a comment
droplet-rl
left a comment
There was a problem hiding this comment.
Approving. 3a028ee addresses all of my earlier feedback, and the program change was already correct.
What changed since my last review
- Error matching in the tests is now exact. The new
tokenError(code)regex requires the failing program to be the token program, plus the full hex code followed by a word boundary. So0x1no longer accepts0x11,0x1f49, or an error raised by the spoke itself.tokenProgramis read when the regex is built, so the Token-2022 loop also checks against the Token-2022 program ID. - Wrong-delegate coverage is back, through the real Gateway. The new test approves the deposit delegate, supplies the correct fill-delegate account, and runs an external fill. It checks for the exact
OwnerMismatch(0x4) log line and confirms rollback of the funding, recipient balance, fill status, payer rent and approval. InsufficientVaultBalanceis removed. It was the last variant, so no other error code shifts.compatibility.rsnow pinsFillOutputAmountTooLow= 8014, so the end of the range is still checked.- The docs are now accurate:
AGENTS.mddescribes self-transfers.- The spec gives the fill-delegate address and bump (they match
constants.rs). - The destination swap sequence now starts with
APPROVE(v5_fill_delegate).
cargo test -p svm-spoke passes 16/16 after the variant removal. I didn't run the anchor TS suites locally, and the "Test verified SVM build" CI job was still pending when I looked.
On the declined suggestion: keeping one transfer path and documenting 0x4 instead of a typed pre-check is a reasonable call. The PDA-bump optimisation being handled in #1573 is fine.
I left one optional wording nit inline.
| Sufficient approval must exist when the fill executes. Missing or wrong delegate approval fails in the token program | ||
| with `OwnerMismatch` (0x4). |
There was a problem hiding this comment.
Optional wording nit. OwnerMismatch (0x4) only applies when the vault's delegate slot is empty or holds some other key. If the slot already holds the fill delegate but the allowance is too small, the token program fails with InsufficientFunds (0x1) instead.
That case includes a zeroed approval: SPL approve with amount 0 keeps the delegate set and only sets the allowance to 0, unless the Gateway calls revoke for zero. Builders debugging a tape will hit both codes, so it may be worth listing both:
| Sufficient approval must exist when the fill executes. Missing or wrong delegate approval fails in the token program | |
| with `OwnerMismatch` (0x4). | |
| Sufficient approval must exist when the fill executes. A missing approval, or one to another delegate, fails in the token program | |
| with `OwnerMismatch` (0x4); an insufficient fill-delegate allowance (including a zeroed one) fails with `InsufficientFunds` (0x1). |
There was a problem hiding this comment.
Fixed in ef3bc66. The spec now distinguishes an unset/wrong delegate (OwnerMismatch, 0x4) from insufficient fill-delegate allowance (InsufficientFunds, 0x1), including zeroed allowance for a nonzero fill. Documentation-only change; the pre-commit checks passed.
Sent from Reinis Martinsons's Codex CLI Agent using gpt-6-astra 🤖
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>
3 participants
No description provided.