fix(bots): normalize the position join key across log events - #189
Open
haydenshively wants to merge 2 commits into
Open
fix(bots): normalize the position join key across log events#189haydenshively wants to merge 2 commits into
haydenshively wants to merge 2 commits into
Conversation
haydenshively
changed the title
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. |
Every position-scoped log event in both liquidators now carries the position in one field, `id`, valued `lensKey(marketId, borrower)` — the composite the helper already lowercases on both halves. A maturity's events therefore group into one row per position with no normalization in the query. Before this, the same position was identified three different ways: `marketId` + `borrower` (checksummed) on the plan and simulate events, a single lowercased `id` on the select and quote events, and that same string under the name `label` on `tx.*`. Grouping by the raw composite returned 26 rows for 13 positions on the 2026-08-28 maturity, which reads as if the bot planned positions it never quoted. `marketId` and `borrower` stay on `plan.built` as human-readable extras. Since a tick is multi-collateral, `id` identifies a POSITION and the per-candidate key is `(id, collateralIndex, postMaturityMode)`. That pair now threads through `QuoteRequest.candidate` so the `@repo/swaps` quote events can separate two candidates of one position; it inherits `id`'s correlation-only contract and nothing in the package branches on it. In the pending queue only the EMITTED field is renamed. `SubmitArgs.label` and the in-flight map keys keep their name because they are behavioral: `inflightLabels().has(label)` is tested every tick, so touching the value would let a second nonce-consuming send go out for a position already in flight. That also changes the `tx.*` log schema of the reallocation bots and midnight-crossed-books, which pass a vault address or a market id as the same key. Closes BOTS-90. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four documentation claims the code did not deliver, and one duplicated comment: - The READMEs globbed `unwrap.*` / `queue.*` / `nonce.*` as carrying `id`. Both now enumerate the position-scoped events exactly and list what is scoped to a tick, a venue pair, the queue, or the process instead — a glob over a mixed set is what makes a `GROUP BY id` drop rows into a null bucket. - `unwrap.preview_reverted` / `unwrap.preview_zero` genuinely were position-scoped and carried no `id`: `previewRedeem` is amount-dependent, so unlike the `asset()` probe it is not memoized and fires per candidate. Correlation now threads through `Unwrapper.resolve`, which is also why `resolveRoute` takes the label. - The claimed `probe.*` exception was false for `probe.error`, which does carry the correlation. Named the four pair-scoped events instead. - `correlationOf`'s TSDoc claimed every event in `@repo/swaps`; the venue selector and both unwrappers carry none. - The `plan.built` comment was duplicated verbatim in both ticks and narrated two self-evident fields. The READMEs are its one home. `docs/CONVENTIONS.md` gains the rule itself, so `lensKey`'s TSDoc, the two READMEs and `SubmitArgs.label` point at one place rather than restating it four times. The three other queue consumers now say what their `tx.*.id` is: a checksummed vault address, or a market id. It joins to nothing in those bots, which key their own events on `vault`. Tests: `@repo/swaps` pins the discriminator spread, the non-shadowing of `id`, and that correlation reaches an unwrap hop; `@repo/bot-kit` pins the emitted field name on `tx.sent` and `tx.confirmed` in the package that owns the schema. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
haydenshively
added a commit
that referenced
this pull request
Sep 1, 2026
A regression sweep over #184/#187/#188/#189 found four behaviors the stack changed without meaning to. None are new functionality; each restores what the bots did on main. Detection-only blue could broadcast. The no-venues gate moved to AFTER unwrap resolution and the swap-free branch lost its `steps.length > 0` guard, so a venue-less deployment returned `kind: 'swap'` — which the tick takes straight to simulate+submit. docker-compose defaults Robinhood (4663) to ALLOW_DETECTION_ONLY with a funded key, documented as skipping every routed liquidation. `swapFreeWithoutVenues` (default false) restores the immediate refusal; midnight opts in, because its loan-as-collateral slots need no route and ALLOW_BAD_DEBT_ONLY is a supported posture there. That same reorder downgraded a transient unwrapper RPC failure from `no_config` (skip) to `failed` (arms backoff), pushing a deliberately unarmed deployment into a suppression state machine it never entered. Refusing before the unwrap chain fixes both at once, and spends no reads doing it. A send REJECTION now arms backoff even when a sibling execution-reverted. Both sets are keyed by position, so `backoffExempt` was cancelling the backoff a broken nonce/funds/RPC send earned; the position then re-sent every block while the send machinery was still broken. Phase A.5 no longer resolves routes for suppressed positions. It ran ahead of the cooldown/backoff gates, so a backed-off position spent an uncached read per candidate per tick — breaking backoff's contract that it bounds API and RPC usage under a backlog, and putting that latency in front of the first send of a maturity burst. Each of the 8 new tests was verified to fail against the pre-fix source. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
haydenshively
force-pushed
the
fix/normalize-position-join-key
branch
from
September 1, 2026 03:52
a86a7a5 to
564ebdd
Compare
haydenshively
marked this pull request as draft
September 1, 2026 03:54
![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: a86a7a520b
ℹ️ 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".
haydenshively
marked this pull request as ready for review
September 1, 2026 03:59
haydenshively
added a commit
that referenced
this pull request
Sep 1, 2026
Triaged all 20 Codex/Devin findings across #184, #187, #188, #189 and #192. Fixed: - the venue-less swap-free exception admitted unwrap-only plans, so an ALLOW_BAD_DEBT_ONLY deployment could broadcast an asset-moving liquidation - candidate ranking trusted stale and incomplete curves that quoting refuses; both now share one exported `curveIsTrusted` - a Pendle PT resolved its unwrap chain twice per tick, both hosted API calls; new `previewTokenOut` seam answers phase A.5 from cache - phase A.5 resolved routes serially, in discovery order - a sibling's `no_route` suppressed a position whose other candidate was `floor_unmet`, which is meant to retry every block as the LIF ramps - the wall-clock cooldown verdict could flip mid-tick, leaving a candidate quoted but unpriced - revert-streak state never expired, so a reused label reported false crossings - `tx.submit_failed` carried no candidate discriminator - docs: the swap-free "iff" guarantee, `selectorConstant`, two README rows Declined, with reasoning for the threads: narrowing the backoff exemption to post-maturity plans (a regression — the sets are per-position while the mode is per-candidate), `stopAfterWinner`, and the sub-1e-18 rate truncation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes BOTS-90. Top of the stack — this PR's own diff is
fix/send-revert-classification...fix/normalize-position-join-key.Why
The correlation id joining a position across pipeline stages differed in field name, shape and letter case, so a naive
GROUP BYsplit one position into rows that looked like different positions. Grouping the 2026-08-28 maturity by the raw composite returned 26 rows for 13 positions — read without care, that says the bot planned 13 positions it never quoted and quoted 13 it never planned.plan.built,simulate.*marketId+borrower, separateselect.ok,quote.*idmarketId:borrowerlowercasedtx.*labelThe canonical key already existed —
lensKey()— and was already the value behind everylabel/id. Only the field name and the checksummed pair diverged.What
logger.*sites across both liquidators,@repo/swapsand@repo/bot-kit, classifying each as position-scoped or not. That table is in the commit history and the READMEs; the issue existed because nobody had one.idon every position-scoped event —plan.built,plan.skipped,cooldown.skip,config.no_swap_path,simulate.*,quote.unprofitable,send.revert_streak,preselect.skipped,route.unresolved,quote.excluded_collateral,select.cold_default, and the@repo/swapsquote events.marketId/borrowerstay onplan.builtas human-readable extras.QuoteRequest— feat(midnight-liquidation): support loan-as-collateral markets #184 means one position yields several(slot, mode)candidates sharing oneid, soselect.okrows were otherwise indistinguishable.idstays the per-position join key;(id, collateralIndex, postMaturityMode)identifies a candidate. Correlation-only: nothing in@repo/swapsbranches on it.label→idacross 16 sites.SubmitArgs.label,Pending.label, thesettledAtkeys andinflightLabels()keep the name — they are behavioral, and changing the value would breakinflightLabels().has(label)and produce duplicate nonce-consuming sends. Only the log key changes; the invariant is documented atSubmitArgs.label.Notes for review
unwrap.preview_reverted/unwrap.preview_zerowere position-scoped and uncorrelated —previewRedeemis amount-dependent, so unlike the memoizedasset()probe they fire per candidate. Fixing that needed correlation threaded throughUnwrapper.resolve.id: probe events (venue-pair scoped — several positions share one probe), Pendle markets-list events, and the queue-widereconcile.failed/queue.nonce_hole_cleared(exactly therefusedclass that is held against no position).send.revert_streakgetsidbut no discriminator — the streak is keyed by position and spans whichever siblings reverted, so attributing it to one(slot, mode)would misreport it.tx.*schema for the three other queue consumers too (both reallocation bots, crossed-books). Intended; their READMEs now state what theirtx.*.idis and that it does not join to their ownvault-keyed events.Monitoring
The BetterStack coupling is not in dashboard SQL — charts query the metrics collection, where
label('…')is the metrics-label accessor, not our field. The real dependency is two metric expressions parsing the raw field (splitByChar(':', coalesce(JSONExtract(raw,'label',…),''))[1|2]→tx_market/tx_borrower), which would have gone silently blank. Both are already updated tocoalesce(id, label)on prod midnight (2607569) and staging (2639416) — a no-op until this ships. Blue has notx_*expressions, and the reallocation bots and crossed-books have no BetterStack sources at all.Dropping the
labelfallback waits until this is deployed.Verification
pnpm test211 files / 2845 tests, 0 failed · lint 0 warnings · knip clean · typecheck across all eight affected packages · fork suite 3/3. The acceptance criterion is a test:plan.builtandtx.sentmust carry the sameid, broadcast through the real queue — verified to fail when the production field is reverted tolabel.🤖 Generated with Claude Code