Skip to content

Add tx-granular ledger snapshot source using meta, RPC, and archives#1657

Open
leighmcculloch wants to merge 48 commits into
mainfrom
snapshot-source-tx
Open

Add tx-granular ledger snapshot source using meta, RPC, and archives#1657
leighmcculloch wants to merge 48 commits into
mainfrom
snapshot-source-tx

Conversation

@leighmcculloch

@leighmcculloch leighmcculloch commented Dec 19, 2025

Copy link
Copy Markdown
Member

What

Add new crates for fetching ledger entries from multiple data sources to enable more seamless and built-in fork testing in the sdk. Add soroban-ledger-snapshot-source-tx for transaction-level snapshot sources.

sequenceDiagram
    autonumber
    actor App as 
    participant SelTxMeta as Query<br/>Ledger<br/>Tx Meta
    participant LedgerMeta as Query<br/>Ledger – 1..N<br/>Tx Meta
    participant Archive as History<br/>Archive
    participant RPC as RPC

    App->>SelTxMeta: Lookup with key
    Note over App,SelTxMeta: If found → use it.<br/>If not → continue.
    
    App->>SelTxMeta: Look in prior txs in same ledger
    Note over App,SelTxMeta: If found → use it.<br/>If not → continue.

    opt Optionally use RPC
        App->>RPC: getLedgerEntries([key])
        Note over App,RPC: Use if (lastModified < queryLedger)<br/>AND (rpcLatestSeen >= queryLedger).<br/>Otherwise continue.
    end

    App->>LedgerMeta: Look in prior ledger meta
    Note over App,LedgerMeta: If found → use it.<br/>If checkpoint ledger not reached → continue to next ledger meta<br/>If checkpoint ledger is reached → continue to archive.

    App->>Archive: Download checkpoint from history archive and search
    Note over App,Archive: If found → use it.<br/>If not → does not exist.
Loading

Why

The current fork testing experience utilising the stellar-cli has low granularity only at the boundaries of ledgers, and requires downloading full history archives and manually identifying footprints ahead of time, which is difficult to do well and a poor developer experience. This change enables the SDK to lazily fetch ledger entries on-demand from the most efficient source available, caching results locally for subsequent runs. Developers will be able to fork test against any ledger and transaction without pre-identifying the footprint.

The change uses a Ledger Meta Storage (SEP-54), an RPC, and a History Archive to collect ledger entries.

The change caches results in three layers. All raw files downloaded are cached in the system cache directory and reused across tests, across workspaces. All ledger entries found are cached in the system cache directory. All ledger entries found for the current workspace are cached in the tests-snapshot-source directory intended to be committed so that CI runs reproducibly without needing to collect entries. Note that the format of that cache is not a ledger snapshot json file because for many tests running concurrently one file per ledger entry is easier to manage.

Close #1448

Try it out

Add the following dependencies to your Cargo.toml:

[dev-dependencies]
soroban-sdk = { version = "25.1.0", features = ["testutils"] }
soroban-ledger-snapshot-source-tx = { git = "https://github.com/stellar/rs-soroban-sdk", branch = "snapshot-source-tx" }
bytes-lit = "0.0.6"

Note: Requires soroban-sdk v23.4.0 or later.

Example

use bytes_lit::bytes;
use soroban_ledger_snapshot_source_tx::{Network, TxSnapshotSource};
use soroban_sdk::{token::TokenClient, Address, Env};

#[test]
fn test_fork_at_tx() {
    // The address of the native asset (test XLM) on testnet.
    const NATIVE_ADDRESS: &str = "CDLZFC3SYJYDZT7K67VZ75HPJVIEUVNIXF47ZG2FB2RMQQVU2HHGCYSC";
    
    // Create a snapshot source at ledger 8655, just BEFORE the specified tx.
    let source = TxSnapshotSource::new(
        Network::testnet(),
        8655,
        Some(bytes!(0x580bad1826d02b45634a3742049a23bf652fb2d4bc0814c83f2f58a7a9810ac9)),
    );
    
    // Setup the environment with the source.
    let env = Env::from_ledger_snapshot(source);
    let contract = Address::from_str(&env, NATIVE_ADDRESS);
    let client = TokenClient::new(&env, &contract);

    // Lookup the balance of the following address.
    let addr = Address::from_str(&env, "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM");
    let bal = client.balance(&addr);
    assert_eq!(bal, 47);  // Balance just before the tx executed
}

Observing State Changes

The TxSnapshotSource looks up state with transaction-level granularity when a transaction hash is provided allowing developers to debug a transaction by starting at the point just before the transaction. For example:

Before a specific tx: Pass the tx hash to get state just before that tx executed:

let source = TxSnapshotSource::new(
    Network::testnet(),
    8655,
    Some(bytes!(0x580bad1826d02b45634a3742049a23bf652fb2d4bc0814c83f2f58a7a9810ac9)),
);
// Balance will be 47 (before this tx added 10)

End of ledger (after all txs): Use None for the tx_hash to get state at the end of the ledger:

let source = TxSnapshotSource::new(Network::testnet(), 8655, None);
// Balance will be 57 (after all txs in ledger 8655)

Different ledgers/txs: Update the test to investigate the state of the balance at each of the following ledgers and transactions to see how the transactions affected the balance. Check out the links to see the operations the transactions performed and how they align with the changes in balances observed.

Ledger TX HASH Change Bal
8511 51b21c588f3b397f957d3c14685debf96224c27c5a45b81b27eb5ee0a56517bf + 1 1
8613 52e9a67fdd78105ed84f6d073f50a464e14c1c587a5e4b45842e4b3386336751 + 2 3
8622 a5dd4e98df837da3b1a7de52e9698c1a28ae15973f52651c765f1654a3033993 + 3 6
8632 7a6143351fc48aed1b34d9d0b1690bfe11404cc10cc10f5d52ec06758d0f2b5b + 6 12
8633 d5f676598fd331867f728c4f1c28ea6045c2d31a80d48bfb873d8302816a6304 + 7 19
8633 34e420d6cf03ec2ad1560545f6f98ae281bbb6ac9837d23dbc44d158b2942a65 + 8 27
8655 52ec73f485fb132fb5777c6f2be6c0358e9587c64f4a9ef1f861b56923493400 +11 38
8655 ccede7115f962f3680dc7ff45b21e52e5ea02004f6c2e5b47cf35b853a91203a + 9 47
8655 580bad1826d02b45634a3742049a23bf652fb2d4bc0814c83f2f58a7a9810ac9 +10 57

Debugging with RUST_LOG

Enable logging to see which data sources are being queried and what entries are found:

RUST_LOG=soroban_ledger_fetch=debug cargo test -- --nocapture

Example output:

2025-12-19T14:56:01.070297Z  INFO soroban_ledger_fetch: fetch, key: {"contract_data":{"contract":"CDLZFC3SYJYDZT7K67VZ75HPJVIEUVNIXF47ZG2FB2RMQQVU2HHGCYSC","durability":"persistent","key":"ledger_key_contract_instance"}}
2025-12-19T14:56:01.070932Z DEBUG soroban_ledger_fetch: fetch from meta range, count: 16, first: 8655, last: 8640
2025-12-19T14:56:01.071069Z DEBUG soroban_ledger_fetch: fetch from meta, ledger: 8655
2025-12-19T14:56:01.990395Z DEBUG soroban_ledger_fetch: fetch from rpc, ledger: 8655
2025-12-19T14:56:02.900716Z DEBUG soroban_ledger_fetch: found from rpc, last_modified: 689, usable: true
2025-12-19T14:56:02.919258Z  INFO soroban_ledger_fetch: found, entry: {"data":{"contract_data":{"contract":"CDLZFC3SYJYDZT7K67VZ75HPJVIEUVNIXF47ZG2FB2RMQQVU2HHGCYSC","durability":"persistent","ext":"v0","key":"ledger_key_contract_instance","val":{"contract_instance":{"executable":"stellar_asset","storage":[{"key":{"symbol":"METADATA"},"val":{"map":[{"key":{"symbol":"decimal"},"val":{"u32":7}},{"key":{"symbol":"name"},"val":{"string":"native"}},{"key":{"symbol":"symbol"},"val":{"string":"native"}}]}},{"key":{"vec":[{"symbol":"AssetInfo"}]},"val":{"vec":[{"symbol":"Native"}]}}]}}}},"ext":"v0","last_modified_ledger_seq":689}
2025-12-19T14:56:02.941928Z  INFO soroban_ledger_fetch: fetch, key: {"contract_data":{"contract":"CDLZFC3SYJYDZT7K67VZ75HPJVIEUVNIXF47ZG2FB2RMQQVU2HHGCYSC","durability":"persistent","key":{"vec":[{"symbol":"Balance"},{"address":"CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM"}]}}}
2025-12-19T14:56:02.942165Z DEBUG soroban_ledger_fetch: fetch from meta range, count: 16, first: 8655, last: 8640
2025-12-19T14:56:02.942205Z DEBUG soroban_ledger_fetch: fetch from meta, ledger: 8655
2025-12-19T14:56:02.950580Z  INFO soroban_ledger_fetch: found, entry: {"data":{"contract_data":{"contract":"CDLZFC3SYJYDZT7K67VZ75HPJVIEUVNIXF47ZG2FB2RMQQVU2HHGCYSC","durability":"persistent","ext":"v0","key":{"vec":[{"symbol":"Balance"},{"address":"CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM"}]},"val":{"map":[{"key":{"symbol":"amount"},"val":{"i128":"47"}},{"key":{"symbol":"authorized"},"val":{"bool":true}},{"key":{"symbol":"clawback"},"val":{"bool":false}}]}}},"ext":"v0","last_modified_ledger_seq":8655}

TODO

Thanks

Thanks @orbitlens for sharing the idea of using transaction meta as a way to collect recent state. Thanks to all the people who provided feedback to me about how they use the existing stellar snapshot create functionality.

@socket-security

socket-security Bot commented Dec 19, 2025

Copy link
Copy Markdown

This comment was marked as resolved.

@leighmcculloch leighmcculloch force-pushed the snapshot-source-tx branch 2 times, most recently from 42f37c9 to 598ad4c Compare December 19, 2025 14:52
@leighmcculloch leighmcculloch changed the title Add new ledger snapshot source using meta, RPC, and archives Add tx-granular ledger snapshot source using meta, RPC, and archives Dec 19, 2025
@leighmcculloch leighmcculloch force-pushed the snapshot-source-tx branch 3 times, most recently from 0a8ae20 to 19904da Compare December 19, 2025 15:34
@earrietadev

Copy link
Copy Markdown

is it possible to set it so the timestamp is also set after the snapshot has been loaded? Currently we need to manually set it after the env is created from the snapshot with something like e.ledger().set_timestamp(1766260837);

@willemneal

Copy link
Copy Markdown
Contributor

This seems like an interesting way of handling a simulation locally. Or at least doing a read call locally.

@orbitlens

Copy link
Copy Markdown

Looks awesome. Thanks, Leigh! ❤️

Comment on lines +77 to +87
let ledger_cache_dir = self.cache_path.join(
self.tx_hash
.map(|h| {
let tx_hash_str: String = h.iter().map(|b| format!("{b:02x}")).collect();
format!("{}-{}-before", self.fetcher.ledger(), tx_hash_str)
})
.unwrap_or_else(|| format!("{}-after", self.fetcher.ledger())),
);

// Ensure cache directory exists
std::fs::create_dir_all(&ledger_cache_dir).expect("failed to create cache directory");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This probably should be the cache crate's responsibility. If it is abstracted more than this crate will never need to use a fs. This way we could eventually use Wasm to run this in the browser.

Comment on lines +79 to +111
fn fetch(&self, key: &LedgerKey) -> Option<LedgerEntry> {
// Compute cache file path: <cache_path>/<ledger>/<tx_hash_or_none>/<hash_of_key>.json
let key_xdr = key.to_xdr(Limits::none()).expect("failed to encode key");
let key_hash = Sha256::digest(&key_xdr);
let ledger_cache_dir = self.cache_path.join(
self.tx_hash
.map(|h| {
let tx_hash_str: String = h.iter().map(|b| format!("{b:02x}")).collect();
format!("{}-{}-before", self.fetcher.ledger(), tx_hash_str)
})
.unwrap_or_else(|| format!("{}-after", self.fetcher.ledger())),
);

// Ensure cache directory exists
std::fs::create_dir_all(&ledger_cache_dir).expect("failed to create cache directory");

// Use cache function to handle reading/writing cache file
let fetch_read = cache(
ledger_cache_dir.join(format!("{:x}.json", key_hash)),
|write| -> Result<(), Box<dyn std::error::Error>> {
// Fetch the data from the underlying fetcher
let result = self.fetcher.fetch(key)?;

// Serialize to JSON
serde_json::to_writer_pretty(write, &result)?;

Ok(())
},
)
.expect("failed to cache entry");

// Parse the cached result
serde_json::from_reader(fetch_read).expect("failed to parse cached entry")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

SnapshotSource::get swallows fetcher errors with .expect().

Every IO/serde/network error in the workspace-level cache layer panics: failed to create cache directory, failed to cache entry, failed to parse cached entry. The underlying LedgerEntryFetcher::fetch returns Result<_, Error>, and SnapshotSource::get itself returns Result<_, HostError> — so errors can be propagated. Panicking from inside a host call is harder to diagnose than a returned error, especially when the test runner only shows the panic backtrace and not the underlying network/cache failure.

Recommend converting these into HostErrors (or a typed wrapper), so callers can at least see the original cause in the test failure.

Tied to @willemneal's comment about abstracting fs access — the same restructuring would let the cache surface typed errors instead of forcing the caller into .expect().

Comment on lines +199 to +210
let fetch_read = cache(
ledger_cache_dir.join(format!("{:x}.json", key_hash)),
|write| {
// Fetch the data
let result = self.fetch_with_dl_cache(key, &cache_path)?;

// Serialize to JSON
serde_json::to_writer_pretty(write, &result)?;

Ok(())
},
)?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No schema versioning on the committed cache.

fetch_with_entry_cache serializes Option<LedgerEntry> to JSON via stellar-xdr's serde representation, and the files live under tests-snapshot-source/ which is checked into the repo. If stellar-xdr's JSON representation of LedgerEntry changes (rename, field add/remove, enum variant shift), every cached file silently becomes incompatible with the new code — the deserialization will succeed-but-with-wrong-data in some cases, or panic in others, and there's no fingerprint that says "this cache was produced by xdr version X".

Two low-effort mitigations:

  • Embed the stellar-xdr version and/or a schema-hash in the JSON wrapper (e.g. {"xdr_version": "26.0.1", "entry": ...}) and reject mismatches.
  • Or pin the XDR version as = in Cargo.toml (you have =26.0.1 already at the workspace level — make sure that's load-bearing here and called out).

Worth deciding before users start committing snapshot fixtures into their repos that they then have to regenerate.

Comment on lines +91 to +95
let bucket_0 = &bucket[0..=1];
let bucket_1 = &bucket[2..=3];
let bucket_2 = &bucket[4..=5];
let bucket_url =
format!("{archive_url}/bucket/{bucket_0}/{bucket_1}/{bucket_2}/bucket-{bucket}.xdr.gz");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Slice indexing will panic on a malformed bucket hash.

&bucket[0..=1], [2..=3], [4..=3] panic if bucket.len() < 6 or if the byte is not on a UTF-8 boundary (any non-ASCII would also panic since these use ..= byte-index slicing). In practice the bucket strings come from History::current_buckets which the archive server returns, so a malformed value would only arise from a compromised/corrupted archive response — but defensive validation here (if bucket.len() < 6 { return Err(...) }) is cheap and would turn a confusing panic into a typed error.

Same pattern in get_history on lines 46–48 is safe because format!("{:08x}") guarantees an 8-char ASCII string.

Comment on lines +10 to +12
[dependencies]
soroban-ledger-snapshot-source-tx = "23"
```

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Version mismatch: the README's example dependency uses "23" but Cargo.toml has version.workspace = true26.0.1. The PR body also says "Requires soroban-sdk v23.4.0 or later", which is fine, but the example here should match the crate's actual published major ("26").

Comment on lines +13 to +28
[dependencies]
soroban-sdk = { workspace = true, features = ["testutils"] }
stellar-xdr = { workspace = true, features = ["curr", "std", "base64", "serde"] }
cargo_metadata = "0.19"
directories = "6.0"
sha2 = "0.10"
hex = "0.4"
serde = { version = "1", features = ["derive"] }
serde_json = "1"
thiserror = "2.0"
tracing = "0.1"
tracing-subscriber = { version = "0.3", features = ["env-filter"] }
reqwest = { version = "0.12", default-features = false, features = ["json", "blocking", "rustls-tls"] }
flate2 = { version = "1.0", features = ["rust_backend"] }
zstd = "0.13"
fs2 = "0.4"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Dependency hygiene concerns flagged by Socket Security.

A few items worth resolving before merge:

  1. fs2 = "0.4"fs2 is unmaintained. Successors are fs4 (drop-in API, actively maintained) or file-lock. Given the cache module relies entirely on FileExt::lock_exclusive, this is a low-risk swap.

  2. reqwest = { default-features = false, features = [..., "rustls-tls"] } — pulls in rustls-webpki@0.101.7 (transitively via stellar-rpc-client) which has GHSA-82j2-j2ch-gfr8 (DoS via malformed CRL, High). Bumping stellar-rpc-client (if you depend on it) or pinning a newer rustls-webpki via a workspace dep would clear this.

  3. License-policy alerts: option-ext (MPL-2.0, via directories) and zstd-sys (GPL-2.0+, via zstd) both triggered Socket warnings. Since the deny.toml in this PR explicitly allows MPL-2.0, the option-ext one is intentional. The zstd-sys GPL note may need an explicit allow or a clarify entry in deny.toml — currently I don't see GPL-2.0+ in the allow list, so this may be a build break under cargo deny check licenses. Worth verifying CI on the bans/licenses/sources job.

  4. Loose version specifiers: cargo_metadata = "0.19", directories = "6.0", sha2 = "0.10", etc. are minimum-version specs. The rest of the workspace pins XDR/env crates with =. For a test utility this is fine, but consider whether reproducible fixture generation argues for tighter pins here too (see my comment on fetch_with_entry_cache about schema versioning).

- parse_ledger now takes the ledger sequence so an empty-batch
  LedgerNotFound error reports the real sequence instead of 0.
- Add publish = false to the crate (matches its exclusion from the
  semver-checks and publish-dry-run CI jobs).
- Tighten the unused convenience wrappers (from_meta_storage::ledger,
  from_history_archive::history) from pub to pub(crate).
- Name the detached meta-prefetch thread and log spawn failures so a
  panic inside it is attributable.
- Clarify that LedgerCloseMetaV1.tx_processing is V0-shaped, so mapping
  it to the V0 variant is by type and not a lossy downcast.
@claude

claude Bot commented Jun 2, 2026

Copy link
Copy Markdown

Claude finished @leighmcculloch's task in 2m 47s —— View job


Code Review — incremental on top of prior pass

Re-reviewed at 8e22ed3 ("Address PR review: safe cleanups for snapshot-source-tx"). Below is what's been resolved since my earlier review and what's still outstanding.

✅ Resolved in 8e22ed3 and 6cc6d85

  • parse_ledger now threads the real sequence into LedgerNotFound (no more LedgerNotFound(0) placeholder).
  • publish = false on soroban-ledger-snapshot-source-tx/Cargo.toml:14 — matches the semver-checks/publish-dry-run exclusion.
  • from_meta_storage::ledger and from_history_archive::history tightened from pub to pub(crate).
  • Prefetch thread is now named (snapshot-source-prefetch-meta) and a spawn failure is logged — panics inside the prefetch are now attributable, though the handle is still detached (intentional, per the new comment at fetch/mod.rs:247–250).
  • LedgerCloseMeta::V1.tx_processing aliasing to Self::V0(..) now has a documenting comment explaining it's a type-correct mapping, not a downcast (fetch/iter.rs:339–346).
  • The RPC fetch path now uses response.error_for_status() and rejects non-success bodies before writing to the cache (from_rpc.rs:44) — this is a meaningful improvement over the original "5xx-as-404 cached forever" failure mode I called out previously.
  • deny.toml allows MPL-2.0 for option-ext. Since publish = false + licenses.private.ignore = true are set, the GPL-2.0+ zstd-sys flag from Socket is effectively dispositioned (please confirm with cargo deny check licenses locally to be sure).

🔴 Still outstanding and worth resolving before stabilizing

  1. Hardcoded testnet date in Network::testnet()fetch/mod.rs:81
    meta_url: "https://aws-public-blockchain.s3.us-east-2.amazonaws.com/v1.1/stellar/ledgers/testnet/2025-12-17". This date predates today (2026-06-02) by ~5 months. Any test calling Network::testnet() will 404 on ledgers outside that date's window. At minimum, surface a useful error; ideally remove the suffix or accept it as a parameter.

  2. TransactionMeta::V1 "before vs. after" TODO is unresolvedfetch/iter.rs:429

    TransactionMeta::V1(m) => Some(&m.tx_changes), // TODO: Should this be before or after, or just ignored?

    tx_changes_after returns None for V1, so the iterator currently treats V1.tx_changes as before-state. For Soroban-era ledgers this is moot (Soroban data starts at V2+), but it deserves a documented decision (or a panic for unsupported variants) before stabilizing.

  3. RPC usability check is still incompletefetch/mod.rs:358

    let usable = entry.last_modified_ledger_seq < ledger;

    The PR's own sequence diagram says (lastModified < queryLedger) AND (rpcLatestSeen >= queryLedger). The second clause is missing. If the RPC node is lagging behind self.ledger, a "stale-but-not-actually-stale" entry can be returned as usable and then persisted to the cache — there's no recovery path on a subsequent run. Recommend a getLatestLedger (or getHealth / latestLedger from getLedgerEntries if available) check first, and only treat the entry as usable when the node has caught up.

  4. SnapshotSource::get still has a .expect() cascadelib.rs:81, 93, 108, 111
    failed to encode key, failed to create cache directory, failed to cache entry, failed to parse cached entry. Underlying LedgerEntryFetcher::fetch returns a typed Result<_, Error>, and SnapshotSource::get returns Result<_, HostError> — propagate. Test failures currently surface as a bare panic with no original cause. (Ties in with @willemneal's request to abstract fs out of this crate; the same restructuring naturally turns these into typed errors.)

  5. No HTTP timeouts anywherefrom_meta_storage.rs:51, from_rpc.rs:38, from_history_archive.rs:52, 97
    All four are reqwest::blocking::Client::new() with no timeout. A hung S3/RPC endpoint will block a test indefinitely. Configurable per-Network is ideal; a default 30s is the minimum.

  6. cargo_metadata is in [dependencies], not [dev-dependencies]Cargo.toml:19
    TxSnapshotSource::new shells out via MetadataCommand::new().exec() (lib.rs:58). Since the crate is publish = false and dev-only this is harmless, but [dev-dependencies] is more accurate intent and would prevent surprise if anyone ever flips publish back on.

🟡 Worth doing, lower priority

  1. Logging side-effects can fail the functionfetch/mod.rs:171, 174
    tracing::info!(key = %serde_json::to_value(key)?, ...) and the matching entry = line propagate any serde error out of fetch(). Use tracing::enabled!(Level::INFO) to gate, or .unwrap_or_else(|_| ..). Also: line 174 logs "found" even when the inner Option is Some(None) (definitively absent) — branch the message.

  2. fs2 is unmaintainedCargo.toml:31
    Swap to fs4 (drop-in API, actively maintained). Low-risk, since the use is just FileExt::lock_exclusive.

  3. No schema fingerprint on committed cache fixturesfetch/mod.rs:199–210
    The JSON wrapping Option<LedgerEntry> is stellar-xdr's serde rep. If stellar-xdr's JSON shape ever changes, every fixture under tests-snapshot-source/ silently mis-deserializes. Workspace already pins stellar-xdr = "=26.0.1", but embedding the version in the JSON wrapper would make it self-describing and fail loud on mismatch.

  4. Slice indexing in bucket-hash path constructionfrom_history_archive.rs:91–93
    &bucket[0..=1], [2..=3], [4..=5] panic if bucket.len() < 6 or if the bytes aren't on UTF-8 boundaries. A corrupted/compromised archive history-*.json response is the only realistic trigger, but a length guard turns a confusing panic into a typed error.

  5. README version mismatchREADME.md:12 says "23", the crate is 26.0.1.

  6. Prefetch thread vs. main fetch racefetch/mod.rs:251–263
    The detached prefetch races with the main thread's fetch_from_meta. The file lock in cache() keeps things correct, but the second arrival does redundant download work. Acceptable for now (especially with publish = false), but a OnceLock or "in-flight" set would avoid the doubled S3 GET on cold runs.

  7. Cosmetic: lock files never removedcache.rs:18 creates path.with_extension("lock") and never unlocks/deletes. Harmless (and .gitignore'd under tests-snapshot-source/) but accumulates.

Notes for previously-raised feedback by others

  • @willemneal lib.rs:93: agreed, abstracting fs out of the cache crate would let this target wasm/browser later and would naturally propagate typed errors (see item 4 above).
  • @earrietadev: the "timestamp also set after snapshot load" ergonomic request is already on the PR TODO list — no action from me on that.
  • Copilot typo at fetch/mod.rs:421 ("evited") and the stray / at lib.rs:135–138: both still present, trivial to apply.

Bottom line

The cleanups in 8e22ed3 materially improve diagnostics (named thread, real sequence in errors, documented V1 close-meta aliasing). The three items I'd treat as merge-blockers for users-can-depend-on-this:

  1. Hardcoded testnet date (item 1) — silent failure mode.
  2. RPC usability missing latest-seen clause (item 3) — incorrect result, persisted.
  3. V1 TransactionMeta TODO (item 2) — documented decision needed.

HTTP timeouts (item 5) and the .expect() cascade (item 4) are the next tier — both are operability/diagnosability rather than correctness, but they will be felt the first time someone hits a flaky RPC.

Todos

  • Diff against prior review (6cc6d85/8e22ed3)
  • Re-read lib.rs, cache.rs, fetch/mod.rs, from_meta_storage.rs, from_rpc.rs, from_history_archive.rs, fetch/iter.rs
  • Cross-check supply-chain items against deny.toml
  • Post updated review
    • Branch: snapshot-source-tx

- Network::testnet now takes the SEP-54 meta_url as an argument instead
  of hardcoding a date-pinned testnet epoch (testnet is periodically
  reset and the AWS dataset partitions meta by epoch, so there is no
  stable default). Mirrors how mainnet() takes rpc_url.
- The RPC usability check now also requires the node to have observed
  the target ledger (latestLedger >= ledger), so a lagging node can no
  longer have a potentially-stale entry persisted to the cache. The
  getLedgerEntries response's latestLedger is parsed (defaulting to 0
  when absent, which is conservatively treated as unusable).
@claude

claude Bot commented Jun 2, 2026

Copy link
Copy Markdown

Claude finished @leighmcculloch's task in 2m 42s —— View job


Code Review — incremental on top of 09e412b

Re-reviewed the changes since 8e22ed3. Only one new commit (09e412b) touching fetch/from_rpc.rs and fetch/mod.rs. Below is the status of each item from my previous review, plus one new finding.

✅ Resolved in 09e412b

  1. Hardcoded testnet date (was item populate #1, blocking) — Network::testnet(meta_url: String) now takes the epoch URL as a parameter, with doc-comment explaining why no stable default exists. network_id_hex_is_stable test was updated to pass an example URL and verifies the network_id is independent of meta_url. ✓
  2. RPC usability missing latestLedger clause (was item Add LICENSE #2, blocking) — fetch_from_rpc now computes usable = entry.last_modified_ledger_seq < ledger && latest_ledger >= ledger. GetLedgerEntriesResponse parses latestLedger (defaulting to 0 when absent so an older cached response is conservatively treated as not-usable). New unit tests cover the default-to-zero and present-value paths. Logic is now consistent with the sequence diagram in the PR description. ✓ (with caveat below)

🔴 New finding from this commit

  1. RPC cache poisoning — non-usable responses are still persisted. Posted inline at fetch/mod.rs:371. The new latest_ledger >= ledger check correctly avoids returning stale data, but the response body is committed to the cache by cache() before the check runs. So a once-lagging node permanently disables the RPC fast path for that (ledger, key) pair — the cached file replays the same stale latestLedger forever. Suggested fix: push the usability check into the collector closure so the temp file is never renamed to its final path on a non-usable response (the existing atomic-write logic in cache.rs will clean up correctly).

🔴 Still outstanding from prior reviews

  1. TransactionMeta::V1 "before vs. after" TODOfetch/iter.rs:429 still has the unresolved inline TODO. For Soroban-era ledgers this is moot, but it should be a documented decision (or a panic for unsupported variants) before stabilizing.

  2. SnapshotSource::get / TxSnapshotSource::fetch .expect() cascadelib.rs:81, 93, 108, 111 still panic on cache/serde failures. Underlying LedgerEntryFetcher::fetch returns Result<_, Error> and SnapshotSource::get returns Result<_, HostError> — propagate.

  3. No HTTP timeouts anywherefrom_meta_storage.rs:51, from_rpc.rs:38, from_history_archive.rs:52, 97 all use reqwest::blocking::Client::new() with no timeout. A hung S3/RPC endpoint will block a test indefinitely.

🟡 Lower priority, still open

  1. Logging side-effects can fail the functionfetch/mod.rs:178, 181 still propagate serde_json::to_value(...)? errors out of fetch(); line 181 also logs "found" when the entry is Some(None).
  2. fs2 unmaintainedCargo.toml:31. Swap to fs4 (drop-in API).
  3. No schema fingerprint on committed cache fixturesfetch/mod.rs:199–210 (cache files under tests-snapshot-source/ are pure stellar-xdr JSON; an XDR format change silently mis-deserializes).
  4. Slice indexing in bucket-hash pathfrom_history_archive.rs:91–93 still panics on bucket.len() < 6.
  5. cargo_metadata in [dependencies] not [dev-dependencies]Cargo.toml:19. Since publish = false, harmless but inaccurate.
  6. README version mismatchREADME.md:11 still says "23", crate is 26.0.1.

Bottom line

The two blocking correctness items from the previous review are addressed (well done on the focused commit). The new finding (#3) is a regression in the cache layer caused by the otherwise-correct usability check — recommend addressing it in the same way before users start depending on the RPC fast path. Items #4#6 remain the next tier (documented V1 decision, typed errors, HTTP timeouts).

Todos

  • Diff against prior review baseline (8e22ed3)
  • Re-read lib.rs, cache.rs, fetch/mod.rs, from_rpc.rs, from_meta_storage.rs, from_history_archive.rs, fetch/iter.rs
  • Verify status of items from the prior reviews
  • Identify new findings introduced by 09e412b
  • Post updated review
    • Branch: snapshot-source-tx

// modified at or after `ledger` once the node catches up, so the
// current response could be stale; fall back to meta/archive rather
// than persisting a potentially wrong answer.
let usable = entry.last_modified_ledger_seq < ledger && latest_ledger >= ledger;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

New finding from this commit: the RPC response is cached even when it's not usable.

The latest_ledger >= ledger check correctly addresses the lagging-node correctness issue I raised previously, but the new check happens after cache() has persisted the response body to rpc-{ledger}-{key_hash}.json. The cached file contains the latestLedger value reported at the moment the cache was written.

Consequence: if the first lookup happens against a lagging node, the cached file permanently reports the same stale latestLedger. Every subsequent run reads the same cached body, gets the same latest_ledger, and the usability gate keeps rejecting it — even though the node has long since caught up and a fresh request would now succeed. The user has no signal that the RPC fast path is permanently disabled for this (ledger, key) pair; they'd have to manually delete files under ~/.cache/<bundle-id>/snapshot-source-tx/.

Two possible fixes:

  • Don't commit non-usable responses to the cache. Move the usability check inside the collector closure passed to cache(), and return an error from the collector when the node is lagging. The atomic-write logic in cache.rs already deletes the temp .dl file on collector error and does not rename to the final path. The downside is you lose the ability to surface a tracing::debug! for the not-usable case from the cached path on subsequent runs, but the trade-off seems worth it.
  • Delete the cache file post-hoc. After parsing, if !usable, drop the file. Simpler to bolt on but races with concurrent readers.

The first approach is cleaner and works with the lock semantics already in place. A small follow-up: once the file is in the cache and the node was non-lagging at the time, subsequent runs do still need to revalidate latest_ledger >= self.ledger against the (frozen) cached value — that comparison happens to be correct because latest_ledger only ever grows on the node, but it's worth a comment so the next reader doesn't worry about it.

Test coverage gap: the existing missing_latest_ledger_defaults_to_zero test confirms parsing, but there's no end-to-end test that a non-usable cached response is either (a) not persisted or (b) revalidated on next call. Worth adding once the fix is in.

Revert the publish = false addition; the crate is intended to be
published.
@claude

claude Bot commented Jun 2, 2026

Copy link
Copy Markdown

Claude finished @leighmcculloch's task in 2m 12s —— View job


Code Review — incremental on top of 2a4b06f

Re-reviewed the changes since 09e412b (my prior baseline). The only new commit is 2a4b06f ("Keep soroban-ledger-snapshot-source-tx publishable"), which reverts the publish = false line added in 8e22ed3.

That looks like a one-line change, but it's load-bearing: it moves several previously-deferred findings from "harmless because dev-only / cargo-deny [licenses.private] ignore = true" into "needs to actually be resolved before publish". Below is what the revert changes, plus the items from prior reviews that are still outstanding.

🔄 Implications of 2a4b06f — items the revert re-opens

  1. cargo_metadata as a runtime [dependencies]Cargo.toml:16. Earlier I only flagged this as a [dev-dependencies] placement nit; with publish = false it was harmless. Now that the crate is publishable, every downstream test crate that takes soroban-ledger-snapshot-source-tx as a [dev-dependencies] will transitively pull in cargo_metadata, and TxSnapshotSource::new (lib.rs:58) shells out to cargo via MetadataCommand::new().exec() at runtime. Two concerns:

    • Build/test environments without cargo on PATH (Docker without rustup, CI runners with cached binaries, sandboxes) will hard-panic at expect("failed to get cargo metadata"). Today this only affects tests, but it's a surprising failure mode for a library marketed as a fork-testing helper.
    • Discovery alternatives. CARGO_MANIFEST_DIR is set by cargo at compile time and locates the consumer's manifest. Walking up to the workspace root from there is one option; another is letting the caller pass an explicit cache_dir to TxSnapshotSource::new. Either avoids the runtime shell-out.
  2. License-policy now applies to this crate's dep tree. deny.toml [licenses.private] ignore = true skipped license checks for unpublished workspace crates; with publish = false removed, the soroban-ledger-snapshot-source-tx dep tree is now in scope. Look at the Socket alert for zstd-sys (GPL-2.0+ via zstd@0.13): GPL-2.0+ isn't in deny.toml's allow list. zstd-sys is typically dual-licensed (the README is BSD-2-Clause OR MIT), so the right resolution is probably a [[licenses.clarify]] entry pinning zstd-sys to its dual license — not a blanket GPL allow. Worth running cargo deny check licenses locally with the merged config to confirm CI passes.

  3. fs2 unmaintained — Cargo.toml:28. Was "low-risk swap" before. Now it's a real consideration: fs2 is the only filesystem-locking dep, the public API used (FileExt::lock_exclusive) is identical in fs4, and shipping with an unmaintained dep on a published crate is a smell users will notice. RUSTSEC-2024-… eventually flags this.

  4. README example version mismatch — README.md:11–12. Still says "23", the crate is 26.0.1. This is now user-visible documentation on crates.io.

  5. SnapshotSource::get .expect() cascade — lib.rs:81, 93, 108, 111. Previously raised. For a published crate this is more important: downstream users hit cryptic panics with no underlying cause when the cache dir isn't writable, the workspace root probe fails, or the XDR JSON shape shifts. Propagate as HostError (already the return type).

  6. No HTTP timeouts anywhere — from_meta_storage.rs:51, from_rpc.rs:38, from_history_archive.rs:52, 97. Previously raised. Hung S3/RPC = hung test forever, with no way for the user to recover other than killing the process. For a library that users will run in their own CI, a 30s default with configuration per-Network is the minimum.

  7. Cache schema versioning — fetch/mod.rs:199–210. Previously raised. Cache fixtures are committed by users into their own repos. When this crate is published and downstream users upgrade stellar-xdr/soroban-ledger-snapshot-source-tx, the cached JSON wrappers will silently mis-deserialize. Embedding the stellar-xdr version in the JSON wrapper (or even a single format_version: u32 field) lets you fail loudly with a useful message instead.

🔴 Still outstanding from earlier reviews

  1. RPC cache poisoning — fetch/mod.rs:357–384. From my previous review. The latest_ledger >= ledger clause at line 371 is the right correctness fix, but the response is still written to rpc-{ledger}-{key_hash}.json via cache() at line 357 before the usability check runs. A first lookup against a lagging node permanently disables the RPC fast path for that (ledger, key) pair, since the cached file replays the same stale latestLedger on every subsequent run. Suggested fix: push the usability check into the collector closure passed to cache() and return an error from the collector when the response is non-usable. The atomic temp+rename in cache.rs will then drop the bad response without committing it.

  2. TransactionMeta::V1 before-vs-after TODO — fetch/iter.rs:429. Unresolved. Soroban data only appears in V2+ meta, so this is unlikely to bite in practice, but a documented decision (or an unimplemented!() for V1) is the right thing to do before this is on crates.io.

🟡 Lower priority, still open

  1. Logging side-effects can propagate error from fetch() (fetch/mod.rs:178, 181).
  2. Slice indexing in bucket-hash path construction (from_history_archive.rs:91–93).
  3. Fire-and-forget prefetch threads never joined (fetch/mod.rs:247–250) — note this was acknowledged with a comment in 8e22ed3 as intentional.
  4. Lock files never removed under tests-snapshot-source/ (cache.rs:18).

Bottom line

Reverting publish = false is fine in principle — the crate's API is reasonable and the architecture is well-thought-out — but it converts the four "harmless because dev-only" items above (cargo_metadata shell-out, license-policy, fs2, README version) into things that should actually be resolved before the next publish cut. The two correctness blockers from prior reviews (RPC cache poisoning, V1 meta TODO) remain.

If publishing is imminent, the two-item shortlist I'd address first:

  • The RPC cache poisoning fix (correctness bug that persists across runs).
  • The cargo_metadata-as-runtime-dep decision (worst-case: hard panic in environments without cargo, which is many of the environments fork tests run in).

Todos

  • Diff against prior review baseline (09e412b)
  • Identify changes introduced by 2a4b06f
  • Verify status of prior outstanding items
  • Post updated review
    • Branch: snapshot-source-tx

Network::testnet now takes testnet_start_date (YYYY-MM-DD) and builds
the AWS testnet meta partition URL internally, rather than requiring the
caller to assemble the whole meta_url.
@claude

claude Bot commented Jun 2, 2026

Copy link
Copy Markdown

Claude finished @leighmcculloch's task in 1m 45s —— View job


Code Review — incremental on top of ddf9e2c

Re-reviewed at ddf9e2c ("Take testnet start date instead of full meta_url"). The only change since my previous review is to Network::testnet's signature. Status of each prior item below.

✅ Resolved in ddf9e2c

Testnet API ergonomics. Network::testnet(meta_url: String)Network::testnet(testnet_start_date: String). The AWS S3 base path is now baked into the constructor and only the reset-epoch date (e.g. "2025-12-17") is supplied by the caller. The doc comment correctly notes that no stable default exists. network_id_hex_is_stable updated to verify the network_id is independent of the start-date input, and testnet_defaults verifies the URL is correctly composed. Good change — simpler API, less typo surface on the AWS URL itself, and the Network struct remains public so users who need a mirror or non-AWS source can still construct one literally. ✓

🟡 Small note on the new API

No validation of the date-string format. Network::testnet("2025-13-99"), Network::testnet("december 17"), or trailing slash Network::testnet("2025-12-17/") all silently compose a URL that 404s at fetch time. For a dev-only test helper this is fine, but since the only contract is "YYYY-MM-DD", a one-line assert! or returning a Result<Self, _> would surface typos at construction rather than buried inside a LedgerNotFound later. Strictly optional.

🔴 Still outstanding from prior reviews (unchanged)

  1. RPC cache poisoningfetch/mod.rs:359–385. The latest_ledger >= ledger clause at line 373 correctly addresses lagging-node correctness, but the response is committed to rpc-{ledger}-{key_hash}.json before the usability check runs. A first lookup against a lagging node permanently disables the RPC fast path for that (ledger, key) pair. Recommend pushing the usability check into the collector closure passed to cache() so the temp file is dropped instead of renamed on non-usable.
  2. TransactionMeta::V1 before-vs-after TODOfetch/iter.rs:429. Still unresolved. For Soroban-era ledgers it's moot (Soroban data is V2+), but documenting the decision (or returning None symmetrically, or unimplemented!()) is worth doing before this stabilizes.
  3. SnapshotSource::get / TxSnapshotSource::fetch .expect() cascadelib.rs:81, 93, 108, 111 (plus lib.rs:60, 68 for MetadataCommand). Six .expect() sites turn diagnosable errors into bare panics with no underlying cause. SnapshotSource::get already returns Result<_, HostError> — propagate.
  4. No HTTP timeouts anywherefrom_meta_storage.rs:51, from_rpc.rs:38, from_history_archive.rs:52, 97 all use reqwest::blocking::Client::new() with no timeout. Hung S3/RPC = hung test forever.

🟡 Lower priority, still open

  1. cargo_metadata is in [dependencies] not [dev-dependencies]Cargo.toml:16. Now that publish = false was reverted in 2a4b06f, this means downstream test crates pull in cargo_metadata runtime, and TxSnapshotSource::new will hard-panic in environments without cargo on PATH (Docker images without rustup, etc.).
  2. fs2 unmaintained — Cargo.toml:28. fs4 is the drop-in replacement.
  3. Logging side-effects can propagate serde_json::to_value(...)? errors out of fetch()fetch/mod.rs:178, 181. Line 181 also logs "found" for Some(None).
  4. No schema fingerprint on committed cache fixtures — fetch/mod.rs:199–210. A future stellar-xdr JSON shape change silently mis-deserializes every fixture under tests-snapshot-source/.
  5. Slice indexing in bucket-hash path construction — from_history_archive.rs:91–93. Panics on bucket.len() < 6.
  6. README example version says "23"README.md:11. Crate is 26.0.1.

Bottom line

ddf9e2c is a clean ergonomic improvement. The shortlist of merge-blockers from prior reviews still stands:

The HTTP timeout and cargo_metadata items (#4, #5) come right behind — both are operability concerns that will surface the first time someone uses this in CI with a flaky network or a slim container.

Todos

  • Diff against prior review baseline (2a4b06f)
  • Identify changes introduced by ddf9e2c
  • Re-read affected files (fetch/mod.rs)
  • Cross-check status of prior outstanding items
  • Post updated review
    • Branch: snapshot-source-tx

Resolve the security advisories reported by cargo deny check advisories
with the smallest possible dependency changes:

- Bump stellar-rpc-client 25.1.0 -> 26.0.0 (dev-dep of test_fork). This
  is the only available newer release; 25.x has no patched variant. It
  moves jsonrpsee 0.20 -> 0.26, dropping the vulnerable rustls 0.21 /
  rustls-webpki 0.101.7 stack in favor of rustls 0.23 /
  rustls-webpki 0.103.13. Clears:
    - RUSTSEC-2026-0104 (CRL parsing panic)
    - RUSTSEC-2026-0098 (URI name constraints)
    - RUSTSEC-2026-0099 (wildcard name constraints)
    - RUSTSEC-2025-0134 (rustls-pemfile unmaintained; removed)
  serde_json was bumped to the minimum required by jsonrpsee-core 0.26
  (1.0.128 -> 1.0.142); no duplicate introduced.
- Bump rand 0.8.5 -> 0.8.6 (in-place patch) to clear RUSTSEC-2026-0097
  (ThreadRng unsoundness).
- Remove the stale RUSTSEC-2024-0388 (derivative) ignore from deny.toml;
  cargo-deny reports advisory-not-detected and the crate is no longer in
  the lockfile. RUSTSEC-2024-0436 (paste) is still matched and retained.

cargo deny check advisories is now ok; bans/licenses/sources still ok.
Lockfile churn is confined to the rpc/jsonrpsee/rustls/webpki subtree
(plus the rand patch); no soroban/serde/quote/syn or test-wasm changes.

https://claude.ai/code/session_015kBhoioxYh6bD9o6eb9Ksh
@claude

claude Bot commented Jun 2, 2026

Copy link
Copy Markdown

Claude finished @leighmcculloch's task in 2m 12s —— View job


Code Review — incremental on top of 65ec7ed

Re-reviewed at 65ec7ed ("Fix RustSec advisories via minimal dependency bumps"). This is a pure supply-chain commit — the only files touched are Cargo.lock, deny.toml, and tests/fork/Cargo.toml. The soroban-ledger-snapshot-source-tx crate code itself is unchanged from my previous review baseline.

✅ Resolved in 65ec7ed

  • rustls-webpki@0.101.7 CVE (GHSA-82j2-j2ch-gfr8) flagged earlier by Socket — gone. Verified: grep "^version = \"0.101\"" Cargo.lock returns nothing; rustls-webpki@0.103.13 is now the only version in the tree. The stellar-rpc-client 25 → 26 bump (which transitively swapped jsonrpsee 0.20 → 0.26, dragging rustls 0.21 → 0.23 with it) is what cleared the chain. Even though snapshot-source-tx itself uses raw reqwest rather than stellar-rpc-client, both reach the same modern rustls now.
  • RUSTSEC-2026-0097 (rand ThreadRng unsoundness) — rand 0.8.5 → 0.8.6 patch bump, no API surface change.
  • RUSTSEC-2025-0134 (rustls-pemfile unmaintained) — crate dropped from the lockfile entirely along with the jsonrpsee upgrade.
  • Stale advisory ignore for RUSTSEC-2024-0388 (derivative) removed from deny.toml since the crate is no longer in the tree. The RUSTSEC-2024-0436 (paste) ignore is correctly retained — it's still pulled transitively via ark-ff/wasmi_core.

The commit is appropriately scoped: lockfile churn is confined to the rpc/jsonrpsee/rustls/webpki subtree plus the rand patch — no soroban/serde/syn/quote drift, which is exactly the conservative shape you want for a security-focused bump.

🟡 Still flagged by Socket / cargo deny

  • zstd-sys GPL-2.0+ license alert from Socket — deny.toml doesn't list GPL in the allow array, but the workspace [licenses] config defaults probably let it through, and cargo deny check licenses is reportedly passing per the commit message. Since zstd-sys is dual-licensed (BSD-2-Clause OR MIT in its actual manifest header — only the bundled upstream zstd/COPYING is GPL), a [[licenses.clarify]] entry pinning zstd-sys to its dual license is cleaner than relying on the implicit pass. Not blocking.
  • fs2@0.4.3 is still in the lockfile (Cargo.lock:988). The RUSTSEC unmaintained advisory will appear eventually if it hasn't already. fs4 remains a drop-in.

🔴 Outstanding from prior reviews (code unchanged, all still apply)

In priority order:

  1. RPC cache poisoningfetch/mod.rs:373. The latest_ledger >= ledger usability check happens after cache() has already persisted the response. A first lookup against a lagging node permanently disables the RPC fast path for that (ledger, key) pair on every subsequent run. Move the usability check into the collector closure so non-usable responses never get renamed to their final path. (prior detail)
  2. TransactionMeta::V1 before-vs-after TODOfetch/iter.rs:429. Document the decision (or unimplemented!() for V1) before stabilizing.
  3. SnapshotSource::get / TxSnapshotSource::fetch .expect() cascadelib.rs:81, 93, 108, 111 (plus lib.rs:60, 68 for MetadataCommand). Six panic sites where typed HostError propagation is available.
  4. No HTTP timeouts anywherefrom_meta_storage.rs:51, from_rpc.rs:38, from_history_archive.rs:52, 97. Hung S3/RPC = hung test.

🟡 Lower-priority, still open

  1. cargo_metadata in [dependencies] not [dev-dependencies]Cargo.toml:16. With publish = false reverted, this is now a runtime dep for downstream consumers.
  2. fs2 unmaintained — see above.
  3. Logging side-effects (fetch/mod.rs:178, 181) — serde_json::to_value(...)? propagates serde errors out of fetch().
  4. No schema fingerprint on committed cache fixtures.
  5. Slice indexing in bucket-hash path construction (from_history_archive.rs:91–93).
  6. README example version says "23"README.md:11. Crate is 26.0.1.

Bottom line

65ec7ed is a focused, well-scoped supply-chain fix and meaningfully reduces the audit surface — three RUSTSEC items plus the rustls CVE all cleared in one bump. No new findings introduced. The four correctness/operability items at the top (RPC cache poisoning, V1 meta TODO, .expect() cascade, HTTP timeouts) remain the shortlist before users-can-depend-on-this.

Todos

  • Diff against prior review baseline (ddf9e2c)
  • Identify changes introduced by 65ec7ed
  • Re-check status of prior outstanding items
  • Post updated review
     • Branch: snapshot-source-tx  •

This reverts commit 65ec7ed. The bump of stellar-rpc-client to 26.0.0
(to clear the rustls-webpki advisory) requires rustc 1.93.0, which
exceeds this workspace's MSRV of 1.91.0. That broke the gating
expand-test-wasms / MSRV build jobs (cargo expand of the test_fork crate
failed under 1.91). The rustls-webpki advisory only surfaces in the
non-gating cargo-deny (advisories) check, so restoring MSRV-compatible
dependencies is the correct tradeoff.

https://claude.ai/code/session_015kBhoioxYh6bD9o6eb9Ksh
@claude

claude Bot commented Jun 2, 2026

Copy link
Copy Markdown

Claude finished @leighmcculloch's task in 1m 36s —— View job


Code Review — incremental on top of 3ef37d2

Re-reviewed at 3ef37d2 ("Revert 'Fix RustSec advisories via minimal dependency bumps'"). This is a pure revert of the prior commit:

git diff ddf9e2c..HEAD --stat   # empty — HEAD is bit-identical to ddf9e2c

Only three files changed vs. 65ec7ed: Cargo.lock, deny.toml (one ignore entry restored), tests/fork/Cargo.toml (stellar-rpc-client = "26""25"). The soroban-ledger-snapshot-source-tx crate code is unchanged.

✅ Rationale is sound

The commit message explains the trade-off well: stellar-rpc-client 26.0.0 requires rustc 1.93.0, while the workspace MSRV is 1.91.0. The expand-test-wasms / MSRV-gated jobs broke. The rustls-webpki@0.101.7 advisory (GHSA-82j2-j2ch-gfr8) surfaces only in the non-gating cargo-deny advisories check. Holding the MSRV vs. clearing a non-blocking advisory is the right call for a workspace that downstream users rely on for compatibility.

Worth verifying once: does stellar-rpc-client 26 strictly require MSRV 1.93 due to a direct rust-version, or via a transitive bump that could itself be pinned? If it's transitive (e.g., a newer jsonrpsee/tokio patch), there may be a way to clear the advisory without the MSRV bump. If it's a direct rust-version = "1.93" in stellar-rpc-client itself, then this revert is the only option until the workspace MSRV moves.

Either way, leaving a TODO/issue to track re-applying once the workspace MSRV catches up would be useful — otherwise the advisory will quietly sit in the tree.

🟡 State of supply chain after revert

  • rustls-webpki@0.101.7 is back in the tree (verified in Cargo.lock). cargo deny check advisories will continue to flag this; ensure that job remains non-gating until a fix lands.
  • RUSTSEC-2024-0388 (derivative, unmaintained) ignore is restored in deny.toml — correct, since the crate is now back via ark-ec/ark-ffsoroban-env-host.
  • RUSTSEC-2026-0097 (rand ThreadRng unsoundness) — the patch bump (0.8.5 → 0.8.6) is also reverted. Per the lockfile it's now rand 0.8.5 again. This is a low-impact unsoundness in ThreadRng thread-locals, not exploitable in this workspace's usage (test-only random key generation), but worth noting. The fix is a pure patch-version bump that should not be MSRV-sensitive — consider re-applying just that one independently.
  • rustls-pemfile unmaintained — back in the tree.

If you want to land just the parts of 65ec7ed that aren't MSRV-sensitive, the rand 0.8.5 → 0.8.6 and the removal of the no-longer-needed RUSTSEC-2024-0388 ignore (if it can be reachieved without the stellar-rpc-client bump) would be the two candidates. Everything else is correctly reverted.

🔴 Outstanding from prior reviews (code unchanged, all still apply)

In priority order — same shortlist as the ddf9e2c review:

  1. RPC cache poisoningfetch/mod.rs:373. The latest_ledger >= ledger usability check happens after cache() has persisted the response, so a first lookup against a lagging node permanently disables the RPC fast path for that (ledger, key) pair. Move the usability check into the collector closure passed to cache() so non-usable responses are never renamed to their final path.
  2. TransactionMeta::V1 before-vs-after TODOfetch/iter.rs:429. Document the decision (or unimplemented!() for V1) before stabilizing.
  3. SnapshotSource::get / TxSnapshotSource::fetch .expect() cascadelib.rs:81, 93, 108, 111 (plus lib.rs:60, 68 for MetadataCommand). HostError propagation is available — typed errors will surface causes that are currently bare panics.
  4. No HTTP timeouts anywherefrom_meta_storage.rs:51, from_rpc.rs:38, from_history_archive.rs:52, 97. Hung S3/RPC = hung test.

🟡 Lower-priority, still open

  1. cargo_metadata in [dependencies] not [dev-dependencies]Cargo.toml:16. Now that publish = false is reverted, downstream test crates pull in cargo_metadata transitively, and TxSnapshotSource::new will hard-panic in environments without cargo on PATH.
  2. fs2 unmaintained — Cargo.toml:28. fs4 is a drop-in.
  3. Logging side-effects can propagate serde_json::to_value(...)? errors out of fetch()fetch/mod.rs:178, 181. Line 181 also logs "found" when the entry is Some(None).
  4. No schema fingerprint on committed cache fixtures — fetch/mod.rs:199–210. An xdr JSON shape change silently mis-deserializes every fixture under tests-snapshot-source/.
  5. Slice indexing in bucket-hash path construction — from_history_archive.rs:91–93.
  6. README example version says "23"README.md:11. Crate is 26.0.1.

Bottom line

3ef37d2 is the right call given the MSRV constraint — the security trade-off is well-reasoned and confined to a non-gating advisories check. No new findings introduced. The four correctness/operability items at the top remain the shortlist for users-can-depend-on-this. Suggest opening a tracking issue to revisit the rustls-webpki and rand advisories once the workspace MSRV bumps past 1.91.

Todos

  • Diff against prior review baseline (65ec7ed)
  • Identify changes introduced by 3ef37d2 (Revert)
  • Re-check status of prior outstanding items
  • Post updated review
    • Branch: snapshot-source-tx

The rustls-webpki advisory (RUSTSEC-2026-0104 and related) can only be
cleared by stellar-rpc-client 26.0.0, which requires rustc 1.93.0. Raise
the workspace MSRV 1.91.0 -> 1.93.0 to allow it and re-apply the bumps
that were reverted for MSRV-compatibility:

- stellar-rpc-client 25 -> 26 (dev-dependency of test_fork), pulling the
  patched rustls-webpki 0.103.13 / rustls 0.23 / jsonrpsee 0.26 stack
- rand 0.8.5 -> 0.8.6 (RUSTSEC-2026-0097)
- remove the now-stale RUSTSEC-2024-0388 ignore from deny.toml

Regenerate tests-expanded/ and the wasm-embedding test_snapshots under
the 1.93.0 toolchain, since cargo-expand output and contract codegen
shift with the compiler version. Snapshots from ignored/network tests
(fork) and feature-gated tests are toolchain-independent and unchanged.

cargo deny check advisories -> advisories ok
cargo deny check bans licenses sources -> bans ok, licenses ok, sources ok

https://claude.ai/code/session_015kBhoioxYh6bD9o6eb9Ksh
@claude

claude Bot commented Jun 2, 2026

Copy link
Copy Markdown

Claude finished @leighmcculloch's task in 2m 12s —— View job


Code Review — incremental on top of 53afdac

Re-reviewed at 53afdac ("Raise MSRV to 1.93.0 and re-apply RustSec advisory fixes"). This is the inverse of the previous revert: the workspace MSRV is bumped from 1.91.0 → 1.93.0 (Cargo.toml:22), tests/fork/Cargo.toml re-bumps stellar-rpc-client 25 → 26, and the deny.toml ignore for RUSTSEC-2024-0388 (derivative) is removed. The soroban-ledger-snapshot-source-tx/ crate code is unchanged from my previous review baseline (zero-line diff on that path).

✅ Resolved in 53afdac

Verified against Cargo.lock:

  • rustls-webpki@0.101.7 CVE (GHSA-82j2-j2ch-gfr8, High DoS) — gone. Only rustls-webpki@0.103.13 remains.
  • RUSTSEC-2026-0097 (rand ThreadRng unsoundness) — rand@0.8.6 patched.
  • RUSTSEC-2025-0134 (rustls-pemfile unmaintained) — crate dropped entirely.
  • RUSTSEC-2024-0388 (derivative unmaintained) — derivative is no longer in the tree at all. The ark-ec 0.4 → 0.5 / ark-ff 0.4 → 0.5 bump that came along with the dependency churn replaces derivative with educe. Removing the ignore is the correct call. ✓
  • RUSTSEC-2024-0436 (paste) ignore correctly retained — paste@1.0.15 is still in the tree via the ark/wasmi_core chain, and there's still no upstream fix.

Lockfile churn (644 lines) is confined to the rpc/jsonrpsee/rustls/webpki/ark subtree plus the rand patch, which is the conservative shape you want for a security-focused bump. Tests-expanded snapshot regenerations look like the consequence of the workspace re-resolve, not anything substantive.

🟡 Worth confirming

  • MSRV is now 1.93.0. Released ~early 2026; widely available in modern rustup, but the bump is workspace-wide so any consumer using a 1.91/1.92 toolchain breaks. CONTRIBUTING.md:27 references "MSRV" generically (no version locked there), and individual member Cargo.tomls correctly inherit from workspace. Worth a one-line release-notes call-out so downstream users aren't surprised.
  • zstd-sys GPL-2.0+ Socket alert remains. deny.toml [licenses.private] ignore = true still skips workspace-internal crates, and zstd-sys is dual-licensed in its actual manifest (BSD-2-Clause OR MIT — only the bundled upstream zstd/COPYING is GPL). A [[licenses.clarify]] entry pinning zstd-sys to its dual license would be cleaner than the implicit pass. Not blocking.

🔴 Outstanding from prior reviews (code unchanged, all still apply)

Same shortlist as my prior reviews, in priority order:

  1. RPC cache poisoningsoroban-ledger-snapshot-source-tx/src/fetch/mod.rs:373. The latest_ledger >= ledger usability check happens after cache() persists the response. A first lookup against a lagging node permanently disables the RPC fast path for that (ledger, key) pair on every subsequent run. Move the usability check into the collector closure passed to cache() so non-usable responses never get renamed to their final path.
  2. TransactionMeta::V1 before-vs-after TODOfetch/iter.rs:429. Document the decision (or unimplemented!() for V1) before stabilizing.
  3. SnapshotSource::get / TxSnapshotSource::fetch .expect() cascadelib.rs:81, 93, 108, 111 (plus lib.rs:60, 68 for MetadataCommand). HostError propagation is available — surfaces causes that are currently bare panics.
  4. No HTTP timeouts anywherefrom_meta_storage.rs:51, from_rpc.rs:38, from_history_archive.rs:52, 97 all use reqwest::blocking::Client::new() with no timeout. Hung S3/RPC = hung test.

🟡 Lower-priority, still open

  1. cargo_metadata in [dependencies] not [dev-dependencies]Cargo.toml:16. With the crate now publishable, downstream test crates pull in cargo_metadata transitively, and TxSnapshotSource::new hard-panics in environments without cargo on PATH (slim Docker images, etc.).
  2. fs2@0.4.3 unmaintained — Cargo.toml:28. fs4 is drop-in.
  3. Logging side-effects can propagate serde_json::to_value(...)? errors out of fetch()fetch/mod.rs:178, 181. Line 181 also logs "found" when the entry is Some(None).
  4. No schema fingerprint on committed cache fixtures — fetch/mod.rs:199–210.
  5. Slice indexing in bucket-hash path construction — from_history_archive.rs:91–93 panics on bucket.len() < 6.
  6. README example version says "23"README.md:11. Crate is 26.0.1.

Bottom line

53afdac is exactly the right follow-up to the 3ef37d2 revert: the MSRV bump unblocks the supply-chain fixes cleanly, and the resulting dep-tree improvement (losing derivative outright) is a free bonus. No new findings introduced. The four correctness/operability items at the top remain the shortlist before users-can-depend-on-this.

If publishing is imminent, the smallest-set merge-blocker list is (1) RPC cache poisoning and (2) HTTP timeouts — both will be felt the first time someone runs this in CI against a flaky network.

Todos

  • Diff against prior review baseline (3ef37d2)
  • Identify changes introduced by 53afdac
  • Verify supply-chain state in Cargo.lock
  • Cross-check status of prior outstanding items
  • Post updated review
     • Branch: snapshot-source-tx  •

The test-with-soroban-examples and test-with-openzeppelin-stellar-contracts
workflows installed Rust with a bare `rustup update` and no toolchain
override, so a rust-toolchain.toml pin (in this repo or the external repo)
could take effect. With the MSRV now at 1.93, an older pinned toolchain
makes cargo's MSRV-aware resolver pick an older soroban-sdk for transitive
deps (e.g. soroban-poseidon) than the path-patched local 26.0.1, compiling
two copies of soroban-sdk and failing with duplicate lang item (panic_impl)
errors. Force RUSTUP_TOOLCHAIN=stable so these builds always use the latest
stable Rust regardless of any pin.

https://claude.ai/code/session_015kBhoioxYh6bD9o6eb9Ksh
@claude

claude Bot commented Jun 2, 2026

Copy link
Copy Markdown

Claude finished @leighmcculloch's task in 1m 49s —— View job


Code Review — incremental on top of 3f183c8

Re-reviewed at 3f183c8 ("Always build external example/contract repos with latest stable Rust"). This is a CI-only follow-on for the MSRV bump in 53afdac — the soroban-ledger-snapshot-source-tx crate code is bit-identical to the previous review baseline (git diff 53afdac..HEAD -- soroban-ledger-snapshot-source-tx/ returns nothing).

✅ Resolved/improved by 3f183c8

env: RUSTUP_TOOLCHAIN: stable added at workflow scope in both test-with-openzeppelin-stellar-contracts.yml:22–23 and test-with-soroban-examples.yml:22–23. The commit message clearly explains the cause:

With the MSRV now at 1.93, an older pinned toolchain makes cargo's MSRV-aware resolver pick an older soroban-sdk for transitive deps (e.g. soroban-poseidon) than the path-patched local 26.0.1, compiling two copies of soroban-sdk and failing with duplicate lang item (panic_impl) errors.

This is the right fix for the right reason. RUSTUP_TOOLCHAIN is rustup's documented override, and setting it at workflow scope is the cleanest way to ensure both the explicit rustup update and the implicit cargo invocations agree. The inline comment matches the commit message, so future maintainers won't need to dig through git log. ✓

🟡 Notes / things to watch on this CI change

  1. The privacy-pools exclusion at test-with-soroban-examples.yml:62 is still needed — and the comment above it correctly explains why this fix doesn't help with that case. Worth double-checking: the new env var addresses MSRV-resolver picks, but not major-version mismatches. The comment at lines 53–60 already captures this; no action needed, just flagging that the two issues are distinct.

  2. RUSTUP_TOOLCHAIN=stable is dynamic — by design, the toolchain version will float upward as stable advances. That's the intended trade-off (always testing against current stable), and it's consistent with how the rest of the repo's CI seems to operate, but downstream breakage from a future rustc release will surface here rather than in MSRV jobs. Acceptable.

  3. rustup update interactionrustup update honors RUSTUP_TOOLCHAIN to determine what to update if the variable is set to a non-installed channel. stable is always present on ubuntu-latest runners, so this is a no-op in practice. ✓

  4. Other workflows in this repo that hit external code or transitive deps may eventually want the same treatment, but everything else in .github/workflows/ builds this repo, where the workspace rust-version = "1.93.0" does exactly the right thing. No spillover needed today.

  5. GitHub permissions reminder: I can't modify files in .github/workflows/ from this review job, so this is feedback-only.

🔴 Outstanding from prior reviews (crate code unchanged, all still apply)

In priority order, same shortlist as my prior review:

  1. RPC cache poisoningsoroban-ledger-snapshot-source-tx/src/fetch/mod.rs:373. The latest_ledger >= ledger usability check happens after cache() persists the response, so a first lookup against a lagging node permanently disables the RPC fast path for that (ledger, key) pair on every subsequent run. Move the usability check into the collector closure passed to cache() so non-usable responses are never renamed to their final path.
  2. TransactionMeta::V1 before-vs-after TODOfetch/iter.rs:429. Document the decision (or unimplemented!() for V1) before stabilizing.
  3. SnapshotSource::get / TxSnapshotSource::fetch .expect() cascadelib.rs:81, 93, 108, 111 (plus lib.rs:60, 68 for MetadataCommand). HostError propagation is available — surfaces causes that are currently bare panics.
  4. No HTTP timeouts anywherefrom_meta_storage.rs:51, from_rpc.rs:38, from_history_archive.rs:52, 97. Hung S3/RPC = hung test.

🟡 Lower-priority, still open

  1. cargo_metadata in [dependencies] not [dev-dependencies]Cargo.toml:16. With the crate now publishable, downstream test crates pull in cargo_metadata transitively, and TxSnapshotSource::new hard-panics in environments without cargo on PATH.
  2. fs2@0.4.3 unmaintained — Cargo.toml:28. fs4 is drop-in.
  3. Logging side-effects can propagate serde_json::to_value(...)? errors out of fetch()fetch/mod.rs:178, 181. Line 181 also logs "found" when the entry is Some(None).
  4. No schema fingerprint on committed cache fixtures — fetch/mod.rs:199–210.
  5. Slice indexing in bucket-hash path construction — from_history_archive.rs:91–93 panics on bucket.len() < 6.
  6. README example version says "23"README.md:11. Crate is 26.0.1.

Bottom line

3f183c8 is a focused CI fix that addresses a real cross-repo build failure with a clear, durable mechanism. No new findings in the crate code, which is unchanged from the prior review. The four correctness/operability items at the top of the outstanding list remain the shortlist before this crate is ready for users-can-depend-on-this.

Todos

  • Diff against prior review baseline (53afdac)
  • Identify changes introduced by 3f183c8
  • Re-check status of prior outstanding items
  • Post updated review
    • Branch: snapshot-source-tx

The MSRV 1.91 -> 1.93 bump (to clear the rustls-webpki advisory via
stellar-rpc-client 26) broke ~70 downstream test-crate jobs in the
soroban-examples and openzeppelin-stellar-contracts workflows: with the
local soroban-sdk at rust-version 1.93, cargo's MSRV-aware resolver no
longer unifies the path-patched local soroban-sdk 26.0.1 with the copy
that transitive crates.io deps (e.g. soroban-poseidon) pull in, so two
soroban-sdk crates compile and the wasm build fails with duplicate lang
item (panic_impl) errors. Forcing RUSTUP_TOOLCHAIN=stable did not help,
confirming the cause is the MSRV declaration itself, not the build
toolchain version.

Revert to MSRV 1.91, restoring the all-green dependency state. The
rustls-webpki advisory remains covered by the existing ignore in the
non-gating cargo-deny advisories check.

This reverts commits 53afdac and 3f183c8.

https://claude.ai/code/session_015kBhoioxYh6bD9o6eb9Ksh
@claude

claude Bot commented Jun 2, 2026

Copy link
Copy Markdown

Claude finished @leighmcculloch's task in 1m 55s —— View job


Code Review — incremental on top of 1e9407b

Re-reviewed at 1e9407b ("Revert MSRV 1.93 bump and external-repo toolchain override"). This reverts both 53afdac and 3f183c8, restoring the workspace to a state bit-identical to ddf9e2c:

$ git diff ddf9e2c..HEAD --stat
(empty)

The soroban-ledger-snapshot-source-tx/ crate code is unchanged from the ddf9e2c review baseline.

✅ Revert rationale is sound

The commit message captures a subtle but important constraint I missed in the 3f183c8 review:

with the local soroban-sdk at rust-version 1.93, cargo's MSRV-aware resolver no longer unifies the path-patched local soroban-sdk 26.0.1 with the copy that transitive crates.io deps (e.g. soroban-poseidon) pull in, so two soroban-sdk crates compile and the wasm build fails with duplicate lang item (panic_impl) errors. Forcing RUSTUP_TOOLCHAIN=stable did not help, confirming the cause is the MSRV declaration itself, not the build toolchain version.

This is the right diagnosis. The MSRV-aware resolver reads rust-version from the workspace metadata, not from the actual toolchain — so the RUSTUP_TOOLCHAIN=stable workaround in 3f183c8 was treating a symptom of the wrong cause. Reverting the declaration itself is the only fix until the workspace's downstream consumers (soroban-poseidon and friends) also publish 1.93-compatible releases that the resolver can unify against the local path-patched copy.

Verified the supply-chain state matches ddf9e2c (lockfile churn reverted):

  • rustls-webpki@0.101.7 back in the tree alongside 0.103.13 (GHSA-82j2-j2ch-gfr8, High DoS) — surfaces in the non-gating cargo-deny advisories check.
  • rand@0.8.5 (RUSTSEC-2026-0097 ThreadRng unsoundness) back.
  • rustls-pemfile (RUSTSEC-2025-0134 unmaintained) back.
  • derivative (RUSTSEC-2024-0388) back; ignore correctly restored in deny.toml.
  • paste ignore (RUSTSEC-2024-0436) unchanged — still correct.

The trade-off is well-reasoned: a non-gating advisory vs. ~70 broken downstream test-crate jobs is the right call.

🟡 Suggestion: track the deferred advisories

Two of the reverted items are independent of the MSRV bump and could potentially be re-applied:

  1. rand 0.8.5 → 0.8.6 — pure patch bump, not MSRV-sensitive. Worth a standalone PR.
  2. derivative removal — this came from the ark-ec 0.4 → 0.5 bump, which transitively required the newer rustc. Likely blocked on the same MSRV constraint, but worth confirming.

The remaining items (rustls-webpki, rustls-pemfile) are gated behind the stellar-rpc-client 25 → 26 bump which carries the MSRV requirement. Opening a tracking issue ("re-apply RustSec fixes once workspace MSRV catches up") would prevent these from quietly persisting.

🔴 Outstanding from prior reviews (crate code unchanged, all still apply)

Same shortlist as my prior ddf9e2c review, in priority order:

  1. RPC cache poisoningsoroban-ledger-snapshot-source-tx/src/fetch/mod.rs:373. Verified still present: the response is committed to rpc-{ledger}-{key_hash}.json at line 359–365 via cache(), then the usable = entry.last_modified_ledger_seq < ledger && latest_ledger >= ledger check happens at line 373. A first lookup against a lagging node permanently disables the RPC fast path for that (ledger, key) pair on every subsequent run. Move the usability check into the collector closure passed to cache() so non-usable responses are never renamed to their final path.
  2. TransactionMeta::V1 before-vs-after TODOfetch/iter.rs:429. Document the decision (or unimplemented!() for V1) before stabilizing.
  3. SnapshotSource::get / TxSnapshotSource::fetch .expect() cascadelib.rs:81, 93, 108, 111 (plus lib.rs:60, 68 for MetadataCommand). HostError propagation is available — surfaces causes that are currently bare panics.
  4. No HTTP timeouts anywherefrom_meta_storage.rs:51, from_rpc.rs:38, from_history_archive.rs:52, 97. Hung S3/RPC = hung test.

🟡 Lower-priority, still open

  1. cargo_metadata in [dependencies] not [dev-dependencies]Cargo.toml:16. With the crate publishable, downstream test crates pull in cargo_metadata transitively, and TxSnapshotSource::new hard-panics in environments without cargo on PATH.
  2. fs2@0.4.3 unmaintained — Cargo.toml:28. fs4 is drop-in.
  3. Logging side-effects can propagate serde_json::to_value(...)? errors out of fetch()fetch/mod.rs:178, 181. Line 181 also logs "found" when the entry is Some(None).
  4. No schema fingerprint on committed cache fixtures — fetch/mod.rs:199–210.
  5. Slice indexing in bucket-hash path construction — from_history_archive.rs:91–93 panics on bucket.len() < 6.
  6. README example version says "23"README.md:11. Crate is 26.0.1.

Bottom line

1e9407b is the right diagnosis-driven revert: setting rust-version = "1.93" in this workspace's metadata is what was causing the resolver split for downstream test-crate consumers, not the runtime toolchain. No new findings introduced in the crate code (still bit-identical to ddf9e2c). The four correctness/operability items at the top of the outstanding list remain the shortlist before this crate is ready for users-can-depend-on-this.

Todos

  • Diff against prior review baseline (3f183c8)
  • Identify changes introduced by 1e9407b
  • Verify supply-chain state in Cargo.lock
  • Cross-check status of prior outstanding items
  • Post updated review
    • Branch: snapshot-source-tx

Resolve Cargo.lock conflict by regenerating the lockfile, retaining
bytes-lit 0.0.5 (tests) and 0.0.6 (soroban-sdk) as required after main's
bytes-literals work. Regenerate tests-expanded/ with the MSRV toolchain
(cargo-expand 1.0.116, cargo-hack 0.5.28) to stay consistent with the
expand-test-wasms CI check.
@claude

claude Bot commented Jun 3, 2026

Copy link
Copy Markdown

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

Base the lockfile on origin/main's resolution and let cargo add only the
snapshot-source-tx feature dependencies on top, instead of regenerating
from scratch. The full regeneration had floated transitive deps (toml
0.9.5->0.9.12 -> indexmap ->2.14.0 -> hashbrown 0.17.1, thiserror 1.0.63->1.0.69,
trybuild 1.0.115->1.0.116) past the versions deny.toml's multiple-versions
skip list pins, tripping 'cargo deny check bans' on a duplicate hashbrown.
Restores hashbrown 0.14.5 / thiserror 1.0.63 / trybuild 1.0.115 while keeping
bytes-lit 0.0.6 for soroban-sdk and 0.0.5 for the tests.
@claude

claude Bot commented Jun 3, 2026

Copy link
Copy Markdown

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

The embedded WASM in test_spec_shaking_v2_tests.rs was generated against
the pre-fix lockfile; regenerate it with the minimized lockfile so the
expand-test-wasms CI check (git diff HEAD --exit-code) passes.
@claude

claude Bot commented Jun 3, 2026

Copy link
Copy Markdown

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

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.

Improve the in SDK fork testing experience

5 participants