refactor: Port ReturnData and StackTraceEntryType
#589
Conversation
|
Xanewok
had a problem deploying
to
github-action-benchmark
GitHub Actions
Error
Xanewok
temporarily deployed
to
github-action-benchmark
GitHub Actions
Inactive
| // used verbatim in JS definitions because napi-rs does not store not allows to | ||
| // reuse the same type unless fully specified at definition site. |
There was a problem hiding this comment.
Typo? The comment isn't clear to me
There was a problem hiding this comment.
Yeah, I could have done better job at explaining this. I tried to be more in-depth about the context for this: the gist is that the type alias is mostly for the Rust side of the bindings. To actually type-check we would have to not use the type alias but the entire type as is, which is very bad, considering it takes 26 lines, so we accept the trade-off and use the type alias (which type-checks to any atm).
When we will limit the exposed API interface, we might specify the type literally in few select places if we deem the trade-off in readability to be acceptable 👌
Hope that makes sense and it's okay.
Xanewok
temporarily deployed
to
github-action-benchmark
GitHub Actions
Inactive
| @@ -280,9 +281,15 @@ function compareStackTraces( | |||
|
|
|||
| // if IR is enabled, we ignore callstack entries in the comparison | |||
| if (isViaIR) { | |||
| trace = trace.filter( | |||
| (frame) => frame.type !== StackTraceEntryType.CALLSTACK_ENTRY | |||
There was a problem hiding this comment.
From my experience during this rewrite it was often the case that when the same type was removed and re-exported under the same name it was not recognized at run-time, even if the types were correct (meaning tsc was happy).
This was true for functions, classes and enums. What often helped was importing it under a different alias and then re-exporting it/using it with the same alias, removing the old code and then replacing with the old/actual name. Whether that's a problem with build cache or something I do not know (I tried cleaning node_modules, doing pnpm install etc. to no avail).
Rather than figuring out the underlying cause I decided not to do any further detective work and just apply the blunt workaround for now. Let's see if this is fixed with a subsequent change; I'd rather not spend more time here and focus on completing the porting work, instead.
There was a problem hiding this comment.
I didn't look into this, but @Xanewok did a very thorough research and couldn't find a good solution. A follow-up PR uses a different workaround, but it's still a workaround.
I don't think this is super important though, since it only affects tests' code.
|
Thanks for the reviews! I'll take the liberty and merge it after pushing a small fix and clarifying the note for the comment made. The next PR will be more substantial 😬 |
* refactor: Port the codebase model construction and EVM decoding from HH * chore: Add a script that regenerates the pnpm patch for Hardhat * patch(hardhat): Cherry-pick and adapt for the unreleased Rip7212 PR See NomicFoundation/hardhat@f944cd5 * patch(hardhat): Cherry-pick a stack trace cleanup PR See NomicFoundation/hardhat@5739893 * patch(hardhat): Use the rewritten compiler to model logic from EDR See NomicFoundation/hardhat@49c66b6 * Port `MessageTrace` and `VMTracer` machinery from Hardhat (#531) * 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 * refactor: Port ContractsIdentifier to Rust (#562) * 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 * refactor: Port the `VmTraceDecoder` from Hardhat (#568) * 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 * refactor: Port `ReturnData` and `StackTraceEntryType` (#589) * 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] * fix: Fully transpile in Mocha tests to correctly inline const enums (#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. * refactor: Port ErrorInferrer and SolidityTracer from Hardhat (#593) * 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 * refactor: Port stack-traces/debug.ts (#596) * Port over debugging facilities from debug.ts * patch(hardhat): Port debug.ts * fix: Make the depth optional in printMessageTrace * fixup: Formatting * Merge `main` into the stack trace port feature branch (#618) * 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]> * chore: Restore old test.ts and remove some `as any` conversions * WIP: Benchmark with patched Hardhat to use new EDR internals * Revert "WIP: Benchmark with patched Hardhat to use new EDR internals" This reverts commit ac2e012. * Remove the gen-hardhat-patches script --------- 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]>
This ports the
StackTraceEntryTypeand related...StackTraceEntryinterfaces from JS to Rust, which are the main external used after being processed by error inferrer et al. to be later encoded into a Node.js call stack trace.Few things to note:
ethereumjs-abi, I usedalloy-sol-typesand the macro that defines and generates corresponding error types at compile-timeinterfaces, they have to be marked as#[napi(object)], which means that they are passed as value types across the napi boundaryStackTraceEntryTypeConstZST that has the according value set as a const param and which implementsToNapiValueandFromNapiValueusing the underlying const param value at run-time.The final JS type being a sum of all of these variants, we use a napi
Either24(ugh) type to achieve the same thing. This retains the compatibility at the JS interface level.@fvictorio please take a look; this is self-contained and acts as a base for the following error-inferrer/solidity-tracer port.
Pulled these commits for the local Hardhat patch (the companion branch is at https://github.com/NomicFoundation/hardhat/tree/refactor/port-stack-trace-entry-data):