Feat: handle deprecated classes and modularize and optimize compile class build #331
Conversation
HermanObst
changed the title
crates/bin/prove_block/src/main.rs
Outdated
|
|
||
| let mut class_hash_to_compiled_class_hash = processed_state_update.class_hash_to_compiled_class_hash.clone(); | ||
| let class_hash_to_compiled_class_hash = processed_state_update.class_hash_to_compiled_class_hash.clone(); |
There was a problem hiding this comment.
You can remove .clone(), it's useless.
| @@ -29,8 +25,7 @@ pub struct FormattedStateUpdate { | |||
| pub(crate) async fn get_processed_state_update( | |||
There was a problem hiding this comment.
You renamed the struct, you can also rename the function to format_state_update.
| }) | ||
| .collect(); | ||
| address_to_nonce | ||
| async fn build_compiled_class_and_maybe_update_class_hash_to_compiled_class_hash( |
There was a problem hiding this comment.
This function deserves to be documented.
| GenericSierraContractClass::from(flattened_sierra_cc) | ||
| } | ||
| starknet::core::types::ContractClass::Legacy(_) => { | ||
| unimplemented!("Fixme: Support legacy contract class") |
There was a problem hiding this comment.
There's no Sierra notion of Sierra class for Cairo0. The compiled class is the contract class. I think the cleanest solution here will be to return a tuple of (compiled_contract_classes, deprecated_compiled_contract_classes).
There was a problem hiding this comment.
This needs to be resolved before merging the PR.
| accessed_addresses: &HashSet<Felt252>, | ||
| address_to_class_hash: &HashMap<Felt252, Felt252>, | ||
| class_hash_to_compiled_class_hash: &mut HashMap<Felt252, Felt252>, | ||
| ) -> Result<HashMap<Felt252, GenericCasmContractClass>, Error> { |
There was a problem hiding this comment.
Now that we moved to the latest Blockifier, don't hesitate to use types like CompiledClassHash, ClassHash etc. This will make the code cleaner and we can get the felt inside by accessing the .0 field.
| for contract_address in accessed_addresses { | ||
| let class_hash = match address_to_class_hash.get(contract_address) { | ||
| Some(class_hash) => class_hash, | ||
| None => &provider.get_class_hash_at(block_id, contract_address).await?, |
There was a problem hiding this comment.
Note that with anyhow, you can add with_context("<some explanation>") to any Result value so that you get a human-readable message + details on failure.
HermanObst
force-pushed
the
refactor-prove-block
branch
from
481ee6e to
4fd6e23
Compare
HermanObst
changed the title
HermanObst
changed the title
crates/bin/prove_block/src/lib.rs
Outdated
| @@ -63,6 +64,9 @@ impl fmt::Display for ProveBlockError { | |||
| ProveBlockError::TreeError(err) => write!(f, "Tree Error: {}", err), | |||
| ProveBlockError::ContractClassError(err) => write!(f, "Contract Class Error: {}", err), | |||
| ProveBlockError::SnOsError(err) => write!(f, "SnOs Error: {}", err), | |||
| ProveBlockError::LegacyContractDecompressionError(err) => { | |||
| write!(f, "Legacy class decompression Error: {}", err) | |||
There was a problem hiding this comment.
I missed this enum in my review of @whichqua 's PR but we should definitely derive thiserror::Error for ProveBlockError instead of implementing this manually.
| let class_hash_to_compiled_class_hash: HashMap<Felt252, Felt252> = format_declared_classes(&state_diff); | ||
| let storage_updates: HashMap<Felt252, HashMap<Felt252, Felt252>> = format_storage_updates(&state_diff); | ||
| let address_to_class_hash = format_deployed_contracts(&state_diff); | ||
| // TODO: Handle deprecated clasees |
There was a problem hiding this comment.
| // TODO: Handle deprecated clasees | |
| // TODO: Handle deprecated classes |
|
|
||
| // Define your error enum | ||
| #[derive(Debug)] | ||
| #[derive(Debug, Error)] |
There was a problem hiding this comment.
Nitpick: there are so many Error traits, specifying that you're deriving thiserror::Error clarifies things.
Description
Problem:
Solution:
Modularize and use cache