feat: da verifier #129
feat: da verifier #129
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
|
|
||
| // TODO: Error naming & other naming: to enum? | ||
| // I am not sure what is better practice, so far we have been using mostly |
There was a problem hiding this comment.
I made this comment because the error names were so long they wouldn't fit into a felt string. But it is probably better to stick to const felts and use abbreviations, like how it is set up now.
| // TODO: data_root_tuple -> data_root & data_root_tuple_root -> data_root? | ||
| // not sure what you mean here |
There was a problem hiding this comment.
This referred to changing the corresponding names to make errors and function names shorter. I think it is fine to leave it as it is now, though.
| //TODO: to u32? | ||
| let mut number_of_shares_in_proofs: u256 = 0; | ||
| // TODO: to u32? | ||
| // currently max square size is 128 => extended square size 256 |
There was a problem hiding this comment.
Thanks for looking into this! Changing the key fields also would be best ( I can do this ); fewer felts == better.
| @@ -130,6 +179,10 @@ mod DAVerifier { | |||
| ) -> (bool, felt252) { | |||
| // check that the data root was commited to by the Blobstream smart contract | |||
| // TODO: safe unwrap? | |||
| // a choice was made to use a u64 instead of a u256 (from the solidity contract) | |||
| // for the `state_proof_nonce` in the blobstreamx contract | |||
| // I suppose the `commit_nonce` field should actually be a u64 then which would | |||
| // currently max square size is 128 => extended square size is 256 | ||
| // max number of side nodes in a a binary tree with 256 leaves is log2(256) = 8 | ||
| let mut i: u32 = 0; | ||
| // same for `extended_square_row_size`, but defining them as u32 may provide some |
There was a problem hiding this comment.
Defining them as u32 is probably best because it provides extra room, as you mentioned, and makes a comparison with len() easier.
Also, there aren't really any performance improvements there since both u8 and u32 are interpreted as 1 felt on the back end.
b-j-roberts
merged commit ab995e5
into
keep-starknet-strange:feat/da-verifier
* Initial work for DA verifier and rollup inclusion proof test * cleanup todos * Scarb fmt * feat: da verifier (#129) * Added doc * test_verifier WIP * test_verifier done * Update * Added doc * test_rollup_inclusion_proofs WIP * test_rollup_inclusion_proofs done * TODOs comments * Scarb fmt * Fix types on indexes and things to reduce number of felts needed, cleaned up TODOs * Use sol_abi encode_packed --------- Co-authored-by: 0xK2 <[email protected]>
Hey, I have implemented
test_rollup_inclusion_proofsandtest_da_verifier.test_rollup_inclusion_proofs::test_invalid_datais ignored because it exceeds default max n steps and needs to be run with--max-n-stepsflag.I also made a few changes to
da_verifier. This is 95% done, I left comments below the TODOs for you to review. I will make necessary changes afterwards.