refactor: Clean up the majority of the stack trace port implementation

[Xanewok]

The current base has already ported the JS code 1:1. This PR aims to reduce the exported N-API surface and simplify some JSisms.

Summary

• this removes the last uses of napi-rs's ClassInstance/Reference and thus also our ClassInstanceRef helpers
• the relationships are naively modeled with Rc<RefCell<...>> for now
  • the ClassInstanceRef::borrow(_mut) are just replaced with the actual RefCell methods
  • as before, the mutation points are well isolated and safe but this could use a rewrite in terms of ownership. I'm leaving this for later, as I wanted to focus on strictly decoupling it from the N-API/napi-rs
  • specifically, because we now have cyclic references between the types and there's no GC anymore to break the cycle/rescue us from the leaks
  • EDIT: The strong ref cycle is now broken via 3b551a0. I confirmed with valgrind that the changes before lead to a leak and with the commit we do not leak anymore: https://gist.github.com/Xanewok/90d7aca888f8541081b2e7c1c703d6ca
• we now prefer using directly OpCode from revm rather than the previously ported/hand-rolled Opcode from JS
• this moves the now napi-decoupled compiler.rs, model.rs, source_map.rs and library_utils.rs back to the edr_solidity crate
  • this replaces the previous implementation of ContractsIdentifier with what was more or less naively ported but is integrated with the rest of the impl and working. I left the unit tests to show that the old impl behaves correctly (and the test suite more or less gets us that too) but it'd be nice to also revisit this implementation and see if we can bring back the RadixTree if we find that API/impl better (sorry @fvictorio, PTAL as you authored that code IIRC?)
  • breaking: The N-API-exported new VmTraceDecoder() now does not accept any arguments and internally already constructs a ContractsIdentifier.

Caveats/future work

1. We will need to bump the base to use the Hardhat v2.22.9 soon.

2. I left the MessageTrace and the SolidityStackTrace types as these are still carried across the FFI boundary and I didn't want to change the architecture/interfacing points for now. EDIT: This is out of the scope for now.

Ownership changes

Introduced in 3b551a0.

[changeset-bot]

⚠️ No Changeset found

Latest commit: 5311522

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[Xanewok]

I broke the ref cycle and now the memory leaks should be gone, see the OP. The only thing that's left for now is adding more documentation.

[Wodann]

Another large change that looks to be steering the code is the right direction. Awesome work!

At a high-level, I think the change to break the circular dependency makes sense. For future work, I can't help but wonder whether we can get away with an arena at the top level and Arcs and Weak for the actual data, instead of requiring a hashmap lookup.

I have some questions to better understand and one concern that I wanted to discuss.

In terms of individual changes, they are too many for me able to look at every line of code in-depth. Are there any parts that require more scrutiny? In that case, would it make sense to have a call to discuss those changes?

[fvictorio]

this replaces the previous implementation of ContractsIdentifier with what was more or less naively ported but is integrated with the rest of the impl and working [...] it'd be nice to also revisit this implementation and see if we can bring back the RadixTree if we find that API/impl better (sorry @fvictorio, PTAL as you authored that code IIRC?)

@alcuadrado implemented those optimizations in a Hardhat branch that was never merged/released. I used that branch as the starting point in my aborted migration attempt. But I think what you did (keeping the 1-1 port from main) is fine, since we know it works and we have proper benchmarks.

[fvictorio]

I tested this with several real repositories and everything worked fine.

[Xanewok]

I have reverted the patch like in #545, so this only contains the updated logic but does not exercise it locally when running tests, however the CI was green with the patch applied, so this should be good to go and @fvictorio tested this as well using some real-life repositories.

The next step will be to update Hardhat (9ca5997 is a good starting point) to use this new logic once we release EDR with these changes.

Thanks everyone for the reviews! 🎉