Streamline `ConsoleLogger` logic and state

NomicFoundation / hardhat

Hardhat is a development environment to compile, deploy, test, and debug your Ethereum software.

https://hardhat.org

Other

7k stars 1.36k forks source link

Streamline `ConsoleLogger` logic and state #5411

Closed Xanewok closed 6 days ago

Xanewok commented 2 weeks ago

[ ] Because this PR includes a bug fix, relevant tests have been included.
[ ] Because this PR includes a new feature, the change was previously discussed on an Issue or with someone from the team.
[x] I didn't do anything of this.

Part of https://github.com/NomicFoundation/edr/issues/246

The starting point for this PR was analyzing how we create the Provider and how different pieces interact with each other in JS and Rust<>JS in general around the stack traces logic and tests.

This removes some of the dead or redundant code and refreshes the console.log signature lookup table generation script. If we decide to move it to Rust, we won't have to call JS anymore and move buffers around just to decode and execute the console.log formatting logic.

After moving the auxiliary script to TS I re-ran it to verify that it outputs the same code (incl. the changes from this PR).

vercel[bot] commented 2 weeks ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
hardhat	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 19, 2024 11:14am

changeset-bot[bot] commented 2 weeks ago

⚠️ No Changeset found

Latest commit: cfd088485df9ecb45a3c54e5d904e78b8e0ffc6b

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

agostbiro commented 2 weeks ago

Hey thanks for this PR @Xanewok ! I had a look, but I don't feel qualified to review it.

fvictorio commented 6 days ago

Checking my understanding here. This PR has two parts:

The most important part is turning ConsoleLogger into a bag of static methods, because (I guess) that will make porting it easier. Is this correct?
The other part is migrating scripts/console-library-generator.js to typescript. I guess you did this as part of checking how the code works, and it doesn't hurt to commit it.

If this is correct, then LGTM.

Xanewok commented 6 days ago

That is correct:

The first part isolates the logic in ConsoleLogger - the class did not in fact depend on some initial state but rather just used the selectors that are static/const and have to be generated by the script. After the refactor we know that we only need to depend on a single function to correctly format the logs in the EDR and it will be easier to port and use it directly rather than having to roundtrip via FFI only to format the console logs.
The script that was originally used to generate the data used by the ConsoleLogger was discovered to be broken in the meantime (there's no bufferToInt anymore), so I migrated it to TS to ensure we catch any such regressions in the future (even if a short one, as we want to port it anyways) and made sure it still works as expected.

fvictorio commented 6 days ago

Makes sense! Feel free to merge this if you want.