add evm_unlockUnknownAccount #622
Conversation
|
Thanks for the PR. We'll need to discuss implementation details (such as the RPC endpoint) before merging. Related issue: #588 |
Thank You! |
|
@davidmurdoch would you be open to using the |
|
I don't think this is something that could be done from within ganache-core, as we don't use web3 (except for a couple of RPC calls when in forking mode). Maybe trufflesuite/truffle could extend their injected web3 with ganache's missing RPC methods, if they don't already. |
|
yes, that's true. Just had a little doubt though. does truffle use ganache-core? |
|
Yes, it does for the default development chain. |
|
@davidmurdoch any update on integrating this endpoint into the suite? |
|
@aaryamannchallani We're discussing this internally. For now, let's change the name to There are some security considerations to be aware of here. As is, this will allow any user to unlock any account, even an intentionally locked account (accounts that are in So we need to a think of a way to secure this RPC endpoint by default, as ganache is often used in collaborative environments. |
|
got it, will make that change |
There was a problem hiding this comment.
We chatted about this internally and decided on this method should be named evm_unlockUnknownAccount.
Before we can merge this the behavior needs to be changed so that it does not unlock accounts found in this.state.personal_accounts, as these can already be unlocked (if previously locked at start up or via personal_lockAccount) via personal_unlockAccount.
lib/subproviders/geth_api_double.js
Outdated
| return callback(null, true); | ||
| } | ||
| // If Account exists, this is the callback | ||
| callback(null, "Account Already Unlocked"); |
There was a problem hiding this comment.
Let's make this should return false instead of "Account Already Unlocked".
lib/subproviders/geth_api_double.js
Outdated
| @@ -574,6 +574,18 @@ GethApiDouble.prototype.debug_traceTransaction = function(txHash, params, callba | |||
| this.state.queueTransactionTrace(txHash, params, callback); | |||
| }; | |||
|
|
|||
| // Unlocks any account after server start | |||
| GethApiDouble.prototype.debug_unlockAccount = function(address, callback) { | |||
There was a problem hiding this comment.
We chatted about this internally and decided on this method should be named evm_unlockUnknownAccount.
lib/subproviders/geth_api_double.js
Outdated
| // Checks if given address is already unlocked | ||
| if (this.state.unlocked_accounts[address.toLowerCase()] !== true) { | ||
| // Unlocks it | ||
| this.state.unlocked_accounts[address.toLowerCase()] = true; |
There was a problem hiding this comment.
Before we can merge this the behavior needs to be changed so that it does not unlock accounts found in this.state.personal_accounts, as these can already be unlocked (if previously locked at start up or via personal_lockAccount) via personal_unlockAccount.
There was a problem hiding this comment.
In the case where the account cannot be unlocked, return an error in the callback:
callback(new Error("cannot unlock known/personal account"));
There was a problem hiding this comment.
Resolved in this commit
|
@davidmurdoch let me know if this is what you require! |
|
It is enabled!
…On Tue, Sep 15, 2020, 20:08 David Murdoch ***@***.***> wrote:
Can you enable "allow edits from maintainers" on this PR so I can commits
some additional work directly to this PR?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#622 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKNQ6FEFX3Y2DDV4VAD57ILSF5347ANCNFSM4QR5USOA>
.
|
davidmurdoch
force-pushed
the
develop
branch
from
010e21e
to
b6761e6
Compare
|
ah thank you! |
|
I didn't mean to close this just yet! I just pushed the wrong local branch :-). I'm working on fixing it back up now. |
|
ah okay cool |
|
@aaryamannchallani Merged! |
|
Awesome! let me know if I can help anywhere else |
|
How do I use this now for unlocking all the accounts from a forked chain? |
|
You'll still need to unlock them individually before using each one. We don't yet have a way of unlocking all accounts. // unlock an address
await provider.send("evm_addAccount", [address, passphrase] );
await provider.send("personal_unlockAccount", [address, passphrase] ); |
|
To add some more detail for any future readers, see #3603 |
This feature is much-needed for on the fly transaction creation, I hope you accept this PR!
I can write more tests if required