Typescript migration #72
Conversation
|
Great that your are tackling this so quickly, cool!
@alcuadrado @micahriggan Ah, I wrote this comment before scanning over the work and explanations here, so comment seems to be already outdated respectively answered through the active process here. @micahriggan Thanks for your work on the original PR and working out the problem space more clearly together! |
Awesome! 🎉
Oops, same here. I answered in the other PR.
I'll open an issue with an explanation in the blockchain repo later today. |
|
I think this PR is ready to be reviewed now. I ported the RPC-related functions, removed the callback-based API, fix the travis config, and documented the missing uncles' validation. As a summary that could be added to the changelog, the main changes that this PR introduces are:
|
|
Please, take a look at the initial message from this PR, as it's relevant here: ethereumjs/ethereumjs-monorepo#566 |
There was a problem hiding this comment.
Just got through, thanks @alcuadrado, that is really a marvelous update with lots of small improvements all over the place, great, also just realizing the amount of work this has likely taken.
Some things to address, nothing too big though.
|
|
||
| const height = new BN(this.header.number) | ||
| return uncleHeader.validate(blockchain, height) | ||
| } |
There was a problem hiding this comment.
I am just not getting the whole picture why this "uncle already included" could be removed, could you give a really quick reasoning?
There was a problem hiding this comment.
It shouldn't be removed, but there's no way to validate it using ethereumjs-blockchain. The previous code was just calling a function of the blockchain that was a no-op.
This really surprised me, so I spent some time trying to figure out how it happened, but couldn't found the reason. All I know is that that functionality was removed in this PR: https://github.com/ethereumjs/ethereumjs-blockchain/pull/47/files#diff-168726dbe96b3ce427e7fedce31bb0bcR481
|
@holgerd77 I addressed all the issues. Just not sure if something different can be done about this one. |
|
Amazing 😍 |
This is a WIP PR migrating this library to Typescript.
Things missing are:
Decide if we keep the json rpc functionality. Personally, as someone that works a lot with the RPC, I'd never expected that from this module.
Update code inline docs.
Keeping the callbacks-based API while modernizing the code (i.e. removing async) was kind of hard. Especially since browserify doesn't work well with
promisifyandcallbackify. Maybe it's because of a combination of TS+borwserify+karma, but it made me lose a lot of time. I'm not sure if keeping that legacy API is worth the hassle. What do you think?Some things to note:
I defined a
Blockchaininterface instead of usingethereumjs-blockchainbecause this way this module can easily work with other implementations. Our blockchain implementation is too slow for many use-cases, so this is important. I'll submit a PR with something similar in the VM.I removed this validation, as it uses a deprecated method in
ethereumjs-blockchain, which is actually a noop.I copied the common-handling logic from chore: remove homestead + update eth dependencies #69. Thanks, @micahriggan !
I'll probably be able to finish this tomorrow.
closes #71, #67, #63, #43, #69, #68