fix(create): a failed backing seed must not report unqualified success (GH#2514) - #2518
Conversation
…s (GH#2514) The v17 sequential launch path wraps the TopUpBackingBucket transaction in a try/catch, warns to the console, and continues to "Market created!". Both backing domains can therefore be left unseeded while the creator is told the launch succeeded. Staying NON-FATAL is right, and this keeps it. The Step 3 comment explains why: a transient RPC error must not strand an otherwise-live market, and a repeat TopUp against an already-Fresh-at-MAX bucket hits the harmless no-op arm. That reasoning still holds, and making the step fatal would reintroduce exactly the stranding it guards against. Staying SILENT is the defect, and what changed underneath it is the size. That comment was written (2026-07-09) when the seed was dust. It is now `backingSeedPerDomain(lp)` per domain -- 100% of LP collateral each at the current policy -- so swallowing the failure hands the creator a success screen for a market missing two allocations worth twice their LP. A best-effort decision made about dust was left in place for something 2x the LP deposit. So: the catch now records `backingSeedFailed` on the launch state, and the success screen renders a soft warning saying the market is live and tradeable BUT that counterparty backing was not seeded and should be retried before the market takes size. Both halves of that sentence matter -- dropping the first turns a soft warning into an apparent failure, dropping the second is the bug. This follows the existing convention for exactly this situation rather than inventing one: `insuranceMintFailed` (GH#1761) is the same shape -- a non-fatal step failure carried on the state and rendered as a warning banner on the success screen -- and `devnetMintError` carries a non-fatal error the same way. Scope is the sequential path only (retry/resume with startStep <= 3, or the pre-broadcast fallback from fresh batching), which is where the catch lives. A fresh batched launch puts the LP deposit and both top-ups in one atomic M3a, so its failure is already fatal after broadcast. Tests assert the wiring across all three files, because the property is "the catch records it AND the screen reads it" -- a render test of LaunchSuccess alone would pass even if the hook never set the flag. The catch block is isolated by index so a match elsewhere in a 3,000-line file cannot satisfy it. Mutation-tested, control either side. The third round is the over-correction, which the tests reject as firmly as the original bug: control 5 passed revert the catch to silent 1 failed drop the success-screen banner 2 failed make the step fatal (over-correct) 1 failed control 5 passed Verified: 305 files, 3075 passed, 17 skipped; tsc --noEmit exit 0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 4 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
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 |
|
Correcting my own PR description. I wrote that this and #2516 touch "different file, no overlap". That is wrong — both edit They do still compose, but now measured rather than claimed:
So either merge order works and neither needs to wait for the other. The accurate statement is "same file, disjoint hunks, verified to compose" — not "different file". The rest of the description stands. |
1 participant
Closes #2514.
The defect, stated precisely
The v17 sequential launch path wraps the
TopUpBackingBuckettransaction in atry/catch, warns to the console, and continues to "Market created!". Both backing domains can be left unseeded while the creator is told the launch succeeded.Staying non-fatal is right, and this PR keeps it. The Step 3 comment explains why, and the reasoning still holds: a transient RPC error must not strand an otherwise-live market, and a repeat TopUp against an already-Fresh-at-MAX bucket hits the harmless no-op arm. Making the step fatal would reintroduce exactly the stranding it guards against — which is why one of the mutations below is the over-correction, and the tests reject it as firmly as the original bug.
Staying silent is the defect — and what changed underneath it is the size. That comment was written on 2026-07-09, when the seed was dust. It is now
backingSeedPerDomain(lp)per domain — 100% of LP collateral each at the current policy. A best-effort decision taken about dust was left in place for something worth twice the LP deposit.The fix
The catch records
backingSeedFailedon the launch state, and the success screen renders a soft warning: the market is live and tradeable, but counterparty backing was not seeded and should be retried before the market takes size.Both halves of that sentence are load-bearing — dropping the first turns a soft warning into an apparent failure, dropping the second is the bug — and there's a test for each.
This follows the existing convention rather than inventing one.
insuranceMintFailed(GH#1761) is the same shape: a non-fatal step failure carried on the state and rendered as a warning banner on the success screen.devnetMintErrorcarries a non-fatal error the same way.Scope
The sequential path only — retry/resume with
startStep <= 3, or the pre-broadcast fallback from fresh batching — which is where the catch lives. A fresh batched launch puts the LP deposit and both top-ups in one atomicM3a, so its failure is already fatal after broadcast. That matches the issue's own scoping.Tests
Assertions span all three files, because the property is "the catch records it and the screen reads it" — a render test of
LaunchSuccessalone would pass even if the hook never set the flag. The catch block is isolated by index so a match elsewhere in a 3,000-line file cannot satisfy it, and the initial-state check countsbackingSeedFailed: falseagainstinsuranceMintFailed: falseso a missed initialiser (which would make the screen readundefinedand silently render nothing) fails.Mutation-tested
Verification
Related
#2516 (open) fixes #2515 in the same area — the launch gate under-requiring by the same two seeds. Different file, no overlap: that one touches
CreateMarketWizard'stotalTokensRequired, this one touches the hook's catch plus the success screen. They compose in either order.