Bytes32 wrapping cleanup across codebase #9678
Conversation
Signed-off-by: Luis Pinto <[email protected]>
…instead of Bytes32 internally Signed-off-by: Luis Pinto <[email protected]>
lu-pinto
force-pushed
the
address-hash-using-bytes
branch
from
8688818 to
d4bdfac
Compare
There was a problem hiding this comment.
why were these deleted? were they ever used?
There was a problem hiding this comment.
I tracked it down using git blame, they were added initially for the Deposit functionality of Ethereum but I believe signature verification was later moved to the CL, if it was ever in the EL at some stage.
Right now we only get the RLP, validate that the fields are there and use them to compute the requests hash, nothing else. Look at DepositLogDecoder.
So we don't need these classes anymore.
| public static Hash wrap(final Bytes bytes) { | ||
| checkArgument( | ||
| bytes.size() == Bytes32.SIZE, | ||
| "A hash must be %s bytes long, got %s", |
There was a problem hiding this comment.
I feel like this could bite at runtime.
There was a problem hiding this comment.
IMO this will be optimized out, specially if we never trip it. If there's a way it doesn't we should make it easier for the compiler to do so.
| } | ||
|
|
||
| /** | ||
| * Wrap bytes to hash. |
There was a problem hiding this comment.
| * Wrap bytes to hash. The input must be 32 bytes in length. |
| @@ -86,7 +87,12 @@ | |||
| * @param bytes the bytes | |||
There was a problem hiding this comment.
| * @param bytes the bytes | |
| * @param bytes the bytes. Must be exactly 32 bytes. |
2 participants
PR description
This PR is a follow-up cleanup of PR #9556 to remove unnecessary
Bytes32usages where appropriate. I've observed that choice betweenBytes32andHashin code is not always appropriate so this helps clarify some parts of the code where we really meanHashand notBytes32.Both are 32 bytes in size, in reality
Hashis actually a keccak256 hashing function value which always outputs 32 byte.Other cleanups will follow but I cut this work here to make it reviewable.
Fixed Issue(s)
Thanks for sending a pull request! Have you done the following?
doc-change-requiredlabel to this PR if updates are required.Locally, you can run these tests to catch failures early:
./gradlew spotlessApply./gradlew build./gradlew acceptanceTest./gradlew integrationTest./gradlew ethereum:referenceTests:referenceTests