Skip to content

Conversation

krpeacock
Copy link
Contributor

@krpeacock krpeacock commented Aug 1, 2023

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:

  • My changes follow the guidelines in CONTRIBUTING.md.
  • The title of this PR complies with Conventional Commits.
  • I have edited the CHANGELOG accordingly.
  • I have made corresponding changes to the documentation.

@krpeacock krpeacock requested a review from a team as a code owner August 1, 2023 22:54
@krpeacock krpeacock changed the title Kyle/sdk 1194 webcrypto nonce feat: crypto nonce randomness Aug 1, 2023
@krpeacock
Copy link
Contributor Author

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 @@
/**
Copy link
Contributor

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);
};

Copy link
Contributor Author

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.

  1. In the interest of keeping package size small, we try to avoid introducing new dependencies, particularly in the @dfinity/agent package.

  2. 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:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Copy link
Contributor Author

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

@github-actions
Copy link
Contributor

github-actions bot commented Aug 2, 2023

size-limit report 📦

Path Size
@dfinity/agent 89.2 KB (+0.09% 🔺)
@dfinity/candid 15.48 KB (0%)
@dfinity/principal 6.77 KB (0%)
@dfinity/auth-client 94.89 KB (0%)
@dfinity/assets 91.7 KB (0%)
@dfinity/identity 91.96 KB (0%)
@dfinity/identity-secp256k1 230.53 KB (+0.05% 🔺)

Co-authored-by: Eric Swanson <64809312+ericswanson-dfinity@users.noreply.github.com>
@krpeacock krpeacock enabled auto-merge (squash) August 2, 2023 19:51
@krpeacock krpeacock merged commit cef6335 into main Aug 2, 2023
@krpeacock krpeacock deleted the kyle/SDK-1194-webcrypto-nonce branch August 2, 2023 19:59
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.

2 participants