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

cairo lang errs #435

Merged
merged 1 commit into from
Apr 25, 2024
Merged

cairo lang errs #435

merged 1 commit into from
Apr 25, 2024

Conversation

lordshashank
Copy link
Contributor

@lordshashank lordshashank commented Apr 22, 2024

Usage related changes

improves error handling

Development related changes

Resolves #198

Checklist:

  • Checked the relevant parts of development docs
  • Applied formatting - ./scripts/format.sh
  • No linter errors - ./scripts/clippy_check.sh
  • No unused dependencies - ./scripts/check_unused_deps.sh
  • Performed code self-review
  • Rebased to the latest commit of the target branch (or merged it into my branch)
  • Updated the docs if needed, including the TODO section
  • Linked the issues which this PR resolves
  • Updated the tests if needed; all passing (execution info)

@lordshashank lordshashank marked this pull request as draft April 22, 2024 10:02
@lordshashank
Copy link
Contributor Author

@marioiordanov is this what you meant (I took sierra errors from cairo_lang_starknet_classes, as cairo_lang_starknet had just abi errors in it) ? I have also not removed the "reason" property, let me know if you want it removed.

@marioiordanov
Copy link
Contributor

@lordshashank just make it as TransactionExecutionError(#[from] TransactionExecutionError)

@lordshashank
Copy link
Contributor Author

@marioiordanov
did that as following

    #[error(transparent)]
    SierraCompilationError(#[from] StarknetSierraCompilationError),

Now, I tried to use ? to auto parse error here but its not working, should I use StarknetSierraCompilationError variants here?

@marioiordanov
Copy link
Contributor

@lordshashank take a look at other error variants that have the same structure

@lordshashank
Copy link
Contributor Author

@marioiordanov , I have looked into the other variants, thing is StarknetSierraCompilationError has many variants here,
Would downcasting work here so as to get the variant automatically like this

                let casm_json = usc::compile_contract(serde_json::to_value(sierra_contract_class).map_err(
                    |err| Error::JsonError(JsonError::Custom { msg: err.to_string() }),
                )?).map_err(|err| {
                    let err_str = err.to_string();
                    if let Some(sierra_error) = err.downcast::<StarknetSierraCompilationError>().ok() {
                        Error::SierraCompilationError(sierra_error)
                    } else {
                        Error::UnexpectedError(err_str)
                    }
                })?;

or should I use some specific variant only?

@marioiordanov
Copy link
Contributor

@lordshashank sorry about misleading you, just add a String to the SierraCompilationError variant and then create the variant like this: Error::SierraCompilationError(err_str)

@lordshashank
Copy link
Contributor Author

lordshashank commented Apr 24, 2024

@marioiordanov I don't understand, do you want StarknetSierraCompilationError Error derivation for SierraCompilationError to be removed here and just make variant from string message, if thats the case, it is already like that in repo's current state, then what changes do we intend to do in the issue?

@marioiordanov
Copy link
Contributor

I just saw that there are two SierraCompilationError variants in different crates. Replace usage of SierraCompilationError variant from starknet_devnet_core::errors::Error with TypesError(starknet_devnet_types::error::Error::SierraCompilationError). Then delete the variant from starknet_devnet_core

@lordshashank lordshashank marked this pull request as ready for review April 24, 2024 13:30
@lordshashank
Copy link
Contributor Author

@marioiordanov did that, let me know if something else is to be done here.

@lordshashank
Copy link
Contributor Author

@marioiordanov should we change issue description if this is all to be done?

@marioiordanov
Copy link
Contributor

@lordshashank we will just make a comment. Have to run the CI to approve your PR

@lordshashank
Copy link
Contributor Author

Ok let me know if CI fails.

@marioiordanov marioiordanov merged commit 779ab19 into 0xSpaceShard:main Apr 25, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use cairo-lang-starknet for handling transaction execution errors
2 participants