Fix contract class unmarshall for modern cairo compilers #656
Conversation
* added contract casm compiled by scarn 2.7.0 compiler * added tests to unmarshall contract class from new casm json format
…compilers * Added bytecode_segment_lengths field to CasmClass, allowing for backward compatibility * Added NestedUInts type to handle deserialisation of CasmClass * Added test to check that ClassHash now respects bytecode_segment_lengths and calculates correct value
… of Declare transation * Simplify nested string
There was a problem hiding this comment.
Thanks for your help, @baitcode!
I left some comments, mostly about testing
| return hash.CalculateTransactionHashCommon( | ||
| PREFIX_DECLARE, | ||
| txnVersionFelt, | ||
| txn.SenderAddress, | ||
| &felt.Zero, | ||
| utils.Uint64ToFelt(0), |
There was a problem hiding this comment.
We need to make tests to check the accuracy of this hash, taking examples and results from trusted sources
There was a problem hiding this comment.
I don't quite understand. I can assure that I created contract myself, compiled it, calculated hash using starknet.py and it didn't match.
There was a problem hiding this comment.
What I meant was that we needed to test this change, but I had forgotten about existing tests like 'TestTransactionHashDeclare'. Sorry.
But about this change, the felt.Zero indeed is equal to 0 as felt. I ran 'TestTransactionHashDeclare' here and it worked the same for both '&felt.Zero' and 'utils.Uint64ToFelt(0)'.
What could have happened is that you changed the value of felt.Zero at some point. felt.Zero is just a pointer to a felt var containing the zero value, and it's used across the entire code as a reference. If you need a zero felt just do new(felt.Felt) instead.
There was a problem hiding this comment.
In the func getByteCodeSegmentHasher, at line 198, you change the value of felt.Zero. Change it to new(felt.Felt) instead
contracts/contracts_test.go
Outdated
| // Returns: | ||
| // | ||
| // none | ||
| func TestUnmarshalContractClassWithNonStringAbi(t *testing.T) { |
There was a problem hiding this comment.
No need to a new test, let's just include this test case in the "TestUnmarshalContractClass" test and leave a comment saying it's a "...contract class made with compiler >= 2.7.0 version..."
There was a problem hiding this comment.
We need to test outputs from the new implementation of the CompiledClassHash. Let's take examples from trusted sources
Ex: starknet.py, cairo-lang
Or we could make new contracts and use these libraries to get the hash, and then compare them with the ones generated by starknet.go
There was a problem hiding this comment.
I used starknet.py on a contract I've created myself. I can use argent contracts for example.
There was a problem hiding this comment.
Great. No need for the argent contract then, please remove it.
Please send here evidence of your test with starknet.py (screenshot, the code itself,...etc)
There was a problem hiding this comment.
Added argent 0.4.0 contract. Fixed review comments
|
@thiagodeev Fixed! Added argent account contract v0.4.0 to hash tests. |
Fixes: #655
Added workaround for abi data that is not string, but json object.
Fix ClassHash calculation for casm compiled classes for modern cairo compilers
Here is the snippet for hash values generation with starknet.py:
https://gist.github.com/baitcode/5adec0f74121d0ae3a09aaf573ff8665