-
Notifications
You must be signed in to change notification settings - Fork 199
Remove swarm hash from bytecode before hashing it #105
Conversation
packages/cli/src/utils/contracts.js
Outdated
} | ||
|
||
function removeSwarmHash(bytecode) { | ||
return bytecode.replace(/a165627a7a72305820.*0029$/, '') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would you mind adding a comment to explain this chunk of bytecode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good @jcarpanelli! Added a minor comment. Regarding the flag, let's not add optional parameters that we don't yet need: if we reach a point where we need to hash stuff without removing the swarm hash, then we can add it.
And last but not least: please add a unit test for this (even though I'm afraid there was none previously).
packages/cli/src/utils/contracts.js
Outdated
// respects the following structure: 0xa1 0x65 'b' 'z' 'z' 'r' '0' 0x58 0x20 <32 bytes swarm hash> 0x00 0x29 | ||
// (see https://solidity.readthedocs.io/en/v0.4.24/metadata.html#encoding-of-the-metadata-hash-in-the-bytecode) | ||
function removeSwarmHash(bytecode) { | ||
return bytecode.replace(/a165627a7a72305820.*0029$/, '') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's assert that the swarm hash is indeed 32 bytes. We can replace .*
in the regex for [a-fA-F0-9]{64}
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it'd be interesting to fail if the replace operation does not match anything to replace, in case we stumble upon a contract with a different swarm hash format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spalladino what do you mean by "fail"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
already discussed it offline.
* Remove swarm hash from bytecode before hashing it * Add a comment explaining the removeSwarmHash fn * Rename removeSwarmHash function and add tests
* Remove swarm hash from bytecode before hashing it * Add a comment explaining the removeSwarmHash fn * Rename removeSwarmHash function and add tests
Fixes #38.
@spalladino should we add an option parameter to the
bytecodeDigest
function, e.g.,{ removeSwarmHash: true/false }
to decide whether to remove it or not?