foundry icon indicating copy to clipboard operation
foundry copied to clipboard

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

Open cicada-fi opened this issue 1 year ago • 2 comments

Component

Anvil

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

  • [X] Foundry
  • [X] 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 avatar Aug 22 '24 09:08 cicada-fi

Can I work on this ?

ShantelPeters avatar Aug 23 '24 03:08 ShantelPeters

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 avatar Aug 27 '24 18:08 DaniPopes

@zerosnacks could i take this one?

jenpaff avatar Sep 02 '24 09:09 jenpaff

@zerosnacks could i take this one?

Sure! Assigning to you

zerosnacks avatar Sep 02 '24 09:09 zerosnacks

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 avatar Sep 16 '24 20:09 jenpaff

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

jenpaff avatar Oct 08 '24 12:10 jenpaff

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

zerosnacks avatar Oct 08 '24 13:10 zerosnacks

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

DaniPopes avatar Oct 14 '24 14:10 DaniPopes