Initial implementation. #1
Conversation
sjrd
force-pushed
the
initial-implementation
branch
from
7666f76 to
36c2666
Compare
There was a problem hiding this comment.
Higher level comment:
There seems to be a lot of duplication with the real implementation. Does it make sense to put them into a single repository and have two sbt projects in it?
The most likely changes I see to this code are:
- JDK interface changes.
- Adjustments to the crypto API detection.
Both of which are shared. So the effort to keep them in sync will be much higher if we put these into separate repos.
build.sbt
Outdated
|
|
||
| jsEnvKind.value match { | ||
| case NodeJS => | ||
| old |
| // setSeed has no effect | ||
| override def setSeed(x: Long): Unit = () | ||
|
|
||
| override def nextBytes(bytes: Array[Byte]): Unit = { |
There was a problem hiding this comment.
Ugh... What is happening here...
Looking at the documentation of Random, you're not allowed to do this: The documentation of next states:
Subclasses should override this, as this is used by all other methods.
Since SecureRandom is not final, a subclass may override next and expect nextBytes to use its overridden implementation.
However, SecureRandom provides its own nextBytes method...
There was a problem hiding this comment.
next is final in SecureRandom. 😉
| private val getRandomValuesFun = SecureRandom.getRandomValuesFun | ||
|
|
||
| // setSeed has no effect | ||
| override def setSeed(x: Long): Unit = () |
There was a problem hiding this comment.
This is odd. Reading the documentation, I would expect the following to end up being deterministic:
val x = new SecureRandom
x.setSeed(0) // seeds x, doc says x will be seeded lazily
x.nextInt()
However, experimentally, it seems that the result is not deterministic. Hence, this implementation is OK.
Maybe comment on that?
There was a problem hiding this comment.
I'll add a comment.
The idea is that, for cryptographically secure RNG, giving a seed can only ever increase the entropy. It is never allowed to decrease it. Given that we don't have access to an API to strengthen the entropy of the underlying RNG, it's fine to ignore it instead.
There was a problem hiding this comment.
Yes, that makes sense as soon as the SecureRandom is seeded. But the documentation is very clear that seeding only happens once a random value is actually required. But yes, maybe the term "seeing" here is a bit overloaded.
| random.nextBytes(buffer) | ||
|
|
||
| // the likelihood of this failing should be one in 4 billion | ||
| assertTrue("nextBytes() must fill the provided buffer", buffer.count(_ == 0) < 4) |
There was a problem hiding this comment.
Is it worth doing this check? Feels like the difference check should be enough.
There was a problem hiding this comment.
The difference check does not protect against accidentally filling only a prefix of the array, for example. This ensures that all bytes are covered.
| } | ||
|
|
||
| override protected final def next(numBits: Int): Int = { | ||
| if (numBits == 0) { |
There was a problem hiding this comment.
While you're here, protect against negative numBits as well? (OTOH it's protected, so maybe OK).
I'm... not sure. It would make sense if we intend to keep developing and evolving both in the foreseeable future. But I'm very much hoping that we will be able to publish the fake one once, as a stop gap migration measure, and then leave it to die out. In that scenario, I think it makes more sense to keep them separate. |
OK. Fair enough. Let's gamble on that, and if need be, we can still merge the repos in the future. |
sjrd
force-pushed
the
initial-implementation
branch
from
36c2666 to
a91e27b
Compare
|
I think I have addressed all the comments in this one. I have also updated to use the latest commit from scala-js/scala-js. |
sjrd
force-pushed
the
initial-implementation
branch
from
a91e27b to
a0aafdb
Compare
|
Updated to Scala.js 1.10.0. |
Goes together with scala-js/scala-js#4659
Draft because the CI script mentions my fork of scala-js/scala-js. We'll need it to change that once scala-js/scala-js#4659 is merged.
But otherwise it's ready to review.