Release: develop -> main - #85
Open
github-actions[bot] wants to merge 1 commit into
Open
Conversation
* Alert before protocol equity reaches MINIMUM_EQUITY (#83) deuro.equity() was already read every cycle and persisted as DeuroState.reserveEquity, but nothing alerted on it. Two paths in Equity.sol are gated on that value falling below MINIMUM_EQUITY (1'000 dEURO): _calculateShares takes its bootstrap branch and assigns a flat 10'000'000 nDEPS for a single deposit, and restructureCapTable becomes callable and burns the balances of the holders passed to it. Both gates are evaluated per transaction, so a single block below the threshold is enough. The watcher computes headroom = reserveEquity - MINIMUM_EQUITY from the value already fetched — no new contract call — and pages on two tiers: a configurable warning floor, and critical once headroom is gone or the trend projects it gone within a configurable horizon. deuro_state is a single upserted row and keeps no history, so the trend had nowhere to come from. A new equity_samples table takes one sample per cycle (30-day retention) and the watcher fits an ordinary least-squares slope over the last 24 h of it; a series that is too thin or too short is never projected from. Dedup, escalation and re-arm follow the PositionState.miniLifetimeAlertedAt pattern: the marker is written only after telegramService.sendCriticalAlert confirms delivery, so a failed send is retried next cycle. A WARNING -> CRITICAL step bypasses the repeat window, a demoted level never lowers the stored marker, and only a recovery above the floor clears it and sends a resolve message. MINIMUM_EQUITY is a private constant of Equity.sol with no on-chain getter (eth_call reverts), so it is hardcoded next to the logic. The alert body names the mitigation, because it is neither obvious nor expensive: equity() is balanceOf(reserve) - minterReserve() and reserve() is the Equity contract itself, so a plain dEURO transfer to that contract raises the headroom 1:1 without minting shares and without a vote. * Correct the alert copy and harden the headroom edge cases Review findings on the first commit: equity() is clamped at zero — DecentralizedEURO.sol returns 0 when balanceOf(reserve) <= minterReserve() instead of a negative number. The mitigation text claimed a transfer raises the headroom 1:1, which is false during a reserve deficit: equity() stays at 0 and the transfer has no effect until the deficit is covered. An operator following that text under pressure would act into the void. The copy now states the clamp, and the alert gains a "Reserve deficit" line — computed from reserveTotal and reserveMinter, which the state already carries — that names how much has to arrive before the headroom moves at all. Both contract gates compare strictly (capitalBefore < MINIMUM_EQUITY and require(dEURO.equity() < MINIMUM_EQUITY)), so at exactly zero headroom neither path is open yet. Alerting there stays as a safety margin, but the wording no longer claims the paths are already live; the reason text now distinguishes reaching the boundary from crossing it. Three edge cases in the pure logic: - computeTrend accepted non-finite timestamps. NaN comparisons are always false, so neither the span guard nor the zero-variance guard caught them and the result was usable:true with a NaN slope. Guarded at both ends now. - formatDeuro printed -0.00 for sub-cent negative amounts, because the sign was taken before cent truncation. - warningFloorWei=0n is documented to mute the warning tier, but a negative headroom still collected a "below warning floor of 0.00" reason into the critical message. The formatDeuro doc comment overstated its precision guarantee and now says what actually holds. Four tests added for the new edge cases (62 total). * Give warnings and the all-clear their own Telegram header Second review round on this branch. Every headroom message went through sendCriticalAlert, which stamps "🚨 CRITICAL ALERT" on whatever it is handed. A warning arrived under a critical banner, and so did the all-clear — the graduated severity this watcher is built around never reached the operator. TelegramService gains sendAlert(icon, header, message); sendCriticalAlert keeps its signature and behaviour and now delegates to it, so the twelve existing call sites are untouched. The watcher passes its own header per level, and the first line of the body no longer repeats what the header already says. The message also claimed both gated contract paths go live the moment headroom turns negative. Only restructureCapTable does. Equity._invest reverts with InsufficientEquity when post-transfer equity is still below MINIMUM_EQUITY, so the bootstrap branch needs a deposit large enough to lift equity back over the line in the same transaction. Said properly now, in the alert body and in the trigger reason. The reserve-deficit line was off by one wei: equity() clamps at balance <= minterReserve, so covering exactly the deficit reaches equality and still yields zero. It now says the headroom starts moving once the balance exceeds the minter reserve. Comment corrections, all of them things that were not true as written: - the watcher registration still described the pre-correction boundary - "NaN/Infinity comparisons are always false" holds for NaN only - the varX === 0 branch is unreachable behind the span guard, and now says so - escalation bypasses the repeat window only out of a WARNING marker; once a reminder has raised the stored level to CRITICAL, a relapse waits for the window. That is the price of the flutter guard and is now written down - the second inconsistency throw is redundant (strictNullChecks is off), kept as fail-loud defence Tests: the "identical timestamps" case was passing for the wrong reason — it never reached the zero-variance guard it claimed to cover, the span guard returns first. Renamed to what it actually exercises. The non-finite guard is now parametrised over NaN, Infinity and -Infinity, and a new case pins the accepted CRITICAL → reminder → relapse suppression (65 total). * Let computeTrend enforce its own window, and stop projecting the past Third review round on this branch. Two hardenings and two comments that described something the code does not do. computeTrend documented TREND_WINDOW_HOURS as its lookback but never enforced it — only the caller's DB query did. That works today and fails quietly the moment anything else calls it: a single old sample outside the window can turn a steeply falling series into a rising slope and the projection then never fires. It now filters relative to its newest sample, so the constant is enforced where it is defined; the caller's query stays as an efficiency measure. Trigger (2) fired on an already-breached floor as well, because hoursToDepletion is 0 there. The message then carried both "equity is below MINIMUM_EQUITY" and "projected to deplete in 0.0 h" — a forecast of something that has happened. The projection now requires positive headroom. Classification is unchanged; trigger (1) already covers that case. decideAction now rejects a non-positive repeat window instead of turning every cycle into a reminder. Not reachable through the current caller, which passes a constant, but this module fails loud rather than degrade. Comment corrections: the module header still described the bootstrap branch as opening on any deposit below MINIMUM_EQUITY (it needs one large enough to lift equity back over the line), and hoursToDepletion is documented as null-when-not-falling while it returns 0 for a breached headroom. Ten tests added (75 total): empty and single-sample series, the window filter, a non-finite slope reached through a finite series, a non-linear irregularly spaced fixture with a hand-derived OLS expectation that a naive two-point slope would fail, and the classification boundaries — headroom exactly at the floor, depletion exactly at the horizon, a muted floor that still allows a critical projection, and a breached headroom that no longer claims to project. * Make the four descriptions of the bootstrap gate agree Fourth review round. Neither lens found a behaviour defect this time; what was left was prose that had drifted and tests that did not pin what their names claimed. The same contract mechanic is described in four places — the logic module header, the checkEquityHeadroom docstring, .env.example and the alert text itself. The qualifier added earlier (the bootstrap branch needs a deposit large enough to lift equity back over MINIMUM_EQUITY, otherwise Equity._invest reverts with InsufficientEquity) had only reached two of them. All four now say the same thing, and the three short ones stay short so they are less likely to drift apart again. The watcher registration comment claimed the alert fires when equity reaches MINIMUM_EQUITY. It has three triggers: the configured warning floor, well above that; a projected breach within the horizon; and the boundary itself. computeTrend's docstring described the regression but not the window it enforces since the last commit, so the returned sampleCount and spanHours looked like they described the raw input. Documented, along with why the caller's DB query is a load bound rather than the window: the query cuts at wall-clock now, computeTrend cuts relative to the newest sample, and since that sample is never in the future its cutoff is always the earlier one — the internal filter can never drop what the query returned. Four tests added (79 total): the window boundary is inclusive to the millisecond, a series that only becomes unusable after filtering reports the filtered metadata, zero headroom with a falling trend emits no depletion projection (a guard against >= creeping back into that condition), and the repeat-window guard rejects negative values too — the existing test only covered zero, so narrowing it to === 0n would have gone unnoticed. * Fail loudly on an empty threshold, and stop overclaiming in comments Fifth review round. The two new thresholds fell back to their defaults through `||`, which also swallows a variable that is set but empty. These two values decide whether the alert fires at all, so an unparseable value has to reach the existing validation instead of vanishing. The default now applies only when the variable is unset. Verified against the real factory: unset gives 5000, "0" stays 0 (it mutes the warning tier), and both an empty value and garbage now fail validation at boot. Two comments claimed more than the code guarantees — the same failure the last round was about: - computeTrend's docstring said sampleCount and spanHours always describe the filtered window. On the early non-finite-timestamp path they do not: it returns the raw sorted length with a span of 0, before any window is formed. - The note on the sample query said computeTrend's cutoff can never drop what the query returned. That holds only while no sample timestamp sits in the future. If the clock jumps backward, computeTrend's window becomes the later bound and does drop samples — which is its documented semantics, but not what the comment promised. The test for a series that only becomes unusable after filtering asserted against a hardcoded span of 93 hours rather than the span of the samples it had just built, so a drifting fixture would have kept the sanity check green. It now derives the span from the timestamps. * Test the marker-after-delivery guarantee, and reject partial numbers Sixth review round. The feature's core promise — the dedup marker is written only after Telegram confirms delivery — had no test. Neither did the recovery path. deuro.service.spec.ts now drives checkEquityHeadroom against a stubbed repository and a stubbed sender: a quiet state alerts nothing, a confirmed warning persists the marker, a failed send leaves it untouched so the next cycle retries, a confirmed recovery clears it, a failed all-clear does not, and each level arrives under its own header. The thresholds still accepted a partially numeric value: parseInt('72h') is 72, so EQUITY_HEADROOM_PROJECTION_HOURS=72h would have booted with a plausible-looking wrong horizon. Number() is not the fix — Number('') is 0, and 0 on the warning floor means "mute the warning tier", so an empty value would silently disable the warning instead of failing. A strict integer check rejects both. monitoring.config.spec.ts pins the whole contract: unset, "0", explicit value, empty, non-numeric, partially numeric, surrounding whitespace, and 0 for the horizon, which @min(1) must reject even though the floor accepts it. Two comment corrections. decideAction said an escalation "must always page" — it is a pure function that can only decide; delivery belongs to the service layer. And the note on the sample query described the clock skew case but not the ordinary one: after an outage the wall-clock query is the tighter bound, so the regression runs over less than TREND_WINDOW_HOURS — fewer data points, not a wrong result. .env.example now states that the defaults apply only when a variable is unset, and that anything set but unparseable stops the boot. 94 tests across 5 suites. * Cover escalation, reminder and the empty first run Seventh review round. Three orchestration paths had no service-level test. The escalation is the one that matters in a real incident: a stored WARNING and a headroom that has since reached zero must page immediately, in the middle of the repeat window, or the deterioration is swallowed by the dedup it was supposed to escape. Also covered now: the reminder that fires once the window has elapsed, and the very first run, where no state is persisted yet and the watcher must return without touching Telegram or the marker. parseIntegerEnv accepted '-1' as a well-formed integer and left the rejection to @min(0), but refused '+5000' outright. The asymmetry was arbitrary; both signs parse now, and the tests pin which layer rejects what — format for exponential and hexadecimal notation, the value constraint for a negative floor. 101 tests across 5 suites. * Make the negative-floor test prove which layer rejects it Eighth review round, one finding. The test named "rejects a negative warning floor via @min(0), not the format regex" asserted only that something throws. Drop the minus from the integer pattern and '-1' would fail at the format check instead — earlier, for a different reason — and the test would stay green while its name became false. It now asserts that the validation error names the min constraint and does not name isNumber, so the layer that rejects the value is pinned, not just the fact that it is rejected. * Describe the constraint check as it actually works Ninth review round, one comment. The note on the negative-floor test said a broken regex would make @isnumber "fail first", implying class-validator stops at the first violation. It does not: NaN violates @min(0) and @isnumber at once and both names land in the message. The assertion was right for that reason, not despite it — the comment now says so.
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.
Automatic Release PR
This PR was automatically created after changes were pushed to develop.
Commits: 1 new commit(s)
Checklist