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

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

Merged
merged 3 commits into from
May 17, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
13 changes: 13 additions & 0 deletions common/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -429,3 +429,16 @@ func (ma *MixedcaseAddress) ValidChecksum() bool {
func (ma *MixedcaseAddress) Original() string {
return ma.original
}

// AddressEIP55 is an alias of Address with a customized json marshaller
type AddressEIP55 Address

// String returns the hex representation of the address in the manner of EIP55.
func (addr AddressEIP55) String() string {
return Address(addr).Hex()
}

// MarshalJSON marshals the address in the manner of EIP55.
func (addr AddressEIP55) MarshalJSON() ([]byte, error) {
return json.Marshal(addr.String())
}
24 changes: 24 additions & 0 deletions common/types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -534,3 +534,27 @@ func TestHash_Format(t *testing.T) {
})
}
}

func TestAddressEIP55(t *testing.T) {
addr := HexToAddress("0x5aaeb6053f3e94c9b9a09f33669435e7ef1beaed")
addrEIP55 := AddressEIP55(addr)

if addr.Hex() != addrEIP55.String() {
t.Fatal("AddressEIP55 should match original address hex")
}

blob, err := addrEIP55.MarshalJSON()
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")
}
}
11 changes: 6 additions & 5 deletions internal/ethapi/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -354,19 +354,20 @@ 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) (common.AddressEIP55, error) {
ks, err := fetchKeystore(s.am)
if err != nil {
return common.Address{}, err
return common.AddressEIP55{}, err
}
acc, err := ks.NewAccount(password)
if err == nil {
log.Info("Your new key was generated", "address", acc.Address)
addrEIP55 := common.AddressEIP55(acc.Address)
log.Info("Your new key was generated", "address", addrEIP55.String())
log.Warn("Please backup your key file!", "path", acc.URL.Path)
log.Warn("Please remember your password!")
return acc.Address, nil
return addrEIP55, nil
}
return common.Address{}, err
return common.AddressEIP55{}, err
}

// fetchKeystore retrieves the encrypted keystore from the account manager.
Expand Down
30 changes: 30 additions & 0 deletions internal/ethapi/api_test.go
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.String()) {
t.Fatalf("address isn't hex encoded: %s", addr)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!