Migrate to typescript #27

s1na · 2019-01-02T15:50:44Z

This PR migrates the library to typescript and updates the tooling (tslint, prettier) to be consistent with ethereumjs/rlp#37.

Additionally, it implements the logic of ethUtil.defineProperties, in the Account class, with the help of a new class BinaryValue.

Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>

coveralls · 2019-01-02T15:54:34Z

Coverage increased (+3.8%) to 41.667% when pulling 8ca5514 on typescript into bcd7207 on master.

holgerd77 · 2019-01-02T20:50:43Z

What's your main rationale to move the logic of this defineProperties method here (not against it, just asking)? And do you think you have met all the functionality relevant here, since the original method contains lot more code?

holgerd77 · 2019-01-02T20:54:14Z

Also, if you have better ideas for the API respectively see some bad design choices feel free to introduce if you're confident on it. I have been moving more in the direction during the last weeks that we actually should allow us to also make API changes if useful and don't necessarily have to stick on transferring one-to-one. Confident that we get this managed, and the occasion is good to get rid of some legacy stuff.

holgerd77 · 2019-01-02T21:08:05Z

//cc @krzkaczor did you also already stumble upon this defineProperties method and have ideas on that?

s1na · 2019-01-03T10:24:57Z

@holgerd77 Well, in the beginning it was because defineProperties modifies the class, adds properties and methods during runtime which typescript doesn't detect during compilation and therefore raises errors. But generally I think even though defineProperties is a neat abstraction, it's less readable and type-safe than explicitly defining properties and validating them.

I'm not sure if the current implementation covers all the cases, I have to test further. But it's less code because Account doesn't need some of the code in defineProperties, and they're there for other libraries, e.g. Transaction.

Agree with what you said on API change. Maybe transition to typescript and a systematic upgrade of API can be undertaken under codename EthereumJS 3.0 😄

package.json

krzkaczor · 2019-01-03T12:32:01Z

I think the previous version with defineProperties allowed to set any properties — no we are missing setters. Frankly, if we consider breaking API we should make this immutable entirely, currently, it's mixed since there are methods like setCode.

On the other hand, if we don't want to break public interface, for now, I think that we could define typescript fields AND still use defineProperties which would override behavior in runtime. I think it should work and maybe even it's a preferred way since boilerplate footprint is small. @s1na What do you think about this approach. I am not sure if this gonna work, I would need to play with it.

s1na · 2019-01-03T15:40:12Z

@krzkaczor sorry, the current implementation is still work in progress, and some stuff that should be there and you mentioned are missing.

The approach you mentioned might be possible, please let me know if it worked out if you got around to playing with it. If we stick with this for now, in my opinion for future explicit definition of properties and their types feels safer and more readable than runtime injection, even if it increases boilerplate a bit. I'm trying to reduce the boilerplate by defining types that handle some of the validation done in defineProperties.

holgerd77 · 2019-01-03T21:22:51Z

@s1na Yes, I also think it's very well worth to get rid of this common defineProperties method and it's worth to add some boilerplate here. These strong ties of core object structure changing to a dependency is really super-implicit, error-prone and has already caused various problems in the past. If this goes along with some changes to the API so be it, as long as it's thought through, provides some future-stable ground and has some reasoning for the changes.

krzkaczor · 2019-01-06T15:34:23Z

@s1na check it out here: https://github.com/ethereumjs/ethereumjs-account/pull/28/files

I think the most important part here is the usage of ! to trick silent compiler warnings.

Treat it as WIP. If you think it's a good idea incorporate please into your PR (you can rewrite it I just did it quick and dirty so maybe there some mistakes)

Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>

s1na · 2019-01-07T15:44:28Z

@krzkaczor Thanks! Integrated your code.

@holgerd77 The PR should be now ready for review, I'd appreciate it if you could have a look.

Defining the Trie interface here is probably redundant, but I didn't want to leave all the types any, and merkle-patricia-tree has not been migrated to typescript yet. After it is migrated, we can import it here and use its defined type.

holgerd77 · 2019-01-08T14:38:04Z

Won't make it to do a review on this today - will try tomorrow - everyone else is also invited!

holgerd77 · 2019-01-09T12:32:54Z

@s1na What happened to the BinaryValue class, did you remove that again?

s1na · 2019-01-09T12:44:39Z

@holgerd77 Yeah, I incorporated Chris's approach to make this PR only for transitioning to typescript and avoid API changes. We can later try to come up with a solution that'd work across projects. I'll create an issue for discussion.

holgerd77 · 2019-01-09T13:02:26Z

@s1na Yeah, makes sense.

These TypeScript PRs are really some beasts, with changes in close to every file and all along the build chain. We should really make sure that we do at least one manual integration test on every transition by injecting the distribution build (npm pack) in the node_modules folder of a library where the respective library is used to check how things behave in production.

I have done this with this PR and it produces the following error when injecting in the VM and run npm run testStateByzantium:

error: TypeError: Account is not a constructor
    at /Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/tests/util.js:358:19
    at /Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/node_modules/async/dist/async.js:3096:16
    at replenish (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/node_modules/async/dist/async.js:998:17)
    at /Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/node_modules/async/dist/async.js:1002:9
    at eachLimit$1 (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/node_modules/async/dist/async.js:3182:24)
    at Object.<anonymous> (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/node_modules/async/dist/async.js:1032:16)
    at Object.exports.setupPreConditions (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/tests/util.js:356:9)
    at /Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/tests/GeneralStateTestsRunner.js:60:16
    at /Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/node_modules/async/dist/async.js:3866:24
    at replenish (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/node_modules/async/dist/async.js:998:17)
    at /Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/node_modules/async/dist/async.js:1002:9
    at eachOfLimit (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/node_modules/async/dist/async.js:1027:24)
    at /Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/node_modules/async/dist/async.js:1032:16
    at _parallel (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/node_modules/async/dist/async.js:3865:5)
    at Object.series (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/node_modules/async/dist/async.js:4721:5)
    at runTestCase (/Users/hdrewes/Documents/DEV/EthereumJS/ethereumjs-vm/tests/GeneralStateTestsRunner.js:48:9)

Not sure why this is happening yet, apparently new Account() constructor is not working properly on the distribution build. Have done some googling, points in some "order of things is important" direction but couldn't completely identify the root cause yet.

This is now the third (so 3/3 😄) TypeScript transition PR together with RLP and ethereumjs-common where we have passing tests but breaking builds. Quite some track record! 😄 😛

krzkaczor · 2019-01-09T13:18:16Z

@holgerd77

This is now the third (so 3/3 😄) TypeScript transition PR together with RLP and ethereumjs-common where we have passing tests but breaking builds. Quite some track record! 😄 😛

This deserves a medal or something :D

I stumbled upon this tweet and I keep wondering if its a big deal: https://twitter.com/bterlson/status/1082750157303603200

Anyways, probably the export "shape" is different.

s1na · 2019-01-09T14:01:56Z

@holgerd77 @krzkaczor Haha :)))

Apparently it's due to differences in module systems... when something is exported by export default, in node it has to be imported by require(...).default. If we don't do default export (and do named exports instead), it'd have to be imported by require(..).Account. The only way I found to keep ES5 projects working as before is by using export = Account, which means in typescript the special syntax import Account = require(..) should be used!

When searching for this, it seems that generally named exports are preferred, unless there's only one object being exported (in which case default export is preferred).

Should we make this breaking change now, or rather go with export = Account for now and then after other libraries (VM) have also transitioned to typescript use proper exports? Or maybe someone has a better approach?

Did the same happen with Common Holger?

Edit: By the way, I tried compiling with esModuleInterop to no effect, it seems to be helpful for when we want to import JS libs (that use old export default method) in typescript, not the other way around.

krzkaczor · 2019-01-09T14:06:23Z

I am a huge hater of import Account = require(..) syntax: it's weird, uncommon and for example, babel doesn't support it.

holgerd77 · 2019-01-09T14:16:19Z

Getting more and more to the stance that we just should do major releases by default on Typescript transition releases. Then we've got somewhat more freedom, the big shift gets clearer and it's generally somewhat likely that the WILL be done breaking changes on most of the transitions.

Probably happen on Common too, couldn't test yet. This will be a major release anyhow now though, just have to remember to test and state this in the release notes if the case.

holgerd77 · 2019-01-09T14:17:42Z

No clear opinion on default vs named here, maybe default, sounds a bit more fitting?

Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>

s1na · 2019-01-09T14:53:15Z

VM has already upgraded to ethereumjs-util v6.0.0 and when I was testing Account in VM, I got errors for sha3 and SHA3_... constants, so I also upgraded ethereumjs-util here. Now all VM tests pass (after changing imports to require('ethereumjs-account').default).

holgerd77 · 2019-01-10T09:50:37Z

Documentation should also be updated along with this, have made this an extra issue which can be addressed after merging: #30

holgerd77 · 2019-01-10T12:12:33Z

@s1na I am not sure what to think about this external dependency (Trie) interface definitions included here. Are you confident that this has no side effects? @krzkaczor what's your stance here?

s1na · 2019-01-10T12:50:35Z

@holgerd77 It shouldn't have any effects beyond typescript compilation. If you think it's an unnecessary complication I can replace them all with any.

holgerd77 · 2019-01-10T13:08:48Z

package.json

@@ -24,14 +36,26 @@
  },
  "homepage": "https://github.com/ethereumjs/ethereumjs-account#readme",
  "dependencies": {
-    "ethereumjs-util": "^5.0.0",
-    "rlp": "^2.0.0",
+    "bn.js": "^4.11.8",


Where is this new bn.js dependency coming from respectively used?

I had mistakenly thought this is also necessary for the error (see below). Removed it now.

holgerd77 · 2019-01-10T13:09:05Z

package.json

+    "@ethereumjs/config-prettier": "^1.0.1",
+    "@ethereumjs/config-tsc": "^1.0.2",
+    "@ethereumjs/config-tslint": "^1.0.0",
+    "@types/bn.js": "^4.11.3",


Same for the types?

I'm not sure why, but without this typescript fails with:

node_modules/rlp/dist/types.d.ts:2:21 - error TS7016: Could not find a declaration file for module 'bn.js'. '.../ethereumjs-account/node_modules/bn.js/lib/bn.js' implicitly has an 'any' type. Try `npm install @types/bn.js` if it exists or add a new declaration (.d.ts) file containing `declare module 'bn.js';` 2 import BN = require('bn.js');

holgerd77 · 2019-01-10T13:10:58Z

@s1na If you are confident on that and @krzkaczor is not stating anything else leave them in, definitely nice to have if side-effect free.

Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>

holgerd77

Rest looks good, thanks!

Yay. ✨ 😄

holgerd77 · 2019-01-10T14:21:11Z

src/index.ts

+  }
+
+  serialize(): Buffer {
+    return rlp.encode([this.nonce, this.balance, this.stateRoot, this.codeHash])


Nice that you took out the raw here, much better readable like this.

Migrate to typescript

e4e9ed6

Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>

s1na added PR state: do-not-merge PR state: WIP type: refactor labels Jan 2, 2019

krzkaczor reviewed Jan 3, 2019

View reviewed changes

package.json Outdated Show resolved Hide resolved

s1na added 4 commits January 7, 2019 15:45

Fix files in package.json

b86beff

Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>

Revert to using defineProperties

08f5a9f

Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>

Add interface for trie

bec2c55

Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>

Prettify index.ts

5877d1f

Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>

s1na added PR state: needs review and removed PR state: WIP PR state: do-not-merge labels Jan 7, 2019

s1na added 3 commits January 9, 2019 15:30

Replace deprecated sha3 with keccak256

7dd5e9a

Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>

Replace deprecated SHA3 consts with KECCAK256

d6a0546

Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>

Upgrade to ethereumjs-util v6.0.0

b7bc322

Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>

holgerd77 mentioned this pull request Jan 10, 2019

Auto-generated documentation with TSDocs #30

Closed

holgerd77 reviewed Jan 10, 2019

View reviewed changes

Rm bn.js dependency

8ca5514

Signed-off-by: Sina Mahmoodi <itz.s1na@gmail.com>

holgerd77 approved these changes Jan 10, 2019

View reviewed changes

holgerd77 merged commit c928a61 into master Jan 10, 2019

This was referenced Jan 14, 2019

use define field WIP #28

Closed

Make Account Object an ES6-compatible Class #17

Closed

s1na deleted the typescript branch January 14, 2019 12:22

s1na mentioned this pull request Jan 14, 2019

New release v3.0.0 #31

Merged

holgerd77 removed the PR state: needs review label Mar 12, 2019

holgerd77 mentioned this pull request Apr 10, 2019

TypeScript Migration ethereumjs/ethereumjs-tx#142

Closed

alcuadrado mentioned this pull request Apr 13, 2019

TypeScript migration ethereumjs/ethereumjs-tx#145

Merged

ryanio mentioned this pull request Jan 10, 2020

Remove use of ethereumjs-util defineProperties #62

Closed

s1na mentioned this pull request Apr 6, 2020

Replace defineProperties with explicit type-safe alternative ethereumjs/ethereumjs-monorepo#683

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to typescript #27

Migrate to typescript #27

s1na commented Jan 2, 2019

coveralls commented Jan 2, 2019 •

edited

Loading

holgerd77 commented Jan 2, 2019

holgerd77 commented Jan 2, 2019

holgerd77 commented Jan 2, 2019

s1na commented Jan 3, 2019

krzkaczor commented Jan 3, 2019

s1na commented Jan 3, 2019

holgerd77 commented Jan 3, 2019

krzkaczor commented Jan 6, 2019

s1na commented Jan 7, 2019

holgerd77 commented Jan 8, 2019

holgerd77 commented Jan 9, 2019

s1na commented Jan 9, 2019

holgerd77 commented Jan 9, 2019

krzkaczor commented Jan 9, 2019

s1na commented Jan 9, 2019 •

edited

Loading

krzkaczor commented Jan 9, 2019

holgerd77 commented Jan 9, 2019

holgerd77 commented Jan 9, 2019

s1na commented Jan 9, 2019

holgerd77 commented Jan 10, 2019

holgerd77 commented Jan 10, 2019

s1na commented Jan 10, 2019

holgerd77 Jan 10, 2019

s1na Jan 10, 2019

holgerd77 Jan 10, 2019

s1na Jan 10, 2019

holgerd77 commented Jan 10, 2019

holgerd77 left a comment

holgerd77 Jan 10, 2019

Migrate to typescript #27

Migrate to typescript #27

Conversation

s1na commented Jan 2, 2019

coveralls commented Jan 2, 2019 • edited Loading

holgerd77 commented Jan 2, 2019

holgerd77 commented Jan 2, 2019

holgerd77 commented Jan 2, 2019

s1na commented Jan 3, 2019

krzkaczor commented Jan 3, 2019

s1na commented Jan 3, 2019

holgerd77 commented Jan 3, 2019

krzkaczor commented Jan 6, 2019

s1na commented Jan 7, 2019

holgerd77 commented Jan 8, 2019

holgerd77 commented Jan 9, 2019

s1na commented Jan 9, 2019

holgerd77 commented Jan 9, 2019

krzkaczor commented Jan 9, 2019

s1na commented Jan 9, 2019 • edited Loading

krzkaczor commented Jan 9, 2019

holgerd77 commented Jan 9, 2019

holgerd77 commented Jan 9, 2019

s1na commented Jan 9, 2019

holgerd77 commented Jan 10, 2019

holgerd77 commented Jan 10, 2019

s1na commented Jan 10, 2019

holgerd77 Jan 10, 2019

Choose a reason for hiding this comment

s1na Jan 10, 2019

Choose a reason for hiding this comment

holgerd77 Jan 10, 2019

Choose a reason for hiding this comment

s1na Jan 10, 2019

Choose a reason for hiding this comment

holgerd77 commented Jan 10, 2019

holgerd77 left a comment

Choose a reason for hiding this comment

holgerd77 Jan 10, 2019

Choose a reason for hiding this comment

coveralls commented Jan 2, 2019 •

edited

Loading

s1na commented Jan 9, 2019 •

edited

Loading