Skip to content

docs: storage tier matrix - #903

Merged
greatest0fallt1me merged 1 commit into
Predictify-org:masterfrom
pheobeayo:doce/storage-tier-docs-matrix
Jul 24, 2026
Merged

docs: storage tier matrix#903
greatest0fallt1me merged 1 commit into
Predictify-org:masterfrom
pheobeayo:doce/storage-tier-docs-matrix

Conversation

@pheobeayo

Copy link
Copy Markdown
Contributor

Pull Request Description

📋 Basic Information

Type of Change

  • 🐛 Bug fix (non-breaking change which fixes an issue)
  • ✨ New feature (non-breaking change which adds functionality)
  • 💥 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 #836
Related to #734 (prior storage-tier audit — see "Notes for Reviewers")

Priority Level

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

📝 Detailed Description

What does this PR do?

Adds docs/STORAGE_TIER.md, a reference matrix documenting the storage tier
and TTL policy for every data key in contracts/predictify-hybrid.

The document separates two concepts that are easy to conflate:

  • Durability tier — the Soroban storage class (instance / persistent /
    temporary) chosen at each call site.
  • TTL tier — for persistent keys only, the rent-extension budget modelled
    by the private StorageTtlTier enum and resolved through
    StorageOptimizer::persistent_ttl_for_tier.

It covers: the four TTL tier constants with their ledger counts and durations,
the StorageConfig fields that override them, the max_ttl() clamping
behaviour, a per-key matrix for both DataKey variants and the non-DataKey
composite keys, guidance for choosing a tier for new keys, and a "Known
deviations" section recording places where the code diverges from the
documented policy.

This is a documentation-only change. No Rust source files are added or
modified.

Why is this change needed?

Tier assignment is currently made at roughly a dozen individual call sites
rather than by a central DataKey -> tier function. There is no single place a
contributor can look to answer "which tier does this key use, and for how
long?" — the answer has to be reassembled by grepping
persistent_ttl_for_tier call sites. That makes it easy to add a new key with
an inappropriate tier, or to change a TTL constant without realising which
keys it affects.

How was this tested?

No automated tests accompany this PR, and none can meaningfully be added — the
change introduces a single markdown file with no executable surface.

Verification performed instead:

  1. Every tier constant, ledger count, and duration in the document was read
    directly from storage.rs rather than inferred.
  2. Ledger arithmetic was recomputed independently against
    LEDGERS_PER_DAY = 17_280 and matches the declared constants:
    Balance 535,680 (~31 d), Market 6,307,200 (~365 d), Event 1,555,200 (~90 d),
    Archive 6,307,200 (~365 d), PlaceBetsIdem 120,960 (~7 d).
  3. Each persistent_ttl_for_tier / set_persistent_with_ttl call site was
    traced to the key it writes, so the matrix maps keys rather than line
    numbers.

cargo test cannot currently be run on this branch. See "Test Results"
below — this is a pre-existing condition on main, not a regression
introduced here.

Alternative Solutions Considered

A larger change was considered and rejected for this PR: making
StorageTtlTier public, adding pub fn tier_for(key: &DataKey) -> StorageTtlTier, routing BalanceStorage::update_balance through it, and
adding a test asserting every DataKey variant maps to a tier. That would make
the matrix enforceable rather than advisory and would satisfy the "tests added"
criterion.

It was rejected here because issue #836 scopes the work to a single new
documentation file and is labelled documentation. It also depends on first
fixing the DataKey compile error (deviation 1 below), which is a separate
concern. Recommended as a follow-up — happy to open it if maintainers agree.


🏗️ Smart Contract Specific

Contract Changes

No contract code is modified by this PR. None of the following apply:

  • Core contract logic modified
  • Oracle integration changes (Pyth/Reflector)
  • New functions added
  • Existing functions modified
  • Storage structure changes
  • Events added/modified
  • Error handling improved
  • 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

Not applicable to a documentation-only change. No executable surface is added,
so there is no access control, reentrancy, input validation, or overflow
exposure introduced.


🧪 Testing

Test Coverage

No tests added — see "How was this tested?" for why, and "Notes for Reviewers"
for the follow-up that would make tests possible.

  • Unit tests added/updated
  • Integration tests added/updated
  • All tests passing locally
  • Manual testing completed (constants and arithmetic verified against source)
  • Oracle integration tested
  • Edge cases covered
  • Error conditions tested
  • Gas usage optimized
  • Cross-contract interactions tested

Test Results

cargo test does not currently run on main. DataKey in
contracts/predictify-hybrid/src/storage.rs declares the same variant twice:

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

Additionally, AntiGriefFloor, GlobalConfig, and PlaceBetsIdem are
constructed in disputes.rs, the governance tests, and bets.rs respectively,
but are absent from the DataKey enum.

This is a pre-existing condition on main and is unrelated to this PR — the
branch inherits it. The 95% coverage requirement in #836 is not satisfiable by
any PR until it is resolved. Flagging it here because it likely blocks other
in-flight work.

Manual Testing Steps

  1. Read the tier constants, StorageConfig fields, and clamping helpers
    directly from storage.rs and cross-checked each against the document.
  2. Recomputed all ledger-count arithmetic independently and confirmed it
    matches the declared constants.
  3. Traced every persistent_ttl_for_tier and set_persistent_with_ttl call
    site to the key it writes, and confirmed each row of the matrix.
  4. Rendered the markdown to confirm table formatting.

📚 Documentation

Documentation Updates

  • README updated
  • Code comments added/updated
  • API documentation updated
  • Examples updated
  • Deployment instructions updated
  • Contributing guidelines updated
  • Architecture documentation updated (new docs/STORAGE_TIER.md)

Breaking Changes

None. No public API, storage layout, or contract behaviour is changed.

Migration Guide: Not applicable.


🔍 Code Quality

Code Review Checklist

  • Code follows Rust/Soroban best practices (no code added; documented
    guidance directs new keys through the existing tier helpers)
  • Self-review completed
  • No unnecessary code duplication
  • Error handling is appropriate (not applicable)
  • Logging/monitoring added where needed (not applicable)
  • Security considerations addressed (none applicable; stated above)
  • Performance implications considered (none)
  • Code is readable and well-commented
  • Variable names are descriptive (not applicable)
  • Functions are focused and small (not applicable)

Performance Impact

  • Gas Usage: No change — no contract code modified.
  • Storage Impact: No change — no on-chain storage written.
  • Computational Complexity: No change.

Security Review

  • No obvious security vulnerabilities
  • Access controls properly implemented (not applicable)
  • Input validation in place (not applicable)
  • Oracle data properly validated (not applicable)
  • No sensitive data exposed

🚀 Deployment & Integration

Deployment Notes

  • Network: Not applicable — no deployment required.
  • Contract Address: Not applicable.
  • Migration Required: No.
  • Special Instructions: None.

Integration Points

  • Frontend integration considered (not applicable)
  • API changes documented (no API changes)
  • Backward compatibility maintained (no behavioural change)
  • Third-party integrations updated (not applicable)

📊 Impact Assessment

User Impact

  • End Users: None — no behavioural change.
  • Developers: Single reference for tier and TTL policy; removes the need to
    grep call sites to determine a key's retention. Records four known code
    deviations that were previously undocumented.
  • Admins: Clarifies which StorageConfig fields affect which keys, and
    that configured TTLs are upper bounds clamped by max_ttl().

Business Impact

  • Revenue: None.
  • User Experience: None directly.
  • Technical Debt: Net reduction in undocumented behaviour, though the
    document records rather than resolves the deviations it identifies. Because
    no test enforces the matrix, it can drift from the code — the follow-up in
    "Alternative Solutions Considered" would close that gap.

✅ Final Checklist

Pre-Submission

  • Code follows Rust/Soroban best practices
  • All CI checks passing — CI cannot pass while main does not compile
    (see "Test Results"); unrelated to this change
  • 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 "Known deviations" section of the new document. It records four
    issues found while writing the matrix. These are documented, not fixed, since
    this PR is documentation-only:

    1. DataKey does not compile (duplicate AdminOverrideNonce; three variants
      used elsewhere but never declared).
    2. BalanceStorage::update_balance calls extend_ttl(&key, 535680, 535680)
      with hardcoded literals instead of persistent_ttl_for_tier(Balance). The
      literal happens to equal BALANCE_TTL_LEDGERS, so behaviour matches
      today — but the write ignores any StorageConfig override and skips the
      max_ttl() clamp.
    3. No central DataKey -> tier mapping exists, so the matrix can drift
      silently.
    4. storage_tier_audit.rs exists but is not declared in lib.rs.
  • Overlap with Add storage-tier classifier audit #734. storage_tier_audit.rs claims to answer that issue
    and exports get_storage_tier_audit, but as unregistered dead code it never
    compiles and its #[cfg(test)] tests never execute. Its table also names
    eight keys that are not DataKey variants (Admin, PlatformFee,
    GovernanceMinBps, CumDisputeFee, DisputeMultiSig, OracleConfidence,
    AdminEmergency, Market) while omitting most that are. Where it disagrees
    with this document, this document reflects the code.

  • Inferred rows in the matrix. Tiers for AntiGriefFloor, GlobalConfig,
    PlaceBetsIdem, and AdminOverrideNonce are inferred from call sites, since
    no valid enum declaration currently exists to read. A few Market-tier rows
    (Whitelisted, Blacklisted) are inferred from the surrounding write
    pattern rather than a direct persistent_ttl_for_tier call. These are the
    rows most worth a maintainer's eye.

Questions for reviewers:

  • Should this supersede Add storage-tier classifier audit #734, or should storage_tier_audit.rs be registered
    in lib.rs and reconciled against this matrix? Two disagreeing tier tables
    in the repo seems worse than either alone.
  • Is the DataKey compile break known and already being addressed in one of
    the open PRs? If not, I'm happy to open a separate PR fixing the duplicate
    variant and adding the three missing ones.
  • Would you like the follow-up adding tier_for(&DataKey) plus an
    exhaustive-mapping test, which would make this matrix enforceable rather than
    advisory?

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
greatest0fallt1me merged commit c667e1c into Predictify-org:master Jul 24, 2026
1 check failed
@greatest0fallt1me

Copy link
Copy Markdown
Contributor

LGTM ✅ appreciate the detailed PR description!

@grantfox-oss grantfox-oss Bot mentioned this pull request Jul 24, 2026
4 tasks
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 storage-tier documentation matrix

2 participants