Skip to content
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

Made error message for entry point be dependent on failing var. #3740

Merged
merged 1 commit into from
Jul 24, 2023

Conversation

orizi
Copy link
Collaborator

@orizi orizi commented Jul 23, 2023

This change is Reviewable

@ilyalesokhin-starkware
Copy link
Contributor

crates/cairo-lang-starknet/src/plugin/entry_point.rs line 95 at r1 (raw file):

            });
        }
        let mut input_data_short_err = format!("`{param_name}` failed to deserialize");

I see that this is not a regression compared to the current state,
but I'm not sure I like having all those constants in the byecode.

Suggestion:

"failed to deserialize `{param_name}`"

Copy link
Collaborator Author

@orizi orizi left a 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 23 files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware)


crates/cairo-lang-starknet/src/plugin/entry_point.rs line 95 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

I see that this is not a regression compared to the current state,
but I'm not sure I like having all those constants in the byecode.

i think i prefer having the param name more than the prefix.

the current one was very confusing - we may add "non-provable" info at some point of the future.

@ilyalesokhin-starkware
Copy link
Contributor

crates/cairo-lang-starknet/src/plugin/entry_point.rs line 95 at r1 (raw file):

Previously, orizi wrote…

i think i prefer having the param name more than the prefix.

the current one was very confusing - we may add "non-provable" info at some point of the future.

"i think i prefer having the param name more than the prefix."
What is the prefix?

Copy link
Collaborator Author

@orizi orizi left a 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 23 files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware)


crates/cairo-lang-starknet/src/plugin/entry_point.rs line 95 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

"i think i prefer having the param name more than the prefix."
What is the prefix?

i meant before the other part.
I'd rather have:
'long_long_long_name' failed to des
than
failed to deserialize 'long_long_long

@ilyalesokhin-starkware
Copy link
Contributor

crates/cairo-lang-starknet/src/plugin/entry_point.rs line 95 at r1 (raw file):

Previously, orizi wrote…

i meant before the other part.
I'd rather have:
'long_long_long_name' failed to des
than
failed to deserialize 'long_long_long

Ok, I wasn't sure if the former is correct grammatically.

@ilyalesokhin-starkware
Copy link
Contributor

crates/cairo-lang-starknet/src/plugin/plugin_test_data/l1_handler line 205 at r1 (raw file):

            
            let __arg_from_address =
                serde::Serde::<u128>::deserialize(ref data).expect('`from_address` failed to deseri');

This is starnge.

Code quote:

'`from_address` failed to deseri'

Copy link
Contributor

@ilyalesokhin-starkware ilyalesokhin-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 23 files at r1.
Reviewable status: 3 of 23 files reviewed, all discussions resolved

@ilyalesokhin-starkware
Copy link
Contributor

crates/cairo-lang-starknet/src/plugin/entry_point.rs line 95 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

Ok, I wasn't sure if the former is correct grammatically.

Maybe you should just give the index of the parameter?

Copy link
Collaborator Author

@orizi orizi left a 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 23 files reviewed, all discussions resolved (waiting on @ilyalesokhin-starkware)


crates/cairo-lang-starknet/src/plugin/plugin_test_data/l1_handler line 205 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

This is starnge.

until we have normal long-string byte arrays - this would still be the easiest way to go.

@ilyalesokhin-starkware
Copy link
Contributor

crates/cairo-lang-starknet/src/plugin/plugin_test_data/l1_handler line 205 at r1 (raw file):

Previously, orizi wrote…

until we have normal long-string byte arrays - this would still be the easiest way to go.

not doing this PR is even easier.

Copy link
Contributor

@ilyalesokhin-starkware ilyalesokhin-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 20 of 23 files at r1.
Reviewable status: 22 of 23 files reviewed, all discussions resolved (waiting on @orizi)

Copy link
Collaborator Author

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 22 of 23 files reviewed, all discussions resolved (waiting on @ilyalesokhin-starkware)


crates/cairo-lang-starknet/src/plugin/entry_point.rs line 95 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

Maybe you should just give the index of the parameter?

i don't really like that idea, but can do - still an improvement i guess.

@orizi orizi force-pushed the orizi/better-inputs-fail-msg branch from 0cabb0b to d8c276a Compare July 23, 2023 14:08
@orizi orizi enabled auto-merge July 23, 2023 15:09
@orizi orizi force-pushed the orizi/better-inputs-fail-msg branch from d8c276a to dac2f15 Compare July 24, 2023 14:15
@orizi orizi added this pull request to the merge queue Jul 24, 2023
Merged via the queue into main with commit 2965c16 Jul 24, 2023
37 of 38 checks passed
@orizi orizi deleted the orizi/better-inputs-fail-msg branch July 24, 2023 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants