fix: light-client tree infos v2 helpers#2244
Conversation
📝 WalkthroughWalkthroughThe change introduces V2 feature-gated state tree resolution in the RPC client. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The review demands careful attention to both V1 and V2 code paths across multiple modified functions, particularly verifying that feature-flag branching logic is correct and that localnet/mainnet/devnet fallback behavior remains intact for V1. The single-file scope and consistent feature-gating pattern moderates overall complexity. Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
SwenSchaeferjohann
force-pushed
the
swen/fix-client-trees
branch
from
32ec22d to
de0beec
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk-libs/client/src/rpc/client.rs (1)
1092-1101: 🧹 Nitpick | 🔵 TrivialRemove or feature-gate
get_random_state_tree_info_v1— it's unused dead code that always fails under V2.This function is never called anywhere in the codebase and will always return
NoStateTreesAvailableunder the V2 feature (sinceget_latest_active_state_treespopulates the cache with V2 trees only). Either remove the dead code entirely or add#[cfg(not(feature = "v2"))]to clarify its V1-only intent and prevent it from appearing as a viable option in the public API when V2 is enabled. Currently it presents a false contract — the trait exposes it unconditionally, but callers get a runtime error under V2.
🤖 Fix all issues with AI agents
In `@sdk-libs/client/src/rpc/client.rs`:
- Around line 1012-1018: The V2 code currently writes to self.state_merkle_trees
inside the V2 branch but other V2 paths (get_state_tree_infos and
get_random_state_tree_info) call default_v2_state_trees() directly, making that
write dead; to fix, remove the redundant cache write in the V2 branch (delete
the assignment to self.state_merkle_trees = trees.clone()) so the function
simply returns default_v2_state_trees().to_vec(), keeping
default_v2_state_trees() as the single source of truth; alternatively, if you
prefer caching, update get_state_tree_infos and get_random_state_tree_info to
read from self.state_merkle_trees (ensuring it's initialized here) — pick one
approach and make all V2 code paths consistent with it.
- Around line 1069-1075: The V2 branch duplicates the random-selection logic and
omits the empty-slice guard; replace the inline selection with a call to the
existing helper select_state_tree_info so you reuse its empty-slice check and
error variant (NoStateTreesAvailable). Locate the V2 block that currently calls
default_v2_state_trees() and randomly indexes into the vector, and instead call
select_state_tree_info(&default_v2_state_trees()) (or adapt the helper's
signature if needed) and return its Result so the same guard and error handling
are preserved.
| // V2: the default batched state trees are the same on every network. | ||
| #[cfg(feature = "v2")] | ||
| { | ||
| let trees = default_v2_state_trees().to_vec(); | ||
| self.state_merkle_trees = trees.clone(); | ||
| return Ok(trees); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
V2 cache write is never read — pick one source of truth.
Line 1016 stores the V2 trees in self.state_merkle_trees, but the other V2 code paths (get_state_tree_infos at line 1059, get_random_state_tree_info at line 1073) call default_v2_state_trees() directly, bypassing the cache entirely. This means the field write here is dead code in V2 mode.
I'd recommend one of two approaches:
- Option A (simplest): Remove the cache write in V2 mode — keeps things honest about where the data lives.
- Option B (consistent): Have all V2 paths read from
self.state_merkle_trees, populated once here. This also avoids reconstructing the array on every call.
Option A: remove redundant cache write
#[cfg(feature = "v2")]
{
- let trees = default_v2_state_trees().to_vec();
- self.state_merkle_trees = trees.clone();
- return Ok(trees);
+ return Ok(default_v2_state_trees().to_vec());
}Then get_state_tree_infos and get_random_state_tree_info V2 paths stay as-is.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // V2: the default batched state trees are the same on every network. | |
| #[cfg(feature = "v2")] | |
| { | |
| let trees = default_v2_state_trees().to_vec(); | |
| self.state_merkle_trees = trees.clone(); | |
| return Ok(trees); | |
| } | |
| // V2: the default batched state trees are the same on every network. | |
| #[cfg(feature = "v2")] | |
| { | |
| return Ok(default_v2_state_trees().to_vec()); | |
| } |
🤖 Prompt for AI Agents
In `@sdk-libs/client/src/rpc/client.rs` around lines 1012 - 1018, The V2 code
currently writes to self.state_merkle_trees inside the V2 branch but other V2
paths (get_state_tree_infos and get_random_state_tree_info) call
default_v2_state_trees() directly, making that write dead; to fix, remove the
redundant cache write in the V2 branch (delete the assignment to
self.state_merkle_trees = trees.clone()) so the function simply returns
default_v2_state_trees().to_vec(), keeping default_v2_state_trees() as the
single source of truth; alternatively, if you prefer caching, update
get_state_tree_infos and get_random_state_tree_info to read from
self.state_merkle_trees (ensuring it's initialized here) — pick one approach and
make all V2 code paths consistent with it.
| #[cfg(feature = "v2")] | ||
| let filtered_trees: Vec<TreeInfo> = self | ||
| .state_merkle_trees | ||
| .iter() | ||
| .filter(|tree| tree.tree_type == TreeType::StateV2) | ||
| .copied() | ||
| .collect(); | ||
| { | ||
| use rand::Rng; | ||
| let mut rng = rand::thread_rng(); | ||
| let trees = default_v2_state_trees(); | ||
| Ok(trees[rng.gen_range(0..trees.len())]) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
V2 path duplicates select_state_tree_info — reuse the existing helper.
select_state_tree_info (line 1341) already handles the "pick a random tree from a slice" logic, including the empty-slice guard. The V2 block here re-implements it inline. This is a DRY violation, and you lose the NoStateTreesAvailable guard (currently safe with 5 hardcoded entries, but fragile if the function signature ever changes to accept a dynamic set).
♻️ Proposed fix
#[cfg(feature = "v2")]
{
- use rand::Rng;
- let mut rng = rand::thread_rng();
+ use rand::thread_rng;
let trees = default_v2_state_trees();
- Ok(trees[rng.gen_range(0..trees.len())])
+ select_state_tree_info(&mut thread_rng(), &trees)
}🤖 Prompt for AI Agents
In `@sdk-libs/client/src/rpc/client.rs` around lines 1069 - 1075, The V2 branch
duplicates the random-selection logic and omits the empty-slice guard; replace
the inline selection with a call to the existing helper select_state_tree_info
so you reuse its empty-slice check and error variant (NoStateTreesAvailable).
Locate the V2 block that currently calls default_v2_state_trees() and randomly
indexes into the vector, and instead call
select_state_tree_info(&default_v2_state_trees()) (or adapt the helper's
signature if needed) and return its Result so the same guard and error handling
are preserved.
|
coderabbit is right this pr looks like duplicating existing logic. What's the goal? |
This reverts commit a5d442e.
3 participants
Summary by CodeRabbit
New Features
Refactor