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

feat!: Stop supporting console.log selectors with "(u)int" aliases #5764

Merged
merged 3 commits into from
Sep 26, 2024

Conversation

Xanewok
Copy link
Contributor

@Xanewok Xanewok commented Sep 19, 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 (@alcuadrado, @fvictorio).
  • [] I didn't do anything of this.

As discussed in Slack, this removes the support for selectors that use the faulty ABI encoding. To reiterate, I searched these selectors in GitHub and I found either someone's Hardhat forks or tools that do not seem to have much usage.

We started shipping correct functions in console.log since bb17b50 (Jul 2022, Hardhat 2.10.2) and it looks like Foundry introduced a patch that normalized our faulty selectors at least since https://github.com/foundry-rs/foundry/pull/6394/files#diff-4726f7deca3fcac31136a3902fd173c49a78f3c3b6322b9bf98c7226b0ebc10a (Nov 2023). I also bumped the changeset to a minor version, as this has a non-zero potential of breaking things.

Because the chances of someone deploying a contract that contains a console.log are slim and even then the value seems to be low, this should be a relatively safe change to make; we can bring it back or re-introduce a grace period then.

@DaniPopes since you worked on this from the Foundry's side of things, do you think there's something else that we'd need to be aware of? Would you still support these fixed selectors in Foundry?

Historically, Hardhat used selectors that computed signatures such as
"log(int)", "log(bool,int)", even though the canonical ABI spec defines
that the keccak256 should be calculated over the canonical type name, so
"int256" or "uint256".

This change removes this support as popular tools already support the
correct ABI encoding or patch the faulty one, instead.
Copy link

vercel bot commented Sep 19, 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 19, 2024 4:23pm

Copy link

changeset-bot bot commented Sep 19, 2024

🦋 Changeset detected

Latest commit: 82d0c84

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

This PR includes changesets to release 1 package
Name Type
hardhat 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

@github-actions github-actions bot added the status:ready This issue is ready to be worked on label Sep 19, 2024
@DaniPopes
Copy link

DaniPopes commented Sep 19, 2024

We support both kinds of console, and treat them exactly the same since foundry-rs/foundry#6504. Before this, people were using console2 from forge-std because the console calls would not show up in traces.

More recently, we made console2 have the same implementation as console in foundry-rs/forge-std#573; this is currently still using the aliases for backwards compatibility, but if you're going to stop supporting it we can also follow.

I'm not aware of any breakage other than very old Hardhat/Foundry versions, so we wouldn't mind dropping support for it entirely too since it would simplify code and reduce binary size.

@Xanewok
Copy link
Contributor Author

Xanewok commented Sep 19, 2024

Sounds good, thanks for the context!

We're waiting for @alcuadrado sign off here; we can then drop the support for the alias signatures moving forward.

@alcuadrado
Copy link
Member

Looks good! Thanks, @DaniPopes, for chiming in.

@Xanewok Xanewok added the status:blocked Blocked by other issues or external reasons label Sep 20, 2024
@Xanewok
Copy link
Contributor Author

Xanewok commented Sep 20, 2024

This is blocked until we release a Hardhat version that uses the EDR-powered stack tracing.

@fvictorio fvictorio merged commit cc619ce into main Sep 26, 2024
106 checks passed
@fvictorio fvictorio deleted the feat/remove-support-for-int-aliased-console-logs branch September 26, 2024 12:23
Wodann added a commit to NomicFoundation/edr that referenced this pull request Oct 22, 2024
Wodann added a commit to NomicFoundation/edr that referenced this pull request Oct 23, 2024
Wodann added a commit to NomicFoundation/edr that referenced this pull request Oct 25, 2024
github-merge-queue bot pushed a commit to NomicFoundation/edr that referenced this pull request Oct 28, 2024
* test: add test to validate HTTP headers

* build: bump Hardhat to v2.22.15

* fix: remove console.log backwards compatibility tests

This was already done in Hardhat in this PR:
NomicFoundation/hardhat#5764
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:blocked Blocked by other issues or external reasons status:ready This issue is ready to be worked on
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants