Skip to content
This repository has been archived by the owner on Jan 21, 2022. It is now read-only.

Support defining accounts instead of the randomly generated ones #24

Merged
merged 3 commits into from
Mar 24, 2016

Conversation

axic
Copy link
Contributor

@axic axic commented Mar 9, 2016

It also adds a command line argument --account=1f1f... which can be passed multiple times to enable more accounts.

@ghost
Copy link

ghost commented Mar 9, 2016

Switching Dapple over to use TestRPC as it seems more actively developed. This feature is something we need in order to successfully make the switch.

@tcoulter
Copy link
Contributor

Great to hear @ryepdx. Looking at it now! And thanks @axic!

@tcoulter
Copy link
Contributor

Question for both of you (@ryepdx, @axic): If the accounts list were input as an optional configuration file, much like a genesis.json file, instead of command line arguments for each account, would that still satisfy your use cases? We've been throwing that one around and we think a genesis.json would be easier to use.

Here's an example: http://jev.io/genesis_block.json

I'm thinking something like

$ testrpc --genesis ./genesis.json

or

$ testrpc --config ./config.json

Edit: we could easily have quick options like -g or -c.

Either we could strictly adhere to the genesis_block.json format and allow you to set all initial values listed there on startup, or we could create our own format that only deals with accounts. In either case, I agree an optional private key would be great to have TestRPC recognize.

@axic
Copy link
Contributor Author

axic commented Mar 10, 2016

With that specific genesis.json you are losing the ability of signing within testrpc (eth_sendTransaction/eth_sign). That can be replicated by provider-engine in many apps, but why lose that capability if it is already written?

I think the

  • proposed genesis file + commandline option
  • proposed genesis file + configuration file
  • an improved genesis file

are all possible options.

In my opinion --account is not mutually exclusive with the genesis file. It serves a different purpose as it stands.

Update: I am personally not using testrpc for signing, I do that myself (via provider-engine). I also do not mind which way is implemented as long as I can define addresses with balances, but I would prefer such a feature to arrive shortly as I don't really want to maintain my branch :)

@axic
Copy link
Contributor Author

axic commented Mar 10, 2016

Giving a second thought I'm leaning towards:

  • supporting the official genesis json format as is
  • having a separate testrpc specific configuration file or command line option
  • optionally: support keystore files for testrpc managed accounts (you can use ethereumjs-wallet to import/export easily)

@tcoulter
Copy link
Contributor

an improved genesis file

Definitely agree the improved genesis file is the right answer, which could include the private keys needed for signing.

In my opinion --accounts is not mutually exclusive with the genesis file. It serves a different purpose as it stands.

I'm not sure I agree with that. The genesis file would include a list of initial accounts and, optionally, their starting balance. If I understand this PR correctly, that's what you're trying to achieve. If the improved genesis file were also included, --account would be another avenue for specifying the same information.

Assuming we go the genesis file route (not set in stone), here's what I imagine it would look like:

{
  // likely won't matter:
  nonce: "0x0000000000000042",

  // won't matter
  difficulty: "0x400000000",

  // specify initial accounts, balances, and private keys
  alloc: {
    "3282791d6fd713f1e94f4bfd565eaa78b3a0599d": {
      balance: "1337000000000000000000"   // Optional. Will be set to default if not specified.
      private_key: "..."                  // Optional. Non-standard.
    },
    ...
  },

  // Could matter in rare cases
  mixhash: "0x0000000000000000000000000000000000000000000000000000000000000000",

  // Easy to set, could matter in rare cases
  coinbase: "0x0000000000000000000000000000000000000000",

  // Likely won't matter
  timestamp: "0x00",

  // Likely won't matter
  parentHash: "0x0000000000000000000000000000000000000000000000000000000000000000",

  // Likely won't matter
  extraData: "0x11bbe8db4e347b4e8c937c1c8370e4b5ed33adb3db69cbdb7a38e1e50b1b82fa",

  // Good for specifying the initial gas limit.
  gasLimit: "0x1388"
}

Alternatively, we could create our own config. Something like:

{
  accounts: {
    "0x1234...": {
      balance: ...
      private_key: ...
    }
  },
  coinbase: "0xabcd...",
  // other options...
}

The good thing about using the genesis.json format is that the TestRPC could share configuration with other Ethereum clients that support it. The bad thing about it is there's extra config that likely won't matter to the TestRPC. In both cases, however, setting initial accounts and their private keys will be one of the main features.

@tcoulter
Copy link
Contributor

@axic Looks like we posted at the same time.

  1. supporting the official genesis json format as is
  2. having a separate testrpc specific configuration file or command line option
  3. optionally: support keystore files for testrpc managed accounts (you can use ethereumjs-wallet to import/export easily)

Completely agree. And supporting keystore files would be 💥 💰 🎉

I'm leaning toward 1 and 3. I suppose the question is: Should we also support --accounts? I could see how it could be useful. There would be many avenues for the same information, though.

@axic
Copy link
Contributor Author

axic commented Mar 10, 2016

@tcoulter if both genesis and the keystore is supported probably there is no need for that tbh. (You can still use bits of the PR - the first two commits.)

@kumavis
Copy link
Contributor

kumavis commented Mar 10, 2016

being able to set a full genesis is awesome, but my primary usage is just getting ether in an externally managed account. currently i do this by starting the testrpc and sending a tx from one of the managed accounts giving ether to my account. but it adds another setup step to the process.

all im looking for is testrpc --account 0xc25d4cb29b1ce5f7b14be118d8b6b1f0493b664a

@tcoulter
Copy link
Contributor

@kumavis The jury's still out on whether this should be merged, but you can write a quick script that will do this for you:

var TestRPC = require("ethereumjs-testrpc");
var Web3 = require("web3");

var server = TestRPC.server();

server.listen(8545, function(err, accounts) {
  if (err) throw err;

  var web3 = new Web3();
  web3.setProvider(new Web3.providers.HttpProvider("http://localhost:8545"));

  web3.eth.sendTransaction({
    from: accounts[0],
    to: "0x1234..."     // your address,
    value: 100000000000 // a gazillion
  }, function(err) {
    if (err) throw err;
    console.log("Success!");
  });
});

This will fire up a TestRPC server and transfer the balances all in one go.

@axic
Copy link
Contributor Author

axic commented Mar 10, 2016

How about having --unlock when:

  • presented with a small number unlocks the relevant keystore file (as in geth)
  • presented with an address unlocks the relevant keystore file
  • presented with a private key loads that key

@kumavis
Copy link
Contributor

kumavis commented Mar 12, 2016

unlocks the relevant keystore file

@axic there are no keystore filesm, everything is in memory

@tcoulter
Copy link
Contributor

@kumavis - I think he's suggesting copying over keystore files from geth.
On Mar 11, 2016 8:03 PM, "kumavis" notifications@github.com wrote:

unlocks the relevant keystore file
@axic https://github.com/axic there are no keystore filesm, everything
is in memory


Reply to this email directly or view it on GitHub
#24 (comment).

@axic
Copy link
Contributor Author

axic commented Mar 12, 2016

@kumavis it has been suggested a few comments up that it could be possible to support keystore files, too

@ghost
Copy link

ghost commented Mar 16, 2016

Just an update regarding the switch to TestRPC: ran into some sort of non-deterministic behavior which led to certain tests in Dapple failing. Ended up being easier to just update EtherSim's dependencies. Will keep an eye on TestRPC going forward, though. If you're interested in poking around yourself, the branch with the sometimes-failing tests is at https://github.com/ryepdx/dapple/tree/testRPC

@tcoulter
Copy link
Contributor

Will definitely check it out @ryepdx. Can you tell me which tests I should
be running?
On Mar 16, 2016 6:24 PM, "Ryan" notifications@github.com wrote:

Just an update regarding the switch to TestRPC: ended up running up
against some sort of non-deterministic behavior in TestRPC which led to
certain tests in Dapple failing. Ended up being easier to just update
EtherSim's dependencies. Will keep an eye on TestRPC going forward, though.
If you're interested in poking around yourself, the branch with the
sometimes-failing tests is at
https://github.com/ryepdx/dapple/tree/testRPC


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#24 (comment)

@ghost
Copy link

ghost commented Mar 17, 2016

npm test should trigger it. IIRC, it was the DSL tests that were being affected.

@tcoulter
Copy link
Contributor

@ryepdx Can you create a ticket that I can comment on on your dapple branch? I'd like to move this discussion there. If not, can you create a new TestRPC ticket for your specific issue? I have some questions, but don't want to bog down this ticket.

@axic @kumavis - I've been coming around to merging this PR, so the functionality exists at least for the short term. All opposed say "eye".

@ghost ghost mentioned this pull request Mar 19, 2016
@ghost
Copy link

ghost commented Mar 19, 2016

Issue created.

@tcoulter
Copy link
Contributor

Update on this. I'm adding a feature that will make the testrpc initial state deterministic, mostly to debug a race condition with the testrpc itself. This PR will be included with that feature. If all goes well, expect something today. (famous last words)

@tcoulter tcoulter merged commit 7d519a2 into trufflesuite:master Mar 24, 2016
tcoulter pushed a commit that referenced this pull request Mar 24, 2016
…. Add -a/--accounts options to specify how many accounts you want to create.
@tcoulter
Copy link
Contributor

Merged this PR. Thanks @axic!

Along with it I added two other options:

  • -s/--seed: Seed the random number generator, so accounts created are deterministic. Usage: testrpc -s someseed
  • -a/--accounts: Specify the number accounts created on initialization. Usage: testrpc -a 2

This is in addition to @axic's --account options.

@tcoulter
Copy link
Contributor

Also added one more option:

  • -d/--deterministic: Use a default seed and make addresses deterministic. This is for when you don't care about what the seed is. Usage: testrpc -d.

@axic
Copy link
Contributor Author

axic commented Mar 24, 2016

Great stuff, thanks @tcoulter!

One note on the deterministic feature, it would be still very useful to either:

  • provide a tool or command line option to print the private keys for the generated ones
  • or display the private keys of the generated ones upon running

One small risk I see is if in the future some other code is referenced by addAccount which uses the rng, then it won't generate the same 10 accounts.

@tcoulter
Copy link
Contributor

Good feature requests @axic, totally agree.

Regarding:

One small risk I see is if in the future some other code is referenced by addAccount which uses the rng, then it won't generate the same 10 accounts.

Can you elaborate? I'm not sure I understand.

@axic
Copy link
Contributor Author

axic commented Mar 24, 2016

It will only create the same set of accounts, if the following code is strictly run in succession and nothing else uses this.rng between account creation:

var secretKey = opts.secretKey || random.randomBytes(32, this.rng);

Right now addAccount is executed via async.times(). Make sure nothing else uses the same this.rng instance at least until the accounts are done.

@axic
Copy link
Contributor Author

axic commented Mar 24, 2016

Speaking of account creation, maybe you could use the lightweight HD wallet feature from ethereumjs-wallet and just generate 10 accounts on a specific HD path? 😎

@axic axic deleted the patch/user-accounts branch March 24, 2016 17:38
@tcoulter
Copy link
Contributor

@axic I think that's handled, mostly due to the initialization code in the Manager. Requests won't be pushed through until the accounts have been created. The only other thing that could use the rng in that case is more of TestRPC's initialization.

Relevant code:

@tcoulter
Copy link
Contributor

Speaking of account creation, maybe you could use the lightweight HD wallet feature from ethereumjs-wallet and just generate 10 accounts on a specific HD path?

I'm not well versed in the lightwallet, but I will accept a PR. :) What benefit would that provide other than being fancy? This is a mock ethereum client so we don't need to be (cryptographically)secure after all.

@tcoulter
Copy link
Contributor

Oh, hmm. If we use the lightwallet, that means the private keys are deterministic as well correct?

@axic
Copy link
Contributor Author

axic commented Mar 24, 2016

Yes, it would be fully deterministic and users could easily generate the same set. No magic required from testrpc's part.

@axic
Copy link
Contributor Author

axic commented Mar 24, 2016

Sample code:

const bip39 = require('bip39')
const hdkey = require('ethereumjs-wallet/hdkey')

var mnemonic = 'awake book subject inch gentle blur grant damage process float month clown'
var wallet = hdkey.fromMasterSeed(bip39.mnemonicToSeed(mnemonic)

var account = wallet.derivePath("m/44'/60'/0'/0/" + index) // index is a number
var address = account.getWallet().getAddressString() // 0x prefixed String
var publicKey = account.getWallet().getPublicKey() // Buffer
var secretkey = account.getWallet().getPrivateKey() // Buffer

If you want it to be really easy to use, offer the configurability of:

  • mnemonic
  • base path (even though the one I used is the suggested one by BIP44/SLIP44, not everyone adheres to that)

@tcoulter
Copy link
Contributor

@axic And you'd do this 10 times like we're doing now? Only issue is mnemonic's are fairly nasty from a UI perspective. Imagine having to input the pneumonic on the command line.

If only there were a less secure, easier to use version (TestRPC doesn't need security). I might opt to simply print out the private keys.

@tcoulter
Copy link
Contributor

Although -- thinking out loud at this point -- if the -d option were all that was needed, and we output the pneumonic for HD wallet users, that'd be a fine experience. But I suppose we could input the pneumonic at that point as well.

@axic
Copy link
Contributor Author

axic commented Mar 24, 2016

You do derivePath and the subsequent lines for each account.

On a commandline:
--mnemonic "awake book subject inch gentle blur grant damage process float month clown"

Assuming testrpc has a default mnemonic built in, the user could use import this mnemonic into the client app too.

Printing the private key on top of that I think is a useful feature anyway.

@tcoulter
Copy link
Contributor

Ya, I'm 👍, we should support it. I created a new ticket so we can discuss there: #42

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants