paritytech / paritytech/web3-storage

Clarify entry-count logging and harden manual key-byte parsing in subxt storage iteration

Open
#174 0 comments 0 reactions 1 assignee View on GitHub

@franciscoaguirre is already working on this.

Since Jun 15, 2026.

enhancement good first issue
Dominant language
Rust
Stars
12
Forks
3
Avg merge
2d 2h
Merged PRs (30d)
33

Description

Follow-up from a review discussion on #159:
https://github.com/paritytech/web3-storage/pull/159#discussion_r3394267967

The original observation

In provider-node/src/subxt_client.rs, an on-chain storage iteration used a separate entry_count counter that was incremented at the top of the loop, before several continue branches (malformed key length, account mismatch, parse failure):

let mut entry_count = 0u32;
while let Some(result) = entries.next().await {
    let entry = match result { Ok(e) => e, Err(e) => { ...; continue } };
    entry_count += 1;                       // counted before the filters below
    if key_len < 104 { ...; continue }
    if account_bytes != our_bytes { continue }
    let bucket_id = match key_bytes[48..56].try_into() { ...; continue };
    bucket_ids.push(bucket_id);
}
if entry_count > 0 {
    tracing::info!("Scanned {} agreement request entries, {} for us", entry_count, bucket_ids.len());
}

hmm, why not use just bucket_ids.len() here?

entry_count is confusing, it is incremented at the beginning between various continue;, so it is not obvious what entry_count means — success, correct, all, partial?

entry_count was meant as "total entries scanned" (distinct from bucket_ids.len() = "entries matching us"), but the name and mid-loop placement make that intent unclear.

Current status

The specific function this was on (fetch_pending_requests / the AgreementChainClient impl) was removed when agreement negotiation moved off-chain (#105), so the exact snippet no longer exists on the branch.

However, the same pattern persists in fetch_replica_agreements in the same file: it iterates the entire StorageAgreements map and parses keys via hardcoded byte offsets (key_bytes.len() < 32 + 16 + 8 + 16 + 32, bucket_id_start = 32 + 16, etc.). So this is worth carrying forward as cleanup rather than closing as moot.

Suggested work
  1. Logging clarity: where a "total scanned vs. matched" log is useful, give the counter an explicit name (e.g. scanned_entries) and a comment, or drop it and log only bucket_ids.len() / agreements.len(). Avoid incrementing a counter mid-loop where its meaning relative to the continue branches is ambiguous.
  2. Harden / replace manual key-byte parsing: the hardcoded offset arithmetic in fetch_replica_agreements (and the value decoders nearby) silently breaks if the on-chain StorageAgreements key/layout changes. Prefer decoding keys/values through subxt's typed/metadata-aware path (see the related discussion on robust value decoding from #159) instead of slicing raw bytes.

Low priority / code-quality. The first item is a good-first-issue-sized change; the second is the more substantial follow-up.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.