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
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions protostar-rust/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion protostar-rust/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ cairo-lang-sierra = { path = "../cairo/crates/cairo-lang-sierra", version = "2.0
cairo-lang-utils = { path = "../cairo/crates/cairo-lang-utils", version = "2.0.0-rc2" }
cairo-lang-casm = { path = "../cairo/crates/cairo-lang-casm", version = "2.0.0-rc2" }
cairo-lang-starknet = { path = "../cairo/crates/cairo-lang-starknet", version = "2.0.0-rc2" }
blockifier = { git = "https://github.com/software-mansion-labs/blockifier.git", branch="kb/use-in-protostar" }
blockifier = { git = "https://github.com/software-mansion-labs/blockifier.git", branch = "kb/use-in-protostar" }
starknet_api = { git = "https://github.com/starkware-libs/starknet-api", rev = "24a7249" }
schemars = { version = "0.8.12", features = ["preserve_order"] }
parity-scale-codec = "3.5.0"
Expand All @@ -42,6 +42,7 @@ ark-std = "0.3.0"
ark-secp256k1 = "0.4.0"
ark-secp256r1 = "0.4.0"
num-traits = "0.2"
regex = "1.8.4"

[lib]
name = "rust_test_runner"
87 changes: 70 additions & 17 deletions protostar-rust/src/cheatcodes_hint_processor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,13 @@ use blockifier::execution::contract_class::{
ContractClass as BlockifierContractClass, ContractClassV1,
};
use blockifier::execution::entry_point::{CallEntryPoint, CallType};
use blockifier::execution::errors::EntryPointExecutionError;
use blockifier::state::cached_state::CachedState;
use blockifier::state::state_api::StateReader;
use blockifier::test_utils::{invoke_tx, DictStateReader};
use blockifier::test_utils::{MAX_FEE, TEST_ACCOUNT_CONTRACT_ADDRESS};
use blockifier::transaction::account_transaction::AccountTransaction;
use blockifier::transaction::errors::TransactionExecutionError;
use blockifier::transaction::transaction_utils_for_protostar::declare_tx_default;
use blockifier::transaction::transactions::{DeclareTransaction, ExecutableTransaction};
use cairo_felt::Felt252;
Expand All @@ -40,6 +42,7 @@ use cairo_vm::vm::errors::hint_errors::HintError;
use cairo_vm::vm::errors::vm_errors::VirtualMachineError;
use cairo_vm::vm::vm_core::VirtualMachine;
use num_traits::{Num, ToPrimitive};
use regex::Regex;
use serde::Deserialize;
use starknet_api::core::{ClassHash, ContractAddress, EntryPointSelector, PatriciaKey};
use starknet_api::deprecated_contract_class::EntryPointType;
Expand Down Expand Up @@ -365,23 +368,51 @@ fn execute_cheatcode_hint(
nonce,
..tx
}));
let tx_result = account_tx.execute(blockifier_state, block_context).unwrap();
let return_data = tx_result
.execute_call_info
.expect("Failed to get execution data from method")
.execution
.retdata;
let contract_address = return_data
.0
.get(0)
.expect("Failed to get contract_address from return_data");
let contract_address = Felt252::from_bytes_be(contract_address.bytes());

insert_value_to_cellref!(vm, deployed_contract_address, contract_address)?;
// todo in case of error, consider filling the panic data instead of packing in rust
insert_value_to_cellref!(vm, panic_data_start, Felt252::from(0))?;
insert_value_to_cellref!(vm, panic_data_end, Felt252::from(0))?;

match account_tx.execute(blockifier_state, block_context) {
Result::Ok(tx_info) => {
let return_data = tx_info
.execute_call_info
.expect("Failed to get execution data from method")
.execution
.retdata;
let contract_address = return_data
.0
.get(0)
.expect("Failed to get contract_address from return_data");
let contract_address = Felt252::from_bytes_be(contract_address.bytes());

insert_value_to_cellref!(vm, deployed_contract_address, contract_address)?;
insert_value_to_cellref!(vm, panic_data_start, Felt252::from(0))?;
insert_value_to_cellref!(vm, panic_data_end, Felt252::from(0))?;
}
Result::Err(e) => {
if let TransactionExecutionError::ExecutionError(
EntryPointExecutionError::VirtualMachineExecutionErrorWithTrace {
source,
trace,
},
) = e
{
let msg = source.to_string();
let unparseable_msg = format!("Deploy failed, full error message: {msg}");
let unparseable_msg = unparseable_msg.as_str();
Arcticae marked this conversation as resolved.
Show resolved Hide resolved

let extracted_panic_data =
try_extract_panic_data(&msg).expect(unparseable_msg);
let mut ptr = vm.add_memory_segment();
insert_value_to_cellref!(vm, panic_data_start, ptr)?;

for datum in extracted_panic_data {
insert_at_pointer(vm, &mut ptr, datum);
}

insert_value_to_cellref!(vm, deployed_contract_address, contract_address)?;
insert_value_to_cellref!(vm, panic_data_end, ptr)?;
Comment on lines +409 to +410
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.

} else {
panic!("Unparseable error message: {e:?}")
}
}
}
Ok(())
}
&ProtostarHint::Prepare { .. } => todo!(),
Expand Down Expand Up @@ -484,6 +515,28 @@ fn felt252_from_hex_string(value: &str) -> Result<Felt252> {
.map_err(|_| anyhow!("Failed to convert value = {value} to Felt252"))
}

fn felt_from_short_string(short_str: &str) -> Felt252 {
return Felt252::from_bytes_be(short_str.as_bytes());
}
Comment on lines +518 to +520
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


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
}
Comment on lines +522 to +538
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


#[cfg(test)]
mod test {
use super::*;
Expand Down
11 changes: 11 additions & 0 deletions protostar-rust/tests/data/deploy_error_handling_test/Scarb.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
[package]
name = "deploy_error_handling_test"
version = "0.1.0"

[[target.starknet-contract]]
sierra = true

[dependencies]
starknet = "2.0.0-rc2"

[tool.protostar]
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
#[starknet::contract]
mod PanickingConstructor {
use array::ArrayTrait;

#[storage]
struct Storage {}

#[constructor]
fn constructor(ref self: ContractState) {
let mut panic_data = ArrayTrait::new();
panic_data.append('PANIK');
panic_data.append('DEJTA');
Comment on lines +11 to +12
Copy link
Member

Choose a reason for hiding this comment

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

😆

panic(panic_data);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
use result::ResultTrait;
use protostar_print::PrintTrait;
use cheatcodes::RevertedTransactionTrait;
use cheatcodes::PreparedContract;
use array::ArrayTrait;

#[test]
fn test_deploy_error_handling() {
let class_hash = declare('PanickingConstructor').expect('Could not declare');
let prepared_contract = PreparedContract {
contract_address: 'addr',
class_hash: class_hash,
constructor_calldata: @ArrayTrait::new()
};

match deploy(prepared_contract) {
Result::Ok(_) => panic_with_felt252('Should have panicked'),
Result::Err(x) => {
assert(*x.panic_data.at(0_usize) == 'PANIK', *x.panic_data.at(0_usize));
assert(*x.panic_data.at(1_usize) == 'DEJTA', *x.panic_data.at(1_usize));
}
}
}
19 changes: 19 additions & 0 deletions protostar-rust/tests/e2e/running.rs
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,25 @@ fn exit_first_flag() {
"#});
}

#[test]
fn test_deploy_error_handling() {
let temp = assert_fs::TempDir::new().unwrap();
temp.copy_from("tests/data/deploy_error_handling_test", &["**/*"])
.unwrap();

runner()
.current_dir(&temp)
.assert()
.success()
.stdout_matches(indoc! { r#"
Collected 1 test(s) and 2 test file(s)
Running 0 test(s) from src/lib.cairo
Running 1 test(s) from tests/test_deploy_error_handling.cairo
[PASS] test_deploy_error_handling::test_deploy_error_handling::test_deploy_error_handling
Tests: 1 passed, 0 failed, 0 skipped
"#});
}

#[test]
fn dispatchers() {
let temp = assert_fs::TempDir::new().unwrap();
Expand Down