This library is too restrictive for testing networks

[alcuadrado]

I'm planning to upgrade ethereumjs-tx in Buidler, and found that it may not be possible to do that if I want to enable EIP155.

The problem is that testing networks normally use non-standard chain ids (e.g. Ganache). If I want to sign a tx locally, I should do something like:

const { Transaction } = require("ethereumjs-tx");
new Transaction({ ... }, { chain: 123 });

But this results in this error:

Error: Chain with ID 123 not supported
    at Common.setChain (/private/tmp/tcommon/node_modules/ethereumjs-common/dist/index.js:35:23)
    at new Common (/private/tmp/tcommon/node_modules/ethereumjs-common/dist/index.js:16:34)
    at new Transaction (/private/tmp/tcommon/node_modules/ethereumjs-tx/dist/transaction.js:65:28)
    at Object.<anonymous> (/private/tmp/tcommon/index.js:2:1)
    at Module._compile (module.js:653:30)
    at Object.Module._extensions..js (module.js:664:10)
    at Module.load (module.js:566:32)
    at tryModuleLoad (module.js:506:12)
    at Function.Module._load (module.js:498:3)
    at Function.Module.runMain (module.js:694:10)

Taking a look at setChain I noticed that it's impossible to use a non-standard chain id.

The only workaround I see is by extending this class or creating a new one altogether, but it doesn't feel right for users to have to do this.

I'd LOVE to have an option to create a chain with the same params than a standard one, except for the genesis state and the chain id.

[holgerd77]

Definitely makes sense - also to directly integrate here into the Common module - we should give the behavioral structure some thought though.

My first thinking was: does this really help that the library throws when e.g. instantiating with a non-defined chainId or name and my first intuition was: why not instantiate with some default settings (e.g. just take the mainnet configuration) and the differing ID.

On a second thought I got more doubts and think this does diminish the library security, e.g. if someone has a typo in mainnet (e.g. meinnet) or uses a wrongly remembered chain ID or whatever and the library silently works on the wrong switch settings.

Third thought, just as a suggestion for discussion: another option strictMode (a more defining naming idea is welcome) being true as default would be a way. This is backwards compatible, on strictMode set to false the library would then (suggestion!):

• Instantiate with default values as described above when e.g. setting a non-existing chain id or chain name (myTestChain)
• Don't throw when resetting the relevant values to something non-existing

Does this make sense? Another idea? Did I forget anything?

[holgerd77]

Side note: there is actually the possibility to just pass a completely custom configuration json file right now (see README section), seems to be not a practicable solution to this problem though since this is too costly for most use cases in this context.

[holgerd77]

@alcuadrado Think we should tackle this relatively quickly, this is likely a serious limitation for using projects, now especially on the tx library. Do you have any thoughts on my comments?

One thing also to consider: this atm also completely disallows chains which are not yet defined within the library. This is another strong reason for tackling this. From the other side we can ease the problem by adding at least basic configs for missing chains. Is there anywhere some up-to-date list with used chain ids?

[Aralun]

Yep, this is a very inconvenient change there. On 2.0 the tx lib is effectively unusable with test chains. This is an extremely serious regression and actively impedes dev flows.

As for your original issue: Why not simply use a well-known record of chains? For example, one could use Common.mainnet, Common.fromId(0) or whichever other namespace you want to pick. So if someone types Common.meinnet it'll undefined away and it's a lot easier ― and healthier ― to catch.

[holgerd77]

Dropping this here for reference: https://github.com/ethereum-lists/chains

Maybe we start conservatively with adding. 😛

[holgerd77]

@alcuadrado after digging into the code a bit, I would cautiously support a new opts.fallbackChainConfig setting on an optional opts dictionary parameter (to remain backwards compatible). This can then be set to the existing/configured chains. If parameter is set (e.g. { fallbackChainConfig: 'mainnet' } or { fallbackChainConfig: 1 } (this should allow just the same formats as setChain for consistency)) library is initialized with respective chain parameters on constructor or setChain calls if chain is not recognized. If parameter is not set library behaves as before/now and will throw.

Seems to be the most flexible solution for me. What do you think?

Side note: was alternatively thinking of just name opts.fallbackChain, not sure.

[alcuadrado]

I think I came up with a better solution. We can create a new static method that makes using the current constructor easier.

I'm not convinced about its name, but it would work something like this:

  static createFromStandardChain(
    standardChain: string | number,
    customChainParams: Partial<Chain>,
    hardfork?: string | null,
    supportedHardforks?: Array<string>,
  ): Common {
    const standardChainParams = Common.getChainParams(standardChain)

    return new Common(
      {
        ...standardChainParams,
        ...customChainParams,
      },
      hardfork,
      supportedHardforks,
    )
  }

This way customizing just parts of the chain params can be as easy as:

const myChainParams = {name: "my-chain",  chainId: 1337, networkId: 1337};
const common = Common.createFromStandardChain("mainnet", myChainParamas, "constantinople")

I think name and chainId will be the most commonly changed params, but this method opens the possibility of customizing whatever you want.

@Aralun any feedback on these proposals?

[Aralun]

@alcuadrado Looks way better than what I had in mind yes. As an alternative name, I'm thinking about Common.custom but it sounds kind of like an oxymoron.

Plus, it solves @holgerd77's point of silently entering wrong configurations as the current constructor behaviour is preserved.

Overall looks very good to me =)

[alcuadrado]

I went ahead and implemented it in #55.

I named the factory method Common.forCustomChain, which I believe is concise and descriptive enough, without sounding confusing like Common.custom.

[holgerd77]

@alcuadrado yes, that's really an elegant solution and a significantly better pattern than my proposal, great!

[alcuadrado]

This has been fixed in #55, which will be released in v1.3.0.