Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Draft pull request: Add Vapor and Linux support #642
Draft pull request: Add Vapor and Linux support #642
Changes from all commits
280316c
1850b46
3461cb8
773ba92
f14a899
33479ee
c8fc0a7
9898f0d
5f90f67
8460d92
b7b020d
d4955d4
855ad8e
936ff2a
268564f
8d7c7df
8486802
44039d2
9e341a3
93a89c1
ec86b96
60b181b
5976670
84528e2
cdf4ad4
616168f
bea91b5
ab991d5
bb0a410
71117e3
3cdb528
1f51faf
96191e9
b7b77a4
f3c7125
71ec91d
ce46ea7
7052ac7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Seems like it's good time to change this sepc256k implementation to those that bitcoin maintains, that did @JeneaVranceanu proposed in the past. I'd bet it's cross platform.
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.
It should be cross-platform as it's written in C but it's not a solution here as the code you are commenting on is our custom extension function and it's just the question of how to get secure random bytes on Linux.
I'd not complicate this PR with the secp256k1 update.
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.
In this proposal I see that URL doesn't included within
FoundationNetwork
, is it necessary here?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.
Let me double check if this one is necessary
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.
If this is necessary, why btw? But if it is, it's better to implement it in a way as backward capability implemented above.
Means implement exact same methods as existed one, but marked it with
@available(obsolete)
on all Apple platforms.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.
The issue is SecRandomCopyBytes isn't available on Linux. I was trying to avoid spinning up a new process and use dev/random, but now believe it might be inevitable.
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.
Why are we importing
libc
here?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.
Guess this file should be excluded from the sources on the Linux platform in a manner that it is now for macOS one. Means it should be ignored in
Package.swift
.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.
We've got to investigate what the hell happening here, and why ERC spec file required
CoreImage
as an import?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.
I believe it's for the creation of the QR code, which is convenient to have on iOS and macOS I assume.
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.
I see, so we really could consider just to crop this file for a Linux platform until we figured out how we could implement it properly.
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.
And this one should be refactored to not use URLSession in it (it was a rough one implementation within 3.0.0 release, so it's could be changed).