Add possibility to login with metamask-like browser wallets

[tonymorony]

Might be very good to research this possibility to make access to webdex easier

e.g. uniswap:

[artemii235]

Thanks for opening the issue! I'm not 100% sure for now, but I guess using a Metamask can be limited to ETH (and its forks) only. But if it allows signing arbitrary data using imported privkey, we can support other protocols using the secp256k1 curve. We will need to make some research on this.

[sergeyboyko0791]

I did a little research on Metamask functionality and how it can be integrated into AtomicDEX.

if it allows signing arbitrary data using imported privkey, we can support other protocols using the secp256k1 curve.

Indeed, there is a way to sign transactions of other protocols based on ECDSA signing algorithm (secp256k1 curve) using eth_sign. It works exactly the same way as ethkey:

let (rec_id, s) = Secp256k1::signing_only().sign_recoverable(&msg, &sec).serialize_compact();

Taking into account that we use Secp256k1::signing_only().sign_recoverable() for UTXO coins too, the Metamask's eth_sign method will work for us.

But there are two critical flaws:

1. As it's said in the documentation:

eth_sign is an open-ended signing method that allows signing an arbitrary hash, which means it can be used to sign transactions, or any other data, making it a dangerous phishing risk.
For this reason, we make this method show the most frightening possible message to the user, and generally discourage using this method in production.

Such a warning looks so unfriendly:

2. I still haven't figured out how to convert the result from eth_getEncryptionPublicKey into secp256k1 public key. Anyway if it's possible, I think we should not allow the user to receive any funds on such generated UTXO addresses, because this way of generating addresses is not supported by other wallets, and generally contradicts the documentation.

[sergeyboyko0791]

Regarding Metamask integration steps, I want to highlight 3 steps:

1. MarketMaker is initialized with a default Iguana seed (as it's done in air_dex to connect Trezor from start page).
   ETH works in Wallet only mode.
2. Refactor MarketMaker so the database, P2P are initialized with an extracted from Metamask public key.
3. Integrate SWAP functionality

[artemii235]

Taking into account that we use Secp256k1::signing_only().sign_recoverable() for UTXO coins too, the Metamask's eth_sign method will work for us.
But there are two critical flaws:

As an initial step, I propose starting with ETH coins support only, as they are native for Metamask. Considering "big red warning", UTXO and other protocols UX will be suboptimal anyway.

Refactor MarketMaker so the database, P2P are initialized with an extracted from Metamask public key.

I doubt that it's worth doing it this way. For maker orders broadcast and sync, we require signing P2P messages periodically, which can be annoying (Metamask will show a pop-up every 30 seconds to sign PubkeyKeepAlive). So, we should either use iguana seed to sign these or random p2p_privkey as it's done for Zcoin orders.

[sergeyboyko0791]

I also researched on what eth_getEncryptionPublicKey returns actually, and how it can be used.
Unfortunately, it returns a public encryption key X25519_XSalsa20_Poly1305 that can be used to encrypt arbitrary messages. There is no way to recover a secp256k1 public key that we use to derive ETH addresses at the swaps
https://github.com/KomodoPlatform/atomicDEX-API/blob/2665257607b3e735993772a899383889281eac9f/mm2src/coins/eth.rs#L700

The best way I can suggest is to send ETH address instead of its public key
https://github.com/KomodoPlatform/atomicDEX-API/blob/2665257607b3e735993772a899383889281eac9f/mm2src/mm2_main/src/lp_swap/maker_swap.rs#L367-L368

It will be easier if this is done #415 (comment) but definitely not required.

Another option that should be considered is extracting public key from signed transactions. But this requires the user either to have already sent transactions from his address, or to sign a dummy transaction via deprecated eth_sign and then extract the public key from the signed tx.

But I think it's better to avoid such crunches and refactor NegotiationMsg protocol

[artemii235]

The best way I can suggest is to send ETH address instead of its public key

The pubkey is directly used, for example, in HTLC redeem script for UTXO
https://github.com/KomodoPlatform/atomicDEX-API/blob/28280d7a9dc912bbb10f908c52e61c3c457e9ccd/mm2src/coins/utxo/utxo_common.rs#L3644
As ETH address is a part of pubkey hash, the other side won't be able to get the pubkey itself, so the swap won't be possible.

[artemii235]

As a workaround, we can request user to sign a message like Login to AtomicDEX using signTypedData_v4 method, recover pubkey from resulting signature and store it in DB. There is a JS example at the end of the page demonstrating it.

[artemii235]

One more question arose from my side: what are we going to do if a user switches Metamask active account when there is an ongoing swap? As I can see, there is no API allowing to forcefully change account back.

It's also important to note that user interaction will be required during swaps to sign and send transactions. Users will actually need to stay in front of the screen during the entire swap process.

cc @tonymorony

[tonymorony]

One more question arose from my side: what are we going to do if a user switches Metamask active account when there is an ongoing swap? As I can see, there is no API allowing to forcefully change account back.

It's also important to note that user interaction will be required during swaps to sign and send transactions. Users will actually need to stay in front of the screen during the entire swap process.

cc @tonymorony

Thats a good question, thanks for brining it up!

We can show warning popup in app (we have to do it on logout option lets say as well, right now we do have default popup when user trying to refresh page)

I've tried few swap interfaces where you have to confirm transactions with metamask - usually best practice is to kinda lock user in such case on swap without possibility to switch to another window and also show explanation "You'll need to confirm transactions in process, please do not close window"

Closer to actual integration into interface we'll work with @polevin to additionally explain to the user necessity of transactions confirm by interaction with metamask

[sergeyboyko0791]

This is a good idea!

As a workaround, we can request user to sign a message like Login to AtomicDEX using signTypedData_v4 method, recover pubkey from resulting signature and store it in DB. There is a JS example at the end of the page demonstrating it.

This is not a problem more since we can extract the pubkey from signTypedData_v4 response.
It's a problem if we'll activate UTXO coins via Metamask and sign UTXO transactions via eth_sign. But if we activated ETH coin with Metamask and want to but it, we can just send

The pubkey is directly used, for example, in HTLC redeem script for UTXO

Just adding in support of this

The best way I can suggest is to send ETH address instead of its public key
If we buy ETH activated with Metamask and sell UTXO activated somehow another way, our Taker coin will be ETH, and our Maker coin will be a UTXO.
And when the maker calls utxo.send_maker_payment, the redeem script will contain our Maker UTXO pubkey.
https://github.com/KomodoPlatform/atomicDEX-API/blob/2665257607b3e735993772a899383889281eac9f/mm2src/mm2_main/src/lp_swap/maker_swap.rs#L680-L682

It's possible to "ask" the user to switch the chainId via wallet_switchEthereumChain.

One more question arose from my side: what are we going to do if a user switches Metamask active account when there is an ongoing swap? As I can see, there is no API allowing to forcefully change account back.

[sergeyboyko0791]

Summarizing the above, the only reason to login to AtomicDEX via Metamask is to swap ETH based coins and their tokens cross-chain. For example, cross BNB and ETH mainnet.
The user experience will be questionable as we'll ask the user to switch the chainId every time we need to send Maker/Taker payment, spend the other Taker/Maker payment or to refund our funds. An example:

1. Ask the user to switch to BNB mainnet
2. Send BNB maker payment
3. Ask the user to switch to ETH mainnet
4. Spend ETH taker payment

[gcharang]

what if, we ask users to export/import metamask seed in atomicdex and deal with address derivation paths/accounts etc., the same way hardware wallets are going to be treated? Users can still access the same balances on metamask, but can trade on atomicdex

I understand this will not really be a metamask integration. but, if metamask makes switching networks (ETH vs BNB), approving each txn, dealing with utxo chains hard, then this can be a good compromise. Users won't need to backup yet another seed phrase, and get the utxo world open up to them with the same metamask seed phrase

[cipig]

what if, we ask users to export/import metamask seed in atomicdex and deal with address derivation paths/accounts etc., the same way hardware wallets are going to be treated? Users can still access the same balances on metamask, but can trade on atomicdex

i do exactly this, pretty often... i can trade on all EVM DEXs and have the traded coins directly in my ADEX Desktop... but i did it the other way around, i imported privkey (of ETH or any other EVM token, since they are all the same) into Metamask

[gcharang]

Really, compared to AtomicDEX, there isn't much functionality in Metamask afaik. It is just a wallet and can send/receive coins/tokens to on chain contract addresses.

It can do on chain really well. and isn't designed to deal with anything cross chain

I feel that atomicdex can replicate all the functionality on metamask and can become the "key to all crypto" vs metamask's "key to EVM chains"

[sergeyboyko0791]

As we decided to integrate MetaMask login just as PoC (till waiting for MetaMask Snaps), I'd like to share my thoughts on how it can be implemented:

Login to AtomicDEX with MetaMask

In this approach the user will be able to activate ETH/ERC20 coins only.

1. GUI runs mm2 without a passhrase, but specifies a flag like metamask: true.
2. mm2 initializes MetaMask with eth_requestAccounts. Then it asks the user to confirm signing of a message like Login to AtomicDEX via signTypedData_v4 - shows a popup.
3. GUI enables ETH/ERC20 coins via enable RPC.

Login with Iguana/BIP39 passphrase

1. GUI runs mm2 with either Iguana or BIP39 passphrase
2. GUI can activate any coin including UTXO, Lightning, Tendermint and even ETH/ERC20
3. In order to activate ETH/ERC20 coin with MetaMask, GUI should use task::enable_eth::init with "priv_key_policy": "MetaMask".
   3.1. On the first coin initialization, mm2 initializes MetaMask with eth_requestAccounts. Then it asks the user to confirm signing of a message like Login to AtomicDEX via signTypedData_v4 - shows a popup.

MetaMask Snaps

We'll likely be able to create our own Snap in JavaScript that will allow us to activate any coin based on ECDSA signing algorithm (secp256k1 curve), sign P2P messages without the scary warning etc. But since it's not allowed right now, I'd prefer Login with Iguana/BIP39 passphrase.

Other details

• GUI will use init_withdraw to generate and sign a transaction. There will be used the friendly eth_sendTransaction with a popup. Also we'll need to ask the user to switch ChainId if it's required.
• eth_sign will be used to sign P2P messages with a scary warning. Probably we'll need to allow the user to create Taker orders because Maker orders require to sign P2P messages every 30-60s
• During the swap we'll require the user to change ChainId and sign transactions via friendly eth_sendTransaction.

@artemii235 @yurii-khi @tonymorony which way will we pick?

[artemii235]

I vote for Login with Iguana/BIP39 passphrase.

[sergeyboyko0791]

I've prepared an example MetaMask Snap that requests the private key and returns to the website.
So it seems that we'll be able to get secp256k1 private key to activate UTXO/QRC20/ETH/ERC20 and other coins.
I'm totally unsure if it will be allowed to reveal the user's private key.
Probably the MetaMask team will ban such Snaps that reveal the private keys, it's better to ask them.

Example: https://github.com/sergeyboyko0791/test-mm-snap
Code:

export const onRpcRequest: OnRpcRequestHandler = async ({ origin, request }) => {
  switch (request.method) {
    case 'hello':
      const res = await wallet.request({
        method: 'snap_getBip32Entropy',
        params:
            {
		path: ['m', "44'", "60'", "0'", "0", "0"],
	        curve: 'secp256k1',
            }
      });
      return res.privateKey;
    default:
      throw new Error('request.method=' + request.method);
  }
};

[tonymorony]

I vote for Login with Iguana/BIP39 passphrase.

agreed, it looks like a best option until they snap feature matures

[sergeyboyko0791]

I ran into several challenges while I worked on #1551:

GuiAuthValidationGenerator

GuiAuthValidationGenerator takes a key-pair on ETH/ERC20 initialization in order to sign a proof. See an example.
When we activate a coin with the MetaMask wallet, we don't have an access to the address's private key.

I see a few workarounds, but they all have cons:

1. Use the built-in MetaMask RPC provider

• Pros: We can unload our RPC nodes and don't refactor GuiAuthValidationGenerator.
• Cons: We can only use ETH chains known to MetaMask. We also can add new chains via wallet_addEthereumChain, but we need to support public nodes for chains that are not in the list below (including ETH mainnet, BNB and different testnets):

2. Refactor `GuiAuthValidationGenerator` to sign a proof on the coin initialization using [eth_signTypedDataV4](https://docs.metamask.io/guide/signing-data.html#sign-typed-data-v4), and then use this proof a limited number of times. For example, 100 requests.

• Pros: We'll support all ETH chains the same way as we do right now.
• Cons: I don't know if it's even acceptable solution.

Withdraw

MarketMaker generates, signs and returns the signed transaction on withdraw, but MetaMask doesn't allow to sign and not to broadcast transactions via the user-friendly eth_sendTransaction.

I'd suggest the following:

1. Add withdraw_and_send RPC
2. Use the frightening eth_sign MetaMask method on withdraw RPC

@artemii235 what do you think about it?

[artemii235]

@sergeyboyko0791

GuiAuthValidationGenerator

I see a few workarounds, but they all have cons:
Use the built-in MetaMask RPC provider

I assumed we would use the built-in RPC provider - Metamask uses Infura with their own API key, so we don't have to bother using another RPC node.

but we need to support public nodes for chains that are not in the list below

It's likely that we don't need it - it's possible to add any chain manually, similarly to Binance Smart Chain https://academy.binance.com/en/articles/connecting-metamask-to-binance-smart-chain.

Withdraw

1. Add withdraw_and_send RPC
2. Use the frightening eth_sign MetaMask method on withdraw RPC

Considering the big red warning on eth_sign, it's preferred to use 1. withdraw->send_raw_transaction flow is made so to allow user to check generated transaction details before sending it. With Metamask, users will do it in a separate confirmation pop-up showed by extension.

[sergeyboyko0791]

Ok, good.

I assumed we would use the built-in RPC provider - Metamask uses Infura with their own API key, so we don't have to bother using another RPC node.

Do I understand correctly, that you suggest the following activation scheme?

• User tries to activate BNB using enable_eth_with_tokens
• We tries to switch ChainId to 56 in order to check if MetaMask is able to work with BNB smart chain
• If it fails, we return an error, otherwise activate the coin successfully.
  So if the user wants to also support BNB smart chain, he needs to add this chain manually before activating coins in mm2.
  We could do it instead by wallet_addEthereumChain, but again: we need to provide our own public nodes, and we also take responsibility for added chains.

It's likely that we don't need it - it's possible to add any chain manually, similarly to Binance Smart Chain https://academy.binance.com/en/articles/connecting-metamask-to-binance-smart-chain.

So can I add withdraw_and_send RPC? At least for now, it will support MetaMask only

With Metamask, users will do it in a separate confirmation pop-up showed by extension.

[artemii235]

Do I understand correctly, that you suggest the following activation scheme?

Yes

So can I add withdraw_and_send RPC? At least for now, it will support MetaMask only

Also yes 🙂 And to avoid adding one more RPC, it might be worth to figure out other possible solutions: like adding broadcast_immediately: bool flag to the existing withdraw.

[sergeyboyko0791]

I prepared two diagrams to describe how MetaMask is integrated into AtomicDEX.

Connect MetaMask

Enable ETH coin

[sergeyboyko0791]

As you can see, we lock the MetamaskSession mutex and call wallet_switchEthereumChain on each RPC such as request balance, block height, sign and send transaction.
This is required to avoid the following case:

Race condition

Fixed race condition