refactor(@embark/blockchain): consistently merge config accounts and node accounts

[michaelsbradleyjr]

This is most definitely a WIP.

Main points:

eth_accounts and personal_listAccounts do exactly the same things in geth's source code (see here and here in the same file; parity should be reviewed too) and therefore web3.eth.getAccounts() and web3.eth.personal.getAccounts() are the same thing. The web3 docs website is not up-to-date and the published docs are misleading and incorrect. Compare these:

https://web3js.readthedocs.io/en/1.0/web3-eth.html#getaccounts

web3/web3.js#2124

So getAccounts(), either eth or personal, simply returns a list of accounts controlled by the node.

What we're really doing in our blockchain connector's provider and the blockchain proxy is making config accounts look as if they're node accounts.

In the case of the provider, the accounts list was always being overwritten by the initial output of getAccounts combined with config accounts.

In the case of the proxy, we were concat'ing the config accounts onto a live result of eth_accounts RPC.

So this WIP PR is trying to make the operations consistent, and it does seem successful. I added something like this to a fresh embark_demo:

accounts: [
      {
        nodeAccounts: true, // Accounts use for the node
        numAddresses: "2", // Number of addresses/accounts (defaults to 1)
        password: "config/development/password" // Password file for the accounts
      },
      // Below are additional accounts that will count as `nodeAccounts` in the `deployment` section of your contract config
      // Those will not be unlocked in the node itself
      {
        privateKey: "<some valid key>"
      }
]

When connecting from a node REPL in a fresh project (npm init -y, not an embark dapp) that only has web3@1.0.0-beta.37 installed, I compared the output of getAccounts when connecting to 8545 vs 8555 of embark blockchain. I did the same in a browser with web3 beta.37. And I compared those with the output in embark run cli dashboard. I made sure to use the web3.eth.personal.newAccount() method and ran getAccounts() in all the environments and I got consistent and expected results, i.e. [eth|personal].getAccounts() is always up-to-date after adding node accounts with personal.newAccount(), and 8555 lacks the { privateKey: "<some valid key>"} account.

I'm not 100% sure yet that the output of the account parser is being handled in exactly the same way when comparing the blockchain connector's provider with embark-blockchain-process/src/{blockchain,proxy}. Also, seems like some more refactoring could be done to reduce duplicated logic. See: #1733 (comment).

This PR doesn't doesn't directly address the decisions we're making as a team re: account config semantics, but hopefully it sheds some light on things that are easily misunderstood. I'm 100% sure I was totally confused before I dove into the code and started messing with web3, our provider, and our proxy. I believe these changes would fix @emizzle's problem re: lightchain and the manual creation of node accounts with web3.

[ghost]

DeepCode Report (#6e7dbb)

DeepCode analyzed this pull request.
There are no new issues.

[emizzle]

Really nice work, I love the descriptiveness and the lengths you went through to really try to tackle this issue!!

Unfortunately, I do not think this will fix the issue and would still require a restart, because web3.eth.getAccounts will not call the eth_accounts RPC method unless it has no wallet accounts (web3 PR that implemented these changes). And with Embark, we always have wallet accounts.

Embark is adding all node accounts plus wallet accounts to its web3 wallet, and thus will always return those accounts for any subsequent web3.eth.getAccounts calls. Because we are adding merged accounts to the wallet, we are forced to keep that wallet up-to-date with node account changes. Therefore, I believe what we could be doing here (IMO, this is still a bandaid approach and could be solved with broader strokes), is that we could be intercept any personal_newAccount RPC calls, and append the new account to Embark's web3 wallet. I have no idea if this is actually possible, mainly because I'm not sure if we have access to the PK at that point.

[michaelsbradleyjr]

Really nice work, I love the descriptiveness and the lengths you went through to really try to tackle this issue!!

Thanks!

Unfortunately, I do not think this will fix the issue and would still require a restart, because web3.eth.getAccounts will not call the eth_accounts RPC method unless it has no wallet accounts (web3 PR that implemented these changes). And with Embark, we always have wallet accounts.

Despite appearances, i.e. the branch names involved in the web3 PR you linked to, those changes aren't and won't be part of web3 1.0.0. Based on community feedback, the maintainer decided that 1.x releases will be based off 1.0.0-beta.37 and all the refactoring and features, etc. made after beta.37 will become part of 2.x. See:

https://github.com/ethereum/web3.js/blob/1.x/packages/web3-eth/src/index.js#L226

https://github.com/ethereum/web3.js/blob/release/1.0/packages/web3-eth/src/index.js#L226

What makes the true behavior of eth.getAccounts even more confusing is that web3's docs website provides wrong information about how it works for 1.x. Fixes to the docs are already merged in web3's repo but they haven't been deployed yet.

Summary: eth.getAccounts of web3 beta.37 and 1.x going forward always makes the eth_accounts RPC call. Also, it never mixes the list of accounts returned from the RPC call with accounts (if any) in web3.eth.accounts.wallet.

Embark is adding all node accounts plus wallet accounts to its web3 wallet, and thus will always return those accounts for any subsequent web3.eth.getAccounts calls. Because we are adding merged accounts to the wallet, we are forced to keep that wallet up-to-date with node account changes. Therefore, I believe what we could be doing here (IMO, this is still a bandaid approach and could be solved with broader strokes), is that we could be intercept any personal_newAccount RPC calls, and append the new account to Embark's web3 wallet. I have no idea if this is actually possible, mainly because I'm not sure if we have access to the PK at that point.

This PR already fixes it: there's no need to intercept personal_newAccount and a restart is not required after calling web3.eth.personal.newAccount.

Let's back up a bit...

What embark is actually doing, effectively, is making the accounts from the dapp config behave like unlocked node accounts.

The reason the list of accounts returned by eth.getAccounts wasn't getting updated is that we were always overwriting the results of the call with a never-updated list. See on the master branch:

https://github.com/embark-framework/embark/blob/master/packages/embark-blockchain-connector/src/provider.js#L136

Once the misunderstanding about the behavior of eth.getAccounts is cleared up it's easier to understand what has to change. That's the realization I made, that realization informed the changes in this PR (please have another look), and those changes do solve the problem.

There's probably some additional cleanup and refactoring needed in this PR. I'm wondering if we really need to intercept eth_accounts in two different places, i.e. maybe we can just do it in the proxy. Even so, I need some help understanding if there are more changes needed in the proxy re: the list of accounts coming from the account parser, i.e. ensuring that list is always identical with the list that gets processed in the provider. I'm hoping @jrainville may be able to give some insight on those two points.

While it may seem like a bandaid approach, I don't think there's another good solution. In fact, making all of the accounts seem like unlocked node accounts from the perspective of browsers and the cli dashboard — which is exactly what we were doing and this PR doesn't change that, just improves it — is probably the best and simplest thing we can do, i.e. for the sake of a smooth development experience. Dapp authors simply want to configure accounts and then use them without having to worry about wallet management during development.

[emizzle]

Thanks for clearing that up, I will take another look. The web3 branching names definitely were causing the confusion here. It makes complete sense that the provider could be the source of error.

Can we assume that what I described will be included in web3 2.0? If it is, we may have this problem again as soon as we make the switch.

While it may seem like a bandaid approach, I don't think there's another good solution. In fact, making all of the accounts seem like unlocked node accounts from the perspective of browsers and the cli dashboard — which is exactly what we were doing and this PR doesn't change that, just improves it — is probably the best and simplest thing we can do, i.e. for the sake of a smooth development experience. Dapp authors simply want to configure accounts and then use them without having to worry about wallet management during development.

This is another point of confusion, because when the developer adds wallet accounts to the config (ie via mnemonic), these accounts are added to embark's wallet, they appear in DApp calls to web3.eth.getAccounts, but they cannot be used to sign txs from the DApp (while they can be used to sign Embark txs, ie contract deployment and console commands). An update for this can be added in another PR if needed and is outside the scope of this PR, but one of my concerns is that if we blur the line between node accounts and wallet accounts, and there are some fundamental differences between the two, it will cause more issues and confusion down the line. The developer will need to develop with the idea of using Metamask at some point, so why add this in when we already have a developer node account at our disposal?

[michaelsbradleyjr]

but they cannot be used to sign txs from the DApp (while they can be used to sign Embark txs, ie contract deployment and console commands).

@emizzle can you give an example of a web3 call that I could run in the browser console of embark_demo (augmented with a wallet account via config) that would work for a true unlocked node account but not for a config-wallet account? I think there are some aspects of this puzzle I don't fully understand and appreciate.

[emizzle]

Before I get to your last question, I wanted to follow up and confirm that you are correct in that web3@1.0.0-beta.37 does always calls eth_accounts. I confirmed this by:

1. new folder with no parent node_modules
2. In term 1, geth --wsapi=eth,web3,net,debug,pubsub,personal --ws --wsorigins=* --datadir=./geth/datadir
3. In term 2:

   node
   const Web3 = require('web3');
   const web3 = new Web3(new Web3.providers.WebsocketProvider("ws://localhost:8546"));
   web3.eth.getAccounts().then(console.log)
   # [] // no default accounts
   
   // create a node account
   web3.eth.personal.newAccount("eric");
   web3.eth.getAccounts().then(console.log);
   # ['0x517d83a71e3981E148a0eEbffF291264758533a6']
   
   // create a wallet account
   web3.eth.accounts.create().then(console.log)
   # { address: '0xf29C7AC5fce635997654930fcc4F1B5fFa971482', 
   privateKey:
   '0x2cded5a23c0737d94c7b3d3cab87b672895e92628f2285e2b02780a15b29e4c2', 
   signTransaction: [Function: signTransaction],
   sign: [Function: sign],
   encrypt: [Function: encrypt],
   index: 0 }
   
   // add wallet account to wallet
   web3.eth.accounts.wallet.add("0x2cded5a23c0737d94c7b3d3cab87b672895e92628f2285e2b02780a15b29e4c2")
   
   // check which accounts are returned to us
   web3.eth.getAccounts().then(console.log)
   # [ '0x517d83a71e3981E148a0eEbffF291264758533a6' ] // this is the original node account, and **NOT** the wallet accounts
   

[emizzle]

Using the v5 config updates branch as a base, I created a new demo. Then, I added the following to config/blockchain.js:

development: {
    clientConfig: {
      miningMode: 'dev' // Mode in which the node mines. Options: dev, auto, always, off
    },
    accounts: [
      {
        nodeAccounts: true,
        password: "config/development/password"
      },
      {
        mnemonic: "YOUR_MNEMONIC",
        hdpath: "m/44'/60'/0'/0/",
        numAddresses: "1"
      }
    ]
  },

After that, I ran embark run.

In the browser, I ran

await web3.eth.getAccounts()
# ["0x4d6C8b5662154C72Aa36cd456a356CF5b728A056", "0x7ebba0510f478c739a7d20A93D2fdB9333aA78A0"]

You can see the first account is the unlocked --dev node account (specified in the blockchain config as nodeAccounts:true. The second account is the wallet account (specified by the mnemonic in the config).

Running

await web3.eth.sendTransaction({ from: "0x4d6C8b5662154C72Aa36cd456a356CF5b728A056", value:0, to:"0x4d6C8b5662154C72Aa36cd456a356CF5b728A056" })
# {blockHash: "0x05202672eb9cb044737e3be6bd67fd0ffa484f2af24627794f84b1e8c74c754b", blockNumber: 16, contractAddress: null, cumulativeGasUsed: 21000, from: "0x4d6c8b5662154c72aa36cd456a356cf5b728a056", …}

works because the from account is the unlocked --dev node account.

Running

await web3.eth.sendTransaction({ from: "0x7ebba0510f478c739a7d20A93D2fdB9333aA78A0", value:0, to:"0x7ebba0510f478c739a7d20A93D2fdB9333aA78A0" })
# Uncaught (in promise) Error: Returned error: unknown account Error: Returned error: unknown account
    at Object.ErrorResponse (errors.js:29)
    at Object.<anonymous> (index.js:140)
    at index.js:121
    at Array.forEach (<anonymous>)
    at WebSocket.WebsocketProvider.connection.onmessage (index.js:98)
ErrorResponse @ errors.js:29
(anonymous) @ index.js:140
(anonymous) @ index.js:121
WebsocketProvider.connection.onmessage @ index.js:98
setTimeout (async)
_fireError @ index.js:71
sendTxCallback @ index.js:491
(anonymous) @ index.js:140
(anonymous) @ index.js:121
WebsocketProvider.connection.onmessage @ index.js:98
errors.js:29 Uncaught (in promise) Error: Returned error: unknown account
    at Object.ErrorResponse (errors.js:29)
    at Object.<anonymous> (index.js:140)
    at index.js:121
    at Array.forEach (<anonymous>)
    at WebSocket.WebsocketProvider.connection.onmessage (index.js:98)

fails because the from account exists only in the wallet in Embark and not in the browser's web3 wallet. Therefore this account is not unlocked.

Under the hood, the browser is sending eth_sendTransaction RPC call, which assumes the tx has been signed by the from account using its PK. We can't do that in the browser in this case because the PK of the account is in Embark, not in the browser. This can be "fixed" by intercepting eth_sendTransaction RPC calls in Embark's proxy, and if the tx is from a wallet account that Embark controls, sign the tx using the accounts PK, and forward the tx on to the network using eth_sendRawTransaction. This is essentially what Metamask does.

[jrainville]

Very nice work. Two small comments

[jrainville]

Why do you do this? We already do that in the response handling, no?

[michaelsbradleyjr]

It's bootstrapping logic and is supposed to mirror how the blockchain provider bootstraps the accounts list.

[jrainville]

Yeah, I understand your idea, but it's not necessary to keep the list of updated accounts, for the moment at least.

accounts can stay just the blockchain config accounts (custom accounts using mnemonic, etc) and above, we just return the result + accounts.

So, in the end, your changes in proxy.js are not really needed, because the proxy already always returned the up to date accounts.

[michaelsbradleyjr]

Yes, but what it led to was the eth.getAccounts() having a different result when comparing web3 usage in the cli dashboard to web3 usage in a browser or a plain Node.js REPL connected to the proxy.

The changes in the provider and the proxy in this PR are intended to keep the getAccounts() list up-to-date, consistently ordered, and free of duplicates.

Ideally, I'd like us to discuss and explore doing this operation in just one place, that's why the PR is a WIP.

[jrainville]

Makes sense for the ordering, even though it's not gonna be perfect in terms of matching the array the user puts in blockchain.js. I had an old branch where I did some work to order the accounts as per the user's config, but never made a PR because I found that using custom accounts in the Dapp didn't work. You can see the branch here: https://github.com/embark-framework/embark/tree/fix/order-blockchain-accounts

IMO, we can go ahead with your changes as is, since it fixes the problem. We can and will come up with a better solution anyway once we do the refactor, so no need for anything fancy for now.

[michaelsbradleyjr]

Makes sense, thanks for the link and the feedback.

[michaelsbradleyjr]

Under the hood, the browser is sending eth_sendTransaction RPC call, which assumes the tx has been signed by the from account using its PK. We can't do that in the browser in this case because the PK of the account is in Embark, not in the browser. This can be "fixed" by intercepting eth_sendTransaction RPC calls in Embark's proxy, and if the tx is from a wallet account that Embark controls, sign the tx using the accounts PK, and forward the tx on to the network using eth_sendRawTransaction. This is essentially what Metamask does.

Thanks @emizzle, that makes a lot of sense.

It seems we are intercepting eth_sendRawTransaction: https://github.com/embark-framework/embark/blob/master/packages/embark-blockchain-connector/src/provider.js#L140-L142.

But I guess we need to generalize that for all RPC calls that require signing, i.e. if there are others besides eth_sendTransaction and eth_sendRawTransaction.