Add EIP-1193 support #414
Add EIP-1193 support #414
Conversation
libsecp256k1 doesn't compile with the WASM target, and you don't need to sign transactions or messages in the browser anyway, since your wallet usually manages the keys.
enolan
force-pushed
the
eip-1193-pr
branch
2 times, most recently
from
18c96f2
to
a44881a
Compare
src/contract/mod.rs
Outdated
| @@ -1,12 +1,17 @@ | |||
| //! Ethereum Contract Interface | |||
|
|
|||
| use crate::api::{Accounts, Eth, Namespace}; | |||
| #[cfg(feature = "signing")] | |||
There was a problem hiding this comment.
Same here an in other places, please group signing-related functionality in a single mod block and only feature gate that block.
There was a problem hiding this comment.
Could you please group all signing related things into a submodule here as well? Basically I'd like a single #[cfg(feature = "signing")] stament instead of sprinkling it all around.
| http = ["hyper", "hyper-proxy", "url", "base64", "typed-headers"] | ||
| http-tls = ["hyper-tls", "native-tls", "http"] | ||
| signing = ["secp256k1"] |
There was a problem hiding this comment.
Instead of disabling signing capabilities in JS it's actually possible to use libsecpk256k1 which compiles to WASM. See how it's done in ethsign. That said, it might be pretty involved and would rather review that separately in a follow up PR, so we can just create an issue for now.
|
|
||
| /// EIP-1193 transport | ||
| #[derive(Clone, Debug)] | ||
| pub struct Eip1193 { |
There was a problem hiding this comment.
To be fair I prefer this notation over one with dash or an underscore. So I'd prefer the feature and the enclosing module to be both called eip1193 and that would align with the struct as well. I'm not super strong on this, so if the - notation is more correct, so be it.
There was a problem hiding this comment.
I'm following the standardized conventions. I don't have a strong preference either, but given that there exists a standard I'd prefer the consistency.
There was a problem hiding this comment.
Yeah, I'm not objecting to the standard, both eip_1993 and eip1993 are valid module names and both adhere to the standard. It's just they come from different "source strings" EIP-1993 and EIP1993.
As I said I'm not super strong on this, if EIP-1993 is the only correct EIP name, so bit it, I can live with eip_1993 module name :)
|
I think I got everything you asked for. Added support for some of the browser-specific events as well. |
src/api/accounts.rs
Outdated
| } | ||
| } | ||
|
|
||
| /// A transaction used for RLP encoding, hashing and signing. | ||
| #[cfg(feature = "signing")] | ||
| struct Transaction { |
There was a problem hiding this comment.
If Transaction is only used in case of signing, why not move it to the mod signing along with all the use statements? The goal is to minimize the number of #[cfg(feature = "signing")] :)
src/contract/mod.rs
Outdated
| @@ -1,12 +1,17 @@ | |||
| //! Ethereum Contract Interface | |||
|
|
|||
| use crate::api::{Accounts, Eth, Namespace}; | |||
| #[cfg(feature = "signing")] | |||
There was a problem hiding this comment.
Could you please group all signing related things into a submodule here as well? Basically I'd like a single #[cfg(feature = "signing")] stament instead of sprinkling it all around.
src/signing.rs
Outdated
| use std::ops::Deref; | ||
|
|
||
| #[cfg(feature = "signing")] |
There was a problem hiding this comment.
And here as well. Perhaps it's better to simply disable the entire signing.rs file?
|
Done. I decided against disabling the whole |
I added support for EIP-1193 and got the library working in the browser. This will be easier to review commit-by-commit.
For an example of the new functionality in use, you can check out the
eip-1193-with-examplebranch from my fork. To run:wasm-pack build -- --no-default-features --features eip-1193 && cd www && npm install && npm run start. Then open http://127.0.0.1:8080 in a browser with MetaMask installed. Reject the transaction when prompted unless you enjoy burning 1 ETH for no reason.This is my first substantial Rust code, feedback is very appreciated.