fix account sending from new accounts #517

noot · 2020-09-17T19:45:30Z

Closes: #XXX

Description

update ante handler to set new accounts in the keeper if an account is not found
allows accounts made with personal_newAccount to send transactions
also update personal api logs

to test:

make install
./init.sh

start rpc server as usual

geth attach http://localhost:8545
> personal.newAccount("password")
"<new address>"
> personal.unlockAccount("<new address>", "password")
true
> eth.sendTransaction({from: <first account that has balance>, to: <new address>, value: 0x1000000000})
<tx hash>
> eth.sendTransaction({from: <new account>, to: <any other address>, value: 0x1})

can use eth.accounts[0], eth.accounts[1] etc so you don't need to copy paste addrs

For contributor use:

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer

For admin use:

Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
Reviewers assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

codecov · 2020-09-17T19:47:36Z

Codecov Report

Merging #517 into development will decrease coverage by 1.59%.
The diff coverage is 43.75%.

@@               Coverage Diff               @@
##           development     #517      +/-   ##
===============================================
- Coverage        72.08%   70.49%   -1.60%     
===============================================
  Files               41       41              
  Lines             2723     2325     -398     
===============================================
- Hits              1963     1639     -324     
+ Misses             615      543      -72     
+ Partials           145      143       -2

Impacted Files	Coverage Δ
app/ante/eth.go	`61.15% <0.00%> (-4.21%)`	⬇️
app/ante/ante.go	`62.31% <50.00%> (-1.02%)`	⬇️
crypto/secp256k1.go	`65.51% <0.00%> (-8.85%)`	⬇️
x/evm/types/journal.go	`80.45% <0.00%> (-5.50%)`	⬇️
x/evm/types/utils.go	`41.50% <0.00%> (-4.40%)`	⬇️
utils/int.go	`29.41% <0.00%> (-3.93%)`	⬇️
x/evm/keeper/querier.go	`68.50% <0.00%> (-2.72%)`	⬇️
crypto/algorithm.go	`78.57% <0.00%> (-2.68%)`	⬇️
core/chain.go	`80.95% <0.00%> (-2.39%)`	⬇️
... and 33 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73d6c41...884ebc7. Read the comment docs.

fedekunze · 2020-09-22T08:14:32Z

rpc/eth_api.go

+	e.keybaseLock.Lock()
+	defer e.keybaseLock.Unlock()
+
+	if e.cliCtx.Keybase == nil {


nit, this condition could be inverted for better readability

fedekunze · 2020-09-22T08:17:58Z

app/ante/ante.go

+	}
+}
+
+func (asd AccountSetupDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) {


I have a couple of questions that I believe should be documented with comments:

why is this ante handler decorator only for MsgEthermint and not MsgEthereumTx too?

any reason why is the decorator on the second position of the chain of ante decorators?

MsgEthereumTx sets up the account in the NewAccountVerificationDecorator so it didn't need to be added separately, MsgEthermint uses the auth ante handlers which can't be modified, so I added this one to set up the account before the other handlers

I put it as the second one since I figured it would be good to set up the account before the rest of the decorators run, but it could be put right before whichever one validates the account

fedekunze · 2020-09-22T08:21:19Z

app/ante/eth.go

 	acc := avd.ak.GetAccount(ctx, address)
 	if acc == nil {
-		return ctx, fmt.Errorf("account %s (%s) is nil", common.BytesToAddress(address.Bytes()), address)
+		acc = avd.ak.NewAccountWithAddress(ctx, address)


The reason why the account is set to store instead of creating an error should be documented on the comments for future reference 👍

fedekunze · 2020-09-22T08:22:43Z

rpc/eth_api.go

+	for _, key := range e.keys {
+		e.logger.Debug("eth_sendTransaction", "key", fmt.Sprintf("0x%x", key.PubKey().Address().Bytes()))
+	}


I think this was a leftover debug log?

will remove!

fedekunze · 2020-09-22T08:24:07Z

rpc/personal_api.go

+	for i, key := range e.ethAPI.keys {
+		if !bytes.Equal(key.PubKey().Address().Bytes(), address.Bytes()) {
+			continue
+		}
+
+		tmp := make([]emintcrypto.PrivKeySecp256k1, len(e.ethAPI.keys)-1)
+		copy(tmp[:i], e.ethAPI.keys[:i])
+		copy(tmp[i:], e.ethAPI.keys[i+1:])
+		e.ethAPI.keys = tmp
+		return true
+	}


not sure what's the context of this change in relation to the PR title

I needed to update the ethAPI keys to allow for it to know about the new account that we are trying to send from

I think this could be improved by having one set of keys for both APIs, I can open an issue for that

fedekunze · 2020-09-22T08:24:26Z

rpc/personal_api.go

+		return common.Address{}, fmt.Errorf("invalid private key type: %T", privKey)
+	}
+	e.ethAPI.keys = append(e.ethAPI.keys, emintKey)
+	e.logger.Debug("personal_newAccount", "address", fmt.Sprintf("0x%x", emintKey.PubKey().Address().Bytes()))


leftover log?

fedekunze · 2020-09-22T08:24:49Z

rpc/personal_api.go


 	e.keys = append(e.keys, emintKey)
+	e.ethAPI.keys = append(e.ethAPI.keys, emintKey)
+	e.logger.Debug("personal_unlockAccount", "address", fmt.Sprintf("0x%x", emintKey.PubKey().Address().Bytes()))


fedekunze · 2020-09-22T08:25:29Z

tests/rpc_test.go

 	require.NoError(t, err, string(rpcRes.Result))

-	require.Equal(t, "0xef7e", gas)
+	require.Equal(t, "0xf552", gas)


I wonder why do we have to change this all the time? shouldn't the minimum always be 21,000?

yeah that's a good question, I'm not totally sure why it changes

fedekunze · 2020-09-22T08:25:45Z

rpc/personal_api.go

 //
 // https://github.com/ethereum/go-ethereum/wiki/Management-APIs#personal_sign
 func (e *PersonalEthAPI) Sign(ctx context.Context, data hexutil.Bytes, addr common.Address, passwd string) (hexutil.Bytes, error) {
+	e.logger.Debug("personal_sign", "data", data, "address", addr)


I added these ones on purpose so it's easier to tell what RPC method is being called, is there another way to see that?

fedekunze · 2020-09-22T08:25:50Z

rpc/personal_api.go

 //
 // https://github.com/ethereum/go-ethereum/wiki/Management-APIs#personal_ecRecove
 func (e *PersonalEthAPI) EcRecover(ctx context.Context, data, sig hexutil.Bytes) (common.Address, error) {
+	e.logger.Debug("personal_ecRecover", "data", data, "sig", sig)


noot added 2 commits September 17, 2020 15:26

fix account sending from new accounts

eca4d1a

cleanup

644a296

more cleanup

bec65dc

noot marked this pull request as ready for review September 17, 2020 20:20

noot requested a review from fedekunze as a code owner September 17, 2020 20:20

noot requested review from J-Thompson12 and araskachoi September 17, 2020 20:20

noot self-assigned this Sep 17, 2020

araskachoi approved these changes Sep 17, 2020

View reviewed changes

fix rpc tests

884ebc7

araskachoi merged commit 517de55 into development Sep 17, 2020

araskachoi deleted the noot/fix-personal branch September 17, 2020 22:04

fedekunze reviewed Sep 22, 2020

View reviewed changes

fix account sending from new accounts #517

fix account sending from new accounts #517

Uh oh!

Conversation

noot commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

codecov bot commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

noot commented Sep 17, 2020 •

edited

Loading

codecov bot commented Sep 17, 2020 •

edited

Loading