fix(trade): block stale oracles for every mode, not an allowlist (GH#2484) - #2485
Open
dcccrypto wants to merge 1 commit into
Open
fix(trade): block stale oracles for every mode, not an allowlist (GH#2484)#2485dcccrypto wants to merge 1 commit into
dcccrypto wants to merge 1 commit into
Conversation
…2484)
The stale-oracle trading gate was an inline ALLOWLIST of oracle modes,
duplicated verbatim across four trade components:
oracleLevel === "stale" && (mode === "admin" || "hyperp" || "keeper")
`OracleMode` has four members. "pyth-pinned" was missing from all four
copies, so a Pyth market whose price stopped advancing stayed tradeable
while admin/hyperp/keeper markets in the identical stale state were
blocked — the user signs a transaction the on-chain Pyth staleness filter
is expected to reject, and any worst-fill bound is derived from the stale
mark.
The allowlist has now leaked twice: "keeper" was missing before (H7), and
"pyth-pinned" after that. GH#2484 reports the order ticket; the same
expression had spread to the three position surfaces, which gate managing
and closing positions.
Extract the predicate to lib/oracle-stale-gate and invert the default:
stale blocks every recognised mode, and exemptions must be declared.
STALE_EXEMPT_MODES is a Record<OracleMode, boolean> rather than an array,
so adding a member to the union is a compile error until it is classified
— a new mode cannot reach production unclassified, which is the failure
this keeps having. Every entry is false today.
Callers keep their own "unavailable" handling, which differs deliberately:
the order ticket excludes it (it has its own message), the position panels
fold it in. Behaviour for admin/hyperp/keeper is unchanged.
Tests assert both halves. The predicate is driven off STALE_EXEMPT_MODES
rather than a hand-written mode list, so a new mode fails the test instead
of silently passing; and each of the four components is asserted to call
the shared gate and to carry no inline allowlist. That second half is what
binds the call sites — a unit test of the predicate alone still passes if
a component reverts to its own copy, which is exactly how this spread.
Mutation-verified: flipping "pyth-pinned" back to exempt fails 2 tests;
re-inlining the allowlist in one component fails 2 more.
Suite: 2939 passed / 0 failed. tsc --noEmit clean.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughChangesThe PR adds a shared Oracle stale gating
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/components/trade/OrderTicket.tsx`:
- Around line 305-310: Update handleTrade to revalidate blockingIssue
immediately before calling trade(...), rejecting the submission when an oracle
becomes stale after the confirmation modal opens. Preserve the existing modal
flow for submissions without a blocking issue, and add a regression test
covering an oracle becoming stale while the confirmation modal remains open.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e9d05515-6d84-4be0-8c5a-e3e06ece04ed
📒 Files selected for processing (6)
app/__tests__/lib/oracle-stale-gate.test.tsapp/components/trade/OrderTicket.tsxapp/components/trade/OtherMarketPositions.tsxapp/components/trade/PositionPanel.tsxapp/components/trade/PositionsDock.tsxapp/lib/oracle-stale-gate.ts
Comment on lines
+305
to
+310
| // GH#2484: this was an inline ALLOWLIST of oracle modes, and it leaked twice — | ||
| // first "keeper" (H7: a stale keeper-priced market never blocked trading, | ||
| // firing for 0/5 live markets), then "pyth-pinned". The predicate now lives in | ||
| // lib/oracle-stale-gate and blocks every recognised mode by default, so the | ||
| // next mode added to the union cannot silently trade on a stale price. | ||
| const oracleStale = !oracleUnavailable && isOracleStaleBlocking(oracleLevel, oracleMode, oracleReady); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Revalidate blocking state in handleTrade.
If the oracle becomes stale after the confirmation modal opens, submitDisabled no longer controls submission. The modal calls handleTrade directly, and handleTrade does not reject blockingIssue before it calls trade(...).
Reject the submission when blockingIssue is present. Add a regression test for an oracle that becomes stale while the confirmation modal is open.
Proposed fix
- if (!marginInput || !userAccount || effectiveSize <= 0n || exceedsBalance) return;
+ if (!marginInput || !userAccount || effectiveSize <= 0n || exceedsBalance || blockingIssue) return;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/components/trade/OrderTicket.tsx` around lines 305 - 310, Update
handleTrade to revalidate blockingIssue immediately before calling trade(...),
rejecting the submission when an oracle becomes stale after the confirmation
modal opens. Preserve the existing modal flow for submissions without a blocking
issue, and add a regression test covering an oracle becoming stale while the
confirmation modal remains open.
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.
Fixes #2484.
The reported bug is real, and wider than reported
Verified the premise on
playground@f2a3bbe5before touching anything:OracleModeis a four-member union (oraclePrice.ts:40) and the gate atOrderTicket.tsx:307listed three of them. Confirmed.The part the issue doesn't cover: the same expression is duplicated in four
components, not one.
components/trade/OrderTicket.tsx:307components/trade/PositionPanel.tsx:243components/trade/PositionsDock.tsx:141components/trade/OtherMarketPositions.tsx:92All four carried the identical
admin || hyperp || keeperallowlist, so a stalePyth market also bypassed the gates on the close/manage surfaces. Fixing only
the order ticket would have left three copies to drift again.
The fix
Took the issue's preferred option (fail-safe default) rather than the minimal
one, since the allowlist has now leaked twice —
keeperwas missing before (theH7 comment is still in the file), then
pyth-pinned.lib/oracle-stale-gate.tsholds one predicate. Stale blocks every recognisedmode; exemptions must be declared:
Record<OracleMode, boolean>rather than the issue's suggested array is the onedeliberate deviation, and it's the point of the change: adding a member to
OracleModeis a compile error until it's classified here. An array-based!STALE_EXEMPT_MODES.includes(mode)is fail-safe at runtime but still lets a newmode ship unconsidered. This makes that impossible rather than merely unlikely.
Callers keep their own
unavailablehandling, which differs on purpose — theorder ticket excludes it (it has its own message), the position panels fold it
in. Behaviour for
admin/hyperp/keeperis unchanged.Tests, and what actually binds them
Two halves, because the first alone would not have caught this bug:
Object.keys(STALE_EXEMPT_MODES), not a hand-written array, so a new modefails the test rather than quietly passing it.
contain no inline allowlist. This is what binds the call sites. A unit test
of the predicate on its own stays green if a component reverts to its own
copy — which is precisely how this defect spread to four files.
Mutation-verified both ways:
"pyth-pinned"back to exempt (restores #2484)PositionsDockonlySeverity
Agree with the issue's Medium, and for its stated reason: the on-chain Pyth
staleness filter is still the fund-safety backstop, so this is a UI safety-gate
gap rather than a direct loss-of-funds path. What it costs users is signing
transactions that are expected to fail, and a worst-fill bound anchored to a
stale mark in the window before the on-chain filter rejects it. The close/manage
surfaces being affected too is why I'd not leave it sitting.
Summary by CodeRabbit