Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core/types: marshal Address as eip-55 instead of hex #24417

Closed
wants to merge 1 commit into from

Conversation

benbaley
Copy link

This PR changes the default format of Address from hex to eip-55
mentioned in #24406

Details(common/types.go):
before

// MarshalText returns the hex representation of a.
func (a Address) MarshalText() ([]byte, error) {
	return hexutil.Bytes(a[:]).MarshalText()
}

after

// MarshalText returns the hex representation of a.
func (a Address) MarshalText() ([]byte, error) {
	return a.checksumHex(), nil
}

Is this OK? can you take a look at this please? @holiman

@benbaley
Copy link
Author

benbaley commented Feb 17, 2022

I have tested the code by run go run build/ci.go test -dlgo -coverage and it‘s finished with no error
I don't know why it was failed in appveyor
Could someone help me with this?

@MariusVanDerWijden
Copy link
Member

Please revert the update of the test submodule

@holiman
Copy link
Contributor

holiman commented Mar 15, 2022

I don't think this is the best approach. I faced a similar problem when implementing eip-55 addresses in clef, and created common.MixedcaseAddress for that purpose.

// MixedcaseAddress retains the original string, which may or may not be
// correctly checksummed
type MixedcaseAddress struct {
	addr     Address
	original string
}

The thing with common.Address is that it's the canonical representation of an address: exactly 20 bytes. But in many cases, what you actually want is to know what the input was, and whether the input was correctly encoded.

This PR makes it so that we spit out checksummed addresses -- but what if the input was an erroneously encoded address, and we just ignore the error, correct it on behalf of the user, and sends it forward to the wallet for signing -- now with a correct checksum? What we should do, in this case, is either a) Reject the address on input, or b) forward the original unmodified string, and let the wallet decide on whether to accept/reject.

I'm not sure exactly where to start with this. I think the parts of geth which are not responsible for signing should not make judgements on inputs, but pass inputs unmodified along to the signer/wallet.

We should also not auto-correct addresses blindly. But in any case where we show things like the accounts that are available (which resides in the signer-parts), we should show eip55 addresses. For command line inputs, like coinbase, we should reject if eip55 checksum mismatches.

So, I don't think this PR will be merged, I think the solution needes to be a bit more nuanced.

@benbaley
Copy link
Author

OK, I see
I will close this PR accordingly

@benbaley benbaley closed this Mar 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants