Skip to content

feat: add per market ttl preflight - #904

Merged
greatest0fallt1me merged 1 commit into
Predictify-org:masterfrom
pheobeayo:feat/add-per-market-ttl
Jul 24, 2026
Merged

feat: add per market ttl preflight#904
greatest0fallt1me merged 1 commit into
Predictify-org:masterfrom
pheobeayo:feat/add-per-market-ttl

Conversation

@pheobeayo

Copy link
Copy Markdown
Contributor

Pull Request Description

⚠️ Read first: the base branch does not compile

master @ 43fdfdd does not build. This is pre-existing and unrelated to this PR, but it means CI cannot pass and cargo test cannot be run — including for the tests added here. Details in "Test Results" below.

A reviewer checking out this branch will see the same errors. They are not from these changes.


📋 Basic Information

Type of Change

  • 🐛 Bug fix (non-breaking change which fixes an issue) — prerequisite DataKey repair
  • ✨ New feature (non-breaking change which adds functionality) — aggregate TTL preflight
  • 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • 📚 Documentation update
  • 🧪 Test addition/update
  • 🔧 Refactoring (no functional changes)
  • ⚡ Performance improvement
  • 🔒 Security fix
  • 🎨 UI/UX improvement
  • 🚀 Deployment/Infrastructure change

Related Issues

Closes #839 Related to #836 (documents the storage tiers this check operates on)

Priority Level

  • 🔴 Critical (blocking other development)
  • 🟡 High (significant impact)
  • 🟢 Medium (moderate impact)
  • 🔵 Low (minor improvement)

📝 Detailed Description

What does this PR do?

#839 asks for a storage-TTL preflight on create_market. A preflight already exists: check_market_creation_rent at storage.rs:41, wired into both creation paths (markets.rs:132, lib.rs:413), with three tests in market_creation_validation_tests.rs under a // ── Storage Rent Pre-flight Tests ── header.

Rather than reimplement it, this PR completes the parts that were scoped out. The code signposts them through two dead symbols:

Symbol Status before this PR
MARKET_CREATION_PERSISTENT_KEYS declared at storage.rs:26, never read, and its value (1) was wrong
Error::InsufficientStorageRentBudget declared at err.rs:239 with display strings at :1555, :1666, :1787 — never constructed

Deliberately not touched: repairing these means restructuring imports and module declarations across a 7,700-line file, which is a different change from "preflight storage TTL pressure" and would likely conflict with several of the 40 open PRs.

Manual Testing Steps

  1. Read lib.rs create_market end to end and enumerated every persistent().set(...) reached during one call, following record_market_created and append_record into their own modules to confirm each write's storage tier.
  2. grep -rn "InsufficientStorageRentBudget" --include=*.rs across the crate — confirmed four occurrences, all declaration/display, zero constructors.
  3. grep -rn "MARKET_CREATION_PERSISTENT_KEYS" --include=*.rs — confirmed a single occurrence (its own declaration).
  4. Recomputed effective_ttl * MARKET_CREATION_PERSISTENT_KEYS boundary values used in the tests against u32::MAX.
  5. Diffed all three edited files against master; confirmed two hunks in markets.rs, two in lib.rs, and the intended edits in storage.rs.

📚 Documentation

Documentation Updates

  • README updated
  • Code comments added/updated — rustdoc on the new function and the corrected constant; inline comments at both call sites explaining why one propagates and one panics
  • API documentation updated — NatSpec-style /// with # Formula and # Errors, matching the existing preflight's style
  • Examples updated
  • Deployment instructions updated
  • Contributing guidelines updated
  • Architecture documentation updated

Breaking Changes

Breaking Changes:

  • None.

Error::InsufficientStorageRentBudget was already a declared variant with a stable numeric code (523). This PR only makes it reachable, so clients switching on error codes see a code that was already in the enum. The new DataKey variants were already being constructed at call sites; the enum simply did not declare them.

Migration Guide: Not applicable.


🔍 Code Quality

Code Review Checklist

  • Code follows Rust/Soroban best practices
  • Self-review completed
  • No unnecessary code duplication
  • Error handling is appropriate — ok_or(...)? on both fallible steps, no unwrap()
  • Logging/monitoring added where needed — no events appropriate for a preflight
  • Security considerations addressed
  • Performance implications considered
  • Code is readable and well-commented
  • Variable names are descriptive
  • Functions are focused and small

Performance Impact

  • Gas Usage: Two u32 arithmetic operations and one max_ttl() read per create_market. Negligible.
  • Storage Impact: None — no additional reads or writes to storage.
  • Computational Complexity: O(1).

Security Review

  • No obvious security vulnerabilities
  • Access controls properly implemented — unchanged; no new entrypoint
  • Input validation in place — operates on ledger state, not user input
  • Oracle data properly validated — not applicable
  • No sensitive data exposed

🚀 Deployment & Integration

Deployment Notes

  • Network: Testnet first — the stricter check should be observed against a real ledger sequence before mainnet.
  • Contract Address: N/A
  • Migration Required: No — no storage layout migration; existing entries are unaffected.
  • Special Instructions: None.

Integration Points

  • Frontend integration considered — not applicable
  • API changes documented — new error surfaced from create_market; clients using try_create_market may now observe code 523
  • Backward compatibility maintained
  • Third-party integrations updated — not applicable

📊 Impact Assessment

User Impact

  • End Users: None in normal operation. The check only rejects ledger states that would have produced a partially-provisioned market.
  • Developers: create_market can now surface InsufficientStorageRentBudget (523). Two previously dead symbols are now live and meaningful.
  • Admins: Market creation fails cleanly rather than leaving a market whose statistics and audit entries lack a full TTL.

Business Impact

  • Revenue: None.
  • User Experience: Marginally improved — a failure mode that would have surfaced later as missing audit or statistics data now fails fast at creation.
  • Technical Debt: Net reduction. Removes two dead symbols, corrects a wrong constant, and eliminates one hardcoded TTL literal. Does not address the larger lib.rs breakage, which remains outstanding.

✅ Final Checklist

Pre-Submission

  • Code follows Rust/Soroban best practices
  • All CI checks passing — cannot pass; base branch does not compile (pre-existing)
  • No breaking changes (or breaking changes are documented)
  • Ready for review
  • PR description is complete and accurate
  • All required sections filled out
  • Test results included — including why the suite cannot run
  • Documentation updated

Review Readiness

  • Self-review completed
  • Code is clean and well-formatted
  • Commit messages are clear and descriptive
  • Branch is up to date with main
  • No merge conflicts

📸 Screenshots (if applicable)

Not applicable.


🔗 Additional Resources


💬 Notes for Reviewers

Please pay special attention to:

  • The value 3 for MARKET_CREATION_PERSISTENT_KEYS. Derived by reading the entrypoint's write path: market record, platform statistics, audit record. If any helper writes more entries than I traced, or a fourth write exists that I missed, the constant is too low and the check is correspondingly too weak. This is the single number most worth a second pair of eyes.

  • Whether commit 1 (the DataKey repair) belongs in this PR. It is scope creep beyond Add per-market TTL preflight #839, included only because nothing compiles without it. It is the first commit and touches nothing else, so it can be cherry-picked out or this PR rebased onto a separate fix. If a repair is already in flight among the open PRs, I will drop it.

  • Keeping both checks vs. consolidating to one. See "Alternative Solutions Considered".

Questions for reviewers:

  • Should Add per-market TTL preflight #839 have been closed as already-implemented? The issue names contracts/predictify-hybrid/src/markets.rs as the file to modify, and that file already contained check_market_creation_rent(env)?; at line 132 before this PR. I have read the issue as asking for completion of the scoped-out aggregate check rather than a reimplementation — please redirect me if that reading is wrong.
  • Is the lib.rs breakage (236 errors) tracked anywhere? I could not find an issue for it, and it blocks any PR that needs to run the test suite.
  • Should the set_balance tier-helper cleanup stay in this PR, or move to its own? It is a separate commit and unrelated to the preflight beyond touching the same file.

Thank you for your contribution to Predictify! 🚀

# Pull Request Description

⚠️ Read first: the base branch does not compile

master @ 43fdfdd does not build. This is pre-existing and unrelated to
this PR
, but it means CI cannot pass and cargo test cannot be run —
including for the tests added here. Details in "Test Results" below.

A reviewer checking out this branch will see the same errors. They are not
from these changes.


📋 Basic Information

Type of Change

  • 🐛 Bug fix (non-breaking change which fixes an issue) — prerequisite DataKey repair
  • ✨ New feature (non-breaking change which adds functionality) — aggregate TTL preflight
  • 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • 📚 Documentation update
  • 🧪 Test addition/update
  • 🔧 Refactoring (no functional changes)
  • ⚡ Performance improvement
  • 🔒 Security fix
  • 🎨 UI/UX improvement
  • 🚀 Deployment/Infrastructure change

Related Issues

Closes #839
Related to #836 (documents the storage tiers this check operates on)

Priority Level

  • 🔴 Critical (blocking other development)
  • 🟡 High (significant impact)
  • 🟢 Medium (moderate impact)
  • 🔵 Low (minor improvement)

📝 Detailed Description

What does this PR do?

#839 asks for a storage-TTL preflight on create_market. A preflight already
exists
: check_market_creation_rent at storage.rs:41, wired into both
creation paths (markets.rs:132, lib.rs:413), with three tests in
market_creation_validation_tests.rs under a
// ── Storage Rent Pre-flight Tests ── header.

Rather than reimplement it, this PR completes the parts that were scoped out.
The code signposts them through two dead symbols:

Symbol Status before this PR
MARKET_CREATION_PERSISTENT_KEYS declared at storage.rs:26, never read, and its value (1) was wrong
Error::InsufficientStorageRentBudget declared at err.rs:239 with display strings at :1555, :1666, :1787never constructed

Two error variants existed for one concept, one unreachable. That reads as an
intended split:

  • InsufficientStorageRent — single-key u32 overflow (was implemented)
  • InsufficientStorageRentBudget — aggregate multi-key budget (was not)

Changes:

  1. MARKET_CREATION_PERSISTENT_KEYS: 1 → 3. The entrypoint writes three
    persistent entries per creation, not one:

    # Write Location
    1 market record lib.rs:418
    2 platform stats, via record_market_createdset_platform_stats statistics.rs:58
    3 audit record, via AuditTrailManager::append_record audit_trail.rs:114

    Each confirmed persistent by reading the function body. The
    MarketCreator::create_market helper writes only entry 1, so the constant is
    documented as an upper bound: a creation that succeeds on the helper path
    cannot then fail partway through the entrypoint path.

  2. New check_market_creation_rent_budget in storage.rs, returning
    InsufficientStorageRentBudget.

  3. Wired into both call sitesmarkets.rs propagates via ?; lib.rs
    panics via panic_with_error! because the entrypoint returns Symbol, not
    Result.

  4. set_balance routed through the tier helper instead of hardcoded TTL
    literals (separate commit, droppable).

  5. Prerequisite: DataKey repaired so the crate compiles at all (separate
    commit, droppable — see Notes for Reviewers).

Why is this change needed?

The existing check has two limits:

  1. It effectively never fires. It trips only when the ledger sequence is
    within effective_ttl (~6.3M ledgers, ~365 days) of u32::MAX — which is
    why the existing test must set sequence_number: u32::MAX - 1_000 to
    trigger it at all.
  2. Key count never enters the formula. One key and three keys produce an
    identical verdict.

The gap: a market record could be stored with a full TTL while its companion
statistics and audit entries could not be given one.

How was this tested?

cargo test could not be run — the base branch does not compile. See "Test
Results". The five tests below are added and are expected to pass once the base
builds.

Manual verification performed instead:

  1. Traced all three persistent writes in the entrypoint to confirm the 3.
  2. greped the whole crate to confirm InsufficientStorageRentBudget had no
    constructor and MARKET_CREATION_PERSISTENT_KEYS had no reader.
  3. Recomputed the tier arithmetic against LEDGERS_PER_DAY = 17_280.
  4. Confirmed brace/paren balance on all three edited files. The -2 paren delta
    in lib.rs is pre-existing (unbalanced parens inside doc-comment prose),
    identical before and after these edits.
  5. Diffed each edited file against master to confirm only the intended hunks
    changed.

Alternative Solutions Considered

Replace check_market_creation_rent instead of adding alongside it. The
aggregate check mathematically subsumes it, making the original redundant at
both call sites. Kept because removing it would break the three existing tests
that assert InsufficientStorageRent specifically. Happy to consolidate to one
call and one error if preferred.

Reorder the fee charge. In markets.rs the preflight still runs after
process_creation_fee. Pre-existing ordering; a rent failure returns Err so
the host rolls back the whole invocation and no funds are lost. Left unchanged
as out of scope.

Change the lib.rs entrypoint to return Result. Would allow propagation
instead of panicking. Not done — breaking signature change for every client.

Reimplement the preflight from scratch per the issue text. Rejected: it
already exists in the exact file #839 names.


🏗️ Smart Contract Specific

Contract Changes

  • Core contract logic modified
  • Oracle integration changes (Pyth/Reflector)
  • New functions added — check_market_creation_rent_budget
  • Existing functions modified — create_market (both paths), set_balance
  • Storage structure changes — DataKey variants (all previously constructed at call sites; no new on-chain key shapes)
  • Events added/modified
  • Error handling improved — InsufficientStorageRentBudget now reachable
  • Gas optimization
  • Access control changes
  • Admin functions modified
  • Fee structure changes

Oracle Integration

Not applicable — no oracle code touched.

Market Resolution Logic

Not applicable — no resolution code touched.

Security Considerations

  • Access control reviewed — no new state-changing entrypoint; both call sites already authenticate upstream
  • Reentrancy protection — not applicable, no external calls added
  • Input validation — no user input; operates on ledger sequence and constants
  • Overflow/underflow protection — checked_mul then checked_add, no wrapping arithmetic, no unwrap()
  • Oracle manipulation protection — not applicable

The change is strictly more conservative: it rejects a superset of what was
rejected before. No input previously accepted-and-fully-written is now
accepted-and-partially-written.


🧪 Testing

Test Coverage

  • Unit tests added/updated — 5 new tests
  • Integration tests added/updated
  • All tests passing locally — cannot run; base does not compile
  • Manual testing completed — see "How was this tested?"
  • Oracle integration tested — not applicable
  • Edge cases covered — u32::MAX, aggregate-overflow boundary, normal sequence
  • Error conditions tested — both error variants asserted
  • Gas usage optimized — negligible impact, not measured
  • Cross-contract interactions tested — not applicable

Tests added to the storage.rs test module under a new
// ── Storage Rent Aggregate Budget Pre-flight Tests ── header:

Test Asserts
test_budget_check_accepts_normal_ledger_sequence realistic sequence → Ok
test_budget_check_rejects_aggregate_overflow aggregate overflow → InsufficientStorageRentBudget
test_budget_check_is_stricter_than_single_key_check at one sequence: single-key Ok, aggregate Err
test_budget_check_rejects_at_u32_max_sequence u32::MAXErr
test_balance_ttl_honours_storage_config_override set_balance respects StorageConfig

The third is the important one: it pins the relationship between the two checks
and fails if the aggregate check is ever weakened to match the single-key one.

Test Results

cargo test
# Cannot run — base branch master @ 43fdfdd does not compile.

Two independent pre-existing breakages:

1. storage.rsDataKey (fixed by commit 1 of this PR)

error[E0428]: the name `AdminOverrideNonce` is defined multiple times
  --> contracts/predictify-hybrid/src/storage.rs:89:5
   |
74 |     AdminOverrideNonce(Address),
   |     --------------------------- previous definition here
...
89 |     AdminOverrideNonce(Address),
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^ `AdminOverrideNonce` redefined here

Plus AntiGriefFloor, GlobalConfig, and PlaceBetsIdem constructed in
disputes.rs, the governance tests, and bets.rs but absent from the enum.

2. lib.rs — 236 errors (NOT addressed in this PR)

Error Cause
Error defined multiple times use err::Error; (L71) and pub use err::Error; (L155)
CircuitBreaker / EventEmitter re-imported L70/L165 and L72/L170
enum import BetStatus is private bets::BetStatus not pub
unresolved events::ClaimInfo lives at crate::types::ClaimInfo
file not found for capability_bitmap_tests mod declared, file absent
statistics, graceful_degradation, market_id_generator, rate_limiter referenced as crate::x::…, never declared mod x; — files exist

Deliberately not touched: repairing these means restructuring imports and module
declarations across a 7,700-line file, which is a different change from
"preflight storage TTL pressure" and would likely conflict with several of the
40 open PRs.

Manual Testing Steps

  1. Read lib.rs create_market end to end and enumerated every
    persistent().set(...) reached during one call, following
    record_market_created and append_record into their own modules to confirm
    each write's storage tier.
  2. grep -rn "InsufficientStorageRentBudget" --include=*.rs across the crate —
    confirmed four occurrences, all declaration/display, zero constructors.
  3. grep -rn "MARKET_CREATION_PERSISTENT_KEYS" --include=*.rs — confirmed a
    single occurrence (its own declaration).
  4. Recomputed effective_ttl * MARKET_CREATION_PERSISTENT_KEYS boundary values
    used in the tests against u32::MAX.
  5. Diffed all three edited files against master; confirmed two hunks in
    markets.rs, two in lib.rs, and the intended edits in storage.rs.

📚 Documentation

Documentation Updates

  • README updated
  • Code comments added/updated — rustdoc on the new function and the corrected constant; inline comments at both call sites explaining why one propagates and one panics
  • API documentation updated — NatSpec-style /// with # Formula and # Errors, matching the existing preflight's style
  • Examples updated
  • Deployment instructions updated
  • Contributing guidelines updated
  • Architecture documentation updated

Breaking Changes

Breaking Changes:

  • None.

Error::InsufficientStorageRentBudget was already a declared variant with a
stable numeric code (523). This PR only makes it reachable, so clients switching
on error codes see a code that was already in the enum. The new DataKey
variants were already being constructed at call sites; the enum simply did not
declare them.

Migration Guide: Not applicable.


🔍 Code Quality

Code Review Checklist

  • Code follows Rust/Soroban best practices
  • Self-review completed
  • No unnecessary code duplication
  • Error handling is appropriate — ok_or(...)? on both fallible steps, no unwrap()
  • Logging/monitoring added where needed — no events appropriate for a preflight
  • Security considerations addressed
  • Performance implications considered
  • Code is readable and well-commented
  • Variable names are descriptive
  • Functions are focused and small

Performance Impact

  • Gas Usage: Two u32 arithmetic operations and one max_ttl() read per
    create_market. Negligible.
  • Storage Impact: None — no additional reads or writes to storage.
  • Computational Complexity: O(1).

Security Review

  • No obvious security vulnerabilities
  • Access controls properly implemented — unchanged; no new entrypoint
  • Input validation in place — operates on ledger state, not user input
  • Oracle data properly validated — not applicable
  • No sensitive data exposed

🚀 Deployment & Integration

Deployment Notes

  • Network: Testnet first — the stricter check should be observed against a
    real ledger sequence before mainnet.
  • Contract Address: N/A
  • Migration Required: No — no storage layout migration; existing entries are
    unaffected.
  • Special Instructions: None.

Integration Points

  • Frontend integration considered — not applicable
  • API changes documented — new error surfaced from create_market; clients using try_create_market may now observe code 523
  • Backward compatibility maintained
  • Third-party integrations updated — not applicable

📊 Impact Assessment

User Impact

  • End Users: None in normal operation. The check only rejects ledger states
    that would have produced a partially-provisioned market.
  • Developers: create_market can now surface
    InsufficientStorageRentBudget (523). Two previously dead symbols are now
    live and meaningful.
  • Admins: Market creation fails cleanly rather than leaving a market whose
    statistics and audit entries lack a full TTL.

Business Impact

  • Revenue: None.
  • User Experience: Marginally improved — a failure mode that would have
    surfaced later as missing audit or statistics data now fails fast at creation.
  • Technical Debt: Net reduction. Removes two dead symbols, corrects a wrong
    constant, and eliminates one hardcoded TTL literal. Does not address the
    larger lib.rs breakage, which remains outstanding.

✅ Final Checklist

Pre-Submission

  • Code follows Rust/Soroban best practices
  • All CI checks passing — cannot pass; base branch does not compile (pre-existing)
  • No breaking changes (or breaking changes are documented)
  • Ready for review
  • PR description is complete and accurate
  • All required sections filled out
  • Test results included — including why the suite cannot run
  • Documentation updated

Review Readiness

  • Self-review completed
  • Code is clean and well-formatted
  • Commit messages are clear and descriptive
  • Branch is up to date with main
  • No merge conflicts

📸 Screenshots (if applicable)

Not applicable.


🔗 Additional Resources


💬 Notes for Reviewers

Please pay special attention to:

  • The value 3 for MARKET_CREATION_PERSISTENT_KEYS. Derived by reading
    the entrypoint's write path: market record, platform statistics, audit record.
    If any helper writes more entries than I traced, or a fourth write exists that
    I missed, the constant is too low and the check is correspondingly too weak.
    This is the single number most worth a second pair of eyes.

  • Whether commit 1 (the DataKey repair) belongs in this PR. It is scope
    creep beyond Add per-market TTL preflight #839, included only because nothing compiles without it. It is
    the first commit and touches nothing else, so it can be cherry-picked out or
    this PR rebased onto a separate fix. If a repair is already in flight among
    the open PRs, I will drop it.

  • Keeping both checks vs. consolidating to one. See "Alternative Solutions
    Considered".

Questions for reviewers:

  • Should Add per-market TTL preflight #839 have been closed as already-implemented? The issue names
    contracts/predictify-hybrid/src/markets.rs as the file to modify, and that
    file already contained check_market_creation_rent(env)?; at line 132 before
    this PR. I have read the issue as asking for completion of the scoped-out
    aggregate check rather than a reimplementation — please redirect me if that
    reading is wrong.
  • Is the lib.rs breakage (236 errors) tracked anywhere? I could not find an
    issue for it, and it blocks any PR that needs to run the test suite.
  • Should the set_balance tier-helper cleanup stay in this PR, or move to its
    own? It is a separate commit and unrelated to the preflight beyond touching
    the same file.

Thank you for your contribution to Predictify! 🚀

@drips-wave

drips-wave Bot commented Jul 24, 2026

Copy link
Copy Markdown

@pheobeayo 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! 🚀

Learn more about application limits

@greatest0fallt1me

Copy link
Copy Markdown
Contributor

Merged into master via admin resolver (-X theirs).

@greatest0fallt1me
greatest0fallt1me merged commit 8da2669 into Predictify-org:master Jul 24, 2026
1 check failed
@grantfox-oss grantfox-oss Bot mentioned this pull request Jul 24, 2026
4 tasks
@greatest0fallt1me

Copy link
Copy Markdown
Contributor

Merged — nice work on this one 🎯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add per-market TTL preflight

2 participants