test: add property-based invariant tests for credit accrual (closes #75) - #139
Open
Cyber-Mitch wants to merge 1 commit into
Open
test: add property-based invariant tests for credit accrual (closes #75)#139Cyber-Mitch wants to merge 1 commit into
Cyber-Mitch wants to merge 1 commit into
Conversation
✅ Deploy Preview for sdcontracts ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
Author
|
@prodbycorne Please review |
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 #75
Adds four property-based tests for the credit-accrual system, each backed by a closed-form derivation rather than a chosen tolerance. One important correction to the issue as filed: the tolerance the issue expects for deposit-granularity and checkpoint-frequency independence does not exist for this implementation — both properties hold as strict equalities, and the analysis below shows why, with the loss bound the issue anticipated shown to be structurally unreachable via those two paths (though reachable via a different, adjacent path — noted at the bottom).
The derivation
Notation:
B(a,p) = ⌊a·p/100⌋(the only nonlinearity in the whole accrual path).compute_total_stake(lib.rs:248-253) reduces to:S(a,p,m) = (a − B(a,p)) + B(a,p)·m = a + B(a,p)·(m−1)
and
compute_credits = S(a,p,m)·r·elapsed.Where truncation loss actually comes from. For a partition
A = a₁+…+aₙ, writingaᵢ·p/100 = qᵢ + fᵢwithfᵢ ∈ [0,1):B(A,p) − Σᵢ B(aᵢ,p) = ⌊Σfᵢ⌋ ∈ [0, n−1]
Upper bound
n−1, tight atp=99, aᵢ=1, n=100. An implementation that bankedBseparately per part would lose up to(n−1)(m−1)·r·elapsedcredits — this is the issue's predicted loss, now with the constant pinned. I validated the formula empirically: 7 users staking 101 each vs. one user staking 707, atp=33, m=7— predicted⌊7·⅓⌋·(m−1) = 12, measured12. Exact match.But this loss is not reachable by splitting a single user's deposit.
stake()doesexisting.amount += amount(lib.rs:688) before any later call tocompute_total_stake, and every call site passes the cumulativestake.amount(lib.rs:271, 859).Bis therefore only ever evaluated on the aggregate — the per-part truncation the issue's suggested tolerance was defending against never occurs in this code path.Property A — deposit granularity → strict equality, tolerance = 0
Two facts: (1) a second
stake()at the same ledger checkpoints withelapsed = 0, socredits_banked += S·r·0 = 0; (2)existing.amount += amountaggregates beforeBis next evaluated. After subdividing a deposit,UserStakeis field-for-field identical to the unsplit case, so every later checkpoint observes identical state. Loss = 0, not bounded-by-ε.Asserted against an independent closed-form oracle (
credits = r · Σⱼ S(Aⱼ, p, m) · Δeⱼ, computed in the test) so the property can't pass by both the contract and the test being wrong the same way.Property B — checkpoint frequency → strict equality, tolerance = 0
Chopping
[t₀, T]into arbitrarily many sub-intervalst₀<t₁<…<t_kwithA, p, mfixed:Σⱼ S·r·(tⱼ₊₁−tⱼ) = S·r·Tby plain integer distributivity — no rounding is introduced by time-partitioning. The floor inBis a function of(amount, pct)only, evaluated once per checkpoint regardless of how many checkpoints occur. This is why the issue's expected tolerance doesn't exist here: the property is strictly stronger (==, not≤ ε) than what was asked for, and a tolerance-bounded version would have been blind to a real regression of up to(n−1)(m−1)·r·e.Preconditions stated in the test's doc comment, since outside them the property is false by design:
credit_ratefixed,global_multiplierfixed (see Property E below),allocation_pctset before the first stake.Property C — monotonicity → strict increase
get_credits = credits_banked + S(A,p,m)·r·elapsed. Every slope factor is≥ 1:S ≥ A ≥ 1(enforced, lib.rs:672),r ≥ 1(#89's ceiling). SoΔcredits ≥ 1forΔelapsed ≥ 1— strictly increasing, not merely non-decreasing. No truncation ambiguity:elapsedappears only as a multiplicand. Covers bothget_creditsand the lock/unlock credit path.Property D — non-negativity
get_creditsis a sum of products of provably non-negative factors:principal = A − B(A,p) ≥ 0(sincep ≤ 100 ⇒ B ≤ A),virtual = B·m ≥ 0,r > 0,elapsed ≥ 0. Generators respect the contract's own gates (amount > 0,min_stake_amount,pct ∈ 1..=100, whitelist default-disabled) so the property is never asserted on unreachable states.Property E — flagged, not implemented (per the issue's own instruction not to encode a guess as a passing test)
Investigating deposit-granularity and checkpoint-frequency turned up a real, undertested asymmetry:
checkpointsnapshotscredit_rateintoUserStake(lib.rs:278), so a mid-flightset_credit_ratechange is not retroactive — but it re-readsglobal_multiplierlive and applies it to the already-elapsed segment. Sinceset_global_multipliercannot checkpoint other users first, this means an admin multiplier change retroactively reprices every user's currently-open segment.Demonstrated concretely:
m=2, p=100, A=10_000earning10_000credits, thenset_global_multiplier(3)with zero ledgers elapsed → credits jump to30_000.test_admin_multiplier_change_applies_from_next_checkpoint(test.rs:553) is named as though snapshot semantics hold, but it deliberately inserts a no-op checkpoint immediately before the multiplier bump (test.rs:562), so it never actually exercises the retroactive case.Question for the maintainer: should
global_multiplierbe snapshotted intoUserStakealongsidecredit_rate(making the change non-retroactive, matching the rate's documented semantics and the sibling test's name)? Or is retroactive repricing of open segments intended? Both readings are defensible and nothing in the codebase settles it — asserting either direction here would encode a guess as a passing test, so I left it as a documented comment block in the diff (test.rs:2638) rather than a test.(Out of scope, recorded for completeness: the
(n−1)(m−1)·r·eloss from the derivation above is reachable — via cross-user aggregation, not deposit splitting. That's an aggregation invariant, not a checkpoint-frequency one, and belongs to a separate issue.)Coordination
proptestanywhere onupstream/main. Added it here; if farming-pool: no fuzz/boundary test exercises i128 overflow boundaries in compute_credits/compute_total_stake #76 lands first, it should reuse this dependency line rather than duplicate it.MAX_GLOBAL_MULTIPLIERandMAX_CREDIT_RATEdirectly rather than restating them, so ranges track any future ceiling change automatically.amount ∈ 1..=10¹²— worst case~4.5×10²⁸, ~10 orders of magnitude belowi128::MAX, deliberately far inside accounting-error territory only; overflow boundaries are farming-pool: no fuzz/boundary test exercises i128 overflow boundaries in compute_credits/compute_total_stake #76's job.Mutation testing — proof these tests have teeth
Rather than trust the assertions by inspection, I deliberately broke the contract four ways and confirmed each mutation is caught by a different assertion independently, then restored
lib.rsto pristine (git diffempty):amount / 100 * pct(divide-before-multiply)checkpointdrops one ledger of elapsed timeset_boostcheckpoint onlyAll four assertions independently live.
Verification
cargo build --workspace --target wasm32v1-none --releasecargo fmt --all -- --checkcargo clippy --workspace --all-targets -- -D warningscargo test --workspaceNew properties specifically (64 cases each): 3.15s / 3.02s / 1.40s / 1.33s individually, 4.34s run in parallel.
proptestconfirmed dev-only —stringson the release WASM shows zeroproptestoccurrences,grep -a proptest farming_pool.wasmabsent, WASM builds clean.Changed files
Cargo.lock— 25 new packages (proptest's dependency tree), zero removed, zero existing versions changed.farming-pool/Cargo.toml—proptest = "1"as a plain[dev-dependencies]entry (not thetestutilsfeature, which forwards intofactory's dependency graph — kept separate so it can never reach the deployable build).farming-pool/src/test.rs— the four property tests plus the Property E comment block.lib.rsuntouched — no contract logic changed.Acceptance criteria
proptestadded as a dev-dependencycapture_snapshot_at_drop: falseprevents one snapshot JSON per proptest caseFound, not fixed
global_multiplierretroactivity (Property E above) — real, demonstrated, needs your call on intended semantics.cargo test --workspacerequires a WASM pre-build step not documented outsideci.yml:38—factoryembedsfarming_pool::WASMviacontractimport!, so a stale/absent artifact throwsError(WasmVm, UnexpectedParameterLen)in 16 factory tests. Not introduced by this PR; worth a README note.test_snapshots/*.jsonare stale, for two independent reasons (non-reproducible embedded-WASM-hash/instruction-count codegen, and some predating theMinStakeAmount/SchemaVersionfields). A fullcargo test --workspacerun rewrites them; I restored them to their committed state before finalizing this diff, so running the suite again will dirty them the same way. Not contract-logic breakage, not introduced here.None of the above involves contract-logic changes —
lib.rsis byte-identical tomain.