Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Change edr_napi::provider::Response::solidity_trace to return a stack trace #734

Draft
wants to merge 24 commits into
base: main
Choose a base branch
from
Draft

Change edr_napi::provider::Response::solidity_trace to return a stack trace #734

wants to merge 24 commits into from

Conversation

agostbiro
Copy link
Member

@agostbiro agostbiro commented Dec 3, 2024 •
edited
Loading

The goal of this PR is to port Solidity stack trace generation to pure Rust and without NAPI dependencies. Doing this also necessitated porting hardhat_addCompilationResult to EDR from Hardhat.

Most of the additions edr_solidity come from moving edr_napi code there and stripping the NAPI dependencies. As the stack trace error inference involves complex heuristics, this code was originally ported verbatim from JS which lead to non-idiomatic Rust code. I originally wanted to clean it up as part of moving inference code from edr_napi to edr_solidity, but I only managed to it partially due to running out of time, so much of it remains unidiomatic Rust code. I'd ask to exclude this aspect from review.

I think these are the important changes in edr_solidity that need review:

  1. I renamed types whose names came from Hardhat + EthereumJS and are no longer exposed to JS to names that I feel make more sense in EDR. E.g. Bytecode is renamed to ContractMetadata, VmTrace is renamed to NestedTrace (to differentiate from flat traces from edr_evm) and VmTraceDecoder is renamed to ContractDecoder.
  2. Introduced an internal nested trace type in NestedTracer that has interior mutability, but it's converted to a nested trace type without interior mutability when returned from NestedTracer. This way the risk of panics from interior mutability is confined to the nested_tracer module.
  3. Using Arc<RwLock> in build model types instead of Rc<RefCell>. This was necessary, because the EDR provider holds on to those types now to support hardhat_addCompilationResult, so they need to be Send. We don't really need to write to the types held by Arc<RwLock> outside the edr_solidity crate, but it took too long to remove that so I gave up on it as part of this PR.

Besides edr_solidity, on the Rust side changes to the provider in edr_provider and edr_napi would be important to review I think.

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Dec 3, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: b8f0703

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@nomicfoundation/edr Minor

Not sure what this means? Click here to learn what changesets are.

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

@agostbiro agostbiro linked an issue Dec 3, 2024 that may be closed by this pull request
Change edr_napi::provider::Response::solidity_trace to return a stack trace #733
Open
@agostbiro agostbiro temporarily deployed to github-action-benchmark December 3, 2024 14:56 — with GitHub Actions Inactive
@agostbiro agostbiro marked this pull request as draft December 3, 2024 14:56
@fvictorio fvictorio temporarily deployed to github-action-benchmark December 4, 2024 13:46 — with GitHub Actions Inactive
@fvictorio
Copy link
Member

@agostbiro I pushed the changes to run the stack traces tests using the new stackTrace getter. You can check that tests are not passing by running this in hardhat-tests:

pnpm mocha test/internal/hardhat-network/stack-traces/test.ts --bail

This will stop at the first failure.

The test that is failing runs this bytecode:

0x608060405234801561001057600080fd5b506004361061002b5760003560e01c8063f8a8fd6d14610030575b600080fd5b61003861003a565b005b6000604051610048906100f0565b604051809103906000f080158015610064573d6000803e3d6000fd5b5090508073ffffffffffffffffffffffffffffffffffffffff1663a9cc47186040518163ffffffff1660e01b8152600401600060405180830381600087803b1580156100af57600080fd5b505af11580156100c3573d6000803e3d6000fd5b505050506040513d6000823e3d601f19601f820116820180604052506100ec91908101906101e1565b5050565b6103078061029983390190565b600082601f83011261010e57600080fd5b815161012161011c8261024f565b610222565b9150818183526020840193506020810190508385604084028201111561014657600080fd5b60005b83811015610176578161015c8882610180565b845260208401935060408301925050600181019050610149565b5050505092915050565b60006040828403121561019257600080fd5b61019c6040610222565b905060006101ac848285016101cc565b60008301525060206101c0848285016101cc565b60208301525092915050565b6000815190506101db81610281565b92915050565b6000602082840312156101f357600080fd5b600082015167ffffffffffffffff81111561020d57600080fd5b610219848285016100fd565b91505092915050565b6000604051905081810181811067ffffffffffffffff8211171561024557600080fd5b8060405250919050565b600067ffffffffffffffff82111561026657600080fd5b602082029050602081019050919050565b6000819050919050565b61028a81610277565b811461029557600080fd5b5056fe608060405234801561001057600080fd5b506102e7806100206000396000f3fe608060405234801561001057600080fd5b506004361061002b5760003560e01c8063a9cc471814610030575b600080fd5b61003861004e565b604051610045919061021b565b60405180910390f35b60606040517f08c379a00000000000000000000000000000000000000000000000000000000081526004016100829061023d565b60405180910390fd5b61009361010d565b81526020019060019003908161008b5790505090506040518060400160405280600181526020016002815250816000815181106100cc57fe5b60200260200101819052506040518060400160405280600281526020016003815250816001815181106100fb57fe5b60200260200101819052508091505090565b604051806040016040528060008152602001600081525090565b600061013383836101dd565b60408301905092915050565b600061014a8261026d565b6101548185610285565b935061015f8361025d565b8060005b838110156101905781516101778882610127565b975061018283610278565b925050600181019050610163565b5085935050505092915050565b60006101aa600883610296565b91507f44206661696c65640000000000000000000000000000000000000000000000006000830152602082019050919050565b6040820160008201516101f3600085018261020c565b506020820151610206602085018261020c565b50505050565b610215816102a7565b82525050565b60006020820190508181036000830152610235818461013f565b905092915050565b600060208201905081810360008301526102568161019d565b9050919050565b6000819050602082019050919050565b600081519050919050565b6000602082019050919050565b600082825260208201905092915050565b600082825260208201905092915050565b600081905091905056fea2646970667358221220fbf4de57de6e06c6bac2a7ec2392518b6eb777da534fdd38b8bd290988583c6764736f6c63430006030033a264697066735822122070cadf9cc927971990ad70f87b31a21294cb7ba5f9498d6595ebde6fab6c9a9b64736f6c63430006030033

with calldata 0xf8a8fd6d (which is just test()). I double-checked that the bytecode is the correct one, and that it indeed fails when that function is called.

@agostbiro agostbiro temporarily deployed to github-action-benchmark December 4, 2024 17:30 — with GitHub Actions Inactive
@agostbiro @fvictorio
refactor: pass tracing config to provider (#738)
70ee8de 
* wip: make tracing config a provider argument

* Patch Hardhat to pass tracing config to provider

---------

Co-authored-by: Franco Victorio <[email protected]>
@agostbiro agostbiro temporarily deployed to github-action-benchmark December 5, 2024 08:20 — with GitHub Actions Inactive
@agostbiro agostbiro temporarily deployed to github-action-benchmark December 5, 2024 09:58 — with GitHub Actions Inactive
@agostbiro agostbiro had a problem deploying to github-action-benchmark December 5, 2024 10:30 — with GitHub Actions Failure
@agostbiro agostbiro had a problem deploying to github-action-benchmark December 6, 2024 10:01 — with GitHub Actions Failure
@fvictorio fvictorio temporarily deployed to github-action-benchmark December 6, 2024 10:05 — with GitHub Actions Inactive
@agostbiro agostbiro temporarily deployed to github-action-benchmark December 10, 2024 13:36 — with GitHub Actions Inactive
@agostbiro agostbiro mentioned this pull request Dec 18, 2024
Open
@agostbiro agostbiro temporarily deployed to github-action-benchmark December 19, 2024 22:46 — with GitHub Actions Inactive
@fvictorio fvictorio had a problem deploying to github-action-benchmark December 20, 2024 08:32 — with GitHub Actions Error
@fvictorio fvictorio had a problem deploying to github-action-benchmark December 20, 2024 08:43 — with GitHub Actions Error
@fvictorio fvictorio temporarily deployed to github-action-benchmark December 20, 2024 08:53 — with GitHub Actions Inactive
@agostbiro agostbiro added the no changeset needed This PR doesn't require a changeset label Dec 20, 2024
@agostbiro agostbiro temporarily deployed to github-action-benchmark December 20, 2024 15:34 — with GitHub Actions Inactive
@agostbiro agostbiro temporarily deployed to github-action-benchmark December 20, 2024 17:10 — with GitHub Actions Inactive
agostbiro
agostbiro commented Dec 20, 2024
View reviewed changes
crates/edr_napi/src/provider.rs
Comment on lines +47 to +51
let build_info_config: edr_solidity::contract_decoder::BuildInfoConfig =
serde_json::from_value(tracing_config)?;
let contract_decoder = ContractDecoder::new(&build_info_config)
.map_err(|error| napi::Error::from_reason(error.to_string()))?;
let contract_decoder = Arc::new(contract_decoder);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This work should be done on a thread from the blocking thread pool to avoid blocking the JS event loop, but that's difficult to achieve, because this is not an async function (precluded by the Env argument not being Send) and the contract decoder needs to be passed in to the logger which also needs the Env.

Since constructing the contract decoder (previously called VmTraceDecoder) was blocking previously as well, and this PR is getting very large, I don't think we have to fix it here.

@fvictorio fvictorio temporarily deployed to github-action-benchmark December 23, 2024 10:28 — with GitHub Actions Inactive
@fvictorio fvictorio had a problem deploying to github-action-benchmark December 23, 2024 11:05 — with GitHub Actions Error
@fvictorio fvictorio removed the no changeset needed This PR doesn't require a changeset label Dec 23, 2024
@fvictorio fvictorio deployed to github-action-benchmark December 23, 2024 11:10 — with GitHub Actions Active
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Change edr_napi::provider::Response::solidity_trace to return a stack trace
2 participants
@agostbiro @fvictorio