-
Notifications
You must be signed in to change notification settings - Fork 290
feat(root): Adding state commitments computation using the bonsai lib #1611
Conversation
commitment_tx.expect("Failed to calculate transaction commitment"), | ||
commitment_event.expect("Failed to calculate event commitment"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
panic? Why no error handling?
where | ||
H: HasherT, | ||
{ | ||
let starknet_state_prefix = Felt252Wrapper::try_from("STARKNET_STATE_V0".as_bytes()).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make it a constant
if classes_trie_root == Felt252Wrapper::ZERO { | ||
contracts_trie_root |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment explaining why?
Is this behavior defined in the Starknet specs? If so can you add a link to it?
/// # Arguments | ||
/// | ||
/// * `CommitmentStateDiff` - The commitment state diff inducing unprocessed state changes. | ||
/// * `BonsaiDb` - The database responsible for storing computing the state tries. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong arguments
/// | ||
/// | ||
/// The updated state root as a `Felt252Wrapper`. | ||
pub fn update_state_root(csd: CommitmentStateDiff, block_number: u64) -> Felt252Wrapper { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look like it's updating anything. It just computes and returns.
We should probably change the method name
where | ||
H: HasherT, | ||
{ | ||
let include_signature = block_number >= 61394; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does not make sense in madara
// Include signatures for Invoke transactions or for all transactions | ||
let signature = invoke_tx.signature(); | ||
|
||
H::compute_hash_on_elements( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can change the signature of HasherT
so that it takes an interator as input.
I will open an issue
&signature.0.iter().map(|x| Felt252Wrapper::from(*x).into()).collect::<Vec<FieldElement>>(), | ||
) | ||
} else { | ||
H::compute_hash_on_elements(&[]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you define this value somewhere as a const?
Rather than computing it again every time.
Even if you have to add it as a const
in the HasherT
trait
let txs = transactions | ||
.par_iter() | ||
.map(|tx| calculate_transaction_hash_with_signature::<PedersenHasher>(tx, chain_id, block_number)) | ||
.collect::<Vec<_>>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to collect as you call into_iter
just after
bonsai_storage.commit(id).expect("Failed to commit to bonsai storage"); | ||
let root_hash = bonsai_storage.root_hash(identifier).expect("Failed to get root hash"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
panic
There hasn't been any activity on this pull request recently, and in order to prioritize active work, it has been marked as stale. |
repository archived in favor of https://github.com/madara-alliance/madara |
draft pr to implement commtiments