foundry-rs / foundry

Foundry is a blazing fast, portable and modular toolkit for Ethereum application development written in Rust.
https://getfoundry.sh
Apache License 2.0
8.3k stars 1.75k forks source link

feat(`compatibility`): use EIP-55 checksummed address notation in Anvil and Forge outputs #8715

Closed cicada-fi closed 4 weeks ago

cicada-fi commented 2 months ago

Component

Anvil

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

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')
ShantelPeters commented 2 months ago

Can I work on this ?

DaniPopes commented 2 months ago

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.

jenpaff commented 2 months ago

@zerosnacks could i take this one?

zerosnacks commented 2 months ago

@zerosnacks could i take this one?

Sure! Assigning to you

jenpaff commented 1 month ago

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 commented 1 month ago

reopening since the implemented solution caused a bug further down the line for reth users e.g. see https://github.com/paradigmxyz/reth/issues/11541#issuecomment-2397852547

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

zerosnacks commented 1 month ago

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

DaniPopes commented 4 weeks ago

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