-
Notifications
You must be signed in to change notification settings - Fork 52
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
fix: consider replace classes as deployed contracts #262
Conversation
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 r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware and @yoavGrs)
src/block_hash/state_diff_hash.rs
line 25 at r1 (raw file):
let mut hash_chain = HashChain::new(); hash_chain = hash_chain.chain(&STARKNET_STATE_DIFF0); hash_chain = chain_updated_contracts(
You can keep calling it deployed. The end goal is to unite these two fields into a single field by 0.13.3
src/block_hash/state_diff_hash_test.rs
line 31 at r1 (raw file):
}; let replaced_classes_0 = indexmap! { 2u64.into() => ClassHash(2u64.into()),
Change the replaced contract address to be in the middle of the two deployed contracts to test the sorting
95b88de
to
aac00d3
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: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware and @ShahakShama)
src/block_hash/state_diff_hash.rs
line 25 at r1 (raw file):
Previously, ShahakShama wrote…
You can keep calling it deployed. The end goal is to unite these two fields into a single field by 0.13.3
But it's confusing for now.
src/block_hash/state_diff_hash_test.rs
line 31 at r1 (raw file):
Previously, ShahakShama wrote…
Change the replaced contract address to be in the middle of the two deployed contracts to test the sorting
This is the case at replaced_classes_1
.
I'm changing the class hash.
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 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ShahakShama and @yoavGrs)
a discussion (no related file):
why did the regression tests result change? I see no change in get_state_diff()
so no replaced classes means the hash should be the same, right?
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, 3 unresolved discussions (waiting on @dorimedini-starkware and @ShahakShama)
a discussion (no related file):
Previously, dorimedini-starkware wrote…
why did the regression tests result change? I see no change in
get_state_diff()
so no replaced classes means the hash should be the same, right?
There were replaced_classes
before, but we didn't consider them.
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, 2 unresolved discussions (waiting on @ShahakShama)
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 all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @yoavGrs)
This change is