Skip to content

contracts: get_commitments() has no pagination and will fail once a pool grows enough #57

Description

@tech-adrian

Area
contracts/pool/src/lib.rs (get_commitments, get_commitment)

Problem
The withdraw flow's core trust story is "rebuild the Merkle tree from get_commitments(), not event scanning" (see README Security Model). But get_commitments() (lib.rs:521-539) loops 0..next_index doing one persistent-storage read per leaf inside a single contract invocation, with no pagination and no bound other than MAX_LEAVES = 2^20 (TREE_DEPTH = 20). Soroban enforces hard per-transaction CPU-instruction and ledger-read-footprint limits well before a pool reaches anywhere near 2^20 leaves. Once a pool accumulates "enough" deposits, get_commitments() will start failing for every caller — and since it's the only documented way to trustlessly rebuild the tree, every subsequent withdrawal proof in that pool becomes unbuildable client-side. get_commitment(index) exists but requires the client to already loop index-by-index off-chain (one RPC round trip each), which doesn't fix the on-chain scaling problem and adds its own RPC cost.

Proposed change
Add a paginated view, e.g. get_commitments_page(env: Env, start: u32, limit: u32) -> Vec<BytesN<32>>, with a sane max limit enforced on-chain. Update the frontend's tree-reconstruction helper (withdraw + compliance pages) to page through results instead of calling the unbounded get_commitments(). Keep get_commitments() for now if useful for small/local pools, but document its ledger-size ceiling clearly.

Acceptance Criteria

  • New paginated view added and unit-tested (including a page landing exactly on next_index, an out-of-range start, and a limit above the enforced max)
  • Frontend tree-reconstruction (withdraw + compliance) updated to page through commitments
  • README Security Model note updated to describe the paginated approach and its ceiling
  • Relevant tests (just test, pnpm test, cargo test, or nargo test as applicable) pass locally and in CI
  • A screenshot or short screen recording is attached to the PR showing the change working (a multi-page reconstruction round-tripping to the same root as get_commitments() on a small pool) — required before this can be merged.

Open your PR against the dev branch, not main. All active development merges into dev.

Discuss this issue / coordinate work: join the DShield contributor Telegram group: https://t.me/+SiGHH24No9U2MDJk

Metadata

Metadata

Assignees

Labels

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions