-
Notifications
You must be signed in to change notification settings - Fork 601
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
Revert breaking changes and add new GenV* interfaces #68
base: master
Are you sure you want to change the base?
Conversation
1 similar comment
I feel like if the goal is to preserve the old API, then these functions should probably preserve the old behaviour, which is to Just my two cents. |
I believe this should be reverted as soon as possible, to do that we should probably only rollback the prior commits to remove any points of contention around the API. Once things are working again I believe that no changes should be made to the API, from a security perspective a panic is the best thing to do. My rational based on my understanding and prior research, though someone may feel free to correct me:
That said once a successful read occurs from crypto.Random, I do not believe a future call may fail. When a call does fail it is very likely to fail for future calls as I imagine if well past boot the entropy pool has not been initialized sufficiently to seed the urandom pool something is very wrong. So I think looping forever in the former case adds no resiliency, and is likely a busy loop in the latter so the panic is more correct here. |
Thanks @packrat386 and @cstockton for providing me with context on the matter. I guess it should just be left to panic then. |
What's the status on this? Are we not getting the author's attention here? |
The author hasn't had any activity since the change 🤷♀️ |
Based on the discussion I saw from issues #66 and #67 I've monkey patched the code for what could be a possible reference point for the future interface of the library. TBH it is a messy and minimal patch and there must be more discussion on how these API changes should roll out. In the meantime this patch should provide the interfaces necessary to keep old code alive while promoting the use of the new interface.
Changes made in the commit:
*This is just an idea that was presented in #66 and I personally thought might be good because the issue addressed in #18 seemed to be a non-deterministic behavior (which I could very well be misunderstanding). If errors are persistent across trials (and after reading the test code I've been getting an impression that they might be so) panicking might be the only option (although I cannot tell whether or not the
global
instance can potentially raise errors caught in the test cases).Things to discuss: