[r2r] MetaMask PoC #1551

sergeyboyko0791 · 2022-11-19T20:19:37Z

MetaMask initialization using eth_requestAccounts.
Activate ETH/ERC20 with priv_key_policy: "Metamask" via enable_eth_with_tokens.
Use MetaMask RPC provider

TODOs:

Check if corresponding ETH chain is known by MetaMask wallet on coin activation using wallet_switchEthereumChain
Figure out how to refactor ETH/ERC20 withdraw taking into account that the only way to sign a transaction is to send it using eth_sendTransaction.
Extract address's public key using eth_singTypedDataV4 as Artem suggested Add possibility to login with metamask-like browser wallets #1167 (comment).

* Add MetaMask initialization RPCs

* Add `EthPrivKeyPolicy`

* Add `MetamaskTransport` * Rename `Web3Transport` to `HttpTransport` * Add `MetamaskSession` to prevent concurrent requests

artemii235

Thanks for the PR! Few questions and minor proposals for the first iteration.

mm2src/coins/eth/v2_activation.rs

mm2src/crypto/src/crypto_ctx.rs

mm2src/mm2_metamask/src/metamask_provider.rs

artemii235 · 2022-11-22T10:32:52Z

mm2src/mm2_main/src/rpc/dispatcher/dispatcher.rs

-        _ => MmError::err(DispatcherError::NoSuchMethod),
+        wasm_only_methods => match wasm_only_methods {
+            "init_metamask::cancel" => handle_mmrpc(ctx, request, cancel_init_metamask).await,
+            "init_metamask::init" => handle_mmrpc(ctx, request, init_metamask).await,


Do I understand correctly that GUI will first have to invoke init_metamask::init before attempting to initialize e.g. ETH in Metamask mode?

Ok, after thinking for some time, I think it's fine and even good. I hope it won't cause confusion or tricky state tracking on GUI side - will clarify it with Flutter team later 🙂

* Compile `EthRpcMode::Metamask` in WASM only * Add `rpc_mode: EthRpcMode` to `EthActivationV2Request` * Update wasm-timer

artemii235

Thanks for the great work! I have just one note for the future.

artemii235 · 2022-11-23T10:03:45Z

mm2src/mm2_metamask/src/eth_provider.rs

@@ -0,0 +1,168 @@
+//! This module is inspired by https://github.com/tomusdrw/rust-web3/blob/master/src/transports/eip_1193.rs


For the next iteration: should we update rust-web3 dependency instead? We use a quite outdated fork now.

This can be difficult due to the fact that Eip1193 transport is local only, so we'll need to make a workaround anyway.
Currently, EthProvider is Send because it's wrapped into the event-driven pattern

This can be difficult due to the fact that Eip1193 transport is local only, so we'll need to make a workaround anyway.

@sergeyboyko0791 Could you please try to find a work around next sprint? It might be great to not reinvent the wheel, if work around itself is not too hard, of course 🙂

onur-ozkan

Awesome feature implementation 🔥

My first review iteration:

onur-ozkan · 2022-11-23T08:47:41Z

mm2src/coins/eth.rs

+impl fmt::Debug for EthPrivKeyPolicy {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match self {
+            EthPrivKeyPolicy::KeyPair(_) => write!(f, "KeyPair"),
+            #[cfg(target_arch = "wasm32")]
+            EthPrivKeyPolicy::Metamask(_) => write!(f, "Metamask"),
+        }
+    }
+}


This isn't used. Should we keep it implemented?

Oh, looks like it used to be used before. Will remove it, thanks

onur-ozkan · 2022-11-23T09:31:24Z

mm2src/coins/eth.rs

+    pub fn key_pair(&self) -> Option<&KeyPair> {
+        match self {
+            EthPrivKeyPolicy::KeyPair(key_pair) => Some(key_pair),
+            #[cfg(target_arch = "wasm32")]
+            EthPrivKeyPolicy::Metamask(_) => None,
+        }
+    }
+
+    pub fn key_pair_or_err(&self) -> MmResult<&KeyPair, PrivKeyPolicyNotAllowed> {
+        self.key_pair()
+            .or_mm_err(|| PrivKeyPolicyNotAllowed::HardwareWalletNotSupported)
+    }


fn key_pair itself isn't used anywhere. What do you think returning MmResult directly in key_pair fn and remove key_pair_or_err?

I repeated the same methods from lp_coins::PrivKeyPolicy, so how about removing EthPrivKeyPolicy::key_pair method, but leaving EthPrivKeyPolicy::key_pair_or_err as it is? If renaming it to key_pair, it's also worth to rename PrivKeyPolicy::key_pair_or_err. But this will result in changes to files not related to this task.
What do you think?

how about removing EthPrivKeyPolicy::key_pair method, but leaving EthPrivKeyPolicy::key_pair_or_err as it is?

I think it's also fine.

mm2src/coins/eth.rs

onur-ozkan · 2022-11-23T09:42:48Z

mm2src/coins/eth/v2_activation.rs

+    /// The function can be used as a default deserialization constructor:
+    /// `#[serde(default = "EthPrivKeyActivationPolicy::context_priv_key")]`


Is there a specific reason why we didn't implement Default trait for this use case?

No, there is no reason. I think I also can implement Default for lp_coins::PrivKeyActivationPolicy

onur-ozkan · 2022-11-23T09:47:38Z

mm2src/coins/eth/web3_transport/metamask_transport.rs

+use web3::helpers::build_request;
+use web3::{RequestId, Transport};
+
+const REQUEST_ID: RequestId = 0;


If this is only related with fn prepare, it can be defined there I think. When we see values defined on top-level, it gives an idea that they are used in different spots.

onur-ozkan · 2022-11-23T09:55:11Z

mm2src/coins/eth/web3_transport/mod.rs

+    }
+
+    #[cfg(test)]
+    #[inline]


Inlining doesn't help much but increases compilation for tests

onur-ozkan · 2022-11-23T09:59:11Z

mm2src/coins_activation/src/eth_with_token_activation.rs

@@ -44,6 +46,10 @@ impl From<EthActivationV2Error> for EnablePlatformCoinWithTokensError {
            EthActivationV2Error::PrivKeyPolicyNotAllowed(e) => {
                EnablePlatformCoinWithTokensError::PrivKeyPolicyNotAllowed(e)
            },
+            #[cfg(target_arch = "wasm32")]
+            EthActivationV2Error::MetamaskCtxNotInitialized => EnablePlatformCoinWithTokensError::PreparationRequired(
+                "MetaMask context is not initialized. Consider using 'task::init_metamask::init' RPC".to_string(),


What about using RPC method name here?

Suggested change

"MetaMask context is not initialized. Consider using 'task::init_metamask::init' RPC".to_string(),

"MetaMask context is not initialized. Consider using 'init_metamask::init' RPC".to_string(),

Its actual name is task::init_metamask::init:
https://github.com/KomodoPlatform/atomicDEX-API/blob/bf3fa88285d1293af54a05ec93318fcc598da9ca/mm2src/mm2_main/src/rpc/dispatcher/dispatcher.rs#L130-L133
https://github.com/KomodoPlatform/atomicDEX-API/blob/bf3fa88285d1293af54a05ec93318fcc598da9ca/mm2src/mm2_main/src/rpc/dispatcher/dispatcher.rs#L250-L252

Oops. I didn't saw it :)

Can it be task::metamask::init/cancel/status or task::metamask_activation::init/cancel/status ?

This definitely makes sense! My initial though was to follow this naming scheme:
task::<Purpose of the task/what it does>::<An action on the task>, so we could add task::update_metamask::<ACTION> or task::check_metamask::<ACTION> RPCs later.
How about task::connect_metamask::<ACTION>?

How about task::connect_metamask::<ACTION>?

Seems completely fine :)

Done, I decided not to rename related structures like InitMetamaskRequest and functions like CryptoCtx::init_metamask_ctx. Is it fine? I'm afraid there will be many changes.

* Rename `task::init_metamask::*` to `task::connect_metamask::*`

sergeyboyko0791 · 2022-11-25T12:08:00Z

@ozkanonur PR is ready for the next review :)

shamardy

Great progress on such an important feature 🔥
Only 2 non-blockers :)

mm2src/mm2_metamask/src/lib.rs

mm2src/mm2_metamask/src/metamask_provider.rs

artemii235

Re-approving

sergeyboyko0791 added 4 commits November 18, 2022 11:55

Design and implement MetaMask provider

2761a7c

* Add MetaMask initialization RPCs

Merge branch 'dev' into metamask-poc

f32a6df

Activate ETH/ERC20 with Metamask priv_key_policy

203f415

* Add `EthPrivKeyPolicy`

Fix udeps

3180771

sergeyboyko0791 mentioned this pull request Nov 20, 2022

Add possibility to login with metamask-like browser wallets #1167

Open

Merge branch 'dev' into metamask-poc

dfe7915

sergeyboyko0791 linked an issue Nov 21, 2022 that may be closed by this pull request

Add possibility to login with metamask-like browser wallets #1167

Open

sergeyboyko0791 self-assigned this Nov 21, 2022

Use built-in MetaMask RPC provider

cb4aaba

* Add `MetamaskTransport` * Rename `Web3Transport` to `HttpTransport` * Add `MetamaskSession` to prevent concurrent requests

sergeyboyko0791 changed the title ~~[wip] MetaMask PoC~~ [r2r] MetaMask PoC Nov 22, 2022

sergeyboyko0791 requested review from artemii235, shamardy and onur-ozkan November 22, 2022 09:46

artemii235 requested changes Nov 22, 2022

View reviewed changes

sergeyboyko0791 added 2 commits November 22, 2022 17:11

Fix PR issues

e357096

* Compile `EthRpcMode::Metamask` in WASM only * Add `rpc_mode: EthRpcMode` to `EthActivationV2Request` * Update wasm-timer

Minor changes

bf3fa88

artemii235 previously approved these changes Nov 23, 2022

View reviewed changes

onur-ozkan requested changes Nov 23, 2022

View reviewed changes

PR fixes

3ef732e

* Rename `task::init_metamask::*` to `task::connect_metamask::*`

sergeyboyko0791 dismissed artemii235’s stale review via 3ef732e November 23, 2022 14:46

Merge branch 'dev' into metamask-poc

82020b1

shamardy previously approved these changes Nov 28, 2022

View reviewed changes

mm2src/mm2_metamask/src/lib.rs Outdated Show resolved Hide resolved

mm2src/mm2_metamask/src/metamask_provider.rs Outdated Show resolved Hide resolved

Fix PR issues

3e162b5

sergeyboyko0791 dismissed shamardy’s stale review via 3e162b5 December 1, 2022 14:27

Merge branch 'dev' into metamask-poc

6e39a11

artemii235 requested a review from onur-ozkan December 5, 2022 10:04

artemii235 approved these changes Dec 5, 2022

View reviewed changes

artemii235 merged commit 7ea1436 into dev Dec 5, 2022

artemii235 deleted the metamask-poc branch December 5, 2022 10:05

sergeyboyko0791 mentioned this pull request Dec 6, 2022

Consider updating web3 crate #1565

Open

sergeyboyko0791 mentioned this pull request Jan 9, 2023

[r2r] Complete MetaMask #1591

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[r2r] MetaMask PoC #1551

[r2r] MetaMask PoC #1551

sergeyboyko0791 commented Nov 19, 2022 •

edited

Loading

artemii235 left a comment

artemii235 Nov 22, 2022

sergeyboyko0791 Nov 22, 2022

artemii235 Nov 22, 2022

artemii235 left a comment

artemii235 Nov 23, 2022

sergeyboyko0791 Nov 23, 2022

artemii235 Nov 30, 2022

onur-ozkan left a comment

onur-ozkan Nov 23, 2022

sergeyboyko0791 Nov 23, 2022

sergeyboyko0791 Nov 23, 2022

onur-ozkan Nov 23, 2022

sergeyboyko0791 Nov 23, 2022

onur-ozkan Nov 23, 2022

sergeyboyko0791 Nov 23, 2022

onur-ozkan Nov 23, 2022

sergeyboyko0791 Nov 23, 2022

sergeyboyko0791 Nov 23, 2022

onur-ozkan Nov 23, 2022

sergeyboyko0791 Nov 23, 2022

onur-ozkan Nov 23, 2022

sergeyboyko0791 Nov 23, 2022

onur-ozkan Nov 23, 2022

sergeyboyko0791 Nov 23, 2022

onur-ozkan Nov 23, 2022 •

edited

Loading

sergeyboyko0791 Nov 23, 2022

onur-ozkan Nov 23, 2022

sergeyboyko0791 Nov 23, 2022

sergeyboyko0791 commented Nov 25, 2022

shamardy left a comment

artemii235 left a comment

		@@ -0,0 +1,168 @@
		//! This module is inspired by https://github.com/tomusdrw/rust-web3/blob/master/src/transports/eip_1193.rs

		/// The function can be used as a default deserialization constructor:
		/// `#[serde(default = "EthPrivKeyActivationPolicy::context_priv_key")]`

	"MetaMask context is not initialized. Consider using 'task::init_metamask::init' RPC".to_string(),
	"MetaMask context is not initialized. Consider using 'init_metamask::init' RPC".to_string(),

[r2r] MetaMask PoC #1551

[r2r] MetaMask PoC #1551

Conversation

sergeyboyko0791 commented Nov 19, 2022 • edited Loading

artemii235 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artemii235 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onur-ozkan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onur-ozkan Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergeyboyko0791 commented Nov 25, 2022

shamardy left a comment

Choose a reason for hiding this comment

artemii235 left a comment

Choose a reason for hiding this comment

sergeyboyko0791 commented Nov 19, 2022 •

edited

Loading

onur-ozkan Nov 23, 2022 •

edited

Loading