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.

Sign up for GitHub

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

Jump to bottom

Handle deploy panic data #118

Merged
merged 13 commits into from
Jul 11, 2023
Merged

Handle deploy panic data #118

merged 13 commits into from
Jul 11, 2023

Conversation

Arcticae
Copy link
Contributor

Closes #47

@Arcticae Arcticae requested review from a team, MaksymilianDemitraszek, abulenok, THenry14 and karol-bisztyga and removed request for a team, THenry14 and karol-bisztyga July 11, 2023 07:39
@piotmag769 piotmag769 self-requested a review July 11, 2023 09:33
@piotmag769
Copy link
Member

software-mansion/protostar#2157 (comment)

@Arcticae Arcticae marked this pull request as ready for review July 11, 2023 11:45
@Arcticae Arcticae requested a review from a team July 11, 2023 11:45
@Arcticae Arcticae requested a review from a team as a code owner July 11, 2023 11:45
@Arcticae Arcticae requested review from THenry14 and karol-bisztyga and removed request for a team, abulenok, THenry14 and karol-bisztyga July 11, 2023 11:45
@Arcticae Arcticae assigned Radinyn and unassigned Radinyn Jul 11, 2023
@Arcticae Arcticae requested a review from Radinyn July 11, 2023 11:46
Radinyn
Radinyn suggested changes Jul 11, 2023
View reviewed changes
Copy link
Member

@Radinyn Radinyn left a comment

Choose a reason for hiding this comment

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

We shouldn't use panic in cheatcodes unless the errors are truly irrecoverable for the whole runner.

starknet-foundry/crates/forge/src/cheatcodes_hint_processor.rs Outdated Show resolved Hide resolved
starknet-foundry/crates/forge/src/cheatcodes_hint_processor.rs Show resolved Hide resolved
piotmag769
piotmag769 reviewed Jul 11, 2023
View reviewed changes
Copy link
Member

@piotmag769 piotmag769 left a comment

Choose a reason for hiding this comment

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

We should get rid of panics

starknet-foundry/crates/forge/tests/data/deploy_error_handling_test/Scarb.toml Outdated Show resolved Hide resolved
starknet-foundry/crates/forge/tests/e2e/running.rs Show resolved Hide resolved
starknet-foundry/crates/forge/src/cheatcodes_hint_processor.rs Show resolved Hide resolved
starknet-foundry/crates/forge/src/cheatcodes_hint_processor.rs Outdated Show resolved Hide resolved
@cptartur cptartur self-requested a review July 11, 2023 13:22
@Arcticae Arcticae requested review from piotmag769 and Radinyn July 11, 2023 14:05
@Arcticae Arcticae enabled auto-merge (squash) July 11, 2023 16:24
@Arcticae Arcticae merged commit 31e0a10 into master Jul 11, 2023
@Arcticae Arcticae deleted the 47-deploy-panic-data-handling branch July 11, 2023 16:42
@cptartur cptartur mentioned this pull request Jul 17, 2023
Closed
Utilitycoder pushed a commit to Utilitycoder/starknet-foundry that referenced this pull request Oct 6, 2023
@shramee
Dev review updates (foundry-rs#118)
dfef0d5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cptartur cptartur cptartur left review comments

@piotmag769 piotmag769 piotmag769 approved these changes

@Radinyn Radinyn Radinyn approved these changes

@MaksymilianDemitraszek MaksymilianDemitraszek Awaiting requested review from MaksymilianDemitraszek

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Handle panic_data in Deploy
4 participants
@Arcticae @piotmag769 @Radinyn @cptartur