-
Notifications
You must be signed in to change notification settings - Fork 65
blockifier_reexecution: allow custom chain id #10643
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
blockifier_reexecution: allow custom chain id #10643
Conversation
8e68696 to
de3837d
Compare
b4b5c24 to
eaf2a13
Compare
de3837d to
802fd8b
Compare
eaf2a13 to
351f6ce
Compare
dorimedini-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dorimedini-starkware reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware)
crates/blockifier_reexecution/src/state_reader/cli.rs line 34 at r1 (raw file):
#[clap(long, short = 'c')] pub chain_id: Option<ChainId>, }
suggestion: use ArgGroup to allow two distinct argument types:
--chain_id- same as before, using the enum--custom_chain_id- accepting a hex string. you can use the logic indeserialize_chain_id_from_hexto parse this
WDYT?
Code quote:
#[derive(Clone, Debug, Args)]
pub struct RpcArgs {
/// Node url.
#[clap(long, short = 'n')]
pub node_url: String,
/// Optional chain ID (if not provided, it will be guessed from the node url).
/// Standard chain IDs: SN_MAIN, SN_SEPOLIA, SN_INTEGRATION_SEPOLIA.
/// Custom chain IDs are also supported.
#[clap(long, short = 'c')]
pub chain_id: Option<ChainId>,
}crates/starknet_api/src/core.rs line 90 at r1 (raw file):
Ok(Self::from(s.to_owned())) } }
I really don't like that the Other variant of ChainId is a string and not a Felt...
you now have two methods of converting from string to chain ID: this one (which actually treats the string as bytes when converting to Felt), and deserialize_chain_id_from_hex, which treats the string as a hex integer.
can you delete this FromStr implementation to avoid confusion?
Code quote:
impl std::str::FromStr for ChainId {
type Err = std::convert::Infallible;
fn from_str(s: &str) -> Result<Self, Self::Err> {
Ok(Self::from(s.to_owned()))
}
}
AvivYossef-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)
crates/starknet_api/src/core.rs line 90 at r1 (raw file):
Previously, dorimedini-starkware wrote…
I really don't like that the
Othervariant of ChainId is a string and not aFelt...
you now have two methods of converting from string to chain ID: this one (which actually treats the string as bytes when converting to Felt), anddeserialize_chain_id_from_hex, which treats the string as a hex integer.
can you delete thisFromStrimplementation to avoid confusion?
Maybe the from str should be deserialize_chain_id_from_hex?
What do you think?
dorimedini-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware)
crates/starknet_api/src/core.rs line 90 at r1 (raw file):
Previously, AvivYossef-starkware wrote…
Maybe the from str should be
deserialize_chain_id_from_hex?
What do you think?
I think it's ambiguous.
the root of the problem is that we have Other(String) instead of Other(Felt), but changing that seems hard (for sure it's out of scope of the current PR).
why not use my above suggestion, and explicitly request a hex string for custom chain IDs? then convert explicitly using deserialize_chain_id_from_hex?
802fd8b to
6aa90d5
Compare
351f6ce to
e70c9de
Compare
dorimedini-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dorimedini-starkware reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware)
6aa90d5 to
9c9a688
Compare
e70c9de to
607df2f
Compare
AvivYossef-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 4 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)
crates/blockifier_reexecution/src/state_reader/cli.rs line 34 at r1 (raw file):
Previously, dorimedini-starkware wrote…
suggestion: use
ArgGroupto allow two distinct argument types:
--chain_id- same as before, using the enum--custom_chain_id- accepting a hex string. you can use the logic indeserialize_chain_id_from_hexto parse thisWDYT?
Done
crates/starknet_api/src/core.rs line 90 at r1 (raw file):
Previously, dorimedini-starkware wrote…
I think it's ambiguous.
the root of the problem is that we haveOther(String)instead ofOther(Felt), but changing that seems hard (for sure it's out of scope of the current PR).
why not use my above suggestion, and explicitly request a hex string for custom chain IDs? then convert explicitly usingdeserialize_chain_id_from_hex?
Done.
9c9a688 to
e0f13bb
Compare
607df2f to
06fa010
Compare
e0f13bb to
5b0310b
Compare
06fa010 to
f7b94bd
Compare
dorimedini-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dorimedini-starkware reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware)
crates/blockifier_reexecution/src/state_reader/cli.rs line 69 at r3 (raw file):
let mut deserializer = serde_json::Deserializer::from_str(&json_str); return deserialize_chain_id_from_hex(&mut deserializer) .expect("Failed to parse hex chain id");
maybe better to extract the string-parsing logic from the deserializer, and using it in two places, instead of creating temporary JSON just to reuse deserialization
Code quote:
let json_str = format!("\"{}\"", hex_str);
let mut deserializer = serde_json::Deserializer::from_str(&json_str);
return deserialize_chain_id_from_hex(&mut deserializer)
.expect("Failed to parse hex chain id");f7b94bd to
e7407fe
Compare
AvivYossef-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 6 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)
crates/blockifier_reexecution/src/state_reader/cli.rs line 69 at r3 (raw file):
Previously, dorimedini-starkware wrote…
maybe better to extract the string-parsing logic from the deserializer, and using it in two places, instead of creating temporary JSON just to reuse deserialization
Done.
dorimedini-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dorimedini-starkware reviewed 4 of 4 files at r4, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @AvivYossef-starkware)
a discussion (no related file):
ah... risky, doing this on main-v0.14.1... can you open a py-side PR to make sure nothing breaks?
e7407fe to
d902ef5
Compare
5b0310b to
bad84a1
Compare
bad84a1 to
9ee63ae
Compare
d902ef5 to
7b91eb9
Compare
7b91eb9 to
5816480
Compare
dorimedini-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dorimedini-starkware reviewed 6 of 6 files at r6, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @AvivYossef-starkware)
AvivYossef-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @AvivYossef-starkware)
a discussion (no related file):
Previously, dorimedini-starkware wrote…
ah... risky, doing this on main-v0.14.1... can you open a py-side PR to make sure nothing breaks?
done

No description provided.