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.

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

refactor: Streamline console.sol generation #5759

Merged
merged 5 commits into from
Sep 19, 2024

Conversation

Xanewok
Copy link
Contributor

@Xanewok Xanewok commented Sep 18, 2024

  • 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.
  • I didn't do anything of this.

I'm working on porting console.log from Hardhat (NomicFoundation/edr#641). To better understand which exactly console.log functions we support and why, I refactored the build script for clarity in the process.

I separated helper logic (e.g. permutation generator) and upfront declare which functions we support:

  • single parameter logUint, ..., logBytes32 and
  • permutations of up to 4 of (uint256, string, bool, address), including empty log()

Since the calculation logic was mixed with rendering (and had to take into account duplicate signatures, for example), I disentangled it and streamlined the (data) flow. Hopefully this will be useful to anyone trying to understand this or modify in the future.

While making the changes, I also changed the function selectors in the logger.ts file to be a hexadecimal integer rather than a decimal one. This is the format everyone expects when working with it so there's no use in using decimals if a hexadecimal integer is natively supported as an object key.

In the future, we can remove the faulty (u)int ABI encoding; I tried to highlight and and document the backwards-compatibility here.

@Xanewok Xanewok added the no changeset needed This PR doesn't require a changeset label Sep 18, 2024
Copy link

vercel bot commented Sep 18, 2024

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 Sep 18, 2024 5:06pm

Copy link

changeset-bot bot commented Sep 18, 2024

⚠️ No Changeset found

Latest commit: 5b410f9

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

@github-actions github-actions bot added the status:ready This issue is ready to be worked on label Sep 18, 2024
Comment on lines +89 to +91
[...Array(4)].flatMap((_, paramCount) => {
return Array.from(
genPermutations(TYPES.length, paramCount + 1),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
[...Array(4)].flatMap((_, paramCount) => {
return Array.from(
genPermutations(TYPES.length, paramCount + 1),
[...Array(5)].flatMap((_, paramCount) => {
return Array.from(
genPermutations(TYPES.length, paramCount),

As a side-note, this base case can handle generating the empty log() just fine but shifted its order in the console.sol file. This is probably meaningless but I wanted to avoid making any changes; let me know if you'd like me to not explicitly handle log() and handle this with this code, instead.

Copy link
Member

Choose a reason for hiding this comment

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

That's a good point, but yes, let's leave it as is.

@Xanewok Xanewok merged commit f93c447 into main Sep 19, 2024
105 checks passed
@Xanewok Xanewok deleted the refactor/streamline-console-library-generator branch September 19, 2024 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changeset needed This PR doesn't require a changeset status:ready This issue is ready to be worked on
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants