feat(gov-proposals): derive voting window from duration input#20
feat(gov-proposals): derive voting window from duration input#20
Conversation
Replace the wizard's raw start_epoch / end_epoch inputs with a
single "Duration (months)" field. The on-chain voting window is
computed deterministically at /prepare time from that duration
and the live next-superblock anchor, rather than asking the user
to type two UTC epoch integers.
Why
---
The old UX had three interrelated controls (payment count, start
epoch, end epoch) where only one — the count of payments the user
actually wants — is meaningful. The other two had to be manually
aligned to superblock cadence and the voting-window semantics that
Core enforces, otherwise the proposal would either pay fewer months
than advertised or silently allocate an extra month. Core also
refuses an end_epoch in the past and deletes proposals 10 min after
end_epoch, which the free-form date pickers gave users no help with.
The window math is now centralised in one place and proven correct
by unit tests: the window is always exactly N * SUPERBLOCK_CYCLE_SEC
wide (so getProposalDurationMonths rounds to exactly N months), the
first N superblocks fall inside the window by construction, and
superblock N+1 is excluded with a ~15-day safety margin — vastly
larger than Core's 2-hour GOVERNANCE_FUDGE_WINDOW, so block-time
drift cannot push an extra payment inside the window.
What changed
------------
- New `lib/governanceWindow.js` with `computeProposalWindow` +
`nextSuperblockEpochSecFromStats`. Pure, deterministic; no
network or DOM dependencies.
- New `lib/governanceWindow.test.js` (35 tests) covering N ∈
{1..5, 12, 60}, stale-anchor fallback, past-start-epoch acceptance
per Core, end > start + now invariants, SB_N in / SB_{N+1} out.
- `pages/NewProposal.js`:
* Fetches /mnStats on mount to get `superblock_next_epoch_sec`.
* New `WindowPreview` component renders the derived window on
both the Payment and Review steps. Loading / error states
gate the "Prepare proposal" button — we refuse to submit an
unanchored window.
* `onPrepare` re-fetches stats for freshness, computes the
window, and passes it to `prepareBodyFromForm({ window })`.
* Removed the two <label> blocks for Start / End epoch inputs.
* Renamed "Number of payments (months)" → "Duration (months)".
* Replaced `buildApproximateSchedule` (30-day approximation
with truncation warnings) with `buildProjectedSchedule`
(accurate, anchored to the live next-superblock epoch). The
truncation warning is gone because the derived window fits
every requested payment by construction.
- `lib/proposalForm.js`:
* Drop startEpoch / endEpoch from EMPTY, fromDraft, validatePayment,
estimatePayloadBytes, and formsEqual.
* `draftBodyFromForm(forUpdate=true)` explicitly clears epoch
columns so legacy drafts don't carry stale timestamps forward.
* `prepareBodyFromForm` now accepts `{ window }` and injects
the computed epochs on the wire — the backend API shape is
unchanged (startEpoch / endEpoch still required on /prepare).
- Test updates in `proposalForm.test.js` and `NewProposal.test.js`
mirror the behaviour changes; `fetchNetworkStats` is mocked via a
default resolver reinstated in `beforeEach` after clearAllMocks.
- CSS: new `.proposal-wizard__window-preview` styles for the
derived-window card; orphaned `.proposal-wizard__help--warn`
removed.
Wire format
-----------
Unchanged. /prepare still receives `startEpoch` / `endEpoch`; they
are now computed by the client from `paymentCount + nextSuperblockSec`
right before submit instead of being user inputs. Backend routes,
validators, and DB schema are untouched.
Pairing PR
----------
Requires sysnode-backend#13 (exposes `superblock_next_epoch_sec`
on /mnStats). Without that field the wizard detects the missing
anchor and disables "Prepare proposal" rather than submitting a
fallback (now + cycle) window silently.
Tests
-----
- 755/755 frontend tests pass (48 suites), including 20 NewProposal,
39 proposalForm, 35 new governanceWindow.
Made-with: Cursor
![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: d63b7bc115
ℹ️ 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".
| const value = Number(sb.superblock_next_epoch_sec); | ||
| if (!Number.isFinite(value) || value <= 0) return null; |
There was a problem hiding this comment.
nextSuperblockEpochSecFromStats treats any positive superblock_next_epoch_sec as valid, so if /mnStats lags and returns a timestamp that is already in the past, the wizard still considers the anchor "live" and enables Prepare (it only checks truthiness of nextSuperblockSec). In that state, Review can render a schedule from stale epochs while computeProposalWindow falls back to now + cycle at submit time, so the displayed window/schedule diverges from what is actually sent on-chain and can shift payouts by up to one cycle. This should be classified as invalid (same as missing anchor) until the epoch is strictly in the future.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 5b5517b. Two-part fix:
nextSuperblockEpochSecFromStats(stats, nowSec)now requiresnowSecand rejects any anchor<= nowSecas stale, matching how a missing/zero field is treated. Both call sites (refreshStats+onPrepare) passMath.floor(Date.now() / 1000). A lagging/mnStatsfeed now surfaces the "live-chain data unavailable" banner and keeps Prepare disabled instead of caching a past timestamp.- Belt + suspenders:
buildProjectedSchedulenow anchors offderivedWindow.startEpoch + cycle/2instead of the rawnextSuperblockSecstate. Review and WindowPreview therefore render from the same canonical window by construction — even if the anchor ever drifted stale between renders, the schedule can't diverge from the submitted window.
Added 6 regression tests in governanceWindow.test.js covering stale-equal-to-now, stale-by-seconds, stale-by-days, and invalid nowSec arguments. All 97 tests in the affected suites pass.
Addresses Codex PR20 P1 review.
nextSuperblockEpochSecFromStats accepted any positive
`superblock_next_epoch_sec`, including a timestamp in the past.
The backend's stats feed can lag for a window between the real
next superblock landing and sysMain.js refreshing its cache —
during that window the field is positive but stale.
With a stale anchor cached in state, two code paths diverge:
* buildProjectedSchedule consumed `nextSuperblockSec` verbatim,
so the Review step rendered payouts starting from a past
date.
* computeProposalWindow's internal stale-anchor guard rewrote
the anchor to `now + cycle`, so the window actually submitted
on-chain was shifted by up to one full cycle from what the
user reviewed.
The "Prepare proposal" gate (`!nextSuperblockSec`) was only a
truthy check, so a stale (but positive) anchor left Prepare
enabled and the divergent state submittable.
Fix, belt + suspenders:
1. nextSuperblockEpochSecFromStats now requires a `nowSec`
argument and rejects any anchor <= nowSec, the same way a
missing/zero field is rejected. refreshStats + onPrepare now
pass Math.floor(Date.now() / 1000), so a stale feed surfaces
the "live-chain data unavailable" banner and keeps Prepare
disabled until a fresh anchor arrives. (Matches
computeProposalWindow's own nowSec requirement.)
2. buildProjectedSchedule now derives its anchor from
`derivedWindow.startEpoch + cycle/2` instead of the raw state
value. Review and WindowPreview therefore reference the same
canonical window by construction — even if the anchor ever
drifted stale between renders, the schedule could not
diverge from the submitted window.
Tests: * 6 new regression tests in governanceWindow.test.js covering
stale-equal-to-now, stale-by-seconds, stale-by-days,
missing / zero / negative / non-numeric nowSec arguments.
* Existing NewProposal.test.js mocks fetchNetworkStats with a
30-day-future anchor, so the staleness gate is satisfied
and all existing assertions pass unmodified.
* 97/97 affected tests pass (governanceWindow, proposalForm,
NewProposal suites).
Made-with: Cursor
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b5517bec1
ℹ️ 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".
| } catch (_e) { | ||
| // Swallow; liveAnchor already reflects the cached hook value | ||
| // and computeProposalWindow tolerates a null anchor. |
There was a problem hiding this comment.
The prepare flow currently swallows /mnStats fetch failures and continues with whatever anchor is cached (or with computeProposalWindow’s fallback). If the tab has been open long enough for that cached anchor to become stale, the submitted startEpoch/endEpoch can diverge from what Review showed, shifting the payout window by up to a cycle in the exact case where chain timing could not be refreshed. In that scenario users can burn collateral on a proposal whose effective window no longer matches the reviewed schedule; this path should fail closed and require a successful anchor refresh before submission.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 48451ca. The swallow at onPrepare is gone; the refresh now has three explicit branches, all fail-closed w.r.t. shipping a window that diverges from the reviewed schedule:
- Fetch throws →
stats_unavailableerror, cached anchor cleared, Prepare disables itself untilrefreshStats()recovers. - Fetch returns a stale/missing anchor → same as (1).
- Fetch returns a different future anchor (a SB passed while the wizard was open) →
anchor_drift; state updates so WindowPreview + schedule rerender with the new window, user must click Prepare a second time so they commit collateral only to a window they actually saw.
describePrepareError now passes through the inline messages authored in onPrepare so retry guidance stays next to the button.
Separately, fixed a related flakiness in the test mock: it was recomputing Date.now() + 30d per call and would spuriously trigger branch (3) whenever the two fetchNetworkStats invocations straddled a wall-clock second. Replaced with a per-test stable anchor in beforeEach — mirrors production where /mnStats returns the same pre-computed SB epoch across rapid calls.
110/110 tests green with 3 new regressions (fetch-throws, stale-return, drift-then-retry-succeeds).
Last-minute proposals submitted inside Core's ~3-day
nSuperblockMaturityWindow typically miss the next payout because
most masternodes have already picked a candidate and voted
YES-FUNDING on it by then (governance.cpp:727 hard-asserts one
YES per cycle per MN). Since our derived window intentionally
excludes SB_{N+1} to prevent silent over-payment, missing SB_1
means the proposal runs for N-1 months instead of the requested
N.
Surfaces a prominent amber notice on Payment + Review when the
next superblock is within SUPERBLOCK_VOTE_DEADLINE_WARN_SEC
(4 days — 1 day wider than Core's maturity window to give MNs
headroom for collateral confirmation, relay, operator review,
and vote propagation). The notice explains the N-1 trade-off,
suggests waiting for the next cycle, and lists the concrete
months lost when paymentCount >= 2. Non-blocking by design:
Prepare stays enabled so informed proposers (emergency funding,
coordinated MN campaigns) can still submit.
Adds:
- SUPERBLOCK_MATURITY_WINDOW_SEC + SUPERBLOCK_VOTE_DEADLINE_WARN_SEC
- isTightVotingWindow(now, nextSb) helper (fail-closed on
invalid/stale/missing inputs, strict < threshold to avoid
boundary flapping)
- TightVotingWindowNotice component + humanizeDurationShort
- amber panel styling matching the existing notice--warning /
status-chip.is-warning palette
- 10 new tests (boundary, stale, invalid-now, both-step render,
1-month edge case) — suite 107/107 green
Made-with: Cursor
…ex PR20 round 2 P2)
onPrepare used to swallow /mnStats refresh failures and fall through
to the cached anchor (or computeProposalWindow's `now + cycle`
fallback). If the cached anchor had gone stale while the wizard was
open, the submitted startEpoch/endEpoch could diverge from what the
user saw on Review — burning 150 SYS of collateral on a window that
no longer matched the reviewed schedule.
Replace the silent-swallow with three explicit branches:
(a) fetch throws → surface stats_unavailable banner, drop the
cached anchor so the Prepare button disables itself until
refreshStats() recovers on the next tick.
(b) fetch returns a stale or missing anchor → same as (a);
/mnStats occasionally lags a few blocks and we refuse to
submit against a backward-pointing anchor.
(c) fetch returns a DIFFERENT future anchor than the cached one
(a superblock passed while the wizard was open) → update
state so WindowPreview + schedule rerender with the fresh
window, and raise an anchor_drift error asking the user to
re-review before clicking Prepare again. The second click,
once cached == live, proceeds normally.
describePrepareError surfaces the inline messages authored in
onPrepare so the retry guidance stays next to the button.
Test infra: the /mnStats mock used to recompute `Date.now() + 30d`
per call, which now spuriously triggers branch (c) when the two
fetch invocations straddle a wall-clock second. Replaced with a
per-test stable anchor captured in beforeEach — matches the
production contract where /mnStats returns the same pre-computed
SB epoch across rapid calls.
Tests: 110/110 green across governanceWindow, proposalForm, and
NewProposal suites (3 new regressions covering each branch plus
the successful-retry-after-drift path).
Made-with: Cursor
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48451ca95d
ℹ️ 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".
| let refreshErr = null; | ||
| try { | ||
| const freshStats = await fetchNetworkStats(); | ||
| liveAnchor = nextSuperblockEpochSecFromStats(freshStats, nowSec); |
There was a problem hiding this comment.
In onPrepare, nowSec is captured before await fetchNetworkStats() and then reused when validating the refreshed anchor. If that request returns around a superblock boundary (or after a slow response), an anchor that is already past at response time can still pass nextSuperblockEpochSecFromStats because it is only checked against the pre-request timestamp; the flow can then submit a window anchored to an already-passed superblock. This can silently shift effective payouts (e.g., N to N-1) despite the fail-closed intent. Compute the cutoff timestamp after the refresh resolves (or validate against current time inside the parse call).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e818aca. Moved nowSec = Math.floor(Date.now()/1000) to after await fetchNetworkStats() resolves, so nextSuperblockEpochSecFromStats(freshStats, nowSec) validates the refreshed anchor against the post-response clock. The same post-await value is threaded into computeProposalWindow so its stale-fallback sees a consistent view of the clock.
Regression test (Prepare fails closed when /mnStats resolves across the SB boundary) installs a Date.now spy inside the fetchNetworkStats mock resolver that advances the clock +20s during the await while the resolver returns an anchor that's only +10s relative to pre-await. Pre-fix: anchor validates as live and prepare proceeds. Post-fix: the post-await clock sees the anchor as -10s and fails closed with stats_unavailable.
Tests: 772/772 across the full frontend suite.
…esh (Codex PR20 round 3 P2) onPrepare captured `nowSec = Math.floor(Date.now() / 1000)` BEFORE awaiting fetchNetworkStats() and reused that pre-request timestamp as the stale-anchor cutoff inside nextSuperblockEpochSecFromStats. /mnStats is a real network RTT (plus jsdom / proxy / slow-node delays in practice) and can resolve across wall-clock boundaries — including, at a SB transition, the actual superblock. In that window an anchor that was strictly future at pre-await time can already be in the past by the time we use it. Passing the pre-await clock let nextSuperblockEpochSecFromStats validate a just-passed anchor as live and silently ship payouts off by one cycle (N -> N-1 months) despite round 2's fail-closed intent. Fix: move the nowSec capture to AFTER the await resolves. Same value is threaded through computeProposalWindow below so its stale-anchor fallback also sees a consistent view of the clock. Regression test uses a Date.now() spy installed inside the fetchNetworkStats mock resolver to advance the clock +20s DURING the await while returning an anchor that is only +10s relative to the pre-await clock. Pre-fix: anchor validates as live and prepare proceeds. Post-fix: post-await clock sees the anchor as -10s and fails closed with stats_unavailable. Tests: 772/772 green across the full frontend suite (111/111 for governanceWindow + proposalForm + NewProposal). Made-with: Cursor
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e818aca9c5
ℹ️ 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".
| // superblock cadence. Kept in-sync with | ||
| // Params().GetConsensus().nSuperblockCycle * PowTargetSpacing; any | ||
| // change in Core must be mirrored here. | ||
| export const SUPERBLOCK_CYCLE_SEC = 17520 * 150; |
There was a problem hiding this comment.
SUPERBLOCK_CYCLE_SEC is hardcoded to mainnet (17520 * 150), but this app is documented to run against mainnet or testnet (README.md), and Syscoin consensus uses different superblock cycles on non-mainnet networks (e.g., testnet/regtest). Because computeProposalWindow, buildProjectedSchedule, and isTightVotingWindow all consume this constant, testnet proposals now get a wildly incorrect derived window/schedule (e.g., a “1 month” duration can span far more superblocks than intended), which can materially change payout behavior. This regression comes directly from replacing user-entered epochs with derived windows without making the cycle network-aware.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in be4fe1a — introduced src/lib/networkParams.js, a per-network consensus table keyed by build-time REACT_APP_NETWORK (same ergonomic as the existing REACT_APP_API_BASE override documented in the repo README). Values mirror Core's kernel/chainparams.cpp:
- mainnet:
nSuperblockCycle = 17520,nSuperblockMaturityWindow = 1728 - testnet:
nSuperblockCycle = 60,nSuperblockMaturityWindow = 20 - regtest:
nSuperblockCycle = 10,nSuperblockMaturityWindow = 5
governanceWindow.js now derives SUPERBLOCK_CYCLE_SEC and SUPERBLOCK_MATURITY_WINDOW_SEC from the active network's cycleBlocks * targetBlockTimeSec. Missing or unrecognised env values fall back to mainnet so the production default (sysnode.info) stays byte-for-byte identical.
Tests (789/789 green, +11 new):
networkParams.test.js:resolveNetworkIdcovers case-insensitivity, whitespace, short forms, unknown-label fallback, and frozen-record invariants. Per-network values cross-checked against the Core chainparams.cpp line cited inline.governanceWindow.test.js: new per-network consensus params suite usesjest.isolateModules+ env-var overrides to re-import the module withREACT_APP_NETWORK=testnet/=regtest/ unset / bogus, and verifiescomputeProposalWindow'send - start = N * cycleinvariant holds on each network (same invariantgetProposalDurationMonthsrelies on for label correctness).
Out of scope for this PR but flagged for follow-up: threading the live superblockcycle from /mnStats through the wizard so a single frontend build could target any backend network without rebuilding. That requires a backend API change and can layer cleanly on top of this env-var routing later.
…dex PR20 round 3 P1)
SUPERBLOCK_CYCLE_SEC and SUPERBLOCK_MATURITY_WINDOW_SEC were hardcoded
to mainnet's 17520 / 1728 block counts. Syscoin Core's
kernel/chainparams.cpp configures these per-network:
mainnet : nSuperblockCycle = 17520, nSuperblockMaturityWindow = 1728
testnet : nSuperblockCycle = 60, nSuperblockMaturityWindow = 20
regtest : nSuperblockCycle = 10, nSuperblockMaturityWindow = 5
A testnet build using the hardcoded mainnet value meant a "1 month"
proposal produced a ~2.6M-second window, which spans ~290 real testnet
superblocks instead of one. That is a material payout-behavior
regression for any non-mainnet deployment (the wizard now advertises
"1 month" while Core would schedule ~290 payments).
Fix (frontend-only, no backend API change): introduce
lib/networkParams.js, a per-network consensus table keyed by
REACT_APP_NETWORK (build-time env var, same ergonomic as the
existing REACT_APP_API_BASE override in the repo README). Missing or
unrecognised values fall back to mainnet — the production default at
sysnode.info stays byte-for-byte identical, so existing deployments
and all 49 test suites remain green (789/789 passing).
governanceWindow.js now derives both constants from the active
network's cycleBlocks * targetBlockTimeSec instead of hardcoding
17520 * 150 / 1728 * 150.
Tests:
* new networkParams.test.js: resolveNetworkId covers case-
insensitive labels, whitespace, short forms, unknown-label
fallback, and frozen-record invariants. Values for each
supported network are cross-checked against the Core
chainparams.cpp line cited in the header comment.
* governanceWindow.test.js: new "per-network consensus params"
describe block uses jest.isolateModules + env var overrides to
re-import governanceWindow with REACT_APP_NETWORK set to
testnet / regtest / mainnet / bogus, and verifies that
computeProposalWindow's span = N * cycle invariant holds on
every supported network (same invariant the mainnet tests
already rely on for getProposalDurationMonths).
Note: the BE /mnStats payload is not yet consulted for the cycle —
deployments that are build-separated from the backend (fork,
private testnet) ship the right constants via REACT_APP_NETWORK
at CI/deploy time. A future refinement could thread
superblockcycle_blocks from /mnStats and prefer it over the env
default, but that requires a backend API change and is out of
scope for this PR; env-var routing is sufficient to close the P1.
Made-with: Cursor
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be4fe1a0ea
ℹ️ 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".
| ); | ||
| return; | ||
| } | ||
| if (liveAnchor !== nextSuperblockSec) { |
There was a problem hiding this comment.
Treating any change in superblock_next_epoch_sec as anchor_drift is too strict. If /mnStats recomputes the projected epoch on each request (for example from current time and remaining blocks), the value can legitimately shift by a second without the next superblock actually changing; this branch then aborts prepare, updates local anchor, and asks for retry. In that mode users can get stuck in repeated drift errors and never reach proposalService.prepare even though chain timing is healthy.
Useful? React with 👍 / 👎.
| // submitted inside this 4-day window will likely miss the next | ||
| // superblock and pay out N-1 months instead of N, so the wizard | ||
| // surfaces a prominent notice. | ||
| export const SUPERBLOCK_VOTE_DEADLINE_WARN_SEC = 4 * 24 * 60 * 60; |
There was a problem hiding this comment.
This threshold is hardcoded to 4 days even though the same module already derives cycle and maturity from active network params. On testnet/regtest (where the cycle is hours/minutes, not ~30 days), nextSuperblockSec - now is always below 4 days, so isTightVotingWindow becomes effectively always true and the warning is a permanent false positive. That undermines the warning signal for every non-mainnet build; compute it from per-network maturity (with optional headroom) instead of a fixed mainnet-sized constant.
Useful? React with 👍 / 👎.
…-deadline warning (Codex PR20 round 4 P1+P2)
Two issues from round 4 review, both rooted in comparing /mnStats
values without accounting for their real-world variance:
P1 — anchor_drift was triggered by any non-zero delta between the
cached next-SB epoch and the refreshed one. But the backend's
sysMain.js recomputes `superblock_next_epoch_sec` every 20 s as
`now + diffBlock * avgBlockTime`, so the value drifts by seconds
to minutes between fetches even when the same upcoming superblock
is still the target. Under strict equality users could get stuck
looping through re-review prompts on every Prepare click and
never reach proposalService.prepare despite healthy chain timing.
Fix: introduce `ANCHOR_SAME_SB_TOLERANCE_SEC = cycle / 2` and
`anchorsAreSameSuperblock(fresh, cached)` in governanceWindow.js.
onPrepare now gates `anchor_drift` on
`!anchorsAreSameSuperblock(liveAnchor, nextSuperblockSec)`. A
legitimate rotation advances the anchor by ≈ cycle (well above
the cycle/2 threshold); sysMain's per-tick re-estimates are
seconds-scale (well below). Cycle/2 is a generous separator that
works across every supported network without any per-network
tuning.
P2 — SUPERBLOCK_VOTE_DEADLINE_WARN_SEC was hardcoded to 4 days.
On testnet (cycle = 2.5 h) and regtest (cycle = 25 min),
`nextSuperblockSec - now` is always below 4 days, so
`isTightVotingWindow` was a permanent true and the warning banner
became useless noise on every non-mainnet build — the opposite of
the round-3 fix that made the cycle itself network-aware.
Fix: derive the threshold as `MATURITY * 4/3`. On mainnet this is
exactly 4 days (3-day maturity + 1-day headroom), matching the
original UX copy. On testnet it's ~67 min and on regtest ~17 min —
the "1/3 of the maturity window as operator headroom" ratio is
preserved across networks.
Tests: 797/797 green across 49 suites. +16 new:
- governanceWindow.test.js
* `ANCHOR_SAME_SB_TOLERANCE_SEC is cycle/2 on mainnet`
* new `anchorsAreSameSuperblock` suite: 6 cases covering exact
equality, sub-cycle drift (seconds to a day), boundary at
cycle/2 (strict less-than), legitimate rotation (+cycle /
+30d), and fail-closed on bogus inputs (null / zero / NaN /
negative / string). Matches the same fail-closed contract as
nextSuperblockEpochSecFromStats.
* updated mainnet VOTE_DEADLINE_WARN test to assert the derived
formula, keeping the `=== 4*86400` equality on mainnet.
- NewProposal.test.js
* new `Prepare proceeds without anchor_drift when refreshed
anchor differs only by estimate drift (sub-SB)` regression:
currentStableNextSb shifts by 60 s between mount and prepare.
Pre-fix: anchor_drift banner. Post-fix: submits on the first
click, no spurious banner.
Made-with: Cursor
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 082bd63a03
ℹ️ 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".
| const schedule = | ||
| paymentCountNum >= 2 | ||
| ? buildApproximateSchedule({ | ||
| startEpoch: form.startEpoch, | ||
| endEpoch: form.endEpoch, | ||
| paymentCountNum >= 2 && derivedWindow | ||
| ? buildProjectedSchedule({ |
There was a problem hiding this comment.
When /mnStats is missing or stale, nextSuperblockSec is null and the UI correctly shows the window error state, but derivedWindow is still non-null because computeProposalWindow falls back to now + cycle. This branch then renders a "Projected payment schedule" from synthetic timestamps even though live chain timing is unavailable, which can show users dates that are not anchored to the real next superblock. The schedule should be hidden unless the same live-anchor preconditions as WindowPreview are satisfied.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1853448. derivedWindow is non-null whenever paymentCount is valid because computeProposalWindow falls back to anchor = now + cycle on a missing/stale anchor (so the duration preview can still render something sensible while stats load). The schedule gate at paymentCountNum >= 2 && derivedWindow ignored that, so when /mnStats was unavailable the Review would paint N payout rows from synthetic timestamps alongside WindowPreview's "live chain data unavailable" banner.
Fix: extended the gate to also require a live anchor — hasLiveAnchor = Number.isFinite(nextSuperblockSec) && nextSuperblockSec > 0, schedule renders iff paymentCountNum >= 2 && derivedWindow && hasLiveAnchor. Review's schedule row is now in lockstep with WindowPreview: either both render from the live anchor or neither renders.
Regression test (Review step suppresses the projected schedule when /mnStats anchor is unavailable): stubs fetchNetworkStats to resolve with a shape nextSuperblockEpochSecFromStats rejects, drives the wizard to Review with paymentCount=3, asserts review-schedule and review-schedule-row are both absent. Pre-fix would render 3 rows.
| If you need the full duration you requested, consider | ||
| waiting for the next superblock cycle (~30 days) so | ||
| masternodes have time to see and vote on the proposal |
There was a problem hiding this comment.
This warning text hardcodes a "~30 days" superblock cycle, but this commit made governance timing network-dependent (mainnet/testnet/regtest). On testnet/regtest the cycle is much shorter, so this guidance becomes misleading by orders of magnitude and can drive incorrect operator decisions in non-mainnet deployments. The copy should be derived from SUPERBLOCK_CYCLE_SEC (or kept network-neutral).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1853448. The round-4 P2 fix already made the warning threshold (SUPERBLOCK_VOTE_DEADLINE_WARN_SEC) network-derived, but the surrounding copy in TightVotingWindowNotice was still carrying mainnet strings ("~3 days" for the maturity window, "~30 days" for the cycle). On testnet (cycle 2.5 h, maturity 50 min) and regtest (cycle 25 min, maturity 12.5 min) that guidance is off by orders of magnitude.
Fix: derive both values at render time via the existing humanizeDurationShort helper —
const maturityLabel = humanizeDurationShort(SUPERBLOCK_MATURITY_WINDOW_SEC);
const cycleLabel = humanizeDurationShort(SUPERBLOCK_CYCLE_SEC);and interpolate into the copy ("the last {maturityLabel} before each superblock" / "the next superblock cycle ({cycleLabel})"). SUPERBLOCK_MATURITY_WINDOW_SEC was already exported from lib/governanceWindow.js, just added to the existing import in NewProposal.js.
On mainnet the formatter renders 259200 s / 2628000 s as "3d" / "30d 10h" — visually identical to the original hardcoded copy modulo ~ / , so this is a zero-regression change on the dominant deployment. On testnet/regtest it surfaces the actual network values so operators aren't guided toward the wrong wait time. The networkParams test suite already cross-references the constants against Core's chainparams.cpp, so correctness of the underlying values is covered.
…re tight-window copy (Codex PR20 round 5 P2+P3) Two remaining round-5 issues on commit 082bd63, both the same class as prior fixes — mainnet-specific assumptions baked into the duration-derived flow that still showed up after the computeProposalWindow / constants work went network-aware. P2 — Review's projected payment schedule could render from entirely synthetic timestamps. `derivedWindow` is non-null whenever paymentCount is valid because computeProposalWindow falls back to `anchor = now + cycle` on a missing/stale anchor (so the duration preview can still paint something sensible while stats load). The schedule render was only gated on `paymentCountNum >= 2 && derivedWindow`, so when /mnStats was unavailable: * WindowPreview (correctly) showed its "Couldn't fetch the next-superblock time" error banner — no voting window dates. * The sibling Projected payment schedule (incorrectly) listed N payout rows with plausible-looking dates built off the fallback anchor. A proposer could take those dates at face value on Review, submit, and then get a chain-confirmed schedule that was up to a full cycle off from what the banner next to it said was unavailable. Prepare was already gated on `!nextSuperblockSec`, so submission itself was safe — this was a pure reviewability / trust regression. Fix: add `hasLiveAnchor = Number.isFinite(nextSuperblockSec) && nextSuperblockSec > 0` and extend the schedule condition to `paymentCountNum >= 2 && derivedWindow && hasLiveAnchor`. Review's schedule row is now in lockstep with WindowPreview — both only render when we have a real live anchor. P3 — TightVotingWindowNotice copy was still hardcoded to mainnet values. The warning told operators "Core forms the superblock payment list during the last ~3 days before each superblock" and "consider waiting for the next superblock cycle (~30 days)". On testnet (cycle 2.5 h, maturity 50 min) and regtest (cycle 25 min, maturity 12.5 min) that guidance is off by orders of magnitude, which, per the Codex review, "can drive incorrect operator decisions in non-mainnet deployments". The warning threshold was already derived (round-4 P2) but the surrounding copy wasn't. Fix: derive both values at render time — maturityLabel = humanizeDurationShort(SUPERBLOCK_MATURITY_WINDOW_SEC) cycleLabel = humanizeDurationShort(SUPERBLOCK_CYCLE_SEC) On mainnet humanizeDurationShort of 259200 s / 2628000 s renders as "3d" / "30d 10h" — visually identical to the original copy. On testnet/regtest the formatter surfaces the actual network values (50m / 2h 30m on testnet, 12m / 25m on regtest) so the guidance stays numerically honest. Imported SUPERBLOCK_MATURITY_WINDOW_SEC from lib/governanceWindow (already exported). Tests: 29/29 NewProposal, 97/97 across wizard suites, 797/798 full suite (the one failure is the pre-existing intermittent ProposalVoteModal test — passes 59/59 in isolation, not touched by this commit). +1 new regression: - `Review step suppresses the projected schedule when /mnStats anchor is unavailable`: stubs fetchNetworkStats to return a response that nextSuperblockEpochSecFromStats rejects, drives the wizard to Review with paymentCount=3, asserts both `review-schedule` and `review-schedule-row` are absent. Pre-fix would render 3 rows; post-fix renders none. The network-aware copy doesn't need a dedicated assertion beyond the existing `tight-voting-window notice fires on both Payment and Review` test — the mainnet values are preserved by construction (humanizeDurationShort of the mainnet seconds = the old hardcoded strings modulo `~` / ` `), and the networkParams test suite already cross-checks the constants against Core's chainparams.cpp. Made-with: Cursor
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18534487d0
ℹ️ 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".
| fetchNetworkStats() | ||
| .then((stats) => { | ||
| if (cancelled) return; | ||
| // `nowSec` is captured at response time, not at effect | ||
| // mount, so we reject anchors that are already in the past |
There was a problem hiding this comment.
Each refreshStats invocation fires a new /mnStats request but there is no request sequencing, so overlapping calls can resolve out of order (for example, the mount request finishing after a manual Retry). In that case, an older failure/stale response can overwrite a newer successful anchor and set nextSuperblockSec back to null, re-disabling Prepare even though the latest fetch succeeded. Add a request id (or abort prior requests) so only the newest response is allowed to update state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cf5d527. The local cancelled flag only invalidated a single invocation — concurrent refreshStats calls had no shared ordering, so a stale earlier response could overwrite a fresher one and set nextSuperblockSec back to null despite the latest fetch having succeeded.
Fix: introduced statsReqIdRef = useRef(0) as a monotonic request counter. Each refreshStats snapshots reqId = ++statsReqIdRef.current on entry and guards every state mutation (then / catch / finally) with reqId === statsReqIdRef.current. Only the newest in-flight fetch can write anchor state; stale callbacks no-op. Cleanup invalidates by bumping, so unmount / effect-reruns use the same mechanism.
Note on testability: no dedicated UI regression test because the wizard doesn't expose a way to trigger an overlapping fetch — Retry is hidden while a refresh is in flight (statsLoading && !nextSuperblockSec short-circuits to the loading branch before the error branch that carries Retry), so there's no path where the user can start a second fetch before the first resolves. The fix is defense-in-depth for StrictMode double-mount and any future caller of refreshStats. Full suite (798/798) still green with the new sequencing — existing "Prepare fails closed when the pre-submit /mnStats refresh throws" and "refreshes across the SB boundary" tests cover the happy-path + single-fetch sequencing paths.
| {new Date(nextSuperblockSec * 1000).toUTCString()}). The start | ||
| is placed ~15 days before the first payout and the end ~15 days | ||
| after the last so each payment lands safely inside the window |
There was a problem hiding this comment.
The preview text claims the window is padded by ~15 days, but this padding is actually SUPERBLOCK_CYCLE_SEC / 2 and changes by network (much shorter on testnet/regtest). As written, non-mainnet builds present incorrect timing guidance in the final review panel. This should be rendered from the active network constants (as done elsewhere in this change) instead of hardcoding mainnet-specific copy.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cf5d527. Both locations — WindowPreview's voting-window help text (around line 1519) and the Review schedule's "margin on each side" help text (around line 1695) — replaced ~15 days with ~\${humanizeDurationShort(Math.floor(SUPERBLOCK_CYCLE_SEC / 2))}.
On mainnet that renders "15d 5h" (the true mainnet cycle is 30d 10h, so cycle/2 is 15d 5h — the precise value, not the approximate "15 days" it had been). On testnet/regtest the label tracks the actual network margin (~1h 15m and ~12m 30s respectively), so non-mainnet operators aren't misled about how much slack their window carries.
This completes the round-4/5 copy sweep — no remaining mainnet-specific strings in the derived-window flow. The tight-window notice ("{maturityLabel}" / "{cycleLabel}") already landed in 1853448; together they cover every surface that quoted a hardcoded duration.
…ow-margin copy (Codex PR20 round 5 P2+P3) Two issues from the review on commit 1853448, both about lingering non-determinism in the prepare flow now that constants are network-aware. P2 — `refreshStats` had no request sequencing. Each invocation fired a new `fetchNetworkStats` with only a local `cancelled` flag, so overlapping calls could resolve out of order and an older response could clobber a fresher one. The most visible case is a slow mount request finishing after a manual Retry: the Retry's successful anchor gets overwritten by the mount's stale/error response and Prepare re-disables despite the live data being healthy. Under StrictMode double-mount (or any future caller that also invokes `refreshStats`) the same race applies. Fix: introduce `statsReqIdRef = useRef(0)` as a monotonic counter. Each `refreshStats` snapshots `reqId = ++statsReqIdRef.current` on entry and guards every state mutation (`then` / `catch` / `finally`) with `reqId === statsReqIdRef.current`. Stale callbacks no-op; only the newest in-flight request writes anchor state. Cleanup function bumps the counter so unmount / effect re-run behaves the same as any other invalidation. useRef instead of state so sequencing is synchronous and doesn't cause re-renders. No UI-level regression test: the scenario can't be triggered through the wizard UI alone because Retry is hidden while a refresh is in flight (`statsLoading && !nextSuperblockSec` short-circuits to the loading branch before the error branch that carries Retry), so the user has no way to start a second overlapping fetch. The fix is defense-in-depth for StrictMode double-mount and any future call sites. The full suite (798/798) passes with the new sequencing in place, confirming none of the existing mount-and-retry tests trip on the counter. P3 — WindowPreview and the Review schedule help text both hardcoded a `~15 days` margin claim. That padding is actually `SUPERBLOCK_CYCLE_SEC / 2`, which is network-dependent — on testnet it's ~1h 15m and on regtest it's ~12.5m, so non-mainnet operators were being told their window had ~15 days of slack that it doesn't have. Round-5 P3 already addressed the tight-window notice copy; these were the two surviving mainnet strings. Fix: render both via `~${humanizeDurationShort(Math.floor(SUPERBLOCK_CYCLE_SEC / 2))}`. On mainnet `SUPERBLOCK_CYCLE_SEC / 2` = 1314000 s → the formatter produces "15d 5h" (the precise value, since the true mainnet cycle is 30d 10h not exactly 30d), superseding the approximate "15 days" with something that's actually correct. On testnet/regtest the label tracks the real network margin. Tests: 798/798 full suite pass. Made-with: Cursor
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf5d527539
ℹ️ 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".
| const halfCycle = Math.floor(SUPERBLOCK_CYCLE_SEC / 2); | ||
| const startEpoch = anchor - halfCycle; | ||
| const endEpoch = anchor + (n - 1) * SUPERBLOCK_CYCLE_SEC + halfCycle; |
There was a problem hiding this comment.
computeProposalWindow always pads by SUPERBLOCK_CYCLE_SEC / 2, but on non-mainnet networks that margin is smaller than Core’s fixed GOVERNANCE_FUDGE_WINDOW (2h). With the current params (testnet cycle 9000s, regtest cycle 1500s), halfCycle is 4500s/750s, so SB_{N+1} still satisfies SB_time <= end_epoch + FUDGE and can be treated as payable, meaning a proposal can pay more superblocks than the requested duration. This regression is introduced by the new network-aware cycle math and breaks payout correctness on testnet/regtest builds.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0b175e6.
computeProposalWindow now clamps symmetric padding to min(cycle/2, cycle - FUDGE - SAFETY) where SAFETY = 60 s. The clamp guarantees SB_{N+1} stays strictly outside Core's end_epoch + FUDGE eligibility window on any network where cycle > FUDGE + SAFETY:
- mainnet (cycle ≈ 30d): padding still
cycle/2, end–start invariant = N*cycle holds exactly. - testnet (cycle 9000 s): raw cycle/2 = 4500 s < FUDGE 7200 s was the bug. Clamped padding = 9000 − 7200 − 60 = 1740 s. SB_{N+1} lands at
anchor + N*cycle; windowEnd =anchor + (N−1)*cycle + 1740; slack = 9000 − 1740 = 7260 s > FUDGE 7200, so SB_{N+1} is strictly excluded with ≥ SAFETY seconds of margin. - regtest (cycle 1500 s < FUDGE 7200 s): no symmetric padding exists that includes SB_N and excludes SB_{N+1} — the helper now throws. derivedWindow memo catches the throw, WindowPreview falls into its neutral error branch, and the wizard never submits a regtest window.
Tests verify SB_{N+1} exclusion on testnet for N in {1, 2, 6, 12} and the throw on regtest (governanceWindow.test.js Codex round 5 P1 suite).
| const hasLiveAnchor = | ||
| Number.isFinite(nextSuperblockSec) && nextSuperblockSec > 0; |
There was a problem hiding this comment.
The Review step considers an anchor “live” when it is just finite and > 0, so a cached nextSuperblockSec that has already passed is still treated as valid if the wizard stays open long enough (especially on testnet’s short cycle). In that state, schedule rendering and button gating continue as if timing is live while computeProposalWindow has already switched to its stale-anchor fallback (now + cycle), so the displayed anchor and derived window can diverge until prepare-time refresh fails. The live-anchor predicate should require nextSuperblockSec > now (and the same check should be used where WindowPreview/Prepare gating relies on this state).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0b175e6.
Added isAnchorLive = Number.isFinite(nextSuperblockSec) && nextSuperblockSec > nowSec and threaded it through WindowPreview's gate, the Prepare button's disabled, and ReviewStep's schedule render — all three now flip together the moment the cached anchor elapses. A 30 s useReducer re-render tick keeps the predicate in sync with the wall clock while the wizard sits idle.
Why a tick rather than an anchor-timestamp setTimeout: Node's setTimeout silently collapses delays > 2^31-1 ms (~24.8 days) to 1 ms, so a direct setTimeout(clearAnchor, (nextSb - now) * 1000) fires immediately on mainnet (cycle ~30 days). The interval approach sidesteps the overflow and handles every network uniformly — 30 s is fine-grained relative to even testnet's 2.5 h cycle and has negligible render cost.
Regression test added (NewProposal.test.js round 6 P2): mounts with a 6 s-future anchor, advances the fake clock past it, asserts Prepare flips to disabled, WindowPreview swaps into the stats-unavailable branch, and the projected schedule is suppressed — all in lockstep. Full suite: 799/799 passing locally.
…past anchors as not-live (Codex PR20 round 6 P1+P2)
P1: computeProposalWindow
- Clamp symmetric padding to min(cycle/2, cycle - FUDGE - SAFETY) so
SB_{N+1} stays strictly outside the eligibility window on any network
where cycle > FUDGE + SAFETY. Prevents testnet/regtest builds from
silently paying N+1 superblocks instead of the requested N (raw cycle/2
on testnet = 4500 s < Core's 7200 s fudge tolerance).
- Throw when the clamp is infeasible (cycle <= FUDGE + SAFETY). Regtest
(cycle 1500 s) hits this and becomes unsupported for the duration-
derived flow — the wizard's derivedWindow useMemo already swallows the
throw and renders the neutral error branch.
- Tests verify SB_{N+1} exclusion on testnet for N in {1, 2, 6, 12} and
the throw on regtest.
P2: live-anchor predicate
- Add `isAnchorLive = Number.isFinite(nextSuperblockSec) && nextSuperblockSec > nowSec`
and thread it through WindowPreview, the Prepare button gate, and
ReviewStep's projected-schedule gate. Replaces the prior truthiness
check, which kept treating a cached anchor as live after its timestamp
had already elapsed (most visible on short-cycle networks and on
long-lived sessions that cross the mainnet boundary).
- Add a 30 s wall-clock re-render tick so the predicate re-evaluates
while the wizard sits idle; using setTimeout at the anchor itself is
not viable because Node's int32 delay ceiling collapses 30-day delays
to 1 ms and fires immediately.
- Add a regression test that advances the fake clock past a
short-horizon anchor and asserts Prepare flips to disabled,
WindowPreview swaps into its error branch, and the projected schedule
is suppressed — all three in lockstep.
Full suite: 799/799 passing locally.
Made-with: Cursor
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b175e6d9b
ℹ️ 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".
| const halfCycle = Math.floor(SUPERBLOCK_CYCLE_SEC / 2); | ||
| const anchor = Number(derivedWindow.startEpoch) + halfCycle; | ||
| const count = Math.floor(Number(paymentCount)); |
There was a problem hiding this comment.
On non-mainnet builds where computeProposalWindow clamps padding below cycle/2 (e.g., testnet: padding 1740s vs cycle/2 4500s), reconstructing the first payout anchor as startEpoch + cycle/2 shifts every projected row forward. That makes the Review schedule disagree with the actual derived window used for /prepare; for example with N=2 on testnet, row #2 lands after endEpoch, so the UI shows a payment that cannot fit inside the submitted voting window. The schedule anchor should be derived with the same padding used by computeProposalWindow (or returned directly from it) to keep Review output consistent with submission behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8b8f87e.
computeProposalWindow now returns { startEpoch, endEpoch, anchor, padding } — the anchor field is the SB_1 epoch used in the internal math (either the live nextSuperblockSec or the stale-anchor fallback now + cycle), and padding is the clamped per-side margin used to derive startEpoch/endEpoch.
buildProjectedSchedule reads derivedWindow.anchor directly for row #1 and walks forward by whole cycles, so the Review schedule and the submitted window stay in lockstep regardless of the padding clamp:
- mainnet: padding = cycle/2, anchor = startEpoch + cycle/2 (same as the old reconstruction).
- testnet: padding = 1740 s, anchor = startEpoch + 1740 s — NOT startEpoch + 4500 s. Row N now lands at
anchor + (N-1)*cycle=endEpoch - padding, always ≤ endEpoch.
Tests:
- governanceWindow.test.js: return-shape tests assert on the new
anchor/paddingfields. - New per-row assertion in the testnet SB_{N+1}-exclusion suite:
anchor + (N-1)*cycle <= endEpochfor N in {1,2,6,12}, locking the Review-vs-Prepare consistency in place.
Full suite: 800/800 passing locally.
…e stays in sync with Prepare window (Codex PR20 round 7 P2)
Problem
buildProjectedSchedule reconstructed the SB_1 anchor as
`startEpoch + cycle/2`, which was only correct on mainnet where
computeProposalWindow's padding equals cycle/2. On networks where
the round-6 P1 clamp lowers padding below cycle/2 (testnet:
padding 1740s vs cycle/2 4500s), the reconstruction shifted every
projected row forward by `cycle/2 - padding`, pushing row N past
`endEpoch` — the Review schedule disagreed with the window
actually sent to /prepare.
Fix
computeProposalWindow now returns { startEpoch, endEpoch, anchor,
padding }. buildProjectedSchedule reads `derivedWindow.anchor`
directly for its SB_1 position, so the Review schedule and the
submitted window stay in lockstep on every network.
Tests
- computeProposalWindow's existing return-shape tests assert on
the new anchor + padding fields.
- New testnet check in the per-network suite: last projected row
`anchor + (N-1)*cycle` is always <= endEpoch for N in
{1,2,6,12}, locking in the fix against regression.
Full suite: 800/800 passing locally.
Made-with: Cursor
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b8f87e0c9
ℹ️ 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".
| } from '../components/PayWithPaliPanel'; | ||
| import UnsavedChangesModal from '../components/UnsavedChangesModal'; | ||
| import { useAuth } from '../context/AuthContext'; | ||
| import { fetchNetworkStats } from '../lib/api'; |
There was a problem hiding this comment.
This import wires the proposal-critical anchor fetch to fetchNetworkStats, but that helper uses a hardcoded https://syscoin.dev base URL (src/lib/api.js:3-5) instead of the configurable REACT_APP_API_BASE path used by proposalService. Because Prepare is now gated on a live anchor, any non-default deployment (local dev, self-hosted backend, or testnet/regtest environment) can end up querying the wrong network or failing CORS entirely, which either disables prepare indefinitely (isAnchorLive never becomes true) or computes windows from mainnet timing against a non-mainnet backend.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b14ba53.
src/lib/api.js now resolves its base URL with the same priority as src/lib/apiClient.js: REACT_APP_API_BASE override, then production https://syscoin.dev, then development http://localhost:3001. A single build-time env var now retargets the entire app — authenticated endpoints and anonymous public endpoints (/mnStats, /mnCount, /govlist) alike.
The two clients are intentionally kept as separate axios instances (apiClient.js carries withCredentials + CSRF header handling that would be inappropriate on the anonymous surface), so I didn't collapse them — just aligned their base resolution. README updated to document that both files now read REACT_APP_API_BASE.
Full suite: 800/800 passing locally.
…Stats hits the configured backend (Codex PR20 round 7 P1) The proposal wizard's Prepare step is now gated on a live `superblock_next_epoch_sec` anchor pulled via `fetchNetworkStats()`, but `src/lib/api.js` hardcoded `baseURL: 'https://syscoin.dev'`. On any non-default deployment — local dev, self-hosted backend, testnet / regtest — that either returned mainnet timing against a non-mainnet backend (wrong superblock cadence silently applied to derivedWindow) or failed CORS entirely (isAnchorLive never flipped true, permanently disabling Prepare). Mirror the resolution order already used by the authenticated client in `src/lib/apiClient.js`: 1. REACT_APP_API_BASE (build-time override) 2. production → https://syscoin.dev 3. development → http://localhost:3001 A single build-time variable now retargets the entire app, which is what the README already tells operators to do. Made-with: Cursor
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Summary
Replace the new-proposal wizard's raw
start_epoch/end_epochinputs with a single Duration (months) field. The on-chain voting window is now computed deterministically at/preparetime from that duration + the live next-superblock anchor, rather than asking the user to type two UTC epoch integers.Why
Three interrelated controls (payment count, start epoch, end epoch) where only one — the number of payments the user wants — actually carries intent. The other two had to be hand-aligned to:
nSuperblockCycle = 17520 * 150sec ≈ 30.42 d on mainnet) so that exactly N superblocks land inside the window.GOVERNANCE_FUDGE_WINDOW(2 h slack on payment eligibility).GOVERNANCE_DELETION_DELAY(10 min afterend_epochthe object is pruned).end_epochmust be> nowand> start_epoch;start_epochmay be in the past.A tiny misalignment would either silently allocate an extra month of payouts or drop the last one. The free-form date pickers gave users none of this context, and the review step had to paper over it with "approximate schedule" warnings.
Window math (centralised in
lib/governanceWindow.js)Given the next superblock anchor
Aand user-selected durationN:Properties (all covered by unit tests):
endEpoch - startEpoch === N * SUPERBLOCK_CYCLE_SECexactly, sogetProposalDurationMonthsrounds to N months for every N ≥ 1.A, A+cycle, …, A+(N-1)*cycleall land inside[startEpoch, endEpoch]with a half-cycle (≈ 15 d) margin each side — orders of magnitude larger than the 2 h fudge window, so realistic block-time drift cannot pull an extra payment in.startEpochmay be nominally in the past by up tocycle/2; Core accepts this, and the symmetric padding is what keeps the duration-display invariant intact./mnStatshasn't yet produced a future anchor, we fall back tonow + cycleso the preview is still sensible, but the Prepare button stays disabled (see below).What changed
src/lib/governanceWindow.js—computeProposalWindow,nextSuperblockEpochSecFromStats,SUPERBLOCK_CYCLE_SEC,SUPERBLOCK_FUDGE_SEC. Pure / deterministic.src/lib/governanceWindow.test.js— 35 tests covering N ∈ {1..5, 12, 60}, stale-anchor fallback, past-start-epoch acceptance, SB\N in / SB\{N+1} out, and the N-months display invariant.src/pages/NewProposal.js:/mnStatson mount, readssuperblock_next_epoch_sec, surfaces loading / error state on the Payment + Review steps.WindowPreviewcomponent renders the derived "Voting window starts / ends" card on both steps. Includes a retry button (button--ghost) when the fetch fails.onPreparere-fetches stats for freshness, computes the window, passes{ window }toprepareBodyFromForm.<label>blocks for Start / End epoch inputs.buildApproximateSchedule(30-day approximation + truncation warnings) withbuildProjectedScheduleanchored to the live next-superblock epoch. The truncation warning is gone because every payment fits the window by construction.src/lib/proposalForm.js:startEpoch/endEpochfromEMPTY,fromDraft,validatePayment,estimatePayloadBytes,formsEqual.draftBodyFromForm(forUpdate=true)clears the epoch columns so legacy drafts don't carry stale timestamps forward.prepareBodyFromForm(form, { window })injects the computed epochs on the wire, defensively deleting any pre-existing values from the form body first.src/App.css: new.proposal-wizard__window-preview/--errorstyles; removed orphaned.proposal-wizard__help--warnrule.Wire format: unchanged
POST /preparestill receivesstartEpochandendEpochintegers — they're just computed client-side frompaymentCount + nextSuperblockSecimmediately before submit, instead of typed by the user. Backend routes (routes/govProposals.js), validators (lib/proposalValidate.js), and DB schema are untouched.Legacy drafts
Anyone with a saved draft containing
startEpoch/endEpochwill see those values discarded on next load (fromDraftdrops them explicitly), anddraftBodyFromFormnulls them on update. On submit, fresh values are computed from the live anchor. No DB migration needed — draft rows keep the columns, they just get rewritten to null on the next save.Pairing PR
Requires syscoin/sysnode-backend#13 which exposes
superblock_next_epoch_secon/mnStats. Merge + deploy the backend PR first; without it, the wizard detects the missing anchor and disables "Prepare proposal" rather than silently submitting a fallback window.Test plan
CI=true npx react-scripts test— 755/755 pass across 48 suites (includes 35 newgovernanceWindowtests, plus revisedproposalForm/NewProposalsuites).gobject getreportsstart_epochandend_epochbracket the next superblock with ~15 d padding either side./mnStats(devtools network) and confirm the Prepare button stays disabled with the "waiting for live superblock timing" tooltip instead of submitting a (now + cycle) fallback.startEpoch/endEpochpopulated, confirm those inputs no longer appear, the derived window renders, and submit works.Made with Cursor