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

Deploy panic_data handling #2157

Closed
wants to merge 24 commits into from
Closed

Deploy panic_data handling #2157

wants to merge 24 commits into from

Conversation

Arcticae
Copy link
Member

@Arcticae Arcticae commented Jun 30, 2023

Checklist

  • Relevant issue is linked
  • Docs updated/issue for docs created
  • Added relevant tests

@Arcticae Arcticae linked an issue Jun 30, 2023 that may be closed by this pull request
Base automatically changed from 1992-contract-calling to master June 30, 2023 13:46
@Arcticae Arcticae changed the title panic_data handling Deploy panic_data handling Jun 30, 2023
@Arcticae Arcticae marked this pull request as ready for review July 3, 2023 09:43
@Arcticae Arcticae requested a review from a team as a code owner July 3, 2023 09:43
@Arcticae Arcticae requested review from MaksymilianDemitraszek, karol-bisztyga and cptartur and removed request for a team and karol-bisztyga July 3, 2023 09:43
Comment on lines +518 to +520
fn felt_from_short_string(short_str: &str) -> Felt252 {
return Felt252::from_bytes_be(short_str.as_bytes());
}
Copy link
Member

Choose a reason for hiding this comment

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

I'd add some simple unit test for that

Comment on lines +522 to +538
fn try_extract_panic_data(err: &str) -> Option<Vec<Felt252>> {
let re = Regex::new(r#"(?m)^Got an exception while executing a hint: Custom Hint Error: Execution failed. Failure reason: "(.*)"\.$"#)
.expect("Could not create panic_data matching regex");

if let Some(captures) = re.captures(err) {
if let Some(panic_data_match) = captures.get(1) {
let panic_data_felts: Vec<Felt252> = panic_data_match
.as_str()
.split(", ")
.map(felt_from_short_string)
.collect();

return Some(panic_data_felts);
}
}
None
}
Copy link
Member

Choose a reason for hiding this comment

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

This could be unit tested

Comment on lines +409 to +410
insert_value_to_cellref!(vm, deployed_contract_address, contract_address)?;
insert_value_to_cellref!(vm, panic_data_end, ptr)?;
Copy link
Member

Choose a reason for hiding this comment

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

Why are we inserting an address and panic data end here? Shouldn't it be panic data start end and?

Copy link
Contributor

@piotmag769 piotmag769 Jul 4, 2023

Choose a reason for hiding this comment

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

Keep in mind this will change anyways after merging #2163 - we will just return plain Span<felt252> every time and extract panic_data in cairo code.

You can look at the draft for handling errors in deploy cheatcode, but feel free to do it however you feel like is the best.

I think we should try to not change the user interface too much while reimplementing it using general cheatcode.

Comment on lines +11 to +12
panic_data.append('PANIK');
panic_data.append('DEJTA');
Copy link
Member

Choose a reason for hiding this comment

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

😆

@piotmag769 piotmag769 self-requested a review July 3, 2023 16:29
@piotmag769
Copy link
Contributor

Please can we wait with merging for general cheatcode impl? 🥺

@piotmag769
Copy link
Contributor

Make sure to remove the TODO after implementing it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle panic_data in Deploy
3 participants