Add class hash check for declarev2 #819
Conversation
Codecov Report
@@ Coverage Diff @@
## main #819 +/- ##
==========================================
+ Coverage 91.72% 91.74% +0.01%
==========================================
Files 54 54
Lines 12503 12530 +27
==========================================
+ Hits 11469 11496 +27
Misses 1034 1034
|
| let casm_class_hash = compute_casm_class_hash(casm_class)?; | ||
| if casm_class_hash != self.compiled_class_hash { |
There was a problem hiding this comment.
Does it make sense to always have this check? Maybe we could have it as an optional feature, or add it only for debugging.
There was a problem hiding this comment.
No, it's not necessary to always check this. We want to check this whenever a casm contract class and casm class hash is sent from the outside, because otherwise we couldn't guarantee that the compiled contract is really the corresponded to that sierra contract class. It wouldn't be a problem if the contract was already compiled on our side. I'll work on that next, but preferred to have this implemented first.
There was a problem hiding this comment.
I'm in favor of making it a debug assertion. Either we computed it and they are the same by construction, or it was passed via one of the unchecked functions and the responsibility lies in the caller. The point of that was not having to compute the hash in the first place.
Unless I'm misunderstanding it, of course.
There was a problem hiding this comment.
Another option may be to don't compile and check at all, and take the casm contract class and class hash as valid. But sound pretty insecure. We should think a better way of guarantee that.
There was a problem hiding this comment.
It is insecure, but that's the compromise made for performance and that's why we have to be extra explicit and opt-in. If you hold it wrong, it breaks.
The only way to guarantee correctness is to compute stuff ourselves, but then we only moved the costs and increased complexity for no gain for the user. If we're gonna check, just compute it on construction and never again.
There was a problem hiding this comment.
Think of it like using unsafe in Rust. The keyword doesn't mean you can do whatever you like, but that you go to battle without the shield, it's your job to dodge the blows now. When the programmer opts for passing the hash themselves, it's their job to make sure it's the correct one, and if they get hurt they'll deal with it.
Co-authored-by: Mario Rugiero <mrugiero@gmail.com>
Co-authored-by: Estéfano Bargas <estefano.bargas@fing.edu.uy>
Co-authored-by: Estéfano Bargas <estefano.bargas@fing.edu.uy>
Description
When compiling the casm class in the DeclareV2 we now check if the received casm_class_hash was correct.
Made on top of #814 (DO NOT MERGE UNTIL GET THAT MERGED)closes #791
Checklist