fix(creditline): chunk borrower loan index into fixed-size persistenpages and audit extend_ttl - #92
Conversation
… pages and audit extend_ttl
EmeditWeb
left a comment
There was a problem hiding this comment.
⚠️ 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 🤖
…on tests, and active debt TTL coverage
Thanks for the detailed audit feedback! I have pushed commit
|
EmeditWeb
left a comment
There was a problem hiding this comment.
⚠️ 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 🤖
|
Please rebase/merge the base branch into your branch and resolve the conflicts — a fresh audit will run automatically once new commits land. |
EmeditWeb
left a comment
There was a problem hiding this comment.
⚠️ 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.ymltrigger change from this security fix PR - Provide diffs demonstrating
extend_ttlwas actually added towrite_loan,increase_user_active_debt, anddecrease_user_active_debtbody (not just doc comment) — or confirm these functions already hadextend_ttlbefore 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:
Audited by stepfi-audit-bot 🤖
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 immediateextend_ttlcall.Changes Made
DataKey::UserLoanAt(Address, u64)key format increditline-contract/src/storage.rswithDataKey::UserLoanPage(Address, u32)usingPAGE_SIZE = 32.append_user_loan_indexto append loan IDs to the current borrower page (count / 32) and write to persistent storage.get_user_loan_ids_paginatedto dynamically fetch across page boundaries given zero-indexedstartandlimit.write_loan,increase_user_active_debt,decrease_user_active_debt,append_user_loan_index) ensuring every.set()call is immediately followed byextend_ttl.PAGE_SIZE = 32pagination storage contract instorage.rsand in theget_user_loansdoc comment inlib.rs.test_200_loan_borrower_stress_and_storage_layout_regressiontocontracts/creditline-contract/src/tests.rsasserting 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.context/progress-tracker.mdto document completion.Verification
Ran
cargo test --offlineacross the full workspace:creditline-contract: 125 passed, 0 failedliquidity-pool-contract: 86 passed, 0 failedparameters-contract: 20 passed, 0 failedreputation-contract: 51 passed, 0 failedvendor-registry-contract: 21 passed, 0 failedvouching-contract: 27 passed, 0 failedContext Files Updated
context/progress-tracker.md