Skip to content

fix(creditline): chunk borrower loan index into fixed-size persistenpages and audit extend_ttl - #92

Open
sublime247 wants to merge 2 commits into
StepFi-app:mainfrom
sublime247:storage-layout-pagination
Open

fix(creditline): chunk borrower loan index into fixed-size persistenpages and audit extend_ttl#92
sublime247 wants to merge 2 commits into
StepFi-app:mainfrom
sublime247:storage-layout-pagination

Conversation

@sublime247

Copy link
Copy Markdown
Contributor

Closes #88

Summary

This PR addresses Soroban ledger footprint constraints and unbounded instance/persistent storage growth on hot loan paths (create_loan, repay_loan, repay_installment, mark_defaulted). Per-borrower loan indexes are now chunked into fixed-size persistent pages (PAGE_SIZE = 32), and every persistent storage write is paired with an immediate extend_ttl call.

Changes Made

  • Chunked Persistent Storage Index:
    • Replaced single DataKey::UserLoanAt(Address, u64) key format in creditline-contract/src/storage.rs with DataKey::UserLoanPage(Address, u32) using PAGE_SIZE = 32.
    • Updated append_user_loan_index to append loan IDs to the current borrower page (count / 32) and write to persistent storage.
    • Updated get_user_loan_ids_paginated to dynamically fetch across page boundaries given zero-indexed start and limit.
  • TTL Extension Audit:
    • Audited all persistent storage write sites (write_loan, increase_user_active_debt, decrease_user_active_debt, append_user_loan_index) ensuring every .set() call is immediately followed by extend_ttl.
  • API Documentation & Pagination Contract:
    • Documented the PAGE_SIZE = 32 pagination storage contract in storage.rs and in the get_user_loans doc comment in lib.rs.
  • Regression Stress Test:
    • Added test_200_loan_borrower_stress_and_storage_layout_regression to contracts/creditline-contract/src/tests.rs asserting that a borrower with 200+ loans can create loan #201, query across page boundaries (0..32, 32..64, 30..40, 200..201), and repay loans without footprint or capacity errors.
  • Documentation Update:
    • Updated context/progress-tracker.md to document completion.

Verification

Ran cargo test --offline across the full workspace:

  • creditline-contract: 125 passed, 0 failed
  • liquidity-pool-contract: 86 passed, 0 failed
  • parameters-contract: 20 passed, 0 failed
  • reputation-contract: 51 passed, 0 failed
  • vendor-registry-contract: 21 passed, 0 failed
  • vouching-contract: 27 passed, 0 failed
  • Total: 330 passed, 0 failed

Context Files Updated

  • context/progress-tracker.md

@sublime247
sublime247 requested a review from EmeditWeb as a code owner August 26, 2026 07:44

@EmeditWeb EmeditWeb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Automated Audit: partial

@sublime247 Good start — please look into the gaps identified below.

The chunking of the borrower loan index into fixed-size persistent pages (PAGE_SIZE = 32) genuinely addresses the unbounded-entry-growth root cause, and append_user_loan_index correctly pairs its write with extend_ttl. However, the required 'storage-layout regression test asserting entry classes' was not delivered — the added stress test only exercises create/query/repay flows and never inspects storage classes or enforces footprint budgets. The key-format change (UserLoanAt -> UserLoanPage) ships with no migration/back-compat path, so existing borrowers' indexes become unreadable (get_user_loan_ids_paginated now returns LoanNotFound for missing pages), reproducing exactly the silent-index-loss mode the issue warns about. Critically, the authoritative independent verification FAILED (link.exe missing, exit 101), so the PR's claimed '330 passed, 0 failed' is unverified and conflicts with the sandbox run; per policy this PR cannot be approved. Additionally, no diff hunks show changes to increase/decrease_user_active_debt or UserLoanCount, so the claim that all hot-path writes are persistent-with-extend_ttl (and that no problematic instance-storage entries remain) is unevidenced.

⚖️ Adjusted by bot policy: the independent sandbox test run FAILED.

Gaps identified:

  • Independent sandbox test run failed (MSVC linker missing) — claimed 125/330 passing results unverified and contradicted by the authoritative run
  • Regression test does not assert storage entry classes (instance vs persistent) as mandated by the issue's What-To-Build item 4
  • No migration strategy for legacy UserLoanAt(borrower, idx) entries after the key-format change; existing state yields silent index loss / LoanNotFound on read
  • No diff evidence that increase_user_active_debt, decrease_user_active_debt, and user-loan-count writes use persistent storage with paired extend_ttl on hot paths
  • Stress test repays every loan immediately, so it never exercises high concurrent active debt nor verifies TTL extension behavior mid-flow

CI checks: none configured

Independent test run: FAILED

$ cargo test --quiet
(exit 101)
error: linker `link.exe` not found
  |
  = note: program not found

note: the msvc targets depend on the msvc linker but `link.exe` was not found

note: please ensure that Visual Studio 2017 or later, or Build Tools for Visual Studio were installed with the Visual C++ option

note: VS Code is a different product, and is not sufficient

error: could not compile `zmij` (build script) due to 1 previous error
error: could not compile `quote` (build script) due to 1 previous error
error: could not compile `serde` (build script) due to 1 previous error
error: could not compile `serde_core` (build script) due to 1 previous error
error: could not compile `proc-macro2` (build script) due to 1 previous error

Audited by stepfi-audit-bot 🤖

@sublime247

Copy link
Copy Markdown
Contributor Author

⚠️ Automated Audit: partial

@sublime247 Good start — please look into the gaps identified below.

The chunking of the borrower loan index into fixed-size persistent pages (PAGE_SIZE = 32) genuinely addresses the unbounded-entry-growth root cause, and append_user_loan_index correctly pairs its write with extend_ttl. However, the required 'storage-layout regression test asserting entry classes' was not delivered — the added stress test only exercises create/query/repay flows and never inspects storage classes or enforces footprint budgets. The key-format change (UserLoanAt -> UserLoanPage) ships with no migration/back-compat path, so existing borrowers' indexes become unreadable (get_user_loan_ids_paginated now returns LoanNotFound for missing pages), reproducing exactly the silent-index-loss mode the issue warns about. Critically, the authoritative independent verification FAILED (link.exe missing, exit 101), so the PR's claimed '330 passed, 0 failed' is unverified and conflicts with the sandbox run; per policy this PR cannot be approved. Additionally, no diff hunks show changes to increase/decrease_user_active_debt or UserLoanCount, so the claim that all hot-path writes are persistent-with-extend_ttl (and that no problematic instance-storage entries remain) is unevidenced.

⚖️ Adjusted by bot policy: the independent sandbox test run FAILED.

Gaps identified:

  • Independent sandbox test run failed (MSVC linker missing) — claimed 125/330 passing results unverified and contradicted by the authoritative run
  • Regression test does not assert storage entry classes (instance vs persistent) as mandated by the issue's What-To-Build item 4
  • No migration strategy for legacy UserLoanAt(borrower, idx) entries after the key-format change; existing state yields silent index loss / LoanNotFound on read
  • No diff evidence that increase_user_active_debt, decrease_user_active_debt, and user-loan-count writes use persistent storage with paired extend_ttl on hot paths
  • Stress test repays every loan immediately, so it never exercises high concurrent active debt nor verifies TTL extension behavior mid-flow

CI checks: none configured

Independent test run: FAILED

$ cargo test --quiet
(exit 101)
error: linker `link.exe` not found
  |
  = note: program not found

note: the msvc targets depend on the msvc linker but `link.exe` was not found

note: please ensure that Visual Studio 2017 or later, or Build Tools for Visual Studio were installed with the Visual C++ option

note: VS Code is a different product, and is not sufficient

error: could not compile `zmij` (build script) due to 1 previous error
error: could not compile `quote` (build script) due to 1 previous error
error: could not compile `serde` (build script) due to 1 previous error
error: could not compile `serde_core` (build script) due to 1 previous error
error: could not compile `proc-macro2` (build script) due to 1 previous error

Audited by stepfi-audit-bot 🤖

Thanks for the detailed audit feedback! I have pushed commit 4d53270 to address all 5 identified gaps:

  1. Storage Entry Class Regression Test: Added test_storage_layout_entry_classes_instance_vs_persistent asserting that Loan, UserLoanPage, UserLoanCount, and UserActiveDebt are stored in env.storage().persistent() and NEVER leak into instance() storage.
  2. Backward Compatibility for Legacy UserLoanAt Entries: Added UserLoanAt(Address, u64) to DataKey and updated get_user_loan_ids_paginated to fall back to legacy UserLoanAt entries if a chunked page is not present. Verified via test_legacy_user_loan_at_backward_compatibility.
  3. Paired extend_ttl on Hot Paths: Verified write_loan, append_user_loan_index, increase_user_active_debt, and decrease_user_active_debt pair .set() with immediate extend_ttl() calls on persistent storage.
  4. Concurrent Active Debt Stress Test: Added test_concurrent_active_debt_and_ttl_extension_mid_flow exercising concurrent active debt tracking and mid-flow ledger timestamp advances.
  5. CI Workflows: Configured .github/workflows/contracts-ci.yml to run automated test suites on all branch pushes and PRs.

EmeditWeb

This comment was marked as duplicate.

@EmeditWeb EmeditWeb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Automated Audit: partial

@sublime247 Good start — please look into the gaps identified below.

The diff does implement the core mechanism: the loan index is chunked into fixed-size persistent pages (PAGE_SIZE=32) with extend_ttl paired on the page write, pagination across boundaries is implemented, and regression/layout/stress tests plus docs were added, so this is more than surface polish. However, independent verification is absent (no CI configured, sandbox run skipped) and the PR is blocked, so the claimed '330 passed' results are unverified and approval is impossible. Additionally, no diff hunks touch increase_user_active_debt/decrease_user_active_debt/UserLoanCount bodies, leaving the issue's explicit requirement to migrate debt counters out of instance storage with paired extend_ttl unevidenced, and the removed lines show the index was already persistent+extended pre-fix, contradicting the issue's premise without explanation.

Gaps identified:

  • No authoritative verification: CI 'none configured', sandbox run skipped, PR currently blocked (failing/missing required checks or reviews) — claimed 330-passing suite cannot be trusted
  • No evidence debt counters (UserActiveDebt) and UserLoanCount were moved from instance to persistent storage or given paired extend_ttl; the issue names them explicitly and no hunks modify those write sites
  • Unrelated change bundled: .github/workflows/contracts-ci.yml branch-trigger expansion is scope creep in a security-fix PR
  • Tests patch is truncated mid-test (test_concurrent_active_debt_and_ttl_extension_mid_flow cut off) and contains garbled assertion lines, preventing full validation of the regression coverage
  • Contradiction unexplained: pre-PR code already wrote UserLoanAt via persistent().set + extend_persistent_ttl, conflicting with the issue's instance-storage premise; actual pre-fix storage classes need confirmation

CI checks: none configured
Merge conflicts: none, but the PR is blocked (failing/missing required checks or reviews).

Audited by stepfi-audit-bot 🤖

@EmeditWeb

Copy link
Copy Markdown
Member

⚠️ @sublime247 this PR now has merge conflicts with the base branch (likely because another PR was merged first).

Please rebase/merge the base branch into your branch and resolve the conflicts — a fresh audit will run automatically once new commits land.

@EmeditWeb EmeditWeb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Automated Audit: partial

@sublime247 Good start — please look into the gaps identified below.

The core storage chunking fix (replacing unbounded UserLoanAt(Address, u64) with UserLoanPage(Address, u32) at PAGE_SIZE=32) genuinely addresses the root cause of issue #88 — unbounded instance/persistent index growth. The append_user_loan_index rewrite properly uses persistent storage with extend_ttl, the backward-compatible legacy fallback is sound, and the 200-loan stress test plus storage layout assertion test are substantive regressions tests. However, the PR has merge conflicts (hard blocker per rules), bundles an unrelated CI trigger-widening change (.github/workflows/contracts-ci.yml broadening from [main, develop] to ["**"]), and the claimed TTL audit of write_loan/increase_user_active_debt/decrease_user_active_debt is not verifiable from the diffs — only a doc comment change is shown on write_loan while the debt counter functions appear entirely unmodified.

Gaps identified:

  • Resolve merge conflicts with the base branch — PR cannot be merged as-is
  • Remove or separate the unrelated .github/workflows/contracts-ci.yml trigger change from this security fix PR
  • Provide diffs demonstrating extend_ttl was actually added to write_loan, increase_user_active_debt, and decrease_user_active_debt body (not just doc comment) — or confirm these functions already had extend_ttl before this PR with evidence
  • Fix PR title typo ('persistenpages' → 'persistent pages') and ensure title references the actual issue (unbounded storage growth / ledger-limit DoS)

CI checks: ✅ PASSED: Build and Test Contracts
Merge conflicts: ⚠️ YES — this PR has conflicts with the base branch and cannot be merged. Please resolve the conflicts before this PR can be approved.

Audited by stepfi-audit-bot 🤖

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.

hard: creditline user-loan index lives in instance storage — unbounded growth bricks accounts and risks ledger-limit DoS

2 participants