fix(HubPoolClient): Guard running-balance fallback query by active pool rebalance route#1472
fix(HubPoolClient): Guard running-balance fallback query by active pool rebalance route#1472droplet-rl wants to merge 2 commits into
Conversation
…ol rebalance route The full-history eth_getLogs fallback added in #1466 fires for any (l1Token, chain) pair without an executed bundle in the lookback window, including pairs that have no pool rebalance route. For those pairs the query scans the entire HubPool history yet can never match a RootBundleExecuted leaf, so it always resolves to a zero running balance anyway -- but the query is expensive and has been observed to stall bundle reconstruction on mainnet. Skip the fallback unless a pool rebalance route is actively defined for the requested (l1Token, chain) pair. The function interface is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 086222db25
ℹ️ 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".
| // (l1Token, chain) pair; without a route there can be no RootBundleExecuted leaf for it, so the fallback | ||
| // is guaranteed to resolve to a zero running balance and the (potentially expensive, full-history) | ||
| // eth_getLogs query would be wasted. | ||
| if (!isDefined(executedRootBundle) && this.l2TokenEnabledForL1Token(l1Token, chain)) { |
There was a problem hiding this comment.
Check the route at the requested block
When a pool rebalance route was enabled for the requested historical block but has since been disabled, l2TokenEnabledForL1Token checks only the latest route state (Number.MAX_SAFE_INTEGER). In that scenario an executed bundle for this pair can exist before the disable, but if it is outside the loaded lookback this guard skips the historical RootBundleExecuted query and returns a zero running balance. This regresses historical balance reconstruction for tokens/chains whose routes were disabled after their last execution; use the route state at the queried block/range instead of the current active route.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 24f10a1. The guard now checks the route state as of the queried block instead of the latest state, via a new poolRebalanceRouteExistedAtBlock(l1Token, chain, block) helper. It returns true if any non-zero route mapping for the pair was published at a block ≤ block, so it stays true even for a pair whose route was later disabled — exactly the "disabled after last execution" case you describe. (I went one step further than checking only the latest mapping ≤ block: a route enabled, then disabled, all before block also keeps it true, since a leaf — and a non-zero running balance — can still exist from when it was active.) Added a test (...route was disabled after its last execution) that disables the route via a zero-address remap after the leaf and confirms the fallback still recovers the balance; it fails under the old Number.MAX_SAFE_INTEGER guard.
| // (l1Token, chain) pair; without a route there can be no RootBundleExecuted leaf for it, so the fallback | ||
| // is guaranteed to resolve to a zero running balance and the (potentially expensive, full-history) | ||
| // eth_getLogs query would be wasted. | ||
| if (!isDefined(executedRootBundle) && this.l2TokenEnabledForL1Token(l1Token, chain)) { |
There was a problem hiding this comment.
Don't treat unloaded routes as disabled
When a client uses the public selective update path such as update(["RootBundleExecuted"]), route events are not requested/backfilled, so l1TokensToDestinationTokensWithBlock remains empty even for routed pairs. This guard then treats the pair as disabled and suppresses the historical RootBundleExecuted query, returning zero for balances that only exist before the lookback; ensure route state is loaded before applying the guard, or fall back when route state is unavailable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, this was a real regression risk — fixed in 24f10a1. The client now tracks which events have actually been queried in a loadedEvents set (populated in update()), and the guard only suppresses the fallback when loadedEvents.has("SetPoolRebalanceRoute"). If route events were never loaded (e.g. a selective update(["RootBundleExecuted"])), mayHaveLeaf is forced true and we fall through to the query — restoring the pre-PR behaviour rather than misreading an empty route map as "every pair disabled". Added a test (...route state was never loaded (selective update)) that updates with only ["RootBundleExecuted"] and asserts the fallback still runs (queryFilter called) and recovers the true balance instead of defaulting to zero.
…load-aware Address Codex review feedback on the fallback-query guard: - Check the pool rebalance route state as of the queried `block` (via the new `poolRebalanceRouteExistedAtBlock`) rather than the latest state. A route that was active when a leaf was created -- but has since been disabled -- no longer wrongly suppresses the historical query. - Only apply the guard when `SetPoolRebalanceRoute` events have actually been loaded (tracked via `loadedEvents`). A selective `update(["RootBundleExecuted"])` no longer leaves an empty route map that gets misread as "every pair disabled", which would silently return zero for real pre-lookback balances. Adds tests for both: an unrouted-but-unloaded selective update still runs the fallback, and a route disabled after its last execution still recovers the balance. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex thanks for the review — both P2 suggestions were valid and are addressed in 24f10a1. 1. Check the route at the requested block. The guard now inspects the pool rebalance route state as of the queried 2. Don't treat unloaded routes as disabled. The client now tracks queried events in a Added two unit tests covering both paths; existing tests, lint, and typecheck pass. Please take another look. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 086222db25
ℹ️ 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".
| // (l1Token, chain) pair; without a route there can be no RootBundleExecuted leaf for it, so the fallback | ||
| // is guaranteed to resolve to a zero running balance and the (potentially expensive, full-history) | ||
| // eth_getLogs query would be wasted. | ||
| if (!isDefined(executedRootBundle) && this.l2TokenEnabledForL1Token(l1Token, chain)) { |
There was a problem hiding this comment.
Check route state at the requested block
When a route existed at block but was later disabled, l2TokenEnabledForL1Token checks the latest route mapping, so this guard skips the historical RootBundleExecuted query and returns a zero running balance. Callers such as addLastRunningBalance pass historical bundle end blocks, so disabling a route after its last execution can make previously carried balances disappear from the next bundle; the route predicate needs to evaluate whether the route existed as of the queried block.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already addressed in 24f10a1 (this review re-ran against the original commit 086222db25). The guard no longer uses the latest route state — it now checks poolRebalanceRouteExistedAtBlock(l1Token, chain, block), which returns true if any non-zero route mapping for the pair existed at a block ≤ the queried block. So a route that was active when a leaf was created but disabled afterward (e.g. for the historical bundle-end blocks addLastRunningBalance passes) still triggers the fallback query and recovers the carried balance. Covered by the new test ...route was disabled after its last execution.
| // (l1Token, chain) pair; without a route there can be no RootBundleExecuted leaf for it, so the fallback | ||
| // is guaranteed to resolve to a zero running balance and the (potentially expensive, full-history) | ||
| // eth_getLogs query would be wasted. | ||
| if (!isDefined(executedRootBundle) && this.l2TokenEnabledForL1Token(l1Token, chain)) { |
There was a problem hiding this comment.
Do not suppress fallback before route events are loaded
If a caller uses the public selective update path, e.g. update(["RootBundleExecuted"]), SetPoolRebalanceRoute events never populate l1TokensToDestinationTokensWithBlock, so l2TokenEnabledForL1Token returns false for every pair and this guard disables the fallback even though routes are simply unloaded. In that context, an older running balance outside the lookback still resolves to zero instead of being recovered from historical RootBundleExecuted logs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already addressed in 24f10a1 (this review re-ran against the original commit 086222db25). The client now tracks queried events in a loadedEvents set, and the guard only suppresses the fallback when loadedEvents.has("SetPoolRebalanceRoute"). Under a selective update(["RootBundleExecuted"]) the route map is empty because routes were never loaded, so the guard is bypassed and the historical query still runs — recovering pre-lookback balances instead of returning zero. Covered by the new test ...route state was never loaded (selective update).
|
This review re-ran against the original commit
Both paths have new unit tests; lint and typecheck pass. No further changes needed — replied on the two inline threads. (Not re-tagging for a re-review since |
Summary
Follow-up to #1466 (commit
79e97a68), which madeHubPoolClient.getRunningBalanceBeforeBlockForChainfall back to a full-historyeth_getLogsquery when noRootBundleExecutedevent for a(l1Token, chain)pair is found within the configured event lookback.That fallback fires for any pair missing from the in-memory lookback — including pairs that have no pool rebalance route at all. For such a pair the query scans the entire HubPool history (deployment block → start of lookback) yet can never match a leaf, so it always resolves to a zero running balance regardless. The query itself is expensive, and on mainnet it has been observed to stall bundle reconstruction (a single hung
eth_getLogsblocking the whole cycle).Change
Guard the fallback with
l2TokenEnabledForL1Token(l1Token, chain)— only issue the query when a pool rebalance route is actively defined for the requested(l1Token, chain)pair. Without a route there can be noRootBundleExecutedleaf for the pair, so the guarded behaviour is identical (still resolves to zero) while skipping the wasted RPC call.async/Promise<TokenRunningBalance>; all existing callers already await it.4.4.4→4.4.5.Test
Added a unit test asserting that for an unrouted
(l1Token, chain)pair the fallbackqueryFilteris not invoked (sinon spy) and the balance defaults to zero. The existing test covering the route-defined fallback path is unchanged and still passes.🤖 Generated with Claude Code