fix(quoter-bot): require unbounded approval, warn on orphaned groups - #198
Conversation
Readiness compared two decreasing quantities against constants, so ordinary operation eventually halted the whole fail-together lifecycle. The loan-allowance gate required `allowance >= MAXIMUM_LEND_EXPOSURE_ASSETS`. An ERC-20 allowance is a cumulative lifetime spend budget, not an outstanding exposure cap, so relending consumes it and the bot halts well above zero while still solvent. Readiness now requires an unbounded approval, and exposure is bounded by maker funds and the ladder's existing per-market and total caps. `MAXIMUM_LEND_EXPOSURE_ASSETS` has no remaining consumer and is removed. The offers gate failed on any group it could not attribute to itself. Group ownership is a local durable record, so redeploying onto a fresh filesystem orphans the bot's own live groups; readiness then cannot recover until every orphan expires. Unattributable groups now warn without blocking readiness, while offers on unconfigured markets and crossed books still fail. `setup.check-failed` now carries `status`, so the record naming the halt cause ships at error level instead of info alongside a causeless `bot.failed`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| invertedMarketIds: readonly Hex[] | ||
| }): SetupCheck['status'] => { | ||
| if (offers.unknownMarketIds.length > 0 || offers.invertedMarketIds.length > 0) return 'failed' | ||
| return offers.unknownNamespaces.length > 0 ? 'warning' : 'passed' |
There was a problem hiding this comment.
🔴 Orphaned offers bypass exposure limits
When ownership state is lost, unsafeOffersStatus permits buy groups that both exposure calculators exclude from reservations. New offers can exceed configured exposure caps.
Prompt for agents
Unattributed groups now produce only a warning in unsafeOffersStatus, but the strategy calculators still count only durably owned IDs. In production-ladder.ts, ladderCashReservations excludes indexed groups absent from bootstrapGroupIds or ladder publication state. In production-bootstrap.ts, readGroupInventory and bootstrapReservedLoanAssets likewise select groups through owned IDs. After ownership storage is lost, the bot therefore resumes and publishes against exposure and cash capacity that ignores each orphaned buy group's remaining amount. Preserve non-blocking readiness while ensuring all relevant live maker buy groups on configured markets count toward market and total reservations. Keep ownership restrictions for reconciliation, cancellation, and replacement so unattributed groups are never treated as strategy-owned.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Confirmed and fixed in e94d162 — this was a real regression, thank you.
Verified both calculators gated exposure on attribution: ladder-cash-reservation.utils.ts:62 skipped any group not in bootstrapGroupIds ∪ ladder publication IDs, and bootstrap-groups.utils.ts:286 filtered on ownedGroups.has(group.id). So an orphaned buy group reserved nothing, unreservedCash ran high and currentExposure ran low, and fresh offers could be sized past maximumTotalExposureAssets. The hard halt I removed had been implicitly covering that blind spot.
Both calculators now count every live buy group in the maker's own book — the groups come from /users/{maker}/offer-groups, so each one commits maker cash whether or not durable ownership still records it. The ownedGroupIds / bootstrapGroupIds parameters no longer participate in either total and were removed. Ownership still gates reconciliation, cancellation, and replacement, exactly as you suggested: replacedGroupIds and ignoredGroupIds are untouched, and unattributed groups are never treated as strategy-owned.
Regression test added (reserves a live buy group the durable ownership store no longer attributes), verified to fail when the ownership filter is restored.
I also corrected the unsafeOffersStatus doc comment, which had asserted the opposite of what the code did — it claimed exposure was derived from live on-chain groups regardless of attribution. It now records that the warning downgrade is only safe because reservations are attribution-independent.
![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: d78d0e7002
ℹ️ 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".
| invertedMarketIds: readonly Hex[] | ||
| }): SetupCheck['status'] => { | ||
| if (offers.unknownMarketIds.length > 0 || offers.invertedMarketIds.length > 0) return 'failed' | ||
| return offers.unknownNamespaces.length > 0 ? 'warning' : 'passed' |
There was a problem hiding this comment.
When a redeploy loses durable ownership—the exact scenario represented by unknownNamespaces—turning this check into a warning lets bootstrap and ladder publish again without reserving the orphaned groups' remaining buy capacity. ladderCashReservations excludes indexed groups absent from its durable owned set (ladder-cash-reservation.utils.ts:55-66), and bootstrap similarly derives active groups and reservations only from owned IDs (production-bootstrap.ts:385-405), so the new offers can commit cash and exposure already promised by orphaned offers; if both fill, transactions can revert for insufficient balance or total/market exposure can exceed configured caps. Unknown groups should be adopted, canceled, or conservatively included in reservations before they stop blocking readiness.
Useful? React with 👍 / 👎.
| check.status === 'failed' || check.status === 'warning' | ||
| ? [{ event: 'setup.check-failed', check: check.name, status: check.status }] |
There was a problem hiding this comment.
In the orphaned-group scenario this emits setup.check-failed even though the check does not fail readiness. The schema remains version 1, while the operator contracts still define this discriminator as a failed check and list only the check field (README.md:577 and docs/reference.md:476), so any existing consumer following that contract and alerting on the event name will report a failure every setup cycle. Preserve the failure-only discriminator for compatibility and emit warnings under a distinct event, or version and update the documented monitoring contract so consumers can reliably filter on status.
Useful? React with 👍 / 👎.
Devin flagged that downgrading unattributable groups to a warning let them bypass exposure limits: both reservation calculators selected through owned IDs, so an orphaned buy group reserved nothing and the bot sized fresh offers against capacity that ignored its remaining amount. The hard halt had been implicitly covering that blind spot. Reservations now count every live buy group in the maker's own book, whether or not durable ownership still attributes it. Ownership continues to gate reconciliation, cancellation, and replacement. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Closes BOTS-98
Also resolves the open question in BOTS-99 ("is automating the approval unsafe?") and produces a negative result for BOTS-71.
Problem
Readiness compared two monotonically decreasing quantities against constants, so ordinary operation eventually halted the entire fail-together lifecycle (
quoter-bot.service.tscallsstopFromWorkflow()on!report.ready).Allowance. The gate required
allowance >= MAXIMUM_LEND_EXPOSURE_ASSETS. An ERC-20 allowance is a cumulative lifetime spend budget, not an outstanding-exposure cap — a maker relending the same capital consumes it indefinitely — so the bot halts well above zero while fully solvent. The maker the "MM Bot Not Quoting" alert watches was set to a finite 35,000 USDC approval on Aug 19 and had drained to 9,266 against a 34,258 balance.Offers. The gate failed on any live group it could not attribute to itself. Group ownership is a local durable file, so redeploying onto a fresh filesystem orphans the bot's own groups; readiness then cannot recover until every orphan expires. Production halted for 29 hours on Sep 2 for exactly this — two groups,
0x53282161…and0xb430b169…, and it only recovered when they aged out.Observability.
setup.check-failedcarries the failing check name but nostatus, andhasFailurein@repo/observabilityselectserroronly onstatus: failed|haltedor anerrorName. So the record naming the cause sat atinfowhile the records announcing the halt (cycle.completed,bot.failed) were aterrorwith no cause.Changes
UNBOUNDED_ALLOWANCE_FLOOR = maxUint256 / 2n— half-max rather than equality because Circle's FiatToken decrements even amaxUint256approval on everytransferFrom(FiatTokenV1), verified on a Base fork. Exposure is bounded by maker funds and the ladder's existingtargetMarketExposureAssets/maximumTotalExposureAssets. Remediation now emitsapprove(midnight, maxUint256).MAXIMUM_LEND_EXPOSURE_ASSETSremoved — it had no remaining consumer.unsafeOffersStatus): unconfigured markets and crossed books still fail; unattributable groups warn without blocking readiness.setup.check-failedcarriesstatus, so the halt cause ships at error level.ladderCashReservations,bootstrapReservedLoanAssets). Both previously selected through owned IDs, so an orphaned buy group reserved nothing — making the warning downgrade above unsafe on its own, since fresh offers could be sized pastmaximumTotalExposureAssets. The groups come from the maker's own book, so each commits maker cash regardless of attribution. Ownership still gates reconciliation, cancellation, and replacement.minimum(cashBalance, allowance)inproduction-ladder.tsis deliberately kept: the setup monitor only re-runs every 60s, so it degrades a mid-run revocation to zero capacity instead of producing offers that revert on fill.setup.maximumLendExposureAssetsmust be deleted from existing YAML/helm values. Unknown keys are rejected andConfigFileError('unknown-key')is sanitized — it does not name the offending key — so a stale entry crash-loops the bot with no indication of which line is at fault. A staleMAXIMUM_LEND_EXPOSURE_ASSETSenvironment variable is inert (the loader iterates an allowlist) and needs no action, so Railway is unaffected. Documented in the README,docs/reference.md, and the helm README; helm chart bumped to0.2.0.Operator action: any maker still holding a finite approval must re-approve at
maxUint256before it will pass readiness.Verification
typecheck✅ ·pnpm lint0 warnings / 0 errors ✅ ·pnpm knip✅ ·jsdoc:build✅ ·npx vitest run2985 passed, 1 skipped ✅Each new behaviour was verified by breaking it: neutering the allowance floor fails both approval tests, restoring the fatal-on-unknown-namespace behaviour fails the warning test, and restoring the ownership filter on reservations fails the orphan-reservation test.
Pre-existing
playground:testfailures (macOS/private/varsymlink assertions) reproduce on a clean tree and are unrelated.Follow-ups not in this PR
LOAN_ASSET_ADDRESSis operator-configurable and every existing finite-approval maker now hits a nonzero→nonzeroapprove, which reverts on approve-race-guarded tokens. The hazard predates this change and every live deployment is USDC;approvePairin@repo/swapsalready exists if we want it.🤖 Generated with Claude Code