TypeScript migration

[alcuadrado]

This is a WIP PR migrating this project to TypeScript. It is heavily inspired on https://github.com/ethereumjs/ethereumjs-account/pull/27. In particular how the code was migrated and which tools are used were copied from there.

This PR branches off #143 as I wanted to have an updated version of the official tests for this migration. That update was made in #131, but #143 fixes an error introduced by it.

Some things were I'd love some early feedback are:

1. I had to define lots of types to avoid using any and still support the whole range of values that the released version supports. Is this OK, or should the new version be more restrictive for TS users?
2. Should those types be defined here or in ethereumjs-util? Many of them are related to defineProperties.
3. I had to add @types/bn.js as a dev dependency because ethereumjs-util and rpl's typing files import them and don't declare it as a dependency. This doesn't feel right to me.

[holgerd77]

Hi @alcuadrado, great that you are tackling this, this looks already great! 😄

[holgerd77]

1. We are generally moving in a direction to make things more strict. From a procedural standpoint I would nevertheless not want to change the API along with this broad changes. This is likely to risky and we should address this separately. So we can either temporarily keep the types you defined or just use any + type comments in TSDocs, as you prefer.

2. Related to 1. these types can be left here.

3. Dependency should be added to util and rlp then.

[alcuadrado]

Thanks for your reply @holgerd77

So we can either temporarily keep the types you defined or just use any + type comments in TSDocs, as you prefer.

I went for the first option, as it plays better with tools like ides and typedoc.

3. Dependency should be added to util and rlp then.

That was my first thought. But I'm not quite sure about it. @krzkaczor do you have any opinion on this?

This PR is almost ready. I tried not to introduce breaking changes, but there are two things I'm not completely sure how to handle:

1) Importing this package

The current version of this library has four ways of importing it:

• require("ethereumjs-tx")
• require("ethereumjs-tx/fake")
• require("ethereumjs-tx/es5")
• require("ethereumjs-tx/es5/fake")

Special configuration (i.e. different from ethereumjs-config's) is needed to preseve the last three options. I didn't implement such configuration. I'd like to know your thoughts about this first.

2) Assigning a field in TS is more restrictive now

I used the same migration approach than ethereumjs-account, and noticed that it introduces a small breaking change for typescript users.

defineProperties adds setters to for field. These setters can receive different types. Now the fields are also defined in TS, so only Buffers can be assigned to them. The setters defined by defineProperties are still being executed. The restriction is imposed by the typechecker.

Javascript users won't notice this, and their code won't break. Typescript users may have to introduce a few toBuffer calls, but at least the error messages will be clear.

Note that constructors aren't affected by this.

Should we keep it this way?

Finally, I tried to run the tests on firefox with karma and typescript. I'm getting an error about the asyncawait library having a syntax error.

Is there a typescript project with browser tests I can look at?

Notes for reviewers:

1. Most commits are small and self-contained.
2. I changed the jsdoc comments to tsdoc, and docummented the types. Some docs were duplicated so I removed them.
3. ethereumjs-config-tslint-fix reformatted lots of things, so the diff looks much bigger than it actually is.

[alcuadrado]

Also, I replaced some instances of this pattern with default parameters.

They are not 100% equivalent. Default parameters only handle undefined, and the old code handles any falsy value.

Should we consider this a breaking change and revert it?

[holgerd77]

On 1., Importing Pre-note: I will do a major release anyhow on the library since latest PRs brought deep reaching changes, so if it makes sense to do breaking changes, we can very well do.

The first two "es5" import options shouldn't be used at all, so it would be very good if we get officially rid of them on this occasion.

If we change Fake transaction import, so be it. My preferred way would be that we move the content from index.ts to a separate transaction.ts file, use index.ts just for exposing the API and then have exposure and import structure on usage similar like in this PR on the VM: https://github.com/ethereumjs/ethereumjs-vm/pull/496

[holgerd77]

2. Field Assignment I think we could live with that (?). We could also use this occasion and do to Buffer requirements all over the place and tighten the API here. @s1na what do you think, do we want that?

[holgerd77]

(sorry, I would love to better format my posts, but I'm always writing on mobile these days and this gets pretty difficult)

[holgerd77]

3. Asyncawait

This has already been addressed in the testing library (by removing the dependency). I still have to do a release on this, then you can directly update the ethereumjs-testing dependency, should happen during this week, will let you know.

[alcuadrado]

If we change Fake transaction import, so be it. My preferred way would be that we move the content from index.ts to a separate transaction.ts file, use index.ts just for exposing the API and then have exposure and import structure on usage similar like in this PR on the VM: ethereumjs/ethereumjs-vm#496

I like this idea. Will update the PR with this.

This has already been addressed in the testing library (by removing the dependency). I still have to do a release on this, then you can directly update the ethereumjs-testing dependency, should happen during this week, will let you know.

Great, that's one of the few things missing here.

[s1na]

Re field assignment: I think it should be fine to restrict it define the types to be Buffer for TS users. We'll eventually want to restrict it even for JS users, and as you mentioned TS users will only get a compile-time error, and runtime allows the other types.

Just had a look, this is looking great!

By the way, do you by any chance know the purpose of FakeTx?

[alcuadrado]

By the way, do you by any chance know the purpose of FakeTx?

TBH I have no idea why FakeTransaction exists. There's not much data about it in its git history, but most of the people there are part of the Truffle/Ganache team. Maybe they use it in Ganache?

[holgerd77]

@alcuadrado Ok, just released v1.2.8 on ethereumjs-testing.

For context: this is always just done as a tagged release and not published on npm due to npm package size limitations (test submodule is too big). So library is just referenced by its version tag.

[holgerd77]

FakeTransaction: haven't looked deeper into it, but here is a code search on ganache, FakeTransaction is used on at least two occasions, didn't look into the usage scenario yet.

[Naderakhlagh]

Unsctibed

در تاریخ پنجشنبه ۱۸ آوریل ۲۰۱۹،‏ ۱:۳۲ Holger Drewes < notifications@github.com> نوشت:

FakeTransaction: haven't looked deeper into it, but here https://github.com/search?q=org%3Atrufflesuite+fake&type=Code is a code search on ganache, FakeTransaction is used on at least two occasions, didn't look into the usage scenario yet.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/ethereumjs/ethereumjs-tx/pull/145#issuecomment-484260113, or mute the thread https://github.com/notifications/unsubscribe-auth/AJWWNGKWFYRHHRTWFMOLIPDPQ6F5TANCNFSM4HFZHYQA .

[alcuadrado]

Re field assignment: I think it should be fine to restrict it define the types to be Buffer for TS users. We'll eventually want to restrict it even for JS users, and as you mentioned TS users will only get a compile-time error, and runtime allows the other types.

Let's leave it like that then.

@alcuadrado Ok, just released v1.2.8 on ethereumjs-testing.

For context: this is always just done as a tagged release and not published on npm due to npm package size limitations (test submodule is too big). So library is just referenced by its version tag.

Thanks for the release and the explanation. I was very intrigued about the git reference in the package.json.

I'm still having errors that prevent me from running the tests in a browser. I installed https://www.npmjs.com/package/karma-typescript, which seems like the most popular way of using Karma with TS. This project doesn't support ES6, so it fails to load ethereumjs-testing. I tried compiling that module, but the tests fail to run for other reasons anyway.

Is any other ethereumjs project running tape tests in the browser?

[holgerd77]

The latest karma test activation was on the VM https://github.com/ethereumjs/ethereumjs-vm/blob/master/karma.conf.js, on the v4 branch this is also running in a TypeScript context, maybe you can get some inspiration from there.

[s1na]

In VM (v4) typescript files are first built and karma is run on the dist files.

[alcuadrado]

Thanks for pointing that out @s1na. I just updated this PR. It builds the tests before running them now.

I couldn't get the official tests to run in a browser. I'm pretty sure ethereumjs-testing can't be run in a browser, so I opened this issue. I also disabled the official tests in browser runs here. Re-enabling them once ethereumjs-testing gets fixed will be trivial.

The last standing thing before considering this ready for review is this:

Also, I replaced some instances of this pattern with default parameters.

They are not 100% equivalent. Default parameters only handle undefined, and the old code handles any falsy value.

Should we consider this a breaking change and revert it?

[alcuadrado]

Small update: I reapplied #138 here manually, as so many things changed that rebasing was harder. Is this ok?

[holgerd77]

@alcuadrado ah sorry, but rebasing is an absolute inviolable precondition for having a PR like this merged, especially if the code changes building on top are so heavy.

[holgerd77]

(but maybe I'm also just not completely understanding what you have done, how would this actually work during merge?)

[holgerd77]

Hmm, I wouldn't see this default parameter thing as too problematic as I am just not seeing the case one could have misused this or this triggers an error. Maybe I am missing something though. For the moment I wouldn't regard this as breaking.

[alcuadrado]

@alcuadrado ah sorry, but rebasing is an absolute inviolable precondition for having a PR like this merged, especially if the code changes building on top are so heavy.

You are right @holgerd77, it would have been a mess. I rebased it.

Hmm, I wouldn't see this default parameter thing as too problematic as I am just not seeing the case one could have misused this or this triggers an error. Maybe I am missing something though. For the moment I wouldn't regard this as breaking.

The only case that comes to my mind is if someone passes null as a no-param value. This is done in the examples. Note that that file is super outdated. I will update it now.

[alcuadrado]

This PR is ready, except that it depends on #143.

The CI will fail until #143 is merged and this one rebased. The problem is that #143 doesn't have #138 applied, and travis is running an unsupported version of node. I already tried applying this PR into master, and that fixes it.

[danjm]

@alcuadrado I've rebased #143, so we should be able to soon merge

[alcuadrado]

143 is merged now. I rebased this PR against master.

[holgerd77]

Hi @s1na, I am still a lot more reduced in time than I hoped for. Could you eventually do some side-path and do the final review here? Or is this too distracting from your current VM work (really don't feel obliged, would be appreciated though, maybe that's also some nice dive into this library)?

[s1na]

Yeah sure, I had in mind to help with the review here. Will try to do an initial round shortly.

[s1na]

Short update: I integrated this into VM and apart from changes needed for imports, it runs and the tests pass! this is already a good sign. Will go through the code in more detail.

Just one quick comment regarding: I'm not sure I understand the benefit of the PrefixedHexString type?

[alcuadrado]

Short update: I integrated this into VM and apart from changes needed for imports, it runs and the tests pass!

That's great :) How did you do it? npm pack and copy the result?

Just one quick comment regarding: I'm not sure I understand the benefit of the PrefixedHexString type?

I like naming types for documentation purpose. I'm also ok with using just string.

[holgerd77]

Ok. Then let's merge here. 😄

[holgerd77]

Damn, sorry, maybe this was too rushed.

I was not aware any more that this PR was not pointing towards master but fix-signing-eip155-transactions. Changes are not showing up on master, a bit helpless on how to proceed. 😒

Sorry for the mess.