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(compatibility): use EIP-55 checksummed address notation in Anvil and Forge outputs #8715

Closed
2 tasks done
cicada-fi opened this issue Aug 22, 2024 · 8 comments · Fixed by alloy-rs/core#742
Closed
2 tasks done
Assignees
Labels
A-compatibility Area: compatibility C-anvil Command: anvil C-forge Command: forge good first issue Good for newcomers T-feature Type: feature
Milestone

Comments

@cicada-fi
Copy link

Component

Anvil

Have you ensured that all of these are up to date?

  • Foundry
  • Foundryup

What version of Foundry are you on?

forge 0.2.0 (fa0e0c2 2024-08-22T00:19:05.396104000Z)

What command(s) is the bug in?

forge script

Operating System

Linux

Describe the bug

Anvil and Forge use lowercase address notation in Anvil's output and when saving addresses to JSON artifacts in the cache folder. This behavior does not comply with EIP-55, which recommends using checksummed addresses.

eth_sendRawTransaction

    Transaction: 0xcf450414be9bd8d8629c7c2e4dc68e993e147e0d14fd8a7f3db863d225b4654c
    Contract created: 0x8ea10641fcb3ce192e6904a7873bd06cbb35a320
    Gas used: 1341778

    Block Number: 20583152
    Block Hash: 0x9d0d021da2e2a2c4dc1b4fa52dc0a87e1abd9e4a5d903b2b73ff97c47909afe0
    Block Time: "Thu, 22 Aug 2024 09:08:34 +0000"


    Transaction: 0x833d15ba1a0d0ccf242119cab40b5c81812a1e1ba71188c3cb0ac20a37ebc3c9
    Contract created: 0x14928b7929e2f1beefb874786a67d00e365cd796
    Gas used: 1250141

    Transaction: 0x47a1d0398d96a3fafacc34faef219698dcc832aab4d5d992bbdf55b058cfe2f1
    Gas used: 1296431

    Block Number: 20583153
    Block Hash: 0xda8f6d0b90480f72a5525c1b8e77cdd562ec5c894b70deb806bb7898a34ae8c1
    Block Time: "Thu, 22 Aug 2024 09:08:35 +0000"
==========================

ONCHAIN EXECUTION COMPLETE & SUCCESSFUL.

Transactions saved to: /app/vault-contract/broadcast/DeployVaultAndStrategy.sol/1/run-latest.json

Sensitive values saved to: /app/vault-contract/cache/DeployVaultAndStrategy.sol/1/run-latest.json

Lowercase format is incompatible with web3 libraries like web3.py and leads to parsing errors like this:

web3.exceptions.InvalidAddress: ('web3.py only accepts checksum addresses. The software that gave you this non-checksum address should be considered unsafe, please file it as a bug on their platform. Try using an ENS name instead. Or, if you must accept lower safety, use Web3.to_checksum_address(lower_case_address).', '0x69f94e46cbc82ab02781ac4fafc3580d21f1a888')
@cicada-fi cicada-fi added T-bug Type: bug T-needs-triage Type: this issue needs to be labelled labels Aug 22, 2024
@zerosnacks zerosnacks added C-forge Command: forge C-anvil Command: anvil A-compatibility Area: compatibility labels Aug 22, 2024
@zerosnacks zerosnacks changed the title Non-Checksum address notation in anvil and forge outputs (EIP-55 format incompatibility) bug(`compatibility): use EIP-55 checksummed address notation in Anvil and Forge outputs Aug 22, 2024
@zerosnacks zerosnacks removed the T-needs-triage Type: this issue needs to be labelled label Aug 22, 2024
@zerosnacks zerosnacks changed the title bug(`compatibility): use EIP-55 checksummed address notation in Anvil and Forge outputs bug(compatibility): use EIP-55 checksummed address notation in Anvil and Forge outputs Aug 22, 2024
@zerosnacks zerosnacks changed the title bug(compatibility): use EIP-55 checksummed address notation in Anvil and Forge outputs feat(compatibility): use EIP-55 checksummed address notation in Anvil and Forge outputs Aug 22, 2024
@zerosnacks zerosnacks added T-feature Type: feature and removed T-bug Type: bug labels Aug 22, 2024
@ShantelPeters
Copy link

Can I work on this ?

@zerosnacks zerosnacks added this to the v1.0.0 milestone Aug 23, 2024
@DaniPopes
Copy link
Member

DaniPopes commented Aug 27, 2024

This is a straightforward fix, we are still using debug formatting (:?) from legacy ethers code in some places.
With alloy, removing it will display addresses in the checksum form.

@ShantelPeters Sure, I've assigned you, feel free to comment here if you can't work on it anymore so someone else can take it.

@DaniPopes DaniPopes added the good first issue Good for newcomers label Aug 27, 2024
@ShantelPeters ShantelPeters removed their assignment Aug 31, 2024
@jenpaff
Copy link
Collaborator

jenpaff commented Sep 2, 2024

@zerosnacks could i take this one?

@zerosnacks
Copy link
Member

@zerosnacks could i take this one?

Sure! Assigning to you

@jenpaff
Copy link
Collaborator

jenpaff commented Sep 16, 2024

This is a straightforward fix, we are still using debug formatting (:?) from legacy ethers code in some places. With alloy, removing it will display addresses in the checksum form.

@ShantelPeters Sure, I've assigned you, feel free to comment here if you can't work on it anymore so someone else can take it.

Hey @DaniPopes ! Could you clarify your comment? If I get it right the issue occurs here when we serialize the Sequence type. We could either add a serialization on the Address type (assuming that we always want to output checksum format address when serializing to json or adding a serialization function for the fields in question. Or did I get something wrong?

@jenpaff
Copy link
Collaborator

jenpaff commented Oct 8, 2024

reopening since the implemented solution caused a bug further down the line for reth users e.g. see paradigmxyz/reth#11541 (comment)

users actually expect a lowercase format and implementing variation of the format should be on the consumer/ui side, so to be decided if we should actually implement this or leave it to the user to implement the correct format

@jenpaff jenpaff reopened this Oct 8, 2024
@zerosnacks
Copy link
Member

zerosnacks commented Oct 8, 2024

users actually expect a lowercase format and implementing variation of the format should be on the consumer/ui side, so to be decided if we should actually implement this or leave it to the user to implement the correct format

On a general note, when working on the client side I've found that consistently casting to lowercase was required when dealing with addresses sourced from external services as most of the time you likely end up with a mix between the two

@jenpaff jenpaff added this to Foundry Oct 14, 2024
@github-project-automation github-project-automation bot moved this to Todo in Foundry Oct 14, 2024
@grandizzy grandizzy closed this as not planned Won't fix, can't repro, duplicate, stale Oct 14, 2024
@github-project-automation github-project-automation bot moved this from Todo to Done in Foundry Oct 14, 2024
@DaniPopes
Copy link
Member

When printing addresses we use checksum by default, in JSON output we don't anymore because this caused issues, so partially fixed, partially wontfix.

@grandizzy grandizzy moved this from Done to Completed in Foundry Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-compatibility Area: compatibility C-anvil Command: anvil C-forge Command: forge good first issue Good for newcomers T-feature Type: feature
Projects
Status: Completed
Development

Successfully merging a pull request may close this issue.

6 participants