fix(monitoring,analytics,crowdfunding): bound growth and replace placeholder metrics - #1241
Closed
JamesEjembi wants to merge 1 commit into
Closed
JamesEjembi wants to merge 1 commit into
JamesEjembi wants to merge 1 commit into
Conversation
…eholder metrics Resolves MettaChain#1195, MettaChain#1196, MettaChain#1197 and MettaChain#1198. 144 tests pass across the three touched crates, up from 98; fmt and clippy are clean on the changed code. Two unbounded Vecs that grew for the lifetime of the contract are now capped, and four placeholder metrics are now derived from recorded state. MettaChain#1198 - crowdfunding reported fabricated analytics as measured outcomes. get_campaign_analytics returned a hardcoded 8_000 bps "80% placeholder" retention rate; get_investor_demographics assumed 70% accredited investors and invented both a jurisdiction split and an investment-size histogram, while reporting them for a campaign found by scanning every campaign id. Retention is now the share of recorded investors who have not refunded, accreditation is counted from stored profiles, and the split and histogram are grouped and bucketed from real per-investor data. Because retention over an empty cohort and a jurisdiction split with no stored profile are undefined rather than zero, both became Option and return None. get_funding_timeline built 30 points on a straight target_amount/30 ramp. investments is keyed by (campaign_id, investor) and holds a running total with no per-investment timestamp, and campaign_investors is an unordered Vec, so no cumulative curve is reconstructible. It now returns None and documents the state change that would make it real. line.rs is deleted: 1 835 lines, never compiled because it is not declared as a module, referenced nowhere, and all 45 of its public function names duplicate methods that already exist in lib.rs. Its unused ink_e2e dev-dependency is also removed. It pulled in trie-db 0.28.0, which does not compile on the nightly this repo pins, so cargo test -p propchain-crowdfunding failed before running a single test. MettaChain#1196 - QuorumGuard appended one entry per proposal to an uncapped Vec, so storage and the participation read grew without bound. History is now a rolling window of MONITORING_MAX_QUORUM_HISTORY, mirroring the snapshot buffer the crate already uses, with a lifetime total_recorded that survives eviction and None rather than a stale value for an evicted proposal. MettaChain#1197 - alerts were on-chain records with no delivery path, so an operator whose indexer was down during an incident missed the signal. Alerts are now also appended to a bounded ring buffer, with a gap-free batch read, a self-contained JSON payload, idempotent acknowledgement, and pending_alert_count as the retry-set size. A stale cursor is clamped forward so a worker that was away resumes rather than silently receiving nothing. docs/alert_delivery.md specifies the contract, including what it does not do. Two further silent-divergence bugs found on the way: batch_update_metrics assigned self.current_metrics in a loop, so only the last entry survived while the event reported the full count. Entries are now combined - volumes and counts sum, average_price is the volume-weighted mean - and the event carries the resulting value so it cannot disagree with storage. This is a behaviour change: last-write-wins became a combined view. batch_add_trends emitted BatchMetricsUpdated, so a consumer tailing that event for metric changes got a spurious one per trend. It now emits BatchTrendsAdded. MettaChain#1195 - the premise needed correcting: get_market_metrics returns stored metrics rather than building a second view, and this contract has no property valuations, listing set or trade tape, so there is nothing on chain to recompute an average price or volume from. An oracle + staking snapshot would mean building an integration that does not exist, and inventing a derivation would repeat MettaChain#1198 here. What is achievable without a second source is now done: an FNV-1a checksum over the metric fields is written on every update and verified by verify_market_metrics_integrity, provenance exposes the writer, timestamp, count and override flag, every write emits MarketMetricsOverridden with the previous and new values, and set_market_metrics is the only writer so none of that can be bypassed. MONITORING_ANALYTICS_RESOLUTIONS.md covers all four issues in detail, including the parts that are deliberately left as follow-ups.
|
@JamesEjembi Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Author
|
Superseded. The analysis of these issues is recorded in #1242 (docs only); the code fix is being reworked. |
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
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 #1195
Fixes #1196
Fixes #1197
Fixes #1198
Summary
Two storage structures that grew for the lifetime of the contract are now capped, and four metrics that were fabricated constants are now derived from recorded state. Along the way two silent-divergence bugs turned up in
analyticsthat are the exact failure mode #1195 is about.144 tests pass across the three touched crates (analytics 43, crowdfunding 40, monitoring 61), up from 98.
cargo fmtandcargo clippyare clean on the changed code.None; duplicateline.rsdeletedquorum_guardhistory grew without boundTwo unbounded Vecs
QuorumGuard.history(#1196) appended one entry per proposal forever. It is now a rolling window ofMONITORING_MAX_QUORUM_HISTORY, mirroring the snapshot buffer the crate already uses. A separate lifetimetotal_recordedsurvives eviction, so the number of proposals observed is never lost — only per-proposal detail of old ones.participation_bpsreturnsNonefor an evicted proposal rather than a stale value.The alert log (#1197) is capped the same way at
MONITORING_MAX_ALERT_LOG.Placeholder metrics, and the state they needed
investor_retention_rate8_000— a literal80% placeholderaccredited_investorstotal_investors * 7 / 10— "assume 70%"jurisdictionsInvestorProfile.jurisdictioninvestment_distributioninvestmentsamountstop_investor_amountmax_investment = 0, so always zeroget_funding_timelinetarget_amount / 30rampNoneTwo of these became
Optionbecause the honest answer is "undefined", not zero: retention over an empty cohort, and a jurisdiction split when no profile is stored. Reporting8_000there is what the issue was about.get_investor_demographicsalso compounded this — it scanned every campaign id to find one, then reported figures unrelated to that campaign's investors.get_funding_timelineis a deliberateNone, not an oversight.investmentsis keyed by(campaign_id, investor)and holds a running total with no per-investment timestamp, andcampaign_investorsis an unorderedVecrather than a chronological log. No cumulative curve is reconstructible, and producing one from that data would be the same fabrication the issue reports. The docs describe the state change that would make it real.line.rsdeleted1 835 lines, never compiled because it is not declared as a module in
lib.rs, referenced nowhere in the repo, and all 45 of its public function names duplicate methods already inlib.rs. It was a live copy of every placeholder above.Its unused
ink_e2edev-dependency is also removed. There is notests/directory and no reference to it, but it pulled in the substrate stack, andtrie-db0.28.0 does not compile on the nightly this repo pins — socargo test -p propchain-crowdfundingfailed before running a single test, onmaintoo.Alert delivery (#1197)
AlertTriggeredstays authoritative; the ring buffer is the operational retry surface.get_recent_alerts(since_alert_id, limit)— gap-free batch read. A cursor older than the window is clamped forward, so a worker that was offline resumes instead of silently receiving nothing and assuming it is up to date.limit = 0means "everything retained" rather than "nothing", since an unset limit returning empty is indistinguishable from a healthy system.alert_payload(alert_id)— self-contained blob with the alert type's stable name, a severity ranking, and canonical JSON, so a consumer needs no follow-up calls and no SCALE enum decoding.acknowledge_alert(alert_id)— idempotent, so a crash between POST and ack is safe to redeliver.pending_alert_count()— the retry-set size; a value that is not falling means delivery is stuck even while the contract is healthy.Two bugs in my own ring-buffer readers, both caught by the eviction tests: the batch cursor treated
since_alert_id = 0as "skip alert 0" instead of the "handled nothing" sentinel, andpending_alert_countread without the buffer modulo, silently missing every id at or above the cap.docs/alert_delivery.mdspecifies the contract, including a recommended worker loop and an explicit list of what the contract does not do.Two further silent-divergence bugs
Both are in
analyticsand both are what #1195 is about, so they are fixed here.1.
batch_update_metricsdiscarded all but the last entry. It looped assigningself.current_metrics, so only the finalMetricUpdatesurvived whileBatchMetricsUpdatedreported the fullcountas though all had been applied. Entries are now combined — volumes and counts sum,average_priceis the volume-weighted mean — and the event carries the resulting value so it cannot disagree with storage.2.
batch_add_trendsemittedBatchMetricsUpdated. Adding a trend emitted the market-metrics event, so a consumer tailing that event got a spurious one per trend with no way to tell them apart. It now emitsBatchTrendsAdded.A correction to #1195's premise
The issue asks for tests pinning
get_market_metricsto a recomputed view. Two things make that not possible as written:get_market_metricsreturnsself.current_metrics.clone()— a single stored value. There is no second derivation path to pin against.historical_trends,property_sentimentsandportfolio_positionscannot yield an average price, a total volume or a listing count.average_priceandtotal_volumeare prices and amounts, and no aggregation of this contract's own state produces them.The proposed
oracle + stakingsnapshot would mean building an oracle integration that does not exist here — a large feature well beyond a consistency-test issue — and inventing a derivation that returns plausible-looking numbers would reproduce #1198 in a different contract. That is left as an explicit follow-up and documented onupdate_market_metricsrather than faked.What is achievable without a second source, and what the acceptance criteria ask for, is now done: make provenance explicit and make a silent divergence impossible to miss.
verify_market_metrics_integrity()recomputes and compares. This is the on-chain expression of the recompute-equals-stored invariant. It is an integrity check, not a commitment — it does not constrain a malicious writer, who recomputes it anyway.get_metrics_provenance()returns the live metrics plus writer, timestamp, lifetimeupdate_count,is_overrideandis_intact, so a consumer can tell an admin-supplied figure from a contract-derived one.MarketMetricsOverriddenwith previous and new values, writer and timestamp.set_market_metricsis the only writer, so the checksum, provenance and event cannot be left out of an update path.Testing
98 → 144 tests, all new ones listed in
MONITORING_ANALYTICS_RESOLUTIONS.md. Two #1198 tests need a state the public API cannot reach —investrequires an onboarded, KYC-approved, accredited profile, so a campaign built through the public API always has exactly one profile per investor. Two#[cfg(test)]-gated helpers (erase_investor_profile,set_investor_accredited) reach a missing or revoked profile; they are compiled out of non-test builds and are not part of the contract interface.Known remaining gap
Eight crates declare
ink_e2e, but onlycontracts/libuses it and notests/directory references it at all. Seven unused declarations remain after this change, so those crates'cargo testis likely still broken for the sametrie-dbreason. Left out of scope here, but worth its own PR.Docs
MONITORING_ANALYTICS_RESOLUTIONS.md— what each issue reported, what the code actually did, the fix, and the verification, including the parts deliberately left as follow-ups.docs/alert_delivery.md— the Monitoring alerts are only on-chain records - no out-of-band delivery hook or retry for critical alerts #1197 delivery contract.