-
Notifications
You must be signed in to change notification settings - Fork 88
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
feat: crypto nonce randomness #747
Conversation
I was aiming initially to reproduce the behavior of Math.random, but I agree that yielding a random integer is preferable. I'll simplify, using your recommendations |
@@ -0,0 +1,30 @@ | |||
/** |
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.
Could you write some rationale here for why we are not requiring some well-established crypto library and always using it here?
import { randomBytes } from 'crypto';
export const randomNumber = (): number => {
return randomBytes(4).readUInt32BE(0);
};
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.
There are two goals we are satisfying we are taking this approach.
-
In the interest of keeping package size small, we try to avoid introducing new dependencies, particularly in the @dfinity/agent package.
-
The native Node.js and Browser crypto libraries are well-established in their own ecosystem. While this approach does require us to inspect the global context to determine which tool is available, it does mean that we can tap into more powerful randomness APIs than are available in any pure JavaScript implementation
See documentation below:
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.
Thanks!
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.
neither API is universally available (webcrypto is available in some but not all versions of Node we support), so using whichever we can detect is a compromise
size-limit report 📦
|
Co-authored-by: Eric Swanson <64809312+ericswanson-dfinity@users.noreply.github.com>
Description
Currently default nonce generation is using Math.random, a pseudorandom source of randomness. We can take advantage of better tools when they are available
Fixes SDK-1194
How Has This Been Tested?
Updated and new unit tests
Checklist: