client/eth/ui: Export ETH wallet to Metamask #1648

martonp · 2022-06-04T08:39:12Z

client/asset: Adds a WalletRestorer interface which requires
the RestorationInfo method to be implemented. RestorationInfo
returns the wallet seed and instructions for the user to restore
the wallet in various external wallets.
client/eth: Implements the WalletRestorer interface. Returns
information about how to restore the wallet in MetaMask.
ui: Displays a new button on the wallet settings page if the wallet
implements WalletRestorer. The user is promped to copy a disclaimer
confirming that they understand the risks of restoring their wallet
in an external wallet software. After doing so, they are able to see
the information returned from the wallet's RestorationInfo function.

client/webserver/locales/en-us.go

client/asset/eth/eth.go

client/core/core.go

client/webserver/api.go

JoeGruffins · 2022-06-07T06:26:59Z

client/asset/eth/eth.go

+	return []*asset.WalletRestoration{
+		&asset.WalletRestoration{
+			Target:       "MetaMask",
+			Seed:         hex.EncodeToString(privateKey),


I don't know what terminology is better to use, but not really a seed. It's an address's private key. Is this what it will be called when using in metamask?

Checking out metamask on firefox browser, it looks like it wants a word phrase:

Should we convert to words? We would rather have the user copy the words with pencil and paper than save the hex or copy paste it, I think.

looks like there is an option to import private key as well (didn't test it, and it might not even work - depending on how Metamask treats these), but only after you are "already logged into Metamask wallet (with another seed phrase for example)":

Should we convert to words? We would rather have the user copy the words with pencil and paper than save the hex or copy paste it, I think.

additionally, copy-pasting seed(or seed-phrase) is not ideal from security standpoint, and it is much much easier to backup and write down (without mistakes) seed-phrase and not a binary seed (and there is a check-sum as well for seed-phrase which will notify user if he's made a mistake right away instead of him wondering why he has 0 balance on his newly imported account),

wanted to suggest this myself as well, but for some reason I though that converting from seed-phrase to seed is a one-way operation (which its not https://github.com/bitcoin/bips/blob/master/bip-0039.mediawiki#generating-the-mnemonic)

Yes, typing the private key in that box works. I should update the instructions to say that the wallet already needs to be initialized before the account can be added.

Going from seed phrase to seed is a one way operation. The link you shared was for generating the mnemonic, but the process for deriving the seed is below that:
https://github.com/bitcoin/bips/blob/master/bip-0039.mediawiki#from-mnemonic-to-seed

ah, right, because BIP39 mnemonic is encoded represenatation for "initial entropy" (not seed),

and, looks like having word-based seed in DEX requires some rework of seed concept (and related PrimaryCredentials structure) and its probably difficult to implement it in a backward-compatible manner ...

DEX requires some rework of seed concept

Do you mean the app seed?

Since this "seed" is something new and not used by anything but eth, we could do whatever we want with this "seed" I think, unless I'm missing something.

But the hex is fine with me. It's an emergency feature? Probably not used regularly.

Actually I'm not sure what the original motivation for exporting this "seed" is, if anyone can refund. Was was it?

We can't convert bytes -> mnemonic?

Just want to point out one more time that taking the m/44'/60'/0'/0/0 derivation steps to get an account private key doesn't have much point if there's no usable seed words corresponding to m. This is not related to the app seed discussion, to be clear.

We just want to go from createWalletParams.Seed -> account private key, which we can get with m/0'/0 for example.

My previous comment would go the other direction and insert another step so there is an (internal) seed phrase to go with the m/44'/60'/0'/0 path, just in case in the future we'd like to obtain an ETH-specific seed phrase that will derive the account private key we are exporting now. I still prefer just exporting the private key though!

Am I missing something or is the seedDerivationPath used just because it's familiar? That's fine - just looking for clarification.

I think just because it's familiar.

Working on some Trevor stuff which also involved just needing an arbitrary, deterministic private key, the argument was made that starting with the seed and doing bip 39 derivation made it easier to derive addresses with other software, if you ever needed to.

Yes we only use the one address atm, but maybe in the future there's more we want to do here.

no usable seed words corresponding to m.

I guess I don't understand what this means. We COULD export m in the future?

Oh, we would need "seed words" for most wallets in order to import the seed? because we cannot go from seed hex -> words apparently? huh...

Right. I get the reasoning of making it "easier to derive addresses with other software, if you ever needed to", but that means words, not the seed bytes that come from words, which are presently non-existent. Hence the playground link I posted, if we want to have yet another dep "just in case" we want words available in the future for this purpose. I dunno, but we should understand we're closing that door if because we cannot go back to words from the seed bytes.

I think it's a good idea to be able to generate the words if needed. I can do a separate PR for that though, that doesn't really affect anything in this one.

JoeGruffins · 2022-06-08T02:35:05Z

Working on gorli testnet. Imported straight from quark.

chappjc · 2022-06-15T16:57:16Z

Reviewing, but I see there's a scss conflict with your address clipboard PR merged.

JoeGruffins

Looks good to me, but there are some conflicts.

JoeGruffins · 2022-06-22T03:06:21Z

client/webserver/locales/en-us.go

@@ -236,4 +236,9 @@ var EnUS = map[string]string{
 	"Update TLS Certificate":      "Update TLS Certificate",
 	"registered dexes":            "Registered Dexes:",
 	"successful_cert_update":      "Successfully updated certificate!",
+	"export_wallet":               "Export Wallet",
+	"pw_for_wallet_seed":          "Enter your app password to show the wallet seed. Make sure nobody else can see your screen. If anyone gets accecss to the wallet seed, they will be able to steal all of your funds.",
+	"export_wallet_disclaimer":    "You WILL LOSE FUNDS if export your wallet and use the external wallet while you have active trades running in the DEX. It is recommended that you do not export your wallet unless you are an experienced user and you know what are doing.",


I don't know if it's worth mentioning, but copying anything will put that plaintext into your clipboard which any program can easily access. That's why words copied with pencil and paper are potentially better. But, I think it's fine to let advanced users decide for themselves if they want to take the risk.

How much more risky is copying to the clipboard than just typing the password? Is accessing someone's clipboard that much easier than installing a keylogger?

It seems clipboard peeking is far worse,

I got curious and tried to look up some info on it, but didn't seems to find good examples right away, however I tired the following:

I've installed a simple go binary that works with clipboard (no root permissions required for this I think)

then I just ran gclip --paste (no root needed as well) and basically got clipboard contents

^ I think this means pretty much any binary on my system (Mac, but I think this is applicable to most OS) running in user space can peek secrets in clipboard at any time.

With key-logging, I wasn't able to find detailed description on how it gets handled by OSes (could be OS-specific and improving over time), but from what I've read looks like root access (or giving out permissions explicitly) is actually required (one, two, three).

That makes sense, I'll add a message, thanks.

buck54321 · 2022-06-17T20:22:50Z

client/webserver/locales/en-us.go

@@ -236,4 +236,9 @@ var EnUS = map[string]string{
 	"Update TLS Certificate":      "Update TLS Certificate",
 	"registered dexes":            "Registered Dexes:",
 	"successful_cert_update":      "Successfully updated certificate!",
+	"export_wallet":               "Export Wallet",
+	"pw_for_wallet_seed":          "Enter your app password to show the wallet seed. Make sure nobody else can see your screen. If anyone gets accecss to the wallet seed, they will be able to steal all of your funds.",


buck54321 · 2022-06-17T20:24:54Z

client/webserver/locales/en-us.go

@@ -236,4 +236,9 @@ var EnUS = map[string]string{
 	"Update TLS Certificate":      "Update TLS Certificate",
 	"registered dexes":            "Registered Dexes:",
 	"successful_cert_update":      "Successfully updated certificate!",
+	"export_wallet":               "Export Wallet",
+	"pw_for_wallet_seed":          "Enter your app password to show the wallet seed. Make sure nobody else can see your screen. If anyone gets accecss to the wallet seed, they will be able to steal all of your funds.",
+	"export_wallet_disclaimer":    "You WILL LOSE FUNDS if export your wallet and use the external wallet while you have active trades running in the DEX. It is recommended that you do not export your wallet unless you are an experienced user and you know what are doing.",


You WILL LOSE FUNDS

I don't think this is necessarily true. You may screw up some trades, but loss of funds (other than fees) is not a likely outcome, is it?

Hows this:

Using an externally restored wallet while you have active trades running in the DEX could result in failed trades and LOST FUNDS.

buck54321 · 2022-06-17T20:35:53Z

client/webserver/site/src/js/wallets.ts

 const activeOrdersErrCode = 35

+const exportWalletRetypeText = 'I will not use any external wallet while I have active trades in the DEX'


Can we get this from intl instead?

buck54321 · 2022-06-17T20:38:32Z

client/webserver/site/src/js/wallets.ts

+    while (page.restoreInfoCardsList.firstChild) {
+      page.restoreInfoCardsList.removeChild(page.restoreInfoCardsList.firstChild)
+    }


Doc.empty(page.restoreInfoCardsList)

buck54321 · 2022-06-17T20:40:03Z

client/webserver/site/src/js/wallets.ts

+    while (page.restoreInfoCardsList.firstChild) {
+      page.restoreInfoCardsList.removeChild(page.restoreInfoCardsList.firstChild)
+    }
+    info.forEach((wr: WalletRestoration) => {


I tend to prefer for (const wr of info) {. No biggie though.

buck54321 · 2022-06-23T02:27:06Z

client/asset/interface.go

@@ -59,6 +60,10 @@ func (wt WalletTrait) IsRecoverer() bool {
 	return wt&WalletTraitRecoverer != 0
 }

+func (wt WalletTrait) IsExporter() bool {


IsRestorer?

buck54321 · 2022-06-23T02:29:42Z

client/webserver/site/src/js/wallets.ts

+    this.restoreInfoCard = page.restoreInfoCard.cloneNode(true) as HTMLElement
+    page.restoreInfoCard.remove()


Use Doc.cleanTemplates so that the id is removed too.

buck54321 · 2022-06-23T02:39:25Z

client/webserver/site/src/js/wallets.ts

+  // displayExportWalletDisclaimer displays a popup which prompts the user to
+  // retype a text confirming that they understand the risks of exporting the
+  // wallet.
+  async displayExportWalletDisclaimer () {


prompts the user to retype a text confirming that they understand the risks of exporting the wallet

Mixed feelings about this new security step. It seems a little random. We allow exporting of the application seed, which gives access to all funds from all native wallets, with just a password check. Why would this need stronger security? Or maybe the app seed needs stronger security. I'm also feeling like a password and good messaging is sufficient though.

Why would this need stronger security?

When exporting the funds to an external wallet, there is the additional risk of losing funds when using the wallet at the same time as using the DEX. Most people are aware that if they lose their seed the cannot recover their funds, but they are not aware of this additional risk.

No comment yet about the actual text here, but the risk is pronounced with ETH and tokens where:

ETH needs to be available as gas for txns, including redeems and refunds, which can be time sensitive

it is very easy to clobber a dex-managed txn by creating a txn in the external non-managed wallet that uses the same nonce. Even if it is just a speed-up, our dex client probably isn't hardened enough to deal with a tx vanishing even if the desired contract interaction is still affected. The flip side is that DEX can create a txn using a nonce that may have been used by the external wallet clobbering that one.

If the external wallet uses a locked UTXO, the trade will be screwed up as well. It's definitely more risky with ETH though.

We allow exporting of the application seed, which gives access to all funds from all native wallets, with just a password check

@buck54321 Gives access by initializing another dexc instance with the app seed or via the assetseed CLI app (or advanced/manual coding)?

Or maybe the app seed needs stronger security.

Possibly a sentence in the view seed dialog advising not to operate two instances of dexc or the native wallets simultaneously.

I'm also feeling like a password and good messaging is sufficient though.

No objection to that either I suppose, if it's fairly strong language, even with bold and/or red font for the sentence that says what not to do and what can happen in the worst case scenario.

I've updated to using red text (you can see in the PR description) and removed the extra security step.

chappjc · 2022-07-03T18:08:23Z

The dialog looks like this:

"Seed" should be "Private Key", right? Also, the formatting can be improved with mono and `fs14:

chappjc · 2022-07-03T18:10:17Z

When I select and copy the private key, it seems to include a trailing space even though I don't see one selected. When I paste into metamask I saw this:

I pasted into a text editor and figured out the trailing space.

martonp · 2022-07-06T15:42:25Z

When I select and copy the private key, it seems to include a trailing space even though I don't see one selected. When I paste into metamask I saw this:

Were you using Firefox? I put the span element into a div and it seems to be working well now.

chappjc · 2022-07-06T16:49:05Z

When I select and copy the private key, it seems to include a trailing space even though I don't see one selected. When I paste into metamask I saw this:

Were you using Firefox? I put the span element into a div and it seems to be working well now.

Yup, Firefox. Is that a quirk of FF you've seen? Selecting the <br> too?

martonp · 2022-07-07T03:16:04Z

Yup, Firefox. Is that a quirk of FF you've seen? Selecting the <br> too?

Yeah I'm guessing that's what it was. There's some discussion online about extra spaces at the end of text after double clicking and copying in Firefox, but after putting it into the <div> the way it is now, it seems to be working. In Chrome it works the previous way as well.

chappjc

LGTM.
Other reviewers should circle back though.

LasTshaMAN

Minor suggestions

LasTshaMAN · 2022-07-07T17:57:58Z

client/asset/eth/eth.go

+				"Then, to import your DEX account into MetaMask, follow the steps below:\n" +
+				`1. Open the settings menu
+				 2. Select "Import Account"
+				 3. Make sure "Private Key" is selected, and paste the private key above into the box`,


might warn about clipboard copy-pasting here as well, otherwise "paste into" kinda suggest to do simple copy-paste ...

I'll just replace "paste" with "enter". I already have the warning on the top, I don't think it needs to be mentioned over and over.

LasTshaMAN · 2022-07-07T18:01:36Z

client/core/core.go

+
+	restorer, ok := wallet.Wallet.(asset.WalletRestorer)
+	if !ok {
+		return nil, fmt.Errorf("wallet for asset %d cannot be restored", assetID)


might wanna be specific and say that "wallet doesn't support exporting functionality"

LasTshaMAN · 2022-07-07T18:01:54Z

client/webserver/locales/en-us.go

+	"export_wallet_disclaimer":    `<span class="warning-text">Using an externally restored wallet while you have active trades running in the DEX could result in failed trades and LOST FUNDS.</span> It is recommended that you do not export your wallet unless you are an experienced user and you know what are doing.`,
+	"retype_instructions":         `Please type the following to confirm that you understand the risks of exporting the wallet:`,
+	"export_wallet_msg":           "Below are the seeds needed to restore your wallet in some popular external wallets. DO NOT make transactions with your external wallet while you have active trades running on the DEX.",
+	"restore_wallet_retype_text":  "I will not use any external wallet while I have active trades in the DEX",


probably not used anymore

LasTshaMAN · 2022-07-07T18:02:34Z

client/webserver/locales/en-us.go

+	"export_wallet":               "Export Wallet",
+	"pw_for_wallet_seed":          "Enter your app password to show the wallet seed. Make sure nobody else can see your screen. If anyone gets access to the wallet seed, they will be able to steal all of your funds.",
+	"export_wallet_disclaimer":    `<span class="warning-text">Using an externally restored wallet while you have active trades running in the DEX could result in failed trades and LOST FUNDS.</span> It is recommended that you do not export your wallet unless you are an experienced user and you know what are doing.`,
+	"retype_instructions":         `Please type the following to confirm that you understand the risks of exporting the wallet:`,


probably not used anymore

buck54321

I haven't tested on testnet, but I was able to import keys and got the correct address when I set up MetaMask on simnet. As discussed on chat.decred.org, I wasn't able to see my balance on simnet for one reason or another. Regardless, it seemed obvious that the key was imported correctly. I'm approving, but if you know how to get MetaMask connected to simnet, we could potentially test out some of the security issues raised.

chappjc · 2022-07-13T22:28:42Z

I wasn't able to see my balance on simnet for one reason or another.

I couldn't make metamask work with simnet either. But to get mm to at least connect to geth I had to add this to alpha:

--http --http.corsdomain "*" --allow-insecure-unlock --http.api "eth,miner,web3,net,personal,rpc,txpool,db,admin,rpc"

Then I could get it to use a custom RPC endpoint at http://127.0.0.1:8545, chain ID 42 (0x2a), and I observed geth fielding a bunch of RPC requests for best block, but no address requests strangely.

I wonder if we should change the chain ID so it isn't the same as Kovan.

- client/asset: Adds a `WalletRestorer` interface which requires the `RestorationInfo` method to be implemented. `RestorationInfo` returns the wallet seed and instructions for the user to restore the wallet in various external wallets. - client/eth: Implements the `WalletRestorer` interface. Returns information about how to restore the wallet in MetaMask. - ui: Displays a new button on the wallet settings page if the wallet implements `WalletRestorer`. The user is promped to copy a disclaimer confirming that they understand the risks of restoring their wallet in an external wallet software. After doing so, they are able to see the information returned from the wallet's `RestorationInfo` function.

martonp force-pushed the exportWallet branch from 2527c00 to 4bed31c Compare June 4, 2022 08:41

LasTshaMAN reviewed Jun 4, 2022

View reviewed changes

client/webserver/locales/en-us.go Outdated Show resolved Hide resolved

JoeGruffins reviewed Jun 7, 2022

View reviewed changes

chappjc added this to the 0.5 milestone Jun 9, 2022

LasTshaMAN mentioned this pull request Jun 11, 2022

dexc/ui: show app seed as words from pgpwordlist #1660

Closed

martonp force-pushed the exportWallet branch from 1ca7d1b to 19b35bc Compare June 17, 2022 05:09

LasTshaMAN mentioned this pull request Jun 19, 2022

DEX app seed: BIP-44 (+ BIP-39) interoperability (DEX-managed wallets) #1670

Closed

JoeGruffins approved these changes Jun 22, 2022

View reviewed changes

buck54321 requested changes Jun 23, 2022

View reviewed changes

martonp force-pushed the exportWallet branch 3 times, most recently from 6eae939 to 0753102 Compare June 25, 2022 05:00

martonp force-pushed the exportWallet branch from 5e2d7bb to 636871c Compare July 6, 2022 15:36

martonp force-pushed the exportWallet branch from 636871c to 7629e3e Compare July 6, 2022 15:45

chappjc approved these changes Jul 7, 2022

View reviewed changes

LasTshaMAN approved these changes Jul 7, 2022

View reviewed changes

martonp force-pushed the exportWallet branch from 7629e3e to ef36ba8 Compare July 8, 2022 05:26

martonp mentioned this pull request Jul 8, 2022

client/eth: Use wallet seed as entropy for BIP-39 mnemonic #1702

Merged

buck54321 approved these changes Jul 13, 2022

View reviewed changes

martonp added 3 commits July 14, 2022 17:37

Fixes based on reviews.

2653fdf

Changes based on review.

bbf9083

Remove text retyping and other minor fixes

540abb8

martonp force-pushed the exportWallet branch from ef36ba8 to 540abb8 Compare July 14, 2022 10:37

Fix gofmt

c48a06d

chappjc merged commit 0a9abc5 into decred:master Jul 14, 2022

chappjc added the ETH label Aug 26, 2022

martonp deleted the exportWallet branch January 20, 2024 13:16

		const activeOrdersErrCode = 35

		const exportWalletRetypeText = 'I will not use any external wallet while I have active trades in the DEX'

		this.restoreInfoCard = page.restoreInfoCard.cloneNode(true) as HTMLElement
		page.restoreInfoCard.remove()

client/eth/ui: Export ETH wallet to Metamask #1648

client/eth/ui: Export ETH wallet to Metamask #1648

Conversation

martonp commented Jun 4, 2022 • edited

Choose a reason for hiding this comment

LasTshaMAN Jun 7, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LasTshaMAN Jun 7, 2022 • edited

Choose a reason for hiding this comment

JoeGruffins Jun 8, 2022 • edited

Choose a reason for hiding this comment

chappjc Jun 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeGruffins Jun 24, 2022 • edited

Choose a reason for hiding this comment

chappjc Jun 25, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeGruffins commented Jun 8, 2022

chappjc commented Jun 15, 2022

JoeGruffins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LasTshaMAN Jul 3, 2022 • edited

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

chappjc Jun 24, 2022 • edited

Choose a reason for hiding this comment

martonp Jun 24, 2022 • edited

Choose a reason for hiding this comment

chappjc Jul 3, 2022 • edited

Choose a reason for hiding this comment

martonp Jul 6, 2022 • edited

Choose a reason for hiding this comment

chappjc commented Jul 3, 2022

chappjc commented Jul 3, 2022

martonp commented Jul 6, 2022

chappjc commented Jul 6, 2022 • edited

martonp commented Jul 7, 2022

chappjc left a comment

Choose a reason for hiding this comment

LasTshaMAN 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

buck54321 left a comment

Choose a reason for hiding this comment

chappjc commented Jul 13, 2022 • edited

martonp commented Jun 4, 2022 •

edited

LasTshaMAN Jun 7, 2022 •

edited

LasTshaMAN Jun 7, 2022 •

edited

JoeGruffins Jun 8, 2022 •

edited

chappjc Jun 22, 2022 •

edited

JoeGruffins Jun 24, 2022 •

edited

chappjc Jun 25, 2022 •

edited

LasTshaMAN Jul 3, 2022 •

edited

chappjc Jun 24, 2022 •

edited

martonp Jun 24, 2022 •

edited

chappjc Jul 3, 2022 •

edited

martonp Jul 6, 2022 •

edited

chappjc commented Jul 6, 2022 •

edited

chappjc commented Jul 13, 2022 •

edited