TypeScript Migration

[chikeichan]

From Pato (Nomic)

We are really interested in the migration to TypeScript, as we use TS for everything in Buidler. Some of the few places where we have untyped code is due to ethereumjs-tx, so we'd love it to be typed. All of its dependencies are already in TS, so migrating it should be straightforward.

Would love to kick off a discussion on whether this is needed/desired. Specifically trying to see if this is an issue that can begin now.

cc-ing recent contributors for discussion: @holgerd77 @danjm @youfoundron

[danjm]

@chikeichan I believe it has been concluded that this is desired. See some discussion here: ethereumjs/organization#28 and the plan on the organizational roadmap: https://github.com/ethereumjs/organization/blob/78199a9ab5462274cf645813549dad0278b12e01/docs/roadmap.rst

For ethereumjs-tx, there may be some bigger redesign needed anyway. We may want to do this prior too, or at the same time as, the typescript rewrite. I will write some notes on broader needed changes and link here, and we can consider best approach before a typescript rewrite states.

Also note that there is an open PR that does some work on typescript in this repo: #88

It will likely be closed once we commit to a clear migration plan for the whole repo.

[alcuadrado]

Hi @danjm, thanks for pointing to those links.

My original plan was to just add types to the library. I wasn't aware of a plan to redesign it, but I'm open to it. After working a little more with the library I see your point.

I also noticed that it's based on ethereumjs-util's defineProperties, which has been deprecated in this commit. Is there a plan to move away from it? Is there a new pattern for ethereumjs libraries?

[holgerd77]

@alcuadrado you can read up on this PR ethereumjs/ethereumjs-account#27 for the account library on the state of the defineProperties discussion, the method-removing code didn't make it in the final version though, mainly for temporary compatibility considerations.

This is also some good TypeScript transition reference PR one can go along, since we also updated lots of tooling along with this change.

[s1na]

One alternative to defineProperties (it's still quite similar) could be defining an RLPField type, which takes the configurations necessary for validation in its constructor and has a getter and setter method (maybe also type conversions). Then in Transaction we'd define all the fields explicitly with the type RLPField and also add a setter/getter for each of them that calls the RLPField's setter/getter. This approach has obviously much more boilerplate though.

One possible addition: define a RLPType which takes RLPFields in its constructor, and has methods like toJSON and encode which serializes all the fields. Then make Transaction (and other classes like Account) inherit from RLPType.

In the end it comes down to how much boilerplate is too much. The current approach in Account also has a nice trick (proposed by Chris), which forces typescript to define attributes on the class even when they'll be injected later dynamically.

An ideal solution would maximize benefiting from typescript types, is readable and doesn't introduce too much boilerplate in each of the types. But if there's no solution we could also continue using defineProperties. What do you folks think?

[s1na]

Oh, and since re-structuring is being discussed, I'd like to ask for comments on something else I've been thinking about (from ethereumjs/ethereumjs-monorepo#494):

Some side note: Currently the types we have are very heavy and serve multiple purposes. E.g. ethereumjs-block contains RLP serialization/deserialization of blocks, db interactions and also validation and other logic. I think we should break them and have separate types for each of these purposes. One type for RLP serialization, one for storing/fetching from db (which uses RLPBlock), one for consensus logic. The same applies to Blockchain and Tx. Not sure yet about details, but thought I'd ask for feedback on this idea. One other benefit for this separation is that we could then have a base class for the consensus logic part of the type, and have a child class for each fork for that type, e.g. PetersburgBlock which contains the logic for blocks after Petersburg.

[alcuadrado]

I branched off #144 and started migrating this library. I'll submit a PR in draft mode, as there are multiple things that I'd like to get feedback about. I went with Chris' approach, as I don't want to introduce breaking changes. Once typed, refactoring should be easier.

@s1na's ideas are rinteresting, should we keep that discussion here, or create a more general issue somewhere else?

Then in Transaction we'd define all the fields explicitly with the type RLPField and also add a setter/getter for each of them that calls the RLPField's setter/getter.

Do you mean explicitly adding them? If not, the situation would be similar to add the types and use defineProperties.

One possible addition: define a RLPType which takes RLPFields in its constructor, and has methods like toJSON and encode which serializes all the fields. Then make Transaction (and other classes like Account) inherit from RLPType.

This type sounds like it would be really handy.

An ideal solution would maximize benefiting from typescript types, is readable and doesn't introduce too much boilerplate in each of the types. But if there's no solution we could also continue using defineProperties. What do you folks think?

Personally, I'm ok with some boilerplate in exchange for easier to understand code, and getting more static guarantees.

I think we should break them and have separate types for each of these purposes. One type for RLP serialization, one for storing/fetching from db (which uses RLPBlock), one for consensus logic.

This sounds good. I once worked on a bitcoin library designed in this way. The base layer was immutable, and mapped to the protocol level representation's of things (that'd be RPL in ethereum). It was a nice design, but the project was abandonned so I can't tell how ergonomic it felt to work with it as a consumer.

One other benefit for this separation is that we could then have a base class for the consensus logic part of the type, and have a child class for each fork for that type, e.g. PetersburgBlock which contains the logic for blocks after Petersburg.

How would this work from an user of the library's perspective? Would they have to be aware of the different classes?

[alcuadrado]

I'm closing this issue now that #145 is merged.