Open tonymorony opened 2 years ago
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.
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.
Regarding Metamask integration steps, I want to highlight 3 steps:
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.
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 https://github.com/KomodoPlatform/atomicDEX-API/issues/415#issuecomment-1283847101 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
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.
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.
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
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
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.
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:
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
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
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"
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:
In this approach the user will be able to activate ETH/ERC20 coins only.
metamask: true
.Login to AtomicDEX
via signTypedData_v4 - shows a popup.enable
RPC.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.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
.
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-60sChainId
and sign transactions via friendly eth_sendTransaction
.@artemii235 @yurii-khi @tonymorony which way will we pick?
I vote for Login with Iguana/BIP39 passphrase
.
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);
}
};
I vote for
Login with Iguana/BIP39 passphrase
.
agreed, it looks like a best option until they snap feature matures
I ran into several challenges while I worked on https://github.com/KomodoPlatform/atomicDEX-API/pull/1551:
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:
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):
GuiAuthValidationGenerator
to sign a proof on the coin initialization using eth_signTypedDataV4, 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.
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:
withdraw_and_send
RPCwithdraw
RPC@artemii235 what do you think about it?
@sergeyboyko0791
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.
- Add withdraw_and_send RPC
- 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.
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?
enable_eth_with_tokens
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.
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
.
I prepared two diagrams to describe how MetaMask is integrated into AtomicDEX.
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:
Might be very good to research this possibility to make access to webdex easier
e.g. uniswap: