Reconcile milestone status against real on-chain assertion state - #179
Conversation
JobsContext previously set local milestone status from the assumed result of a transaction, without ever reading get_assertion_state back. Now every action (submit/dispute/vote/finalize) reconciles from a real read afterward, and any milestone with an assertionId that isn't settled gets polled every 30s so status advances even when another party's action or an expired challenge window is what changed it. Adds a client-side, env-configurable challenge-window hint for finalize eligibility, since the contract has no getter for it. Closes drydocs#147
| refreshMilestone(jobId, milestone.id, address); | ||
| }, POLL_INTERVAL_MS); | ||
| return () => clearInterval(id); | ||
| }, [address, assertionId, settled, jobId, milestone.id, refreshMilestone]); |
There was a problem hiding this comment.
refreshMilestone's identity in JobsContext.tsx gets recreated on every jobs state change, so this effect's dependency array means any single milestone's poll tick or manual refresh resets the setInterval timer for every other actively-polling MilestoneRow on the page. With several submitted or disputed milestones open at once, if refreshes across them land more often than every POLL_INTERVAL_MS in aggregate, no individual milestone's timer ever survives a full interval uninterrupted, so its background reconciliation can be indefinitely postponed even though this looks like it guarantees a poll every interval. Consider stabilizing refreshMilestone with useCallback or useRef so one milestone's refresh doesn't reset another's timer.
| * `Assertion.opened_at` read; it never gates the `finalize` call itself — | ||
| * the contract remains the source of truth and rejects it if called early. | ||
| */ | ||
| export const CHALLENGE_WINDOW_SECS: number = import.meta.env.VITE_CHALLENGE_WINDOW_SECS |
There was a problem hiding this comment.
This is parsed with a bare Number() and no NaN or negative check. If VITE_CHALLENGE_WINDOW_SECS is set to a non-numeric value, Number() yields NaN, and every readyToFinalize comparison against it is then permanently false, silently hiding the ready to finalize hint for the life of the deployment with no error surfaced anywhere. Please validate the parsed value and fall back to the default (or throw) on NaN.
|
@collinsezedike Thanks for catching both — pushed a fix in b68aa16:
|
| try { | ||
| const { getAssertionState } = await loadTholosClient(); | ||
| const assertion = await getAssertionState(BigInt(assertionId), readAs); | ||
| setJobs((current) => updateMilestone(current, jobId, milestoneId, mapAssertionToPatch(assertion))); |
There was a problem hiding this comment.
This is an unconditional overwrite with no ordering guard against the read it came from. A background poll and an action-triggered reconcile can both be in flight for the same milestone at once, if the poll's RPC read is slower and resolves after the action's, its stale snapshot overwrites the newer status this same function just set moments earlier, silently reverting the UI (e.g. showing submitted again after a dispute just landed) until the next poll cycle corrects it. Consider tracking a request sequence number or timestamp per milestone and dropping a reconcile result that's older than the last applied one.
|
@JohnArayaE finalizeMilestone and the decided-vote branch of voteOnMilestone swallow reconcile-read failures with only a console.warn, instead of falling back to the already-known deterministic status. If the follow-up getAssertionState call throws right after a successful on-chain finalize or vote, the UI keeps showing stale pending status with active action buttons until the next 30s poll or a manual refresh, even though the money already moved. Please set state from the deterministic result you already have when the reconcile read itself fails. |
…ailure, guard against out-of-order reconciles
|
@collinsezedike Fixed both in d96c818:
pnpm run build and pnpm run lint both pass. Ready for another look. |
collinsezedike
left a comment
There was a problem hiding this comment.
Thanks for this, the fix is correct and thoroughly verified. Merging now.
|
@JohnArayaE If you have a moment, a star on the repo would be appreciated! |
Summary
get_assertion_stateback — so status never advanced when someone else's action (or a quietly-expired challenge window) was what actually changed the assertion.get_assertion_stateread after it runs, and any milestone with anassertionIdthat isn't settled (released/returned) polls that same read every 30s, plus a manual "Refresh" button in the UI.VITE_CHALLENGE_WINDOW_SECS(default matches the canonical testnet deployment, 21600s) to show a "ready to finalize" hint from a realAssertion.opened_atread — the contract has no getter for its own configured window, so this is a hint only;finalizeitself remains the real gate and rejects early calls.Test plan
pnpm exec tsc -b— no errorspnpm lint(oxlint) — 0 warnings, 0 errorsCloses #147