permaweb / ao

The ao component and tools Monorepo - 🐰 🕳️ 👈
https://cookbook_ao.arweave.dev
Other
187 stars 60 forks source link

Standardize ETH address format case #936

Closed kunstmusik closed 2 months ago

kunstmusik commented 2 months ago

Related to https://github.com/permaweb/ao/issues/862 and https://github.com/permaweb/ao/pull/863, I was working with ETH addresses and found there are issues stemming from case insensitivity for ETH addresses and wanted to get clarity on address format.

What I found in testing with ETH signed messages is that the msg.From could have mixed-case values, e.g.,

0xFCAd0B19bB29D4674531d6f115237E16AfCE377G

While Arweave addresses are case-sensitive, EVM addresses are not, as they are string representations of the 20-byte address. In EVM ecosystems, given address strings that differ only in case, they will resolve to the same address.

However, in AO, common practice thus far has been to use values such as msg.From or msg.Recipient directly. (e.g., https://cookbook_ao.g8way.io/guides/aos/blueprints/token.html). While msg.From is derived from msg.Owner for ETH addresses, things like msg.Recipient may be entered in by users. If the case differs, token transfers may end up being recorded in multiple entries in the Balance table.

Additionally, when sending messages like Credit notices, I do not currently know what would happen if a message sent to 0xFCAd0B19bB29D4674531d6f115237E16AfCE377G and one sent to 0xfcad0b19bb29d4674531d6f115237e16afce377g would be routed to the same address. I would like to see the behavior here defined.

Ultimately, I think how ETH addresses are represented in AO messages should be clarified and a standard case used. I do think due to adhoc user input that AO devs will have to do some manual handling of ETH addresses in their process code, but at a protocol level I think it needs to be clarified for message parameters and targets.

(I think if further signature types are added (e.g., SOL), case sensitivity should be considered.)

TillaTheHun0 commented 2 months ago

Thanks @kunstmusik

You're correct that Ethereum addresses are not case-sensitive, in terms of their functionality. Having said that, there is a convention known as "EIP-55 mixed-case checksum" that uses case sensitivity to add an additional layer of verification against typos.

Using the checksum address format helps in detecting typographical errors. If an address does not follow the correct checksum format, it indicates a likely typo. Wallets and other Ethereum tools often display addresses in checksum format to provide this additional verification layer. In other words, this mixed-case is a common and well adopted convention in the Ethereum ecosystem.

This is what the ref impl of the ao Compute Unit implements. You can see the entire implementation for converting the base64url encoded public key to an Ethereum address here.

kunstmusik commented 2 months ago

@TillaTheHun0 Thanks for clarifying that. So is it safe to say for guidance in working with ETH addresses:

  1. msg.From and msg.Owner will be EIP-55 mixed-case checksum ETH addresses.
  2. Processes that handle ETH addresses (e.g., tokens) should always normalize any ETH address input parameter to EIP-55, considering user's may input values in any case. This may affect using addresses as keys to a Balance table as well as calling ao.Send( {Target = eip55EthAddress, ...}).

If so, I think it would be nice to have this documented somewhere (if not already) and for blueprints to be updated accordingly.

TillaTheHun0 commented 2 months ago

@kunstmusik

  1. Correct.
  2. It's ultimately up to process' own logic whether it will leverage the additional verification afforded by EIP-55 -- the process could choose to verify the mixed casing as an additional layer of input verification, or the process logic could choose to ignore it, normalizing any address to whatever makes sense for its use-case. In this sense, it would be not unlike verifying any other user-input.

I hope that makes sense. We will try to get this documented somewhere in the ao Cookbook (PRs welcome 😃), and perhaps as well as the spec. Thank you for bringing it to the team's attention!

zyjblockchain commented 2 months ago

https://github.com/permaweb/ao/pull/941 the pr add EIP-55 verify on token standard

TillaTheHun0 commented 2 months ago

Documented this behavior in ao Cookbook here

@kunstmusik is this issue good to be closed out?

kunstmusik commented 2 months ago

@TillaTheHun0 Yes I think so. If anything else comes up I'll raise a new issue. Thanks!