-
Notifications
You must be signed in to change notification settings - Fork 42
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 RemoteTransactionProver
struct
#921
Conversation
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.
Looks good! Thank you! I left just a few small comments inline.
One other thing I was wondering: since this is not not just a pure binary - should it still be in the bin
directory? Probably not something we need to address as a part of this PR, but maybe we should move it to a crates
directory (together with other crates at the root level). Let's create an issue for this.
[target.'cfg(all(target_arch = "wasm32", target_os = "unknown"))'.dependencies] | ||
tonic-web-wasm-client = { version = "0.6", default-features = false } | ||
tonic = { version = "0.12", default-features = false, features = ["prost", "codegen"] } | ||
getrandom = { version = "0.2", features = ["js"], optional = true } | ||
|
||
[target.'cfg(not(all(target_arch = "wasm32", target_os = "unknown")))'.dependencies] | ||
tonic = { version = "0.12", default-features = false, features = ["prost", "codegen", "transport"] } |
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.
Question: does target_os = "unknown"
basically mean any OS?
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 we wanted to mean any OS, we would not specify target_os
(ie, [target.'cfg(all(target_arch = "wasm32"))'.dependencies]
). I could be wrong, but I think target_os = "unknown"
basically means any environment "without" OS. That is, for instance, for WASI environments this dependencies separator would not match. I added the target_os
because of the tonic-web-wasm-client
doc which mentions it supports this target explicity, but I think it would not matter much in practice if we removed it.
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.
Looks good! Thank you!
Adds
RemoteTransactionProver
struct to themiden-tx
.