fix!(rpc): disallow non-prefixed hex strings in RPC input#3328
fix!(rpc): disallow non-prefixed hex strings in RPC input#3328
Conversation
There was a problem hiding this comment.
this shouldn't say optional now
There was a problem hiding this comment.
LGTM
Not sure if it was tested against:
https://github.com/starknet-io/starknet.js
https://github.com/software-mansion/starknet.py
Changes the JSON-RPC deserialization logic to reject hex strings without the `0x` prefix. This brings us closer in line with the Starknet API spec.
sistemd
force-pushed
the
sistemd/enforce-hex-prefix
branch
from
edcaba9 to
134afd1
Compare
|
I tried running the test suites of both I also found something reassuring while looking at these libraries - they both have helpers for working with hex strings (and felts) that are used throughout their respective RPC clients and those match the format from the spec. For example:
and |
| ### Changed | ||
|
|
||
| - `starknet_getEvents` now returns `block_number` for events from pre-confirmed and pre-latest blocks. | ||
| - BREAKING: JSON-RPC input no longer accepts hex strings that are missing the `0x` prefix, in line with the Starknet API spec. |
There was a problem hiding this comment.
We've never used BREAKING in the CHANGELOG.
I'd suggest we remove it. We'll highlight it in the release notes (+ major version change).
| let expected = ComputedClassHash::Cairo(class_hash!( | ||
| "056b96c1d1bbfa01af44b465763d1b71150fa00c6c9d54c3947f57e979ff68c3" | ||
| )); | ||
|
|
||
| // Known contract which triggered a hash mismatch failure. | ||
| let extract = tokio::task::spawn_blocking(move || -> anyhow::Result<_> { | ||
| let hash = compute_class_hash(CAIRO_0_8_NEW_ATTRIBUTES)?; | ||
| Ok(hash) | ||
| }); | ||
| let calculated_hash = extract.await.unwrap().unwrap(); | ||
|
|
||
| assert_eq!(calculated_hash, expected); |
There was a problem hiding this comment.
Is this at all related to this PR? 👀
There was a problem hiding this comment.
It's a rebase related hiccup (I refactored this test to use the hash() helper and it was merged to main). This change should be reverted.
| @@ -323,22 +323,22 @@ pub fn starkhash_to_dec_str(h: &Felt) -> String { | |||
|
|
|||
| /// A helper conversion function. Only use with __sequencer API related types__. | |||
| fn starkhash_from_dec_str(s: &str) -> Result<Felt, anyhow::Error> { | |||
There was a problem hiding this comment.
This method seems to be duplicated in crates/common/src/event.rs. Might be worth consolidating in a shared space for everyone to consume from a single source of truth.
4 participants
Changes the JSON-RPC deserialization logic to reject hex strings without the
0xprefix. This brings us closer in line with the Starknet API spec.Closes: #3309
It goes without saying that this change could break many existing clients, so it might be worth considering a more gradual deprecation strategy.
Also, while we're on the topic, I noticed another inconsistency between our deserialization logic and the spec. Patterns like
^0x(0|[a-fA-F1-9]{1}[a-fA-F0-9]{0,62})$do not allow empty strings after the0xprefix, but our deserialization logic currently allows them and treats them as zero.