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

Fix WARNING: Missing strong random number source #309

Closed
camsjams opened this issue May 31, 2022 · 8 comments · Fixed by #310
Closed

Fix WARNING: Missing strong random number source #309

camsjams opened this issue May 31, 2022 · 8 comments · Fixed by #310
Assignees

Comments

@camsjams
Copy link
Contributor

camsjams commented May 31, 2022

This looks to have been introduced with the updated build process, as the dist files generated by wallet are optimized for the browser explicitly.

The best fix would be to use the internal universal crypto -> random bytes: https://github.com/FuelLabs/fuels-ts/blob/master/packages/keystore/src/universal-crypto.ts

@AlicanC
Copy link
Contributor

AlicanC commented May 31, 2022

This looks to have been introduced with the updated build process

What do you mean by "the updated build process"?

@camsjams
Copy link
Contributor Author

From what I can tell, using ESM modules on tsup triggered a "browser" version of the ethersproject random package, causing this warning message to appear: ethers-io/ethers.js#556.

When running locally, if you look at the dist file generated inside the wallet package for instance, it shows that its using the randomBytes function from the wrong source.

@camsjams
Copy link
Contributor Author

At fuels@0.6.0 the build command was just a TypeScript compile tsc, but with tsup there is a format option esm that seems to have caused this ESM processing issue.

These are just my inference, I can roll back to that state and see for sure, but with this PR the warning goes away and another ethers project dependency is removed.

@AlicanC
Copy link
Contributor

AlicanC commented May 31, 2022

I can roll back to that state and see for sure

I was just curious if you meant the changes on #238.

another ethers project dependency is removed

To be clear, I believe @ethersproject/random is ok. It seems safer to source this functionality from Ethers which is battle-tested and also tested on browsers and even on React Native: https://github.com/ethers-io/ethers.js/blob/master/package.json#L32-L40

When I was agreeing with the "getting rid of Ethers" discussion, I was meaning the parts that aren't suitable for us, which are just '@ethersproject/abi' and '@ethersproject/logger' if I'm not missing anything.

@camsjams
Copy link
Contributor Author

Thank you for the context - yes I think the @ethersproject/random is ok too

It seems safer to source this functionality from Ethers which is battle-tested

This internal one from keystore is using the native Browser and Node randomBytes functions, so there is some safety net there as well, albeit not directly in the CI build process.

When I was agreeing with the "getting rid of Ethers" discussion, I was meaning the parts that aren't suitable for us

I don't feel the need to replace all references to ethersproject either, totally agree!

@camsjams
Copy link
Contributor Author

Alternatively this can be attacked on a different PR looking into tweaking build process, but I'd be worried about breaking things for the browser instead.

@QuinnLee
Copy link
Contributor

QuinnLee commented Jun 1, 2022

Just to recap:
ESM can be for either node or browser environments and the ESM version of ethers uses the browser version.

Could we try doing something like setting the default to be CJS and the browser to default to ESM?

@camsjams
Copy link
Contributor Author

camsjams commented Jun 1, 2022

the ESM version of ethers uses the browser version.

Yes - correct. I will try another route on a separate branch aimed at adjusting the build configs for better Node compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants