feat(l1): default omitted block to latest on eth state methods#6780
feat(l1): default omitted block to latest on eth state methods#6780MysticRyuujin wants to merge 3 commits into
Conversation
…orageValues The state methods (eth_getBalance, eth_getCode, eth_getStorageAt, eth_getTransactionCount, eth_getProof) required an exact param count, so omitting the block returned -32000 'Expected N params'. Make the block parameter optional and default to latest (per execution-apis), via a new Default for BlockIdentifierOrHash (latest). Also implement eth_getStorageValues (previously -32601 method not found): batch storage-slot reads for multiple accounts at a block, mirroring eth_getStorageAt, with the same optional-block default and a 1024-slot cap.
…s parse Parse-layer tests proving eth_getBalance/getCode/getStorageAt/ getTransactionCount/getProof resolve an omitted block to latest, and that eth_getStorageValues parses a request (and rejects an empty one).
MysticRyuujin
changed the title
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR aligns several state-related RPC methods with execution-apis by making the block parameter optional (defaulting to latest) and adds a new eth_getStorageValues RPC for batch storage reads.
Changes:
- Add
DefaultforBlockIdentifierOrHashto represent the omitted-block default (latest). - Update state RPC request parsing to accept an omitted block parameter and default it to
latest. - Introduce
eth_getStorageValuesrequest/handler with slot-count limiting and tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/networking/rpc/types/block_identifier.rs | Adds Default for BlockIdentifierOrHash to support omitted block params defaulting to latest. |
| crates/networking/rpc/rpc.rs | Wires new eth_getStorageValues method into the request router and imports. |
| crates/networking/rpc/eth/account.rs | Makes block param optional for state methods; adds GetStorageValuesRequest implementation and tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let raw: HashMap<String, Vec<U256>> = serde_json::from_value(params[0].clone())?; | ||
| let mut requests = HashMap::with_capacity(raw.len()); | ||
| let mut total_slots = 0usize; | ||
| for (address, slots) in raw { | ||
| total_slots += slots.len(); | ||
| if total_slots > MAX_STORAGE_VALUES_SLOTS { | ||
| return Err(RpcErr::BadParams(format!( | ||
| "too many slots (max {MAX_STORAGE_VALUES_SLOTS})" | ||
| ))); | ||
| } | ||
| let address: Address = serde_json::from_value(Value::String(address))?; | ||
| let slots = slots.iter().map(H256::from_uint).collect(); | ||
| requests.insert(address, slots); | ||
| } |
| let Some(block_number) = self.block.resolve_block_number(&context.storage).await? else { | ||
| return Err(RpcErr::Internal( | ||
| "Could not resolve block number".to_owned(), | ||
| )); | ||
| }; |
Greptile SummaryThis PR aligns ethrex with execution-apis#812 by making the block parameter optional (defaulting to
Confidence Score: 4/5The core optional-block-default logic is correct and well-tested; the new eth_getStorageValues handler works end-to-end but has two surface-area rough edges around slot format parity and response address key casing. The Default chain and all five amended parse methods are straightforward and consistent. The two open questions — decimal slot values and checksummed address key matching — are usability concerns that don't break existing callers but may trip up new consumers of eth_getStorageValues. crates/networking/rpc/eth/account.rs — specifically the GetStorageValuesRequest parse and handle implementations
|
| Filename | Overview |
|---|---|
| crates/networking/rpc/eth/account.rs | Makes block parameter optional (defaulting to latest) for 5 state methods and adds new eth_getStorageValues handler; slot parsing and response address key format may diverge from eth_getStorageAt conventions |
| crates/networking/rpc/types/block_identifier.rs | Adds Default impl for BlockIdentifierOrHash (delegates to BlockIdentifier::default() → BlockTag::Latest); correct and safe |
| crates/networking/rpc/rpc.rs | Registers eth_getStorageValues in the eth dispatch table; minimal one-line change, straightforward |
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
crates/networking/rpc/eth/account.rs:300
**Slot deserialization may reject decimal values accepted by `eth_getStorageAt`**
`eth_getStorageAt` uses `serde_utils::u256::deser_hex_or_dec_str` for the storage slot, which accepts both `"0x1"` (hex) and `"1"` (decimal). Here `slots` are deserialized via `serde_json::from_value::<Vec<U256>>`, which relies on `U256`'s built-in serde. If a client sends decimal-formatted slot indices (e.g. `{"0xaddr": ["1", "2"]}`), this call will fail while the equivalent `eth_getStorageAt(addr, "1", ...)` would succeed — a silent behavioural asymmetry between the two methods.
### Issue 2 of 2
crates/networking/rpc/eth/account.rs:348
**Response address keys use lowercase hex — callers sending checksummed or mixed-case addresses can't do a direct key lookup**
`format!("{address:#x}")` renders the address as lowercase `0x`-prefixed hex. A client that sends `{"0xDeAdBeEfDeAdBeEfDeAdBeEfDeAdBeEfDeAdBeEf": ["0x0"]}` receives a response keyed on `"0xdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef"`. Any code that naively looks up its original request-address string against the response map will miss the entry. Returning the address in EIP-55 checksummed form (or normalising to lowercase at parse time and documenting it) would make the correlation unambiguous.
Reviews (1): Last reviewed commit: "test: state methods default omitted bloc..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return Err(RpcErr::BadParams("Expected 1 or 2 params".to_owned())); | ||
| }; | ||
| // params[0] is an object mapping address -> array of storage slots. | ||
| let raw: HashMap<String, Vec<U256>> = serde_json::from_value(params[0].clone())?; |
There was a problem hiding this comment.
eth_getStorageAt
eth_getStorageAt uses serde_utils::u256::deser_hex_or_dec_str for the storage slot, which accepts both "0x1" (hex) and "1" (decimal). Here slots are deserialized via serde_json::from_value::<Vec<U256>>, which relies on U256's built-in serde. If a client sends decimal-formatted slot indices (e.g. {"0xaddr": ["1", "2"]}), this call will fail while the equivalent eth_getStorageAt(addr, "1", ...) would succeed — a silent behavioural asymmetry between the two methods.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/networking/rpc/eth/account.rs
Line: 300
Comment:
**Slot deserialization may reject decimal values accepted by `eth_getStorageAt`**
`eth_getStorageAt` uses `serde_utils::u256::deser_hex_or_dec_str` for the storage slot, which accepts both `"0x1"` (hex) and `"1"` (decimal). Here `slots` are deserialized via `serde_json::from_value::<Vec<U256>>`, which relies on `U256`'s built-in serde. If a client sends decimal-formatted slot indices (e.g. `{"0xaddr": ["1", "2"]}`), this call will fail while the equivalent `eth_getStorageAt(addr, "1", ...)` would succeed — a silent behavioural asymmetry between the two methods.
How can I resolve this? If you propose a fix, please make it concise.| .unwrap_or_default(); | ||
| values.push(format!("{:#x}", H256::from_uint(&value))); | ||
| } | ||
| result.insert(format!("{address:#x}"), values); |
There was a problem hiding this comment.
format!("{address:#x}") renders the address as lowercase 0x-prefixed hex. A client that sends {"0xDeAdBeEfDeAdBeEfDeAdBeEfDeAdBeEfDeAdBeEf": ["0x0"]} receives a response keyed on "0xdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef". Any code that naively looks up its original request-address string against the response map will miss the entry. Returning the address in EIP-55 checksummed form (or normalising to lowercase at parse time and documenting it) would make the correlation unambiguous.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/networking/rpc/eth/account.rs
Line: 348
Comment:
**Response address keys use lowercase hex — callers sending checksummed or mixed-case addresses can't do a direct key lookup**
`format!("{address:#x}")` renders the address as lowercase `0x`-prefixed hex. A client that sends `{"0xDeAdBeEfDeAdBeEfDeAdBeEfDeAdBeEfDeAdBeEf": ["0x0"]}` receives a response keyed on `"0xdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef"`. Any code that naively looks up its original request-address string against the response map will miss the entry. Returning the address in EIP-55 checksummed form (or normalising to lowercase at parse time and documenting it) would make the correlation unambiguous.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Thanks Copilot — reviewed both:
|
|
Thanks Greptile — both are accurate observations; my take on each:
|
Summary
Two related JSON-RPC changes aligning ethrex with ethereum/execution-apis#812 by @manusw7 (Block parameter optional, default
latest):lateston the state methods that previously required it.eth_getBalance,eth_getCode,eth_getStorageAt,eth_getTransactionCount, andeth_getProofdid a strictparams.len() != Ncheck and returned-32000 "Expected N params"when the block was omitted. They now accept the block as optional and default it tolatestvia a newimpl Default for BlockIdentifierOrHash(latest).eth_getStorageValues(previously-32601 method not found): batches storage-slot reads for multiple accounts at a block, mirroringeth_getStorageAt, with the same optional-block default and a 1024-slot cap. Registered in theethdispatch.Motivation
Aligns ethrex with execution-apis#812 (@manusw7) and matches the other clients implementing the same change. Full credit to @manusw7 for the proposal.
Testing
cargo test -p ethrex-rpc --lib— 64 passed, 0 failed. New parse-layer tests assert each state method resolves an omitted block tolatest(test_state_methods_default_block_to_latest_when_omitted) and thateth_getStorageValuesparses (and rejects an empty request).rpc-compatsimulator that the omitted-block request now produces the same response as an explicit"latest"request for every method (previously the omitted form errored).Note on hive rpc-compat value fixtures (out of scope)
ethrex does not currently pass the rpc-compat value fixtures for these state methods in the hive default chain — but this is pre-existing and unrelated to this change: a stock ethrex build returns the same results for an explicit
"latest"request (e.g.eth_getBalance(addr, "latest")returns0x0where other clients return the funded balance), and*-blockhashrequests error with "Could not resolve block number". That points to an ethrex/hive default-chain state-serving issue, which is out of scope here. This PR makes the omitted-block path behave identically to the explicit-latestpath (the spec change), proven by the unit tests above.Related