Closed alcuadrado closed 5 years ago
Great that your are tackling this so quickly, cool!
Blockchain
interface: whew, that's a deep-dive-analytical change, pretty interesting to hear (blockchain
slow) and very cool that you stumble and directly tackle stuff like this.
@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!
- Definitely not worth keeping - we have e.g. already decided on the VM to completely drop the callback based API in exchange with @s1na and some extended community-expert-hearing 😛 - so just throw the old stuff away 😄, that would be a great along-improvement.
Awesome! 🎉
I wrote this comment before scanning over the work and explanations here
Oops, same here. I answered in the other PR.
Blockchain
interface: whew, that's a deep-dive-analytical change, pretty interesting to hear (blockchain
slow) and very cool that you stumble and directly tackle stuff like this.
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:
Promise
-based API now.Error
instead of strings.ethereum-util
and ethereumjs-tx
to their latest versions.Please, take a look at the initial message from this PR, as it's relevant here: https://github.com/ethereumjs/ethereumjs-vm/pull/566
@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
promisify
andcallbackify
. 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
Blockchain
interface instead of usingethereumjs-blockchain
because 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 #69. Thanks, @micahriggan !
I'll probably be able to finish this tomorrow.
closes #71, #67, #63, #43, #69, #68