fix: account proof storage key with leading 0s#12980
fix: account proof storage key with leading 0s#12980protolambda merged 7 commits intoethereum-optimism:developfrom
Conversation
|
/ci authorize f97e71c |
ajsutton
left a comment
There was a problem hiding this comment.
LGTM assuming CI passes.
The encoding of key in geth is "a bit special", encoding as a hash with leading zeros if it is 32 bytes or as a big without leading 0s if not: https://github.com/ethereum-optimism/op-geth/blob/ee3d19b4cba58e6f45f3d4aee4535b572d655882/internal/ethapi/api.go#L774-L778
The execution-apis spec which is unfortunately hard to link to defines it as a string and allows leading 0s (as opposed to value which explicitly forbids them)
|
@meyer9 CI didn't pass - looks like another place that needs updating for the new type: |
|
ah sorry, not sure how that wasn't caught before. Should be fixed now! |
|
/ci authorize 3c88c8e |
|
Sorry for the delay, I just checked out the error and it seems like it still had trouble handling the case where there were an odd number of hex digits even with I think we have to introduce a new type here "StorageKey" or serialize it as a string. I added the new type and tests since I think deserializing from string every time we want to use the value would be annoying. Let me know what you think! |
|
/ci authorize 1186238 |
ajsutton
left a comment
There was a problem hiding this comment.
LGTM - just a stray println to remove.
|
/ci authorize 5994e1d |
protolambda
left a comment
There was a problem hiding this comment.
Looks good, but recommend to use the text marshaler interface instead of JSON-specific interface. The JSON lib will use it, but this way the type can be reused in other contexts, e.g. TOML or YAML.
|
/ci authorize f82102d |
|
rebased! |
f82102d to
4a0925a
Compare
|
/ci authorize 4a0925a |
* fix: account proof storage key with leading 0s * Add test for leading 0s * Remove extra whitespace * Fix one more usage of key * Use new storage key type instead of hexutil * remove debugging statement * use TextMarshaler instead of json.Marshaler
* fix: account proof storage key with leading 0s * Add test for leading 0s * Remove extra whitespace * Fix one more usage of key * Use new storage key type instead of hexutil * remove debugging statement * use TextMarshaler instead of json.Marshaler
* fix: account proof storage key with leading 0s * Add test for leading 0s * Remove extra whitespace * Fix one more usage of key * Use new storage key type instead of hexutil * remove debugging statement * use TextMarshaler instead of json.Marshaler
Description
hexutil.Bigerrors decoding if there are leading 0s, andcommon.Hasherrors decoding if there aren't exactly 32 bytes of hex.This switches to
hexutil.Bytesso hashes with leading 0s can be serialized.Tests
Added test for leading 0s.