You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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):
letmut entry_count = 0u32;whileletSome(result) = entries.next().await{let entry = match result {Ok(e) => e,Err(e) => { ...;continue}};
entry_count += 1;// counted before the filters belowif 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
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.
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 Reuse one SubxtChainClient struct that implements all chain client traits in provider-node #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.
Follow-up from a review discussion on #159:
#159 (comment)
The original observation
In
provider-node/src/subxt_client.rs, an on-chain storage iteration used a separateentry_countcounter that was incremented at the top of the loop, before severalcontinuebranches (malformed key length, account mismatch, parse failure):entry_countwas meant as "total entries scanned" (distinct frombucket_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/ theAgreementChainClientimpl) 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_agreementsin the same file: it iterates the entireStorageAgreementsmap 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
scanned_entries) and a comment, or drop it and log onlybucket_ids.len()/agreements.len(). Avoid incrementing a counter mid-loop where its meaning relative to thecontinuebranches is ambiguous.fetch_replica_agreements(and the value decoders nearby) silently breaks if the on-chainStorageAgreementskey/layout changes. Prefer decoding keys/values through subxt's typed/metadata-aware path (see the related discussion on robust value decoding from Reuse one SubxtChainClient struct that implements all chain client traits in provider-node #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.