fix: add an upper limit to # of accounts that can be generated by ganache

[tenthirtyone]

Issue #736 calls for a cap on the totalAccounts generated by ganache. This change sets a cap at 1000 accounts. 1000 was chosen because it was the largest power of ten that kept its unit tests under < 1s.

Fixes #736

[jeffsmale90]

Other than my one query, I give this the 👍

[tenthirtyone]

Since I have to back out the changes in the cli and move them to wallet && I borked the conventional commit, I am going to close this pr/del branch and re-open with the appropriate conventional commit.

[davidmurdoch]

Not sure why you need to close. The conversations here may be valuable in the future and it would be nice to be able to reference this if we need to; that becomes more difficulty in a closed PR simple because it's not as visible (since you'd have to link to it from another PR to know to get to it.)

[tenthirtyone]

@davidmurdoch Alrighty.

[tenthirtyone]

Summary:

• We decided to move this change out of options and into wallet.ts
• Nix console.log for logger.log
• Nix a soft/hard cap in favor of a warning
• Set the warning at 100000

Cool.

I thought logger.log was available in wallet.ts but EthereumInternalOptions["wallet"] does not include logger so I passed options.logging to the constructor. I expect that to change. Can I get some clarity on logger.log inside wallet?

Specifically, should I pass the whole EthereumOptions in and pull out wallet/logger or what is preferred ?

[jeffsmale90]

should I pass the whole EthereumOptions in and pull out wallet/logger or what is preferred ?

My preference is to pass in just what is required, narrow the interface and all that.

[tenthirtyone]

My preference is to pass in just what is required, narrow the interface and all that.

Since EthereumOptions object is the options for multiple classes and the logger was previously unavailable to the wallet, part of the conversation is do we update all those class constructors as well or leave wallet as a snowflake/unicorn?

My assumption is uniformity. So do we update every instantiation of chain, database, miner, and fork to bolt on the extra logger param, so they will now match wallet even though they have no reason to be coupled to logger. Or do we let the class constructors describe the interface with destructuring and now the instantiation is uniform but classes that do not need logger are not unnecessarily coupled.

here

const wallet = new Wallet(options.wallet); // becomes
const wallet = new Wallet(options.wallet, options.logging);

And the constructor becomes

here

  constructor(opts: EthereumInternalOptions["wallet"]) { // becomes
  constructor(
    opts: EthereumInternalOptions["wallet"],
    logging: EthereumInternalOptions["logging"]
  ) {

or, use destructuring

      const wallet = new Wallet(options.wallet); // becomes
      const wallet = new Wallet(options); 

and the constructors

  constructor({
    wallet: opts,
    logging // only for classes that need logger
  }: {
    wallet: EthereumInternalOptions["wallet"];
    logging: EthereumInternalOptions["logging"];
  }) {

Or my assumption is wrong about uniformity. Or something else.

[MicaiahReid]

My preference would be to pass in only what is required (so we'd now be passing in options.wallet, options.logging rather than expanding it to options).

And I'd extend that preference to pass in only what is required to all of the classes over having uniformity between those classes. I don't see the need to pass logging options into chain, database, etc. just because we're using them for the wallet.

[davidmurdoch]

I think we should leave it as you have it right now new Wallet(options.wallet, options.logging);; if only so we can finally close this issue. :-p

[tenthirtyone]

finally close this issue. :-p

yes.

[jeffsmale90]

I love it!

[davidmurdoch]

LGTM lol