feat: Port existing stack trace refinement 1:1 from Hardhat #545
Conversation
|
|
I'm applying the "no changeset needed" for now not to hide the overall CI status at a glance while we're making the changes. I'll drop the label once we're ready to merge this to main. |
|
@NomicFoundation/edr Could I get an initial review of this? It'd be great to land changes incrementally and this already covers quite a bit of logic as it recreates the existing codebase model construction logic from Hardhat. |
There was a problem hiding this comment.
LGTM. I only skimmed the Rust code, but I guess in general is as much a 1-1 translation as possible, right?
Something I wondered while looking at this is why some logic lives completely on the edr_napi crate (e.g., source maps), instead of living in edr_solidity and edr_napi just importing and exposing them.
Xanewok
force-pushed
the
refactor/port-solidity-stack-traces
branch
from
0ed3644
to
8d0e5af
Compare
Xanewok
force-pushed
the
refactor/port-solidity-stack-traces
branch
from
8d0e5af
to
b14ec96
Compare
The main difficulty here is to retain the exact same object/class relationship, so I tried to be as faithful as possible. For instance, while some of the source map logic is abstract, ultimately it has to create the JS-facing class instances using facilities from |
* Some small improvements * Port `MessageTrace` and `VMTracer` machinery from Hardhat * Move HH-specific ExitCode logic to edr_napi * Remove unused `ExitCode::STATIC_STATE_CHANGE` * patch(hardhat): Re-use MessageTrace and VmTracer from EDR now See NomicFoundation/hardhat@3aeeb56 * chore: Re-run pnpm build * Address PR feedback * Address PR feedback * fixup: Apply formatting
* chore: Port the BytecodeTrie from ContractsIdentifier * Migrate some library utils * Migrate some Opcode helpers * Finish the ContractsIdentifier port * Remove unused get_library_address_positions * Simplify the ContractsIdentifier port * patch(hardhat): Re-use the ContractsIdentifier from EDR * fixup: Fix a simple test
* fix: Correctly deserialize BuildInfo with `_format` field * refactor: Port the VmTraceDecoder from Hardhat * fix(napi): Correctly encode and decode `MessageTrace` types * refactor: Remove now unneeded functions * patch(hardhat): Port the VmTraceDecoder from Hardhat * Reformat and remove unused imports * fixup: Use a more correct type for the XYZTrace::bytecode field * fixup: Adjust test to now optional `deployedContract` property
* feat: Port solidity-stack-trace.ts * feat: Port return-data.ts * patch(hardhat): Port return-data.ts and solidity-stack-trace.ts * fixup: Reformat files * fixup: let's see how tests feel about a number instead of a const enum variant * Make the comment about Rust type alias more clear [skip ci]
…594) It seems transpile-only transpiles per module, however we need to perform full "compilation" in order to correctly see and inline values for const enums. We use const enums because that's how they are emitted by napi-rs and there's no option to change that. Rather than invest into supporting that or working around it, let's just fully compile the test harness once. See https://www.typescriptlang.org/tsconfig/#isolatedModules for more context.
* Start porting SolidityTracer * Start porting ErrorInferrer * WIP: Keep Reference in MessageTraces * Port 99% of the ErrorInferrer Except `ErrorInferrer.inferAfterTracing`. I discovered that the original code creates a shallow copy of the stack trace during select heuristics and returns the modified one or the *original* one if the heuristic does not hit. One could keep track of possible series of changes that would have to be subsequently applied if the heuristic hits and returns a modified the stack trace but instead, I want the code to do exactly what the original code did. Rather than wrap every entry in a shareable `Rc` (they are not modified), I decided to make them clonable in the next PR, which means we need to not keep the `ClassInstance` around to derive the Clone trait. * Port mapped-inlined-internal-functions-heuristics.ts * refactor: Prune StackTraceEntry.message to make it clonable See previous commit "Port 99% of the ErrorInferrer". * refactor: Drop unused inner funcs in the ErrorInferrer * feat: Finish porting `ErrorInferrer` * feat: Finish porting `SolidityTracer` * refactor: Remove now unused utils IntoEither/IntoOption * patch(hardhat): Port ErrorInferrer and SolidityTracer * fixup! refactor: Prune StackTraceEntry.message to make it clonable * fixup: Reformat * Document the unsafety surrounding napi-rs's References * refactor: Remove some now unused functions * Address review feedback * fix a missing period * fix alphabetical sorting
* Port over debugging facilities from debug.ts * patch(hardhat): Port debug.ts * fix: Make the depth optional in printMessageTrace * fixup: Formatting
* fix: use remote chain id for pre-fork simulation (#567) * fix: use remote chain id for pre-fork simulation * Fix error message * chore: Bump hardhat to 2.22.7 (#571) * chore: Bump hardhat to 2.22.7 * fixup: Don't enable RIP-7212 in our tests * Adapt for NomicFoundation/hardhat#5411 * fix: prevent crash when returning large JSON responses (#569) * ci: update collaborator check in the benchmarks workflow (#574) * edr-0.5.1 (#559) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * fix: add json alias property in provider response (#582) * fix: add json alias property in provider response * Create empty-bobcats-refuse.md * edr-0.5.2 (#583) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * breaking change: rename response.json to response.data (#584) * breaking change: rename response.json to response.data * Create sour-donkeys-draw.md * Update sour-donkeys-draw.md * fix: improve error message and option to skip unsupported transaction type in debug trace (#606) * fix: improve error message and option to skip unsupported tx type in debug trace * Address code review feedback * Handle unsupported transaction type requested * Fix test setup * ci: remove review-related slack notifications (#612) * chore: js tooling improvements (#609) * Fix pnpm warning * Add syncpakc * Add syncpack * Run syncpack format * Run syncpack on CI * Add setup-node action * Fixes related to upgrading prettier * Run prettier * Add setup-rust action * Run prettier in edr_napi * Add lint scripts to packages * Run prettier in crates/tools/js/benchmark * Port benchmark code to typescript * Add eslint to all packages * Upgrade @types/node to v20 * Fix broken build:edr script * Resolve benchmark output path * chore: upgrade hardhat and add patch (#613) * Upgrade Hardhat to v2.22.9 * Add patch for hardhat#5664 * chore: Bump to vanilla Hardhat 2.22.9 * chore: Regenerate the Hardhat patch to use the new EDR internals * chore: Bump @napi-rs/cli to fix duplicated napi typedefs See <napi-rs/napi-rs#2088> * fixup: Prettify test.ts * Fixes after testing in OZ (#625) * fix: add bounds checks * fix: check that steps is not empty before traversing it * fixup: formatting [skip ci] --------- Co-authored-by: Igor Matuszewski <[email protected]> --------- Co-authored-by: Agost Biro <[email protected]> Co-authored-by: Wodann <[email protected]> Co-authored-by: Piotr Galar <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Franco Victorio <[email protected]>
…y-stack-traces This also drops the local Hardhat patch used to verify that the local stack trace tests work.
Xanewok
had a problem deploying
to
github-action-benchmark
GitHub Actions
Error
Xanewok
temporarily deployed
to
github-action-benchmark
GitHub Actions
Inactive
Xanewok
temporarily deployed
to
github-action-benchmark
GitHub Actions
Inactive
|
Copied benchmark summaries from both of the latest runs: Benchmark - sanity check, just adding new logic (vs latest
|
| Benchmark suite | Current: 272b4c7 | Previous: fdf1583 | Ratio |
|---|---|---|---|
All Scenarios |
342935.803493 ms |
342710.45338099997 ms |
1.00 |
neptune-mutual-blue-protocol_8db6480 |
33781.23079 ms |
33761.015113 ms |
1.00 |
openzeppelin-contracts_0a5fba7a |
21761.936611 ms |
20441.195663 ms |
1.06 |
rocketpool_6a9dbfd8 |
21576.112301 ms |
22240.754139999997 ms |
0.97 |
safe-contracts_914d0f8 |
1684.9832250000002 ms |
1703.387087 ms |
0.99 |
seaport_4f4e7c20 |
7113.415929999999 ms |
6752.069426999999 ms |
1.05 |
synthetix_9a3a109f |
250565.653007 ms |
251436.68594 ms |
1.00 |
uniswap-v3-core_d8b1c63 |
6452.471629 ms |
6375.346011 ms |
1.01 |
Benchmark - patching Hardhat to use the new, ported logic (vs latest main)
| Benchmark suite | Current: ac2e012 | Previous: fdf1583 | Ratio |
|---|---|---|---|
All Scenarios |
305142.086981 ms |
342710.45338099997 ms |
0.89 |
neptune-mutual-blue-protocol_8db6480 |
29736.097993 ms |
33761.015113 ms |
0.88 |
openzeppelin-contracts_0a5fba7a |
17890.910111 ms |
20441.195663 ms |
0.88 |
rocketpool_6a9dbfd8 |
19060.544268 ms |
22240.754139999997 ms |
0.86 |
safe-contracts_914d0f8 |
1316.443957 ms |
1703.387087 ms |
0.77 |
seaport_4f4e7c20 |
5582.380408 ms |
6752.069426999999 ms |
0.83 |
synthetix_9a3a109f |
225767.973043 ms |
251436.68594 ms |
0.90 |
uniswap-v3-core_d8b1c63 |
5787.737201 ms |
6375.346011 ms |
0.91 |
I'll remove the local patch for now as discussed with @fvictorio and this should be good to review, I hope? 🤞 🎉
This reverts commit ac2e012.
Xanewok
had a problem deploying
to
github-action-benchmark
GitHub Actions
Error
Xanewok
temporarily deployed
to
github-action-benchmark
GitHub Actions
Inactive
There was a problem hiding this comment.
Thanks LGTM overall and congrats @Xanewok, this is a big one!
I have one doubt, but not a merge blocker: there are some parts where info is printed using println!. I'm assuming this is because the corresponding JS code would console.log there. There were some discussions about how we want to handle logging from EDR and this won't compose well with our planned usage of a log facade. I added a discussion point on this to today's technical sync.
|
Thanks everyone for the reviews! I'm happy that this can finally land now.
we discussed this and the code is only being used in the test suite. I can refactor it in the follow-up to use a Debug trait and add some comments that the logging should only for debugging/in the test suite or let it go through the |
Based on #544.
This feature branch will track porting the existing stack trace logic 1:1 from Hardhat. The main goal is to gradually port it while retaining the existing logic. While porting, we aim to retain the JS interface ideally as-is; once the port is complete, we can then attempt to simplify the internal code (as it won't necessarily have to be exported/accessed via JS as much) or doing the decoupling work outlined in #246.
To continuously test the changes, we will use the vendored
hardhat-testssuite. However, to not have to vendor the entire Hardhat repository, we will collect patches (created viapnpm patch) to facilitate the review and to allow better grouping of the related changes. Each commit with a patch should ideally point to a commit in the NomicFoundation/hardhat repository.I tried to use the patch mechanism from the github repository directly via package.json but I don't think it's possible to do given the Hardhat repository setup (monorepo, the
rootpackage workaround) as I encountered errors. I'd be happy to switch to that, instead, if we found a way to make it work.The initial work included here tries to tackle #247. We consume the compiler input/output JSON from the JS and create the relevant model in the Rust side, while exposing the same class instances and relationship to JS via
napi-rs.To test this locally, you can also:
edr_napicrate) locally viapnpm linkhardhat-corepackage (and to optionally speed things up, only run the relevant suite viadescribe.only("Stack traces", ...).