Fixed eth_sendTransaction routing to the local node #351

tiabc · 2017-09-20T13:29:18Z

As the result of #345 eth_sendTransaction was erroneously routed to the upstream node which is incorrect. This PR fixes it.

Todo:

if sendTransaction is used to call a contract function, it does not specify gasPrice. As it's a required argument, infura returns an error transaction underpriced. We need to get gasPrice if it's missing.

…:status-im/status-go into bugfix/eth-send-transaction-routing-#350

adambabik · 2017-09-22T09:13:17Z

geth/rpc/client.go

+// as concrete types are unknown.
+func setResultFromRPCResponse(result, response interface{}) (err error) {
+	defer func() {
+		if r := recover(); r != nil {


Check if it's possible to detect if the panic comes from value.Set() as we want to recover only that one.

tiabc

LGTM except for some minor comments.

Guys, please, merge it yourself once:

TestCallRPCSendTransaction passes (at least, locally).
missing value for required argument 1 doesn't occur.

tiabc · 2017-09-22T11:11:48Z

geth/api/backend_test.go

+		"params": [{
+			"from": "` + TestConfig.Account1.Address + `",
+			"to": "0xd46e8dd67c5d32be8058bb8eb970870f07244567",
+			"gasPrice": "0x9184e72a000",


What's the reason for setting gasPrice here, by the way?

The same - no idea.

I guess it's not needed. However, we need to add support for missing gasPrice when using the upstream. As it's described in the Todo section.

Let's remove it unless we test gasPrice setting here.

tiabc · 2017-09-22T12:30:05Z

geth/jail/execution_policy.go

@@ -96,7 +55,9 @@ func (ep *ExecutionPolicy) executeWithClient(client *rpc.Client, vm *vm.VM, req
 	if client == nil {
 		resp = newErrorResponse("RPC client is not available. Node is stopped?", &req.ID)
 	} else {
-		err = client.Call(&result, req.Method, req.Params...)
+		// TODO(adam): check if context is used
+		ctx := context.WithValue(context.Background(), common.MessageIDKey, messageID)


I thought we didn't need MessageID any more?

Removing messageID is part of jail refactoring and is done in another branch (based on this one). https://github.com/status-im/status-go/tree/refactor/jail-%23350

@divan an in progress PR would help.

tiabc · 2017-09-22T12:38:55Z

geth/node/txqueue_manager.go

+		"to", toAddr.Hex(),
+		"gas", gas,
+		"gasPrice", gasPrice,
+		"value", priceVal,


Let's rename to just value or something?

tiabc · 2017-09-22T12:43:33Z

geth/rpc/client.go


 	"github.com/ethereum/go-ethereum/node"
 	"github.com/status-im/status-go/geth/params"

 	gethrpc "github.com/ethereum/go-ethereum/rpc"
 )

+// Handler defines handler for RPC methods.
+type Handler func(context.Context, ...interface{}) (interface{}, error)


Let's rename to RPCHandler or something. Handler is way too general and it's non-obvious for the reader what it's all about at all.

Actually, typical usage of this type is rpc.Handler, and go lint suggested to rename it from RPCHandler to Handler exactly because of this :) rpc.RPCHandler is a bit clunky.

tiabc · 2017-09-22T12:44:03Z

geth/rpc/client.go

@@ -21,6 +28,9 @@ type Client struct {
 	upstream *gethrpc.Client

 	router *router
+
+	mx       sync.RWMutex       // mx guards handlers


handlersMu? Sort of classic. Or handlersMx.

tiabc · 2017-09-22T12:51:07Z

geth/rpc/client.go

+		return err
+	}
+
+	// if result is nil, just ignore result


@tiabc hmm, I remember I've read in the docs that response without result is totally valid JSON-RPC, but now quick check revealed that result is required. I'll double check it, because I remember clearly reading this in some doc (hence the comment, emphasizing that it's ok).

In theory, it's possible to call the RPC client like client.CallContext(ctx, nil, "eth_sendRawTransaction", data), thus handling nil case.

Found: I've read about it in rpc.CallContext comment:

You can also pass nil, in which case the result is ignored.

https://github.com/status-im/status-go/blob/develop/vendor/github.com/ethereum/go-ethereum/rpc/client.go#L248

Clear! Let's add a comment?

tiabc · 2017-09-22T13:10:05Z

geth/api/backend_test.go

@@ -256,6 +258,54 @@ func (s *BackendTestSuite) TestCallRPC() {
 	}
 }

+func (s *BackendTestSuite) TestCallRPCSendTransaction() {


Can this test pass at least once?

No idea. Just copied this test from other place.

I can get it passed in Ropsten easily but it does want to in RinkebyNetworkID :/

Let's change it to Ropsten.

…endTransactionRPCHandler

adambabik · 2017-09-23T22:53:10Z

The only test failing is

--- FAIL: TestJailWhisper (2.13s)
	backend_jail_test.go:574: test 0: ensure correct version of Whisper is used
	backend_jail_test.go:574: test 1: encrypted signed message from us (From != nil && To != nil)
        Error Trace:    backend_jail_test.go:592
	Error:      	Received unexpected error:
	            	TypeError: 'newMessageFilter' is not a function

This reverts commit f9066f9.

Fixed eth_sendTransaction routing to the local node

864938d

tiabc added the in progress label Sep 20, 2017

divan approved these changes Sep 20, 2017

View reviewed changes

divan added 6 commits September 20, 2017 22:32

Add RegisterHandler to RPC package.

40c1ff9

Improve RPC local handlers.

fc0ffc5

Use RegisterHandler for eth_accounts.

ec42dca

Improve RPC local handlers.

eb0a20b

Implement EthAccountsHandler.

ac648b2

Update interfaces and mocks.

e0866e6

influx6 approved these changes Sep 21, 2017

View reviewed changes

adambabik and others added 9 commits September 21, 2017 12:02

create eth_sendTransaction rpc handler

f8d00e3

Rename RPC Handler method for Accounts()

be95351

Update tests and mocks.

0d7a082

Merge branch 'bugfix/eth-send-transaction-routing-#350' of github.com…

72983aa

…:status-im/status-go into bugfix/eth-send-transaction-routing-#350

Remove duplicated queue call.

c2b3aef

remove separate handler for eth_sendTransaction from jail

f6ff00d

fix setting result in rpc.Client.callMethod

97511a8

extract setting result to setResultFromRPCResponse

ae467c9

clean up

6394cb1

adambabik removed the in progress label Sep 21, 2017

divan and others added 2 commits September 21, 2017 20:30

Fix routing tests

2c22742

add context param to rpc.Handler

d69e65a

adambabik reviewed Sep 22, 2017

View reviewed changes

oskarth mentioned this pull request Sep 22, 2017

Update status-go to 'bugfix-eth-send-transaction-routing-#350' status-im/status-mobile#1914

Closed

tiabc commented Sep 22, 2017

View reviewed changes

adambabik mentioned this pull request Sep 22, 2017

Fix SendTransaction signature in go-ethereum #355

Merged

divan added 2 commits September 22, 2017 17:43

Add requested changes

a4dc0e9

Expand arguments for callMethod

fca15f4

divan mentioned this pull request Sep 23, 2017

Refactor Jail/TxQueue #357

Closed

12 tasks

fix params in SendTransactionRPCHandler; get gasPrice if missing in S…

1dd554f

…endTransactionRPCHandler

adambabik added 6 commits September 25, 2017 09:33

Merge branch 'develop' into bugfix/eth-send-transaction-routing-#350

7524ace

add TestCallRPCSendTransactionUpstream test

7660de6

update binddata and fix some TestJailWhisper test cases

f9066f9

Revert "update binddata and fix some TestJailWhisper test cases"

830f06d

This reverts commit f9066f9.

fix generating token in TestJailWhisper

1afcdc2

update bindata to the proper web3.js

d45f4e9

divan merged commit fc8f59e into develop Sep 25, 2017

This was referenced Sep 28, 2017

Fix TestJailWhisper #336

Closed

CallRPC() does not handle eth_sendTransaction properly #350

Closed

divan mentioned this pull request Oct 5, 2017

Handle 'web.accounts' call in Jail properly #385

Closed

adambabik deleted the bugfix/eth-send-transaction-routing-#350 branch October 29, 2017 22:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed eth_sendTransaction routing to the local node #351

Fixed eth_sendTransaction routing to the local node #351

tiabc commented Sep 20, 2017 •

edited by adambabik

adambabik Sep 22, 2017

tiabc left a comment

tiabc Sep 22, 2017

divan Sep 22, 2017

adambabik Sep 22, 2017

tiabc Sep 22, 2017

tiabc Sep 22, 2017

divan Sep 22, 2017

tiabc Sep 22, 2017

tiabc Sep 22, 2017

tiabc Sep 22, 2017

divan Sep 22, 2017 •

edited

tiabc Sep 22, 2017

tiabc Sep 22, 2017

divan Sep 22, 2017 •

edited

adambabik Sep 22, 2017

divan Sep 22, 2017

tiabc Sep 22, 2017

tiabc Sep 22, 2017

divan Sep 22, 2017

adambabik Sep 22, 2017

tiabc Sep 22, 2017

adambabik commented Sep 23, 2017

Fixed eth_sendTransaction routing to the local node #351

Fixed eth_sendTransaction routing to the local node #351

Conversation

tiabc commented Sep 20, 2017 • edited by adambabik

Todo:

Choose a reason for hiding this comment

tiabc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divan Sep 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divan Sep 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adambabik commented Sep 23, 2017

tiabc commented Sep 20, 2017 •

edited by adambabik

divan Sep 22, 2017 •

edited

divan Sep 22, 2017 •

edited