Use Enums to define chain names, forks and other parameter values

[evertonfraga]

I believe that there's some benefit to use Enums to define chain names and forks. I made some mistakes while writing goerli in the past, and I am probably not the first to make typos around chain and fork names.

GIven that goerli in German is originally görlitzer, that should be a valid source of confusion.

Using Enums, developers also take advantage of code completion.

Rough example:

const goerli = new common(Chains.Goerli)

[holgerd77]

Yes, that makes very much sense.

[holgerd77]

I still find this desirable and already thought a couple of times about this, @rumkin recently mentioned this again along a review on #937, which triggered these additional comments.

I have the following open questions here:

A. Parameter Choice

What parameters should get enum types? Candidates I would see:

1. chains (e.g. string 'mainnet')
2. hardforks (e.g. string 'istanbul')
3. eips (e.g. number 2537) (yes/no? I have some tendency to do as well)
4. Consensus types (e.g. string 'pow')
5. Consensus algorithm (e.g. string 'ethash')

B. Library Extendability

My biggest concern with enums e.g. for chains is that we might have the side effect that this restricts the extendability of the library, e.g. if someone wants to fork the library and would want to add an own SOME_CHAIN.json file or something, another use case worth to mention might be our own (living a bit on the sideline) forCustomChain() functionality.

If we use these enum types for parameter typing - so e.g. do in CommonOpts hardfork?: Hardforks (or whatever the naming is) we would prevent people from being able to use their custom hardfork names, do I get this right? Has anyone a suggestion for a pattern for this to circumvent? Should we just leave the typing "as is" and do the enum types as an optional convenience which we then use ourselves? 🤔

C. API

What is a good API and/or integration choice for that so that import is easy and usage is simple and maybe don't produce to long function calls?

Some suggestion from my side, but rather just a first shot on this:

class Common() {
  public static enum ConsensusType = {
    PoW = 'pow',
    PoA = 'poa'
  }
}

And the usage would be:

Common.ConsensusType.PoW

Update: just realizing on trying that that this is not possible. So is the only option to do an export separate from the class here?

[jochem-brouwer]

The enum stuff is possible?

enum ConsensusType = {
    PoW = 'pow',
    PoA = 'poa'
  }

class Common { 
  public static ConsensusType = ConsensusType
}

I have to think a bit about (2) 🤔 Is adding an enum type which is specifically set to "Custom" an option here?

[jochem-brouwer]

Since this is an issue which has to do with introducing new types, if a PR is created we might want to also include #914 there?

[holgerd77]

I do get a bit nervous here regarding the release deadlines. My suggestion is to not get side-tracked here and resist to do any more work here, especially under consideration of (2), this whole custom chain thing is still underdeveloped in the whole Common library and I think this would generally deserve some extra thought and care which we can't provide in this context.

One way on this is always that we only introduce the enum types as optional helpers on a first round and keep the function signatures as we have now just with strings. Then we are not risking of breaking anything but nevertheless already have the new types introduced. We can then approach this more slowly and eventually do the type introductions along v6 releases. We don't have to do this work now though but can do sometime along the v5 release series work.

Regarding #914 I have the impression that this is something which we might still want to include, since changes are limit but breaking and the cleanup benefit is somewhat significant.

[jochem-brouwer]

Yes - #914 is mostly an internal thing, I don't foresee this would lead to any problems (these changes should mostly have to do with our internal implementations). I do agree that this is too big to include before this deadline. Maybe we should create a new tag and mark these specific issues (which are likely to lead to breaking changes) with a V6 label?

[holgerd77]

@jochem-brouwer yes, I think I will start early on with v6 planning, optimally directly once we have v5 out. Think it likely makes sense if we do a release planning a bit more integrated along the day-to-day development, having a dedicated label here is also a good idea to get things sorted a bit.

[holgerd77]

Some update here from my experiences when working on #1315 (tx active capabilities requesting via a Capabilities enum): TypeScript is actually not complaining when one is using an input other than an enum target value, see:

So there I am doing just a random number input to tx.active(), normal inputs would be the EIP numbers supported, e.g. 1559 given by Capabilities.EIP1559FeeMarket. I also tested compilation on this directly, also works without problems.

So I guess we are very much safe here to continue in the realm of a non-breaking release, and I would also think that we should very much do so, this would be indeed a big improvement. 😄

To make things simple I would suggest that we concentrate just on chain and hardfork parameters and initially do two enums Chains (referencing: see section 1) below) and Hardforks (referencing the lower-case hardfork strings like istanbul) in types.ts.

And then we use this for the following methods and update the usage of these methods throughout the monorepo:

• constructor (actually the most important switch)
• forCustomChain()
• setChain()
• setHardfork()
• paramByHardfork()
• hardforkIsActiveOnBlock()
• hardforkGteHardfork()
• gteHardfork()
• hardforkIsActiveOnChain()
• hardforkBlock()
• hardforkBlockBN()
• isHardforkBlock()
• nextHardforkBlock()
• nextHardforkBlockBN()
• isNextHardforkBlock()
• forkHash()

I guess that chain/HF use case is very much the most important thing to do here and then we are not side tracked (on the first iteration) by these other cases.

I guess this would be a good task for one of the new team members, @emersonmacro would you e.g. have some ressources to give this a start (not urgent, but would be good in the next 1-3 weeks (starting, not finishing))? Eventually other team members can also join in along the way if doing all these methods is getting too laborious. 😜

---

1. Here it gets a bit tricky since we are running into the out-of-safe-integer chainID question again (the baikal network had a very large chain ID which wasn't supported by the number type any more, so we switched everything to BN). I wouldn't feel too comfortable if we reference the lower case strings here (mainnet) (that's just a "feeling" though, or should we do?), and on the other hand we (likely ?) also can't use complex objects like BN as an enum value (and it is also very much the question if we want to enforce that here). So my take on this would be, also having a look at the methods being affected and the fact that baikal is now out again: we should simply reference the number value for now and maybe update this along the bigint transition. Since the chain type inputs would just be extended (e.g. setChain(chain: string | number | BN | object) -> setChain(chain: string | Chains | BN | object)) this would not bring any limitations on API usage and it feels somewhat the cleanest way to do to me right now.

[holgerd77]

(side note: please read all these long posts from this morning "on site" and not in mail, I've done a lot of edits and added additional clarifications after posting)

[emersonmacro]

@holgerd77 Yep, I can get started on this

[holgerd77]

Ah, at least half (or a quarter 😛) step back here: seems like what I said on function parameter input typed with an enum would allow e.g. other strings (if the enum values themselves are strings) is not correct.

Just working on a custom chain Common instantiation overhaul and have added some custom chain enum there and this does not allow for some other input:

This likely needs some more context from a PR I still need to push to get the whole picture (will push soon though).

[holgerd77]

(don't know what this means yet, maybe we need to still allow string input where we want to add enums as an additional explicit input type or something)

[emersonmacro]

@holgerd77 I put up an initial WIP PR #1322. Wanted to make sure I'm on the right track before getting into the "update the usage of these methods throughout the monorepo" part. Looking OK so far?

[holgerd77]

@emersonmacro yeah, looks great, thanks for opening this up! 😄

One open question I have - maybe also for @ryanio - does this have any benefits to do the typing with e.g. Chains at all if we keep string anyhow? Respectively - other way around - does this have any drawbacks and might there be reasons to just keep this as string for now? 🤔

[ryanio]

keeping string input would be good for compatibility so we don't all of a sudden have breaking changes everywhere, but gradually introducing enums seems like a smart way to move toward typed options that would reduce accidental user error for typos and introduce greater dev ux like auto completion.

[alcuadrado]

Personally, I think I'd keep the input types as string forever.

You can extend an enum, but it's not something trivial to do in ts, and probably painful in js.

But still, having enums for the non-custom params would be great

[rumkin]

I think plain objects could be good alternative to enums. As suggested by TypeScript's enum documentation, it's possible to use plain objects as dictionaries and a type source (ts playground):

// Create dictionary of allowed values
const Features = Object.freeze({
    EIP1: 'EIP1',
    EIP2: 'EIP2',
} as const)

// Define type from dictionary keys
type Feature = typeof Features[keyof typeof Features]

// Consume type
function useFeature(dir: Feature) {
    if (dir === Features.EIP1) {
      console.log('EIP1')
    }
    else if (dir === Features.EIP2) {
        console.log('EIP2')
    }
    else {
        console.log('other')
    }
}

// ✅ No errors
useFeature(Features.EIP1)
useFeature('EIP1')
useFeature('EIP2')

// ❌ Errors
useFeature('')
useFeature(1)

Pros

1. It allows string as input.
2. It doesn't allow input of unknown values.
3. It's more JS than TS way.

Cons

1. It's wordy.
2. The syntax is awkward and requires comments.

[alcuadrado]

There's an alternative type of enums, const enum, @rumkin. It creates no runtime values. Take a look at this example

PS: I also added an explanation about how to augment/extend enums in there.

[rumkin]

@alcuadrado Thanks for extension example. This is good when used in TS, but these enums could disappear when used in JS. I personally would like to have these values available as JSON/JS wrapped into package to use as dependency.

[ryanio]

Resolved with #1322 and #1363 🎉