-
Notifications
You must be signed in to change notification settings - Fork 107
fix: state error undeclared class hash format #2053
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 @AvivYossef-starkware and @Yoni-Starkware)
crates/blockifier/src/state.rs
line 6 at r1 (raw file):
pub mod state_api; #[cfg(test)] pub mod errors_test;
Suggestion:
error_format_test
crates/blockifier/src/state/errors_test.rs
line 8 at r1 (raw file):
#[rstest] fn test_error_forrmat() {
weird the formatter didn't catch this
Suggestion:
use crate::state::errors::StateError;
#[rstest]
fn test_error_forrmat() {
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, 4 unresolved discussions (waiting on @AvivYossef-starkware)
crates/blockifier/src/state/errors.rs
line 24 at r1 (raw file):
#[error("Requested {0:?} is unavailable for deployment.")] UnavailableContractAddress(ContractAddress), // TODO: (Aviv, 8/7/2024): use lowerhex print instead of debug in the next starknet-api version.
Suggestion:
// TODO(Aviv, 8/7/2024):
crates/blockifier/src/state/errors_test.rs
line 10 at r1 (raw file):
fn test_error_forrmat() { let error = StateError::UndeclaredClassHash(ClassHash(Felt252::from(2))); assert_eq!(error.to_string(), "Class with hash ClassHash(0x2) is not declared.");
This is the goal for all error messages
Suggestion:
"Class with hash 0x2 is
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, 4 unresolved discussions (waiting on @Yoni-Starkware)
crates/blockifier/src/state/errors_test.rs
line 10 at r1 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
This is the goal for all error messages
To achieve this without changing the starknet-api,I need to define a new wrapper struct and update it everywhere in the code.
in starknet-api all I need is to change 1 line,
do it?
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, 4 unresolved discussions (waiting on @AvivYossef-starkware)
crates/blockifier/src/state/errors_test.rs
line 10 at r1 (raw file):
Previously, AvivYossef-starkware wrote…
To achieve this without changing the starknet-api,I need to define a new wrapper struct and update it everywhere in the code.
in starknet-api all I need is to change 1 line,
do it?
Oh, no. Let's do it later through SN API
259a8aa
to
bf39fcc
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: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)
crates/blockifier/src/state.rs
line 6 at r1 (raw file):
pub mod state_api; #[cfg(test)] pub mod errors_test;
Done.
crates/blockifier/src/state/errors_test.rs
line 8 at r1 (raw file):
Previously, dorimedini-starkware wrote…
weird the formatter didn't catch this
Done.
crates/blockifier/src/state/errors_test.rs
line 10 at r1 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
Oh, no. Let's do it later through SN API
I can also use a helper function that returns the nested field. For some reason, I can't access the nested field directly with .0.0.
For now, I think that's the best option
326f91a
to
0837281
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 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @AvivYossef-starkware and @Yoni-Starkware)
crates/blockifier/src/state/error_format_test.rs
line 7 at r2 (raw file):
use crate::state::errors::StateError; #[rstest]
I think #[test]
is enough here, no need for rstest
(the rstest
crate is used for the cases
, case
and values
attributes, for example)
Suggestion:
use starknet_api::core::ClassHash;
use crate::state::errors::StateError;
#[test]
crates/blockifier/src/state/error_format_test.rs
line 9 at r2 (raw file):
#[rstest] fn test_error_undeclared_class_hash_forrmat() { let error = StateError::UndeclaredClassHash(ClassHash(Felt252::from(2)));
wrong felt :)
Suggestion:
use starknet_types_core::felt::Felt;
use rstest::rstest;
use starknet_api::core::ClassHash;
use crate::state::errors::StateError;
#[rstest]
fn test_error_undeclared_class_hash_forrmat() {
let error = StateError::UndeclaredClassHash(ClassHash(Felt::TWO));
crates/blockifier/src/state/errors.rs
line 24 at r2 (raw file):
#[error("Requested {0:?} is unavailable for deployment.")] UnavailableContractAddress(ContractAddress), #[error("Class with hash {:#064x} is not declared.", **.0)]
@Yoni-Starkware you mentioned (here) that you want 0x2
and not 0x000...0002
?
Is it important or are leading zeros ok?
Code quote:
{:#064x}
0837281
to
0c856f6
Compare
0c856f6
to
19e3116
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: 3 of 4 files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)
crates/blockifier/src/state/error_format_test.rs
line 7 at r2 (raw file):
Previously, dorimedini-starkware wrote…
I think
#[test]
is enough here, no need forrstest
(therstest
crate is used for thecases
,case
andvalues
attributes, for example)
Done.
crates/blockifier/src/state/error_format_test.rs
line 9 at r2 (raw file):
Previously, dorimedini-starkware wrote…
wrong felt :)
Done.
crates/blockifier/src/state/errors.rs
line 24 at r2 (raw file):
Previously, dorimedini-starkware wrote…
@Yoni-Starkware you mentioned (here) that you want
0x2
and not0x000...0002
?
Is it important or are leading zeros ok?
I tried to be consistent with other errors
Code snippet:
#[error("Entry point {:#064x} of type {typ:?} is not unique.", .selector.0)]
DuplicatedEntryPointSelector { selector: EntryPointSelector, typ: EntryPointType },
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 r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Yoni-Starkware)
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 @Yoni-Starkware)
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 4 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @AvivYossef-starkware)
change error format to hex
This change is