make get_hash return hex #257
Conversation
src/nile/common.py
Outdated
| @@ -197,4 +197,6 @@ def get_contract_class(contract_name, overriding_path=None): | |||
| def get_hash(contract_name, overriding_path=None): | |||
| """Return the class_hash for a given contract name.""" | |||
| contract_class = get_contract_class(contract_name, overriding_path) | |||
| return compute_class_hash(contract_class=contract_class, hash_func=pedersen_hash) | |||
| return hex( | |||
There was a problem hiding this comment.
I think we must pad the class_hash, similar to what we do with addresses. We have an hex_address method in utils, we can add another one like hex_class_hash. (If we don't pad, short class hashes because of leading 0s, could be as confusing as short addresses because of leading 0s).
There was a problem hiding this comment.
agree, i considered it but wasn't sure
There was a problem hiding this comment.
If we pad get_hash, we should update declare's class hash registration for consistency because it's storing the unpadded hex. Also, @ericglau will probably need to adjust one of the hash values in declare_impl: https://github.com/OpenZeppelin/openzeppelin-nile-upgrades/blob/main/src/nile_upgrades/declare_impl.py#L18
There was a problem hiding this comment.
i insist in killing the declarations file. it serves no purpose and it's actually annoying when your declare tx fails.
There was a problem hiding this comment.
That also resolves the inconsistency :)
There was a problem hiding this comment.
@ericnordelo how long class hashes should be? 67?
There was a problem hiding this comment.
65 ("0x" + 63 hex) is enough because the length is 250 bits (to fit in a felt). But maybe we should pad to 66 ("0x" + 64), even if we always add a 0 at the beginning (after the 0x) because 32 bytes is a standard in Ethereum? (just thinking out loud, I don't have a strong case for any option).
Starkscan uses 66 (pad with extra 0), while voyager uses 65 (without the leading 0).
There was a problem hiding this comment.
i wonder why they pad to 64. how are addresses 64 then? that shouldn't fit a felt
|
|
||
|
|
||
| def hex_class_hash(number): | ||
| """Return the 65 hexadecimal characters length class hash.""" |
There was a problem hiding this comment.
Either change the number here to 63 or change the comment in hex_address's number to 66 to keep consistency (I didn't consider the "0x" prefix as part of the length of the hexadecimal characters when I added the comment).
There was a problem hiding this comment.
I think the comment is misleading, it should say the full length, including the 0x. therefore this comment should say 65, and hex_address's 66
fixes #258