Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Upgrade of Ethereum JS to v6 has introduced technical debt #3693

Closed
jeffsmale90 opened this issue Sep 21, 2022 · 7 comments · Fixed by #3971
Closed

Upgrade of Ethereum JS to v6 has introduced technical debt #3693

jeffsmale90 opened this issue Sep 21, 2022 · 7 comments · Fixed by #3971

Comments

@jeffsmale90
Copy link
Contributor

jeffsmale90 commented Sep 21, 2022

Various changes to types and member accessibility in Ethereum JS has required changes to Ganache. In #3656, an amount of technical debt was introduced in order to support the merge.

Including, but not limited to:

  • any type assertions
  • private / protected members accessed from outside of the instance
  • weird and convoluted shimming / facade wrapping of level database stuff
  • @ts-ignore
@MicaiahReid
Copy link
Contributor

Some hacks introduced into our build process:

@MicaiahReid
Copy link
Contributor

We should also go through @jeffsmale90's review on #3656 for some tech debt he's found

@MicaiahReid
Copy link
Contributor

ethereumjs also introduced a new hashing library that is much slower as a default, so we need to make ejs use our current keccak instead.

@davidmurdoch
Copy link
Member

We should probably rip out api-extractor and rethink our types approach.

@davidmurdoch
Copy link
Member

We should discuss removing our alternative lightweight ethereumjs classes and replace them with the expected ethereumjs classes. While we'd likely take a performance hit, the maintenance overhead of keeping them feels like too much.

We have our own block, transactions (transactions and transaction factory), and account classes that we may want to reconsider.

@davidmurdoch
Copy link
Member

In tags.ts i added the new tags in two different places... and they are in different orders. It's not an actual problem, but it'll drive me crazy.

@MicaiahReid
Copy link
Contributor

MicaiahReid commented Oct 19, 2022

A little something to track the remaining work:

Problem Description PR Status
Implement suggestions in the review of #3656 #3912 Reivew
Fix types building/remove associated hacks (fd9fe25, 45d9e39) #3913, microsoft/rushstack#3702 Review
Remove webpack hacks 13b6eba #3916 Review
Replace homemade "lightweight" block class with ejs' block Another large refactor. It would simplify a lot of conversions to Quantity because we can deal directly with Buffers and Bigints
Replace homemade "lightweight" account class with ejs' account Moderate effort for little to no reward
Fix ordering of new tags in tags.ts Merged (well, accidentally pushed directly)
Fix race condition with "does not persist changes to vm or state trie" test #3825 Merged
Swap out default ejs hashing library #3820 Merged
Replace homemade "lightweight" transaction class with ejs' transactions This is a really large refactor. It would remove a lot of code and simplify things, but should probably be a different PR
Simplify/document/improve wrappping of level db #3880 Merged
Replace homemade "lightweight" address class with ejs' address #3777 Merged
Remove added any type assertions #3778 Merged
Remove added instances of accessing private/protected members from outside of instance #3778 Merged
Remove added @ts-ignores #3778, #3821 Merged
Open PR with ethereumjs to make DB type more generic No longer needed. See ethereumjs/ethereumjs-monorepo#2393

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants