improve(gasless): bound batching's paging, fault isolation and 1-call batches - #3746
improve(gasless): bound batching's paging, fault isolation and 1-call batches#3746droplet-rl wants to merge 4 commits into
Conversation
![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: f049349a9e
ℹ️ 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".
| const depositKey = this._getDepositKeyFromMessage(depositMessage); | ||
| const { destinationChainId, inputToken, inputAmountForValidation, outputToken, outputAmount } = | ||
| extractGaslessDepositFields(depositMessage); | ||
| return validateDeposit( | ||
| // @dev ??= is safe for a false verdict: false is not nullish, so it is not re-evaluated. | ||
| return (this.validatedMessages[depositKey] ??= validateDeposit( |
There was a problem hiding this comment.
When a batch estimation fails or is gas-deferred, its claim is deleted and the API message is evaluated again on a later poll, but this cache key contains only the input token, origin chain, and predicted deposit ID. If the API supplies a revised request with that same key but different destination token or amounts, the earlier true verdict bypasses the token-pair and inputAmount >= outputAmount checks, allowing the relayer to submit and potentially fill data that validateDeposit would reject. Cache only the paging side effect, or key the verdict by the request ID/full validation inputs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, and fixed in 8e9c845 — took the "cache only the paging side effect" option.
The verdict cache was the wrong tool for the problem. Paging sits downstream of every check that can reject and doesn't feed the return value, so suppressing the log is the whole of what was needed; caching the verdict on top of that was over-broad, and the deposit key (inputToken, originChainId, depositId) doesn't pin the output side, exactly as you say.
_validateDepositMessage() now re-runs validateDeposit() on every call and only withholds the logger:
const pageKey = `${this._getDepositKeyFromMessage(depositMessage)}:${inputAmountForValidation.toString()}`;
const pagingLogger = this.pagedDeposits.has(pageKey) ? undefined : this.logger;
this.pagedDeposits.add(pageKey);One thing worth flagging beyond what you found: the key carries the input amount. Keying the suppression on the deposit key alone would have had a second failure mode in the other direction — a deposit first seen under the threshold, then revised above it, would be permanently silenced and the alert lost. The old verdict cache did exactly that.
Both directions are now regression-tested in test/GaslessRelayer.ts, and both fail against the previous cache (verified by reverting the source change):
Re-validates a revised request sharing a deposit key with an accepted one— same key,outputAmount > inputAmount, must be rejected rather than served the cachedtrue. Previously returnedtrue.Pages again when a revision crosses the threshold the original sat under— previously paged 0 times, now 1.
|
Addressed the Codex review in 8e9c845. One inline comment, one P1, and it was correct. What it caught. The verdict memoisation keyed on Fix. Took the "cache only the paging side effect" option. Paging is One thing beyond the review. The suppression key carries the input amount. Keying on the deposit key alone fails in the other direction: a deposit first seen under the threshold, then revised above it, would be permanently silenced and the page lost. The old verdict cache did exactly that — it wasn't just a validation-bypass risk, it was also dropping an operational alert. Both directions are regression-tested, and I confirmed both fail against the previous cache by reverting the source change and re-running:
The pre-existing one-page-per-deposit test still passes unchanged. Full That was the whole review — nothing deferred, no further iteration needed on my side. |
evaluateApiSignatures() has no per-message error boundary, so one bad message takes down the whole poll. The API re-supplies it next tick, so it does so once per tick indefinitely. Add the boundary, and split the two failure classes it catches, because they want opposite handling: Malformed -- a derivation from the message payload throws. Deterministic, so the message is dropped permanently and logged once per requestId. _isProcessable() checks these at ingestion in _queryGaslessApi(), which is also the only boundary initialize()'s observation pass gets: a message whose inputToken won't parse used to fail every restart before any healthy sibling was reached. Operational -- an RPC or submission failure inside the state machine. Never dropped; an outage hits every message at once, so treating one as unprocessable would exclude healthy deposits with it. The message is deferred: the fill lock is handed back, or later deposits from the same depositor strand on an owner that is gone, and the state rewinds to unclaimed only while nothing can be on chain yet. Past DEPOSIT_SUBMIT it stays put, since re-running from INITIAL could submit the deposit twice. restructureGaslessDeposits() needed the same isolation one level lower; that is #3747, submitted separately. Co-Authored-By: Claude <noreply@anthropic.com>
… batches Paging. processBatch() releases a message's claim when it fails estimation or is gas-deferred, so messageFilter() re-admits it on the next poll and re-runs validateDeposit(), which pages on a deposit over the USD threshold. At the default 1s poll interval a large deposit that keeps failing estimation paged roughly once per second. Memoise the verdict on the deposit key: one page per deposit, however often it is re-presented. Fault isolation. Claiming, validation, batch grouping and calldata encoding all run outside the per-message state machine, where a throw takes down the whole poll. Each now sits behind a per-message boundary. validateDeposit()'s token lookups are the live case (ACB-552); those are message properties, so the offender is dropped. Single-call batches. The trivial-batch check ran before planning, so pruning could leave one call wrapped in tryAggregate, paying the wrapper and forfeiting the direct path's spray. Re-check after planning and let the survivor fall through to individual submission. Co-Authored-By: Claude <noreply@anthropic.com>
Memoising validateDeposit()'s return value on the deposit key was too broad a tool for the problem. The deposit key is (inputToken, originChainId, depositId) and doesn't pin the output side, so a revised request re-presented under the same key inherited the earlier verdict and skipped the token-pair and inputAmount >= outputAmount checks. Paging is the only side effect and sits downstream of every check that can reject, so it never feeds the return value. Suppress just that: withhold the logger once the deposit has paged and re-run the checks every call. The dedupe key carries the input amount, so a revision crossing the threshold pages afresh instead of inheriting the original's silence -- the verdict cache dropped that alert entirely. Both properties are covered by tests that fail against the previous cache. Co-Authored-By: Claude <noreply@anthropic.com>
8e9c845 to
013d7b8
Compare
1 participant
Rebuilt on the slimmed #3745. Its own contribution is unchanged; the diff shown here includes #3745 as a merge because it uses
_deferMessage/_dropMessage, and that part disappears once #3745 lands or #3721 is retargeted onto it.Paging.
processBatch()releases a message's claim on estimation failure or gas deferral, somessageFilter()re-admits it next poll and re-runsvalidateDeposit()— which pages for a deposit over the USD threshold. At the default 1s poll interval a large deposit that keeps failing estimation paged ~1/s.Only the paging side effect is suppressed, never the verdict: the logger is withheld once a deposit has paged, and the checks re-run on every call. An earlier revision of this cached
validateDeposit()'s return value on the deposit key, which was wrong — the key is(inputToken, originChainId, depositId)and doesn't pin the output side, so a revised request re-presented under the same key inherited the old verdict and skipped the token-pair andinputAmount >= outputAmountchecks. The dedupe key carries the input amount, so a revision that crosses the threshold pages afresh instead of inheriting silence.Fault isolation. Claiming, validation, grouping and calldata encoding all run outside the per-message state machine, where a throw takes down the poll. Each is now behind a per-message boundary.
validateDeposit()'s token lookups are the live case (ACB-552,TOKEN_SYMBOLS_MAPmisses on long-tail swap tokens); those are deterministic message properties, so the offender is dropped.Single-call batches. The trivial-batch check ran before planning, so pruning could leave one call wrapped in
tryAggregate— paying the wrapper and forfeiting the direct path'sspray. Re-checked after planning; the survivor falls through to individual submission.Two existing tests asserted a one-call batch as the expected outcome; rewritten with three messages so they still cover release/defer against a real (≥2) batch.
🤖 Generated with Claude Code