Closed jeluard closed 8 months ago
mcdee
commented
9 months ago 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
commented
9 months ago 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
commented
9 months ago 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
commented
9 months ago 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.
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 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
commented
9 months ago 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
commented
9 months ago 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.
jeluard
commented
9 months ago @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
commented
9 months ago 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
commented
9 months ago I'm 99% sure teku is using EIP-55 encoding...
Looks like you do, there is a custom ssz type that extends bytes20
public class Eth1Address extends Bytes20 {Something we could easily adopt in Lodestar as well
rolfyone
commented
8 months ago I think this has been superceded by ERC-55 mixed-case addresses... ok to close?
jeluard
commented
8 months ago Good point!
This one upper-case character in the hex address in this example prevents automated tests being performed after encoding/decoding (where case is lost).