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

Refactor parser utility to use keccak hash from ethers.js #103

Closed
wants to merge 1 commit into from

Conversation

hzhu
Copy link

@hzhu hzhu commented Aug 29, 2022

Problem

When signing a message in the browser for certain frontend frameworks, a reference error keccak.js:41 Uncaught ReferenceError: Buffer is not defined in the browser console is thrown. This happens because the keccak package uses a global Node API (Buffer) that isn't available in the browser.

Frameworks

Next.js

This isn't a problem in Next.js because Next.js shims with a browser implementation of Buffer. SIWE can be implemented with no problems.

Remix

Remix prefers not to shim Node globals into the browser. As a result, using keccak produces an ReferenceError for Buffer and requires developers to hack around in order to get SIWE to work.

Solution

This pull request refactors the isEIP55Address utility to compute the keccak256 hash using the @ethersproject/hash package which doesn't depend on Node globals. It also removes the keccak peer dependency because it is no longer used. There should be no changes to the current behavior.

The isEIP55Address utility now works in both browser and Node environments ✅ .

@tfalencar
Copy link
Contributor

tfalencar commented Aug 31, 2022

I'm having issues using SIWE library from SvelteKit, and I believe this PR fixes the root cause. @skgbafa @sbihel could you please have a look at this? maybe it can still go in with the next release? This problem is unfortunately blocking usage of SIWE in my dapp. Thanks!

@sbihel
Copy link
Member

sbihel commented Aug 31, 2022

Hiya, thank you for raising this issue/pr. I'm afraid this solution is not ideal as the reason why we separated this library into two packages was to have a parser that did not rely on large libraries such as ethers. The appropriate solution is still to be decided.

@tfalencar
Copy link
Contributor

Hiya, thank you for raising this issue/pr. I'm afraid this solution is not ideal as the reason why we separated this library into two packages was to have a parser that did not rely on large libraries such as ethers. The appropriate solution is still to be decided.

Then how about using noble-hashes instead? I think it won't get more minimalistic than that library..

@sbihel
Copy link
Member

sbihel commented Aug 31, 2022

That seems reasonable

@tfalencar
Copy link
Contributor

tfalencar commented Aug 31, 2022

@sbihel I've create a PR for this. Please let me know what you think: #107

@w4ll3
Copy link
Member

w4ll3 commented Aug 31, 2022

Closing this one in favor of #107

@w4ll3 w4ll3 closed this Aug 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants