internal/ethapi: make NewAccount return EIP-55 format

[alexmylonas]

This PR implements the suggestion of #24406 to return the address as EIP-55 encoded when creating a new account.

[rjl493456442]

An alternative approach is to define the customized json marshaller

e.g.

func (a Address) MarshalJSON() ([]byte, error) {
	return json.Marshal(a.Hex())
}

But in this case, the address will always be marshalled in checksum'd format. I am not sure this is what we want.

[rjl493456442]

Or we can define ChecksumedAddress type which has this customized marshal rule and return it instead.

[holiman]

Or we can define ChecksumedAddress type which has this customized marshal rule and return it instead.

👍 from me. AddressEIP55 or AddressWithChecksum or EIP55Address maybe?

[alexmylonas]

Like adding something like this in types.go?

type AddressEIP55 [AddressEIP55Length]byte // AddressEIP55Length is 42

// NewAddressEIP55 creates a new AddressEIP55 from an Address.
func NewAddressEIP55(addr Address) AddressEIP55 {
	return BytesToAddressEIP(addr.checksumHex())
}

func BytesToAddressEIP(b []byte) AddressEIP55 {
	var a AddressEIP55
	copy(a[:], b)
	return a
}

func (a AddressEIP55) String() string {
	return string(a[:])
}

[rjl493456442]

diff --git a/common/types.go b/common/types.go
index 218ca0be4c..1431857372 100644
--- a/common/types.go
+++ b/common/types.go
@@ -429,3 +429,12 @@ func (ma *MixedcaseAddress) ValidChecksum() bool {
 func (ma *MixedcaseAddress) Original() string {
 	return ma.original
 }
+
+// AddressEIP55 is an alias of Address with a customized json marshaller
+// defined in the manner of EIP55.
+type AddressEIP55 Address
+
+// MarshalJSON marshals the address in the manner of EIP55.
+func (addr AddressEIP55) MarshalJSON() ([]byte, error) {
+	return json.Marshal(Address(addr).Hex())
+}
diff --git a/common/types_test.go b/common/types_test.go
index 94492278d8..d41b4b1d7e 100644
--- a/common/types_test.go
+++ b/common/types_test.go
@@ -534,3 +534,21 @@ func TestHash_Format(t *testing.T) {
 		})
 	}
 }
+
+func TestAddressEIP55(t *testing.T) {
+	addr := HexToAddress("0x5aaeb6053f3e94c9b9a09f33669435e7ef1beaed")
+	blob, err := json.Marshal(AddressEIP55(addr))
+	if err != nil {
+		t.Fatal("Failed to marshal AddressEIP55", err)
+	}
+	if strings.Trim(string(blob), "\"") != addr.Hex() {
+		t.Fatal("Address with checksum is expected")
+	}
+	var dec Address
+	if err := json.Unmarshal(blob, &dec); err != nil {
+		t.Fatal("Failed to unmarshal AddressEIP55", err)
+	}
+	if addr != dec {
+		t.Fatal("Unexpected address after unmarshal")
+	}
+}

A very quick implementation. You can probably polish a bit on top.

[alexmylonas]

Thanks for the feedback @rjl493456442!

Pushed a commit that addresses this properly I hope!

[holiman]

Generally LGTM, but maybe we don't need all that

[holiman]

I'm not sure this test really brings anything new? All it does it tests

1. That an account can be created
2. That the response is hex-encoded

Unless I missed something, might aswell remove it

[alexmylonas]

Yes, that is exactly what it does.

I saw that this part of the codebase didn't have much unit testing and took it as an opportunity to add a couple.

I can remove it if you feel is superficial!

[holiman]

Sorry for late response. Yes, please remove it, IMO it doesn't add anything. Since you are using NewPersonalAccountApi (not going via any url dispatcher), you are guaranteed to obtain a common.AddressEIP55.
The check common.IsHex.. is basically a long-winded unit-test, and IsHex doesn't even check anything related to EIP-55.

[alexmylonas]

Yes, that makes sense while it helped me validate that this actually works initially, I can see it doesn't add a lot in terms of testing. Reverted!

[holiman]

LGTM