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

feat: Add cryptorand package for random string and number generation #32

Merged
merged 3 commits into from
Jan 19, 2022

Conversation

kylecarbs
Copy link
Member

This package is taken from the monorepo, and was renamed from crand
for improved clarity. It will be used for API key generation.

This package is taken from the monorepo, and was renamed from crand
for improved clarity. It will be used for API key generation.
@kylecarbs kylecarbs self-assigned this Jan 19, 2022
@codecov
Copy link

codecov bot commented Jan 19, 2022

Codecov Report

Merging #32 (1c4b6e3) into main (4dc6e35) will decrease coverage by 1.56%.
The diff coverage is 49.30%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #32      +/-   ##
==========================================
- Coverage   71.01%   69.44%   -1.57%     
==========================================
  Files          18       40      +22     
  Lines         138     1630    +1492     
  Branches        7        7              
==========================================
+ Hits           98     1132    +1034     
- Misses         36      394     +358     
- Partials        4      104     +100     
Flag Coverage Δ
unittest-go-macos-latest 63.31% <49.30%> (?)
unittest-go-ubuntu-latest 69.10% <49.30%> (?)
unittest-go-windows-latest 63.02% <49.30%> (?)
unittest-js 71.01% <ø> (ø)
Impacted Files Coverage Δ
cryptorand/numbers.go 40.70% <40.70%> (ø)
cryptorand/strings.go 80.64% <80.64%> (ø)
database/models.go 0.00% <0.00%> (ø)
provisionersdk/transport.go 72.22% <0.00%> (ø)
database/query.sql.go 0.00% <0.00%> (ø)
peerbroker/dial.go 77.27% <0.00%> (ø)
peer/channel.go 88.05% <0.00%> (ø)
peerbroker/listen.go 80.62% <0.00%> (ø)
site/embed.go 70.32% <0.00%> (ø)
database/postgres/postgres.go 72.09% <0.00%> (ø)
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4dc6e35...1c4b6e3. Read the comment docs.

Copy link
Contributor

@bryphe-coder bryphe-coder left a comment

Choose a reason for hiding this comment

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

Thanks for splitting this out!

@@ -0,0 +1,11 @@
package cryptorand
Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming this is brought in mostly unchanged (aside from the name-change). Thanks for making the name clearer btw

There is little precedence of functions leading with Must being
idiomatic in Go code. Ignoring errors in favor of a panic is
dangerous in highly-reliable code.
// random Int63 data.

// Int64 returns a non-negative random 63-bit integer as a int64.
func Int63() (int64, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, the Must* helpers are useful because I don't think rand.Reader can actually fail since it uses urandom https://pkg.go.dev/crypto/rand#pkg-variables

Copy link
Contributor

Choose a reason for hiding this comment

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

we could maybe simplify by removing the non-must variants and just panicing instead

Copy link
Member Author

Choose a reason for hiding this comment

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

This package could be used on Mac and Windows too, where the guarantee of entropy isn't there!

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess both are useful, since we don't care if tests panic/fail, but production code should check errors 🤷‍♂️

@kylecarbs kylecarbs merged commit 6e6eee6 into main Jan 19, 2022
@kylecarbs kylecarbs deleted the cryptorand branch January 19, 2022 17:55
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

3 participants