Have hex addresses all lower-case

[jeluard]

This one upper-case character in the hex address in this example prevents automated tests being performed after encoding/decoding (where case is lost).

[mcdee]

That looks like an EIP-55 checksummed execution address. Not sure that lower-casing it in this instance would be considered a fix, could you include EIP-55 encoding for execution addresses in the automated tests?

[jeluard]

According to this site it's not proper EIP-55 encoding (assuming this site is correct). Looks more like a typo to me. Also I have not seen other addresses being checksummed in the examples.
And does it make sense at all to have hex with upper-case produced by the API? My (limited) understanding is that it does not in this scenario.

[mcdee]

There are very few execution addresses in the beacon API, this may even be the only place where one is present.

Having the API output execution addresses in checksummed format would be good practice. It isn't mandated anywhere in this API, so down to individual implementations to implement (or not).

If there is a change to be made, I'd prefer to see any and all instances of execution addresses using checksum format in the specification, but it's only a mild preference given how frequently APIs don't bother outputting checksummed addresses.

[nflaig]

It does look like the encoding is invalid according to EIP-55, correct would be 0x9Be8d619c56699667c1feDCD15f6b14D8B067F72 but even if it was valid I think it still does make sense to use all lower-case.

• Consensus / beacon API do not use any EIP-55 encoded addresses (e.g. ExecutionAddress, added in #178)
• BLSToExecutionChange is part of the consensus spec and the SSZ type for to_execution_address uses Bytes20, there is no special type for execution address
• The events API is a bit of an outlier were the examples are used as "schema" due to OpenAPI limitations Swagger for Server Sent Events OAI/OpenAPI-Specification#396
• (EIP-55 encoding seems more useful for user facing applications like wallets / explorers)

As I doubt any consensus client implements EIP-55 encoding for execution addresses, it might be better to use all lower-case to maintain consistency and avoid misleading assumptions for API consumers.

[mcdee]

I realize that I'm likely fighting a losing battle here, but IMO API's should absolutely state that execution addresses should be output in EIP-55 format. It's incredibly cheap to do, and shows best practice. It also allows cut and paste of addresses directly from the output of the API into systems that expect checksummed addresses.

The more systems that use EIP-55 for addresses the more that others will do so for compatibility as well as recognizing best practice. Even if it's optional and not implemented in consensus clients, it would be good to have it in the spec.

[nflaig]

I realize that I'm likely fighting a losing battle here

I am not really opposed to this, just stated the status quo. You have more experience in the usefulness of this (building cli tooling that consumes APIs) and we should probably push for a standard encoding as this improves spec consistency and allows for easier testing against spec values as all example addresses in the API spec would then have to use ERC-55 format. I don't really see this as part of consensus spec though.

• Use ERC-55 mixed-case checksum address encoding for execution address #396

[jeluard]

@mcdee your point makes sense, and I feel like it's reasonable to address the EIP-55support as a separate issue as @nflaig did.

The current PR fixes what looks like a typo, as currently the execution address is not in EIP-55 format. As a first step towards improved data format I suggest to have every addresses in lower-case.

[rolfyone]

As I doubt any consensus client implements EIP-55 encoding for execution addresses, it might be better to use all lower-case to maintain consistency and avoid misleading assumptions for API consumers.

I'm 99% sure teku is using EIP-55 encoding...

[nflaig]

I'm 99% sure teku is using EIP-55 encoding...

Looks like you do, there is a custom ssz type that extends bytes20

Eth1Address.java

public class Eth1Address extends Bytes20 {

Something we could easily adopt in Lodestar as well

[rolfyone]

I think this has been superceded by ERC-55 mixed-case addresses... ok to close?

[jeluard]

Good point!