Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

web3.utils.sha3 and web3.utils.soliditySha3 return null when an empty byte sequence is hashed #1961

Closed
smarx opened this issue Sep 20, 2018 · 5 comments · Fixed by #3226
Closed
Labels
1.x 1.0 related issues 2.x 2.0 related issues Bug Addressing a bug

Comments

@smarx
Copy link

smarx commented Sep 20, 2018

Based on the code, this appears to be done intentionally for some reason, but it seems like a very bad idea to return the wrong answer. There's also no mention of this non-standard behavior in the documentation.

From https://github.com/ethereum/web3.js/blob/1.0/packages/web3-utils/src/utils.js#L434,L435:

var SHA3_NULL_S = '0xc5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470';
...
    if(returnValue === SHA3_NULL_S) {
        return null;

Removing those lines would fix the problem.

@smarx smarx changed the title web3.utils.sha3 and web3.utils.soliditySha3 return null when an empty string is hashed web3.utils.sha3 and web3.utils.soliditySha3 return null when an empty byte sequence is hashed Sep 20, 2018
@nivida nivida added the Needs Clarification Requires additional input label Oct 31, 2018
@jefflau
Copy link

jefflau commented Feb 27, 2019

I've had this issue crop up in ENS as well.

@nivida nivida added Bug Addressing a bug and removed Needs Clarification Requires additional input labels Apr 16, 2019
@nivida nivida added 1.x 1.0 related issues 2.x 2.0 related issues labels Jun 20, 2019
@nivida
Copy link
Contributor

nivida commented Oct 31, 2019

I would propose to add an additional parameter called raw_null which would be of type boolean and does configure the function to return the real hash value of it instead of the mapped null value.

@cgewecke @smarx @jefflau do you agree with this solution?

@smarx
Copy link
Author

smarx commented Oct 31, 2019

I would much rather see it return the actual hash by default, but it's better than nothing.

I imagine the argument against fixing this is concern about backward compatibility... maybe it's possible to deprecate the old function and create a new one that has the fix by default?

@nivida
Copy link
Contributor

nivida commented Nov 20, 2019

I imagine the argument against fixing this is concern about backward compatibility... maybe it's possible to deprecate the old function and create a new one that has the fix by default?

Yes, this is correct. I think adding new functions with close to the same behavior will lead to confusion by most developers. It's better to just add a configuration property for now and to apply the breaking change later with a minor release.

@nivida
Copy link
Contributor

nivida commented Nov 20, 2019

@smarx I've just applied this change to the utils.sha3 method which does work but because of the dynamic parameter detection of the utils.soliditySha3 method do we actually have to create new functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.x 1.0 related issues 2.x 2.0 related issues Bug Addressing a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants