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.

Sign up for GitHub

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

Jump to bottom

internal/ethapi: make NewAccount return EIP-55 format #26973

Merged
merged 3 commits into from
May 17, 2023
Merged

internal/ethapi: make NewAccount return EIP-55 format #26973

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
37b3c4f
chore: updated NewAccount to return address as EIP-55 format
alexmylonas Mar 24, 2023
22b1be8
chore: address PR feedback
alexmylonas Mar 31, 2023
940eabd
chore: remove redudant test
alexmylonas Apr 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions internal/ethapi/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,19 +354,19 @@ func (s *PersonalAccountAPI) DeriveAccount(url string, path string, pin *bool) (
}

// NewAccount will create a new account and returns the address for the new account.
func (s *PersonalAccountAPI) NewAccount(password string) (common.Address, error) {
func (s *PersonalAccountAPI) NewAccount(password string) (string, error) {
Copy link
Member

@rjl493456442 rjl493456442 Mar 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

👍 from me. AddressEIP55 or AddressWithChecksum or EIP55Address maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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[:])
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback @rjl493456442!

Pushed a commit that addresses this properly I hope!

ks, err := fetchKeystore(s.am)
if err != nil {
return common.Address{}, err
return "", err
}
acc, err := ks.NewAccount(password)
if err == nil {
log.Info("Your new key was generated", "address", acc.Address)
log.Info("Your new key was generated", "address", acc.Address.Hex())
log.Warn("Please backup your key file!", "path", acc.URL.Path)
log.Warn("Please remember your password!")
return acc.Address, nil
return acc.Address.Hex(), nil
}
return common.Address{}, err
return "", err
}

// fetchKeystore retrieves the encrypted keystore from the account manager.
Expand Down
30 changes: 30 additions & 0 deletions internal/ethapi/api_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,24 @@ import (
"math/big"
"testing"

"github.com/ethereum/go-ethereum/accounts"
"github.com/ethereum/go-ethereum/accounts/keystore"
"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/core/types"
"github.com/ethereum/go-ethereum/crypto"
"github.com/ethereum/go-ethereum/params"
)

type backendTest struct {
backendMock
backends []accounts.Backend
}

func (b *backendTest) AccountManager() *accounts.Manager {
ac := accounts.Config{InsecureUnlockAllowed: false}
return accounts.NewManager(&ac, b.backends...)
}

alexmylonas marked this conversation as resolved.
Show resolved Hide resolved
func TestTransaction_RoundTripRpcJSON(t *testing.T) {
var (
config = params.AllEthashProtocolChanges
Expand Down Expand Up @@ -157,3 +169,21 @@ func allTransactionTypes(addr common.Address, config *params.ChainConfig) []type
},
}
}

func TestNewPersonalAccount(t *testing.T) {
d := t.TempDir()
ks := keystore.NewKeyStore(d, keystore.StandardScryptN, keystore.StandardScryptP)
b := &backendTest{
backends: []accounts.Backend{ks},
}
nonceLock := new(AddrLocker)
pa := NewPersonalAccountAPI(b, nonceLock)
addr, err := pa.NewAccount("password")
if err != nil {
t.Fatalf("failed to create account: %v", err)
}

if !common.IsHexAddress(addr) {
t.Fatalf("address isn't hex encoded: %s", addr)
}
}