docs: record resolutions for the four assigned issues - #1242
Open
JamesEjembi wants to merge 1 commit into
Open
JamesEjembi wants to merge 1 commit into
JamesEjembi wants to merge 1 commit into
Conversation
Index of the analysis behind PR MettaChain#1241 for MettaChain#1195, MettaChain#1196, MettaChain#1197 and MettaChain#1198, plus three corrections to the issues' stated premises that affect how the fix should be reviewed: - MettaChain#1196 says the guard iterates history to count participation. It does not - record() only reads .last(), which is O(1). history is unbounded and write-only, so nothing ever queries it. That makes the defect worse (pure storage cost, no consumer) and the cap safer (no participation behaviour changes), but the issue's acceptance criterion is not measurable against this code. - MettaChain#1195 asks for a test pinning reported metrics to recomputed ones. get_market_metrics returns a single stored value and the crate stores no valuations, listing set or trade tape, so there is nothing to recompute from. Recorded as a follow-up rather than faked, since inventing a derivation would reproduce MettaChain#1198 here. - MettaChain#1197 frames the gap as a record with no delivery. There is no on-chain alert log at all, so a consumer has no cursor to resume from. Documentation only - no source changes.
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.
Closes #1195
Closes #1196
Closes #1197
Closes #1198
Documentation only — no source changes. Adds
ASSIGNED_ISSUE_RESOLUTIONS.md, the index to the analysis behind #1241 for the four issues currently assigned to me, confirmed againstmainatd4d28a17.All four are already implemented in #1241, which also adds the detailed write-ups (
MONITORING_ANALYTICS_RESOLUTIONS.md,docs/alert_delivery.md) and the code. This PR adds the index and the three premise corrections below.#1241 carries the fixes; this PR only closes the issues. Land #1241 first — merging this one alone closes four issues against a documentation change.
All four share one root cause
A value the code cannot honestly compute was replaced with a plausible constant. The instances are in three crates, but it is one design decision repeated — which is why they are worth fixing together and worth reviewing as one question: what does this contract do when it does not know? In every case the honest answer became
None, and in every case the previous answer was a number a regulator or deal desk could read as a measurement.Three corrections to the issues' premises
#1196 — there is no O(n) participation scan
The issue says the guard "counts participation by iterating"
history. It does not.record()incontracts/monitoring/src/quorum_guard.rsreads only.last(), which is O(1); the sole other accessor ishistory_len().That makes it worse in one respect and better in another:
historyis unbounded and effectively write-only. Every entry before the last is storage cost with no consumer.The correct framing is "unbounded write-only storage", not "unbounded read". Worth knowing when reviewing: the issue's acceptance criterion, "participation query bounded by a constant after compaction", is not measurable against this implementation because there is no participation query to bound. The cap satisfies the intent; the literal criterion has nothing to attach to.
One existing test needs updating:
history_accumulatesassertshistory_len() == 2and will need to assert against the cap.#1195 — the proposed test is not possible as written
The issue asks for a test asserting offline-recomputed
MarketMetricsmatch the stored ones. Two independent reasons block that:get_market_metrics(contracts/analytics/src/lib.rs:223) isself.current_metrics.clone()— a single stored value, with no second derivation path to pin against.historical_trends,property_sentimentsandportfolio_positionscannot yield an average price, a total volume or a listing count.The suggested
oracle + stakingsnapshot would mean building an oracle integration that does not exist here — well beyond a consistency-test issue. Inventing a derivation that returns plausible numbers would reproduce #1198 in a different contract, which is the exact failure mode the issue is about. So it is recorded as an explicit follow-up and documented onupdate_market_metricsrather than faked.What is achievable without a second source — and what the acceptance criteria actually ask for — is done: integrity checksum, provenance (
get_metrics_provenance), override tracing, and a single write path so none can be bypassed.Worth being explicit that the checksum is an integrity check, not a cryptographic commitment. It catches accidental divergence; it does not constrain a malicious writer, who recomputes it.
#1197 — there is no on-chain alert log at all
The issue frames this as alerts being "only on-chain records" with no delivery. In fact there is no durable record either:
check_and_trigger_alerts(contracts/monitoring/src/lib.rs:519) emitsAlertTriggeredand returns. No storage, no retrieval message, no acknowledgement.That widens the gap. An indexer that is not polling when an alert fires has no way to learn it happened except by scanning the contract's full event history, and there is no cursor to resume from — so #1241's bounded ring buffer is not just a delivery convenience, it is what makes recovery after downtime possible at all.
Two silent-divergence bugs found while verifying #1195
Both are the failure mode #1195 reports, so both are fixed in #1241.
batch_update_metricsdiscarded all but the last entry (:245-263). The loop assigns rather than combines, so with N updates only the last survives — whileBatchMetricsUpdatedreportscount: Nas though all landed. A consumer trusting the event count would believe N updates applied when 1 did. Now combined, with the resulting value in the event so it cannot disagree with storage.batch_add_trendsemittedBatchMetricsUpdated(:268-281). Adding a trend emitted the market-metrics event, so a consumer tailing it got a spurious event per trend batch with nothing in the payload to tell them apart. Now emitsBatchTrendsAdded.Both are invisible to tests that only exercise single-entry calls, which matches #1195's report that nothing pins the reported numbers to anything.
#1198 —
get_funding_timelinereturningNoneis deliberateinvestmentsis keyed by(campaign_id, investor)and holds a running total with no per-investment timestamp;campaign_investorsis an unorderedVec, not a chronological log. No cumulative curve is reconstructible from that state, and producing one anyway would be the same fabrication the issue reports. The docs describe the state change that would make it real.Also noted:
line.rsis 1 835 lines that are never compiled — it is not declared as a module anywhere, is referenced nowhere, and all 45 public function names duplicate methods already inlib.rs. It also carried an unusedink_e2edev-dependency, andtrie-db0.28.0 does not compile on the pinned nightly, socargo test -p propchain-crowdfundingfailed before running a single test — onmaintoo. The orphaned file was preventing the crate's tests from running at all.Known remaining gap
Eight crates declare
ink_e2e, but onlycontracts/libuses it and notests/directory references it. Seven unused declarations remain after #1241, so those crates'cargo testis likely still broken for the same reason. A greencargo testin this repository is not yet evidence of anything. Out of scope here, but it deserves its own PR.Verification
Not re-run for this documentation-only change. The figures in the doc — 144 tests passing (analytics 43, crowdfunding 40, monitoring 61), up from 98, with
cargo fmtandcargo clippyclean — are from #1241. Line references here were checked againstd4d28a17.