TypeScript migration #145

alcuadrado · 2019-04-13T22:49:56Z

This is a WIP PR migrating this project to TypeScript. It is heavily inspired on ethereumjs/ethereumjs-account#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:

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?
Should those types be defined here or in ethereumjs-util? Many of them are related to defineProperties.
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 · 2019-04-14T15:26:22Z

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

holgerd77 · 2019-04-15T07:24:01Z

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.
Related to 1. these types can be left here.
Dependency should be added to util and rlp then.

alcuadrado · 2019-04-16T21:54:41Z

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.

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:

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.

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:

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

alcuadrado · 2019-04-16T22:01:52Z

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 · 2019-04-17T09:22:37Z

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: ethereumjs/ethereumjs-monorepo#496

holgerd77 · 2019-04-17T09:28:10Z

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 · 2019-04-17T09:29:25Z

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

holgerd77 · 2019-04-17T09:32:43Z

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 · 2019-04-17T14:46:39Z

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 · 2019-04-17T17:59:53Z

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 · 2019-04-17T18:50:33Z

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 · 2019-04-17T20:58:23Z

@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 · 2019-04-17T21:02:14Z

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.

alcuadrado · 2019-04-18T18:10:35Z

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 · 2019-04-19T08:10:18Z

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 · 2019-04-19T10:20:18Z

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

alcuadrado · 2019-04-22T16:12:54Z

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 · 2019-04-22T16:23:13Z

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

holgerd77 · 2019-04-22T18:28:17Z

@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 · 2019-04-22T18:29:48Z

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

holgerd77 · 2019-04-22T18:35:12Z

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 · 2019-04-22T19:12:28Z

@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 · 2019-04-22T19:55:04Z

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.

This was used in previous versions of this library to export ES5-compatible versions of the code. This is no longer needed.

alcuadrado · 2019-04-23T14:21:01Z

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

holgerd77 · 2019-04-25T11:04:58Z

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 · 2019-04-25T11:24:12Z

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

s1na · 2019-04-25T18:02:49Z

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 · 2019-04-25T18:12:20Z

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.

s1na

Very good PR overall, thanks @alcuadrado :) Just a few minor questions/points.

s1na · 2019-04-26T07:29:26Z

package.json

@@ -2,16 +2,25 @@
  "name": "ethereumjs-tx",
  "version": "1.3.7",
  "description": "An simple module for creating, manipulating and signing ethereum transactions",
-  "main": "es5/index.js",
+  "main": "dist/index.js",
+  "types": "dist/index.d.ts",


Probably also needs files to point to dist/**/*

You are right! I added

"files": [ "dist" ]

as that's how it's set here.

s1na · 2019-04-26T08:07:31Z

src/transaction.ts

+      }
+      this._common = opts.common
+    } else {
+      const chain = opts.chain ? opts.chain : 'mainnet'


@holgerd77 Is this maybe also a good occasion to drop some deprecated options? namely opts.chain and opts.hardfork which are now covered by opts.common

That would simplify the constructor a lot. I don't know how much of an impact this change would have though.

These Common and chain/hardfork options were introduced in parallel along with the the Common class introduction. Reasoning is to be able to use a shared common class more in a programmatic/dynamic context as well as keep a simple option for users who just want to instantiate e.g. a chain:goerli tx without having to deal with the extra Common dependency.

Not sure if this is ideal but this has been introduced along all libraries using the Common library as a shared pattern so for now I would like to stick with that if there is no strong reasoning against.

Oh, sorry I thought they were for backwards compatibility. No I don't feel very strongly about this.

s1na · 2019-04-26T08:14:03Z

src/transaction.ts

+    if (chainId < 0) chainId = 0
+
+    // set chainId
+    if (opts.chain || opts.common) {


Sorry this is not directly related to your PR, but could you maybe add a short note to the tsdoc that explains the order of priority for chainId? if I'm not mistaken its 1. opts.chain || opts.common.chainId(), 2. chainId from signature and lastly data.chainId 4. defaults to 0! Not sure why so complex.

I'll document it if we decide to keep it. I agree that it's too complex.

//cc @danjm

See related comment from @danjm here: #131 (comment)

I understand this has been to avoid breaking changes and for the convenience of the user. I do think however that it makes it very easy for the user to make mistakes given that a single value can be set from 4 sources in an arbitrary order. What I propose:

Make common a mandatory option, remove opts.chain, data.chainId and throw if common.chainId() doesn't match the one from signature (if present). Making common mandatory can also allow us to deprecate chain/hardfork and allow users to migrate.

That said, I don't feel very strongly and this was simply my (relatively uninformed) opinion.

@alcuadrado what do you think? We can address this in a separate PR though but whatever eases the process.

I'm not sure if making common mandatory is the best, as users that aren't familiar with it will find it confusing.

One option is to throw whenever there are incompatible sets of options. E.g. if you pass it common and chain, throw despite their values.

I opened #149 to continue this discussion.

if you pass it common and chain, throw despite their values.

Just a note that this is already done here: https://github.com/ethereumjs/ethereumjs-tx/blob/master/src/transaction.ts#L70

Ok, then let's move on here with keeping Common and alternatively chain/hardfork passing "as is" together with the throw mechanism on incompatible options @danjm mentioned. Otherwise we'll implement what Sina proposed above:

Make common a mandatory option, remove opts.chain, data.chainId and throw if common.chainId() doesn't match the one from signature (if present). Making common mandatory can also allow us to deprecate chain/hardfork and allow users to migrate.

And we then access chainId solely through the passed or generated Common instance.

Does this make sense?

What is with this 0 default value. Is this necessary respectively does it make sense?

@alcuadrado Can you prepare a PR on this once we have an agreement?

s1na · 2019-04-26T08:25:01Z

src/transaction.ts

+   * Returns the rlp encoding of the transaction
+   */
+  serialize(): Buffer {
+    // Note: This never gets executed, defineProperties overwrites it.


Hm, I think I had also added this to ethereumjs-account. But I think you're right that it gets overwritten by defineProperties. In that case would there be a reason for keeping it (apart from documentation)?

It's needed for TS. You wouldn't be able to call tx.serialize() otherwise, as it won't be present in the Transaction type.

We may be able to do declare it as a field, something like serialize!: () => Buffer. Not sure what's more confusing.

We probably want to do that for toJSON. Otherwise, we'll have to duplicate a lot of logic.

s1na · 2019-04-26T08:28:12Z

src/transaction.ts

+  validate(): boolean
+  validate(stringError: false): boolean
+  validate(stringError: true): string
+  validate(stringError: boolean = false): boolean | string {


Is this last definition necessary for typescript? Don't the first three definitions cover all cases?

This is something tricky, but really useful, about typescript. When you are overloading functions/methods, the last one is not exposed. It's just the implementation of the preceding overloads.

This may look weird initially, but it lets you expose very clean overloads. If the implementation were exposed, there will always be a weird overload.

s1na · 2019-04-26T08:41:10Z

karma.conf.js

    },
    singleRun: true,
-    plugins: [


These aren't needed anymore? in that case maybe we can also drop their dependencies.

It's just the declaration that isn't needed. I had to update karma, and the newer version autoloads plugins AFAIK.

s1na · 2019-04-26T08:43:13Z

package.json

@@ -24,26 +33,34 @@
    "ethereumjs-util": "^6.0.0"
  },
  "devDependencies": {
-    "async": "^2.0.0",
-    "babel-cli": "^6.22.2",
-    "babel-preset-env": "^1.6.1",


The .babelrc file can also be removed

Good catch! :)

s1na · 2019-04-26T08:47:39Z

package.json

+    "prepublishOnly": "npm run test && npm run build",
+    "coverage": "ethereumjs-config-coverage",
+    "coveralls": "ethereumjs-config-coveralls",
+    "docs:build": "typedoc --out docs --mode file --readme none --theme markdown --mdEngine github --excludeNotExported src",


By the way, what do you think about --gitRevision master for the docs? see ethereumjs/ethereumjs-util#181 for context

I have mixed feelings.

I think it'd be good to keep the docs pointing to their corresponding commit, but we are regenerating them manually, and they can easily get outdated.

Most people will get to them through the repo, so pointing to master will be right in the majority of the cases. It will only be wrong when navigating a specific version.

I guess the best solution would be for some service to regenerate them on every push/merge. Something like netlify. But that'd mean hosting them somewhere else.

holgerd77 · 2019-04-29T09:27:28Z

Ok. Then let's merge here. 😄

holgerd77 · 2019-04-29T09:33:32Z

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.

alcuadrado mentioned this pull request Apr 16, 2019

Exporting sources and declarationmaps ethereumjs/ethereumjs-config#21

Closed

alcuadrado added the PR state: WIP label Apr 17, 2019

This comment has been minimized.

Sign in to view

alcuadrado mentioned this pull request Apr 22, 2019

This module can't be run on a browser ethereumjs/ethereumjs-testing#40

Open

alcuadrado marked this pull request as ready for review April 22, 2019 19:27

alcuadrado added PR state: needs review and removed PR state: WIP labels Apr 22, 2019

alcuadrado added 11 commits April 23, 2019 11:13

Autoformat

968b762

Rename index.ts to transaction.ts

ff89a29

Export everything in index.ts

3309a90

Regenerate docs

d27a7a9

Remove es5 folder.

d3b3e5b

This was used in previous versions of this library to export ES5-compatible versions of the code. This is no longer needed.

Explicitly import Buffer in the tests

660cb76

Build tests before running them in the browser

3b4aa3c

Add comment to karma config

8e26dd4

Fix fake's test

baf79cc

Update examples

be17f37

Autoformat

17f2fc6

This was referenced Apr 23, 2019

Update .travis.yml to include nodejs v11 #139

Closed

Add node 11 to travis.yml #146

Merged

s1na suggested changes Apr 26, 2019

View reviewed changes

s1na reviewed Apr 26, 2019

View reviewed changes

alcuadrado added 2 commits April 26, 2019 12:03

Add files field to package.json

cdc8db1

Remove .babelrc

3cbfb5e

s1na approved these changes Apr 29, 2019

View reviewed changes

holgerd77 merged commit 0d87acf into ethereumjs:fix-signing-eip155-transactions Apr 29, 2019

alcuadrado mentioned this pull request Apr 29, 2019

Simplify Transaction constructor's options #149

Closed

alcuadrado deleted the typescript-migration branch April 29, 2019 19:24

alcuadrado mentioned this pull request May 9, 2019

TypeScript Migration #142

Closed

alcuadrado mentioned this pull request May 17, 2019

Simplify transaction's constructor and EIP155 handling #153

Merged

TypeScript migration #145

TypeScript migration #145

Conversation

alcuadrado commented Apr 13, 2019 • edited Loading

holgerd77 commented Apr 14, 2019

holgerd77 commented Apr 15, 2019

alcuadrado commented Apr 16, 2019

alcuadrado commented Apr 16, 2019 • edited Loading

holgerd77 commented Apr 17, 2019

holgerd77 commented Apr 17, 2019

holgerd77 commented Apr 17, 2019

holgerd77 commented Apr 17, 2019

alcuadrado commented Apr 17, 2019

s1na commented Apr 17, 2019 • edited Loading

alcuadrado commented Apr 17, 2019

holgerd77 commented Apr 17, 2019

holgerd77 commented Apr 17, 2019

This comment has been minimized.

alcuadrado commented Apr 18, 2019 • edited Loading

holgerd77 commented Apr 19, 2019

s1na commented Apr 19, 2019

alcuadrado commented Apr 22, 2019 • edited Loading

alcuadrado commented Apr 22, 2019

holgerd77 commented Apr 22, 2019

holgerd77 commented Apr 22, 2019

holgerd77 commented Apr 22, 2019

alcuadrado commented Apr 22, 2019 • edited Loading

alcuadrado commented Apr 22, 2019

alcuadrado commented Apr 23, 2019

holgerd77 commented Apr 25, 2019

s1na commented Apr 25, 2019

s1na commented Apr 25, 2019

alcuadrado commented Apr 25, 2019

s1na left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holgerd77 commented Apr 29, 2019

holgerd77 commented Apr 29, 2019

alcuadrado commented Apr 13, 2019 •

edited

Loading

alcuadrado commented Apr 16, 2019 •

edited

Loading

s1na commented Apr 17, 2019 •

edited

Loading

alcuadrado commented Apr 18, 2019 •

edited

Loading

alcuadrado commented Apr 22, 2019 •

edited

Loading

alcuadrado commented Apr 22, 2019 •

edited

Loading