-
Notifications
You must be signed in to change notification settings - Fork 37
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
feat(starknet_patricia,starknet_committer): stop using custom ContractAddress type #4002
base: main
Are you sure you want to change the base?
Conversation
2acf756
to
3ace87c
Compare
8624d02
to
f14107d
Compare
3ace87c
to
c654b17
Compare
f14107d
to
c211f84
Compare
c654b17
to
2d8c7c7
Compare
c211f84
to
36ca70a
Compare
2d8c7c7
to
c6638c4
Compare
36ca70a
to
a325b53
Compare
c6638c4
to
17def8d
Compare
c7cb0d5
to
488241c
Compare
17def8d
to
890c24c
Compare
488241c
to
a8b53c0
Compare
890c24c
to
d87ae13
Compare
a8b53c0
to
fa692c7
Compare
d87ae13
to
564f9e5
Compare
fa692c7
to
1571247
Compare
564f9e5
to
6a03047
Compare
a013b82
to
9619c3c
Compare
6a03047
to
8617d38
Compare
9619c3c
to
788989c
Compare
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.
Reviewed 6 of 19 files at r1, 15 of 15 files at r2.
Reviewable status: 18 of 21 files reviewed, 3 unresolved discussions (waiting on @TzahiTaub)
crates/starknet_committer_and_os_cli/Cargo.toml
line 32 at r2 (raw file):
serde_repr.workspace = true starknet-types-core.workspace = true starknet_api = { workspace = true, features = ["testing"] }
do you use the feature, not in test file?
crates/starknet_committer/src/block_committer/input.rs
line 18 at r2 (raw file):
pub mod input_test; pub fn try_from_node_index_for_contract_address(
The starknet_patricia
crate can be depended on the starknet_api
crate, right?
If so, you can
impl TryFrom<&NodeIndex> for ContractAddress {
Next to NodeIndex
deifinition
crates/starknet_committer/src/block_committer/input.rs
line 34 at r2 (raw file):
} pub fn from_contract_address_for_node_index(address: &ContractAddress) -> NodeIndex {
the same as above
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: 18 of 21 files reviewed, 3 unresolved discussions (waiting on @AvivYossef-starkware and @TzahiTaub)
crates/starknet_committer/src/block_committer/input.rs
line 18 at r2 (raw file):
Previously, AvivYossef-starkware wrote…
The
starknet_patricia
crate can be depended on thestarknet_api
crate, right?
If so, you can
impl TryFrom<&NodeIndex> for ContractAddress {
Next toNodeIndex
deifinition
this means that the starknet_patricia crate will "be aware of specific leaf types", which is something I'd prefer to avoid. WDYT?
crates/starknet_committer/src/block_committer/input.rs
line 34 at r2 (raw file):
Previously, AvivYossef-starkware wrote…
the same as above
same as above
crates/starknet_committer_and_os_cli/Cargo.toml
line 32 at r2 (raw file):
Previously, AvivYossef-starkware wrote…
do you use the feature, not in test file?
Done.
8617d38
to
9fe7449
Compare
788989c
to
90f3bea
Compare
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: 16 of 21 files reviewed, 3 unresolved discussions (waiting on @AvivYossef-starkware and @TzahiTaub)
crates/starknet_committer/src/block_committer/input.rs
line 18 at r2 (raw file):
Previously, dorimedini-starkware wrote…
this means that the starknet_patricia crate will "be aware of specific leaf types", which is something I'd prefer to avoid. WDYT?
starknet_api defines ContractAddress,
starknet_patricia defines NodeIndex,
and starknet_committer defines "ContractAddress as a leaf"
Benchmark movements: |
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.
Reviewed 3 of 3 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware and @TzahiTaub)
crates/starknet_committer/src/block_committer/input.rs
line 18 at r2 (raw file):
Previously, dorimedini-starkware wrote…
starknet_api defines ContractAddress,
starknet_patricia defines NodeIndex,
and starknet_committer defines "ContractAddress as a leaf"
mmm, it sounds reasonable to me.
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 @TzahiTaub)
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 @TzahiTaub)
crates/starknet_committer/src/block_committer/input.rs
line 18 at r2 (raw file):
Previously, AvivYossef-starkware wrote…
mmm, it sounds reasonable to me.
not sure what you mean. want me to implement TryFrom<&NodeIndex> for ContractAddress
in the patricia crate? or does the crate separation sound reasonable to you?
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 @TzahiTaub)
crates/starknet_committer/src/block_committer/input.rs
line 18 at r2 (raw file):
Previously, dorimedini-starkware wrote…
not sure what you mean. want me to implement
TryFrom<&NodeIndex> for ContractAddress
in the patricia crate? or does the crate separation sound reasonable to you?
I'm ok with the crate separation and the current PR
90f3bea
to
48e3dca
Compare
No description provided.