Feature: handle all tx traces #330
Conversation
ftheirs
force-pushed
the
ft/handle_all_tx_traces
branch
3 times, most recently
from
c87d216 to
618d446
Compare
crates/bin/prove_block/src/utils.rs
Outdated
| @@ -24,13 +22,35 @@ pub(crate) fn get_subcalled_contracts_from_tx_traces(traces: &[TransactionTraceW | |||
| ExecuteInvocation::Success(inv) => { | |||
There was a problem hiding this comment.
If you only need the Success variant, consider using if let instead of match.
There was a problem hiding this comment.
@ftheirs please modularize all the different tx with each version.
see
starknet_rs_tx_to_internal_tx
| sn_core_tx: &starknet::core::types::Transaction, | ||
| provider: &JsonRpcClient<HttpTransport>, | ||
| block_number: u64, | ||
| ) -> Result<blockifier::transaction::transaction_execution::Transaction, Box<dyn Error>> { |
There was a problem hiding this comment.
For another PR, but we really need to return something better than Box<dyn Error> here.
There was a problem hiding this comment.
FYI The PR in #333 includes a ProveBlockError type which I was unable to avoid when converting to a function.
| }; | ||
|
|
||
| // TODO: improve this to avoid retrieving this twice. Already done in main.rs from prove_block | ||
| let starknet_contract_class = provider.get_class(BlockId::Number(block_number), class_hash).await?; |
There was a problem hiding this comment.
Could you put the ClassInfo generation in a new function?
| }; | ||
|
|
||
| let flattened_sierra = generic_sierra_cc.clone().to_starknet_core_contract_class()?; | ||
| let contract_class = generic_sierra_cc.compile()?.get_blockifier_contract_class()?.clone(); |
There was a problem hiding this comment.
There's (or at least should be) a to_blockifier_contract_class method that hides the clone.
|
|
||
| // TODO: improve this to avoid retrieving this twice. Already done in main.rs from prove_block | ||
| let starknet_contract_class = provider.get_class(BlockId::Number(block_number), class_hash).await?; | ||
| let generic_sierra_cc = match starknet_contract_class { |
There was a problem hiding this comment.
To deal with the legacy/deprecated/Cairo0 (these are synonyms) contracts, you simply need to move the compilation code + conversion to Blockifier ContractClass here and do the appropriate thing for Cairo0 in the other arm of the match. Please resolve it in this PR.
| } | ||
| Transaction::L1Handler(_tx) => { | ||
| unimplemented!("starknet_rs_tx_to_blockifier() with L1Handler txn"); | ||
| Transaction::L1Handler(tx) => { |
There was a problem hiding this comment.
As @HermanObst already suggested, please split every arm of this match into separate functions.
| }; | ||
|
|
||
| // TODO: Check Fee value | ||
| let fee = Fee(0); |
There was a problem hiding this comment.
Looking at the explorer, this seems to be correct. Replace the TODO by a comment saying that fees are 0 for L1 handler txs.
ftheirs
force-pushed
the
ft/handle_all_tx_traces
branch
2 times, most recently
from
b09e460 to
cb60274
Compare
There was a problem hiding this comment.
@ftheirs good job man!
one last comment: remove all the unwrap() that you are using in favor to correctly handle the errors.
In general, we never accept unwrap() as they can make the program panic, basically bypassing Rust safety.
We only accept if there is a comment, clearly explaining why a given unwrap() will never fail. But handling the error is better practice.
Sorry that I didnt catch it before
ftheirs
force-pushed
the
ft/handle_all_tx_traces
branch
from
d1dc4ee to
cfd80a1
Compare
ftheirs
force-pushed
the
ft/handle_all_tx_traces
branch
from
cfd80a1 to
c9627f5
Compare
|
Closing this since it will be split in several PRs |
Problem: at the moment only Invoke transactions were supported
Solution: process traces and add type conversion for reverted Invoke transaction, DeployAccount, Declare and L1Handler
Issue Number: N/A
Type
Description
Breaking changes?