Add flexible rpc client to tendermint by ethanfrey · Pull Request #418 · tendermint/tendermint

ethanfrey · 2017-02-22T19:05:01Z

This code to call the rpc was in light-client, but I abstracted it to make it more testable, and provided mocks, so we can actually can simulate the server.

A bit of clean up of the existing code to test the rpc interface, and then a lot of code in rpc/client/*

I would like to get this in by 0.9.0 and then use it as the basis for light-client (so I can clean up that code as well).

Please let me know if I should change anything. Mocks are not finished, especially as faking sigs is hard, but they are halfway done and already provide a boon for testing (client/helpers_test.go).

ethanfrey · 2017-02-22T19:06:04Z

Forgot to mention, this is in response to issue #416

codecov-io · 2017-02-22T19:16:38Z

Codecov Report

Merging #418 into develop will increase coverage by 1.51%.
The diff coverage is 71.12%.

@@            Coverage Diff             @@
##           develop    #418      +/-   ##
==========================================
+ Coverage    50.08%   51.6%   +1.51%     
==========================================
  Files           38      44       +6     
  Lines         4460    4897     +437     
==========================================
+ Hits          2234    2527     +293     
- Misses        2028    2156     +128     
- Partials       198     214      +16

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 1f1dcea...d56cb2a. Read the comment docs.

melekes · 2017-02-22T19:39:34Z

Wow! So much code!

melekes · 2017-02-22T19:58:05Z

rpc/client/http/rpc_test.go

+
+	// subscribe to a transaction event
+	_, _, tx := MakeTxKV()
+	// this causes a panic in tendermint core!!!


old comment... I think I pushed a fix to tendermint in October, before I got hired even.

this client code was born out of a personal project. But yeah, due to some typos and crazy interface magic, subscribing to a tx event would cause tendermint to panic:
#285

Removing this comment now.

melekes · 2017-02-23T08:26:12Z

rpc/client/local/app_test.go

+)
+
+// MakeTxKV returns a text transaction, allong with expected key, value pair
+func MakeTxKV() ([]byte, []byte, []byte) {


not a fan of go-common, but code duplication here hurts me a bit.

Didn't know this was in go-common, will take a look and fix

No, I meant that we should probably move this into one package and reuse rather than duplicating the code.

that code belongs in merkleeyes actually... will put it there.

RandAsciiBytes is almost the same as common.RandStr, will use RandStr instead

melekes · 2017-02-23T08:27:19Z

rpc/client/local/client.go

+package local
+
+import (
+	nm "github.com/tendermint/tendermint/node"


nm - node module?

yup, i use node too much as a local variable and didn't want naming conflict

melekes · 2017-02-23T08:33:09Z

rpc/client/local/rpc_test.go

@@ -0,0 +1,183 @@
+package local_test


also, this testing suite is duplicated in 2 places. I understand we want to test both implementations. Not sure what is the best way to deal with this.

Yes, this hurts me a bit too. If you have a clean solution so I can convert this to a table-driven test, let me know.

Also, for code coverage, it only really measures the coverage from tests in the same package.

I'll think of a way to do code reuse. Let me know if it is too ugly.

melekes · 2017-02-23T08:35:32Z

rpc/client/mock/abci.go

+	App abci.Application
+}
+
+func (a ABCIApp) _assertABCIClient() client.ABCIClient {


Why do we need such methods (_assertIsClient, _assertABCI...)?

Not needed by nice helpers. Talked with Bucky about them, he liked the idea too.

Basically, when you create Struct explicitly to fulfill an interface, you can assert so if the methods don't match, you get a compile error in the module itself, not when someone else tried to use it.

Matt had this problem with endblock changing SIG and then quietly being ignored in basecoin without any compiler warnings.

we don't need this anymore. It was only an issue because there was an embedded interface type in abci.Application (the BlockchainAware bit). Now that has been moved into the abci.Application, so as long as that interface is being passed around (it is, everywhere), then the compiler already type checks

In this case, I want to make sure my mock structs properly implement the client interface.

the abci app was just to explain to anton why the idea came up.

melekes · 2017-02-23T08:41:41Z

rpc/client/mock/abci.go

+}
+
+func (a ABCIApp) BroadcastTxAsync(tx types.Tx) (*ctypes.ResultBroadcastTx, error) {
+	d := a.App.DeliverTx(tx)


I think it should be CheckTx here, no? and empty result in this case

// Returns right away, with no response func BroadcastTxAsync(tx types.Tx) (*ctypes.ResultBroadcastTx, error) { err := mempool.CheckTx(tx, nil) if err != nil { return nil, fmt.Errorf("Error broadcasting transaction: %v", err) } return &ctypes.ResultBroadcastTx{}, nil }

Yes, will fix. Good eyes.

We want this action to actually happen in the mock, but I change it to return the results of CheckTx, and trigger DeliverTx in a go-routine on success, so we can actually simulate this race in test cases if we want.

melekes · 2017-02-23T08:42:02Z

rpc/client/mock/abci.go

+}
+
+func (a ABCIApp) BroadcastTxSync(tx types.Tx) (*ctypes.ResultBroadcastTx, error) {
+	d := a.App.DeliverTx(tx)


same here (CheckTx)

melekes · 2017-02-23T09:04:13Z

rpc/test/helpers.go

-	clientJSON = client.NewClientJSONRPC(requestAddr)
-	clientGRPC = core_grpc.StartGRPCClient(grpcAddr)
+// f**ing long, but unique for each test
+func makePathname() string {


https://golang.org/pkg/io/ioutil/#TempDir ?

melekes · 2017-02-23T09:11:38Z

Nice job! Learned a lot by reading the code!

ethanfrey · 2017-02-23T13:18:50Z

@melekes I pulled local and http subpackages into client, renamed all structs, rewrote the rpc_tests to be table driven and accept any number of client.Client implementations.

Oh, and rebased everything on develop (which is why all commits are "new")

Please take a look at 7fe9ff5 and let me know what you think.

ethanfrey · 2017-02-23T13:22:54Z

@ebuchman I did lots of cleanup from anton's comments, please take a look. I'd like to pull out the code in light-client.

melekes · 2017-02-23T14:11:19Z

LGTM

ebuchman

Fantastic improvements. Love that we have a merkle proof test in there now. Thanks Frey.

ebuchman · 2017-02-23T18:37:27Z

rpc/client/interface.go

+type NetworkClient interface {
+	NetInfo() (*ctypes.ResultNetInfo, error)
+	// remove this???
+	DialSeeds(seeds []string) (*ctypes.ResultDialSeeds, error)


remove it.

maybe we should add DumpConsensusState tho - some reason not too?

ebuchman · 2017-02-23T18:40:47Z

rpc/client/localclient.go

+}
+
+/** websocket event stuff here... **/
+


To be implemented with the local event system?

ebuchman · 2017-02-23T18:50:46Z

rpc/client/mock/client.go

+	return core.Validators()
+}
+
+/** websocket event stuff here... **/


remove or replace

ebuchman · 2017-02-23T18:54:12Z

rpc/client/rpc_test.go

+		assert.EqualValues(h, block.BlockMeta.Header.Height)
+
+		// check blockchain info, now that we know there is info
+		// TODO: is this commented somewhere that they are returned


No - we ought to add more detail here https://tendermint.com/docs/internals/rpc

ebuchman · 2017-02-23T18:58:38Z

rpc/client/rpc_test.go

+		require.Nil(err, "%d: %+v", i, err)
+		assert.Equal(block.Block.LastCommit, commit2.Commit)
+
+		// and we got a proof that works!


ebuchman · 2017-02-23T19:01:14Z

rpc/client/rpc_test.go

+	select {
+	case res := <-r:
+		checkData(res, ctypes.ResultTypeEvent)
+		// this is good.. let's get the data... ugh...


hm ... so what's up here?

ethanfrey · 2017-02-23T23:41:51Z

I'll clean this up tomorrow. Thanks for the review.

Yeah, all the event stuff issue stuff is not really supported well. I added what I had to the client.HTTP only and the user has to do all the casting. I want to figure out a good way of exposing events, getting feedback from people trying to use it, and then make a nice event system for 0.10 and add this to the Client interface.

Note I had trouble parsing the event that came out - some json blob... maybe we can overhaul the json format for 0.10 as well...

…tp.Client

…iven tests

ethanfrey · 2017-02-24T16:17:08Z

Addressed some changes. Docs missing and thinking about how/if to expose websockets? Maybe just expose an EventSwitch on the client? Does it make sense to work on that today? Or rather targeting 0.9.1?

…over websocket

ebuchman · 2017-02-24T21:06:10Z

rpc/client/helpers.go

+	evts, quit := make(chan events.EventData, 10), make(chan bool, 1)
+	// start timeout count-down
+	go func() {
+		time.Sleep(1 * time.Second)


huh? comment on the name of the channel?
I have a timeout as time.Duration, so I picked another name. Don't mind changing names, if you have an idea.

shouldn't time.Sleep(1 * time.Second) be time.Sleep(timeout * time.Second)

ebuchman · 2017-02-24T21:06:39Z

rpc/client/helpers.go

+// This handles subscribing and unsubscribing under the hood
+func WaitForOneEvent(evsw types.EventSwitch,
+	evtTyp string, timeout time.Duration) (types.TMEventData, error) {
+	listener := cmn.RandStr(12)


this might not be a good idea. maybe we push it up to the interface and let the tests pass in random junk

Which part is a bad idea? Waiting? subscribing for the events?

sorry I just meant using a random string for the listener. I'm just thinking of the day there might be many calls to WaitForOneEvent and no one will know which listener is which

the "listener" is just for that one call-back. I can let the caller decide, but it will probably be hard-coded various places. and duplicate names is worse, then second might not get the message.

ebuchman · 2017-02-24T21:06:59Z

rpc/client/helpers.go

+	})
+	// make sure to unregister after the test is over
+	// TODO: why doesn't the other call work???
+	// defer evsw.RemoveListenerForEvent(listener, evtTyp)


what happens?

On client.Local, if I just call RemoveListener, it doesn't properly deregister. And in the end the listener fills up the channel and the node blocks and tests hang.

It seemed to deep to debug (Node.EventSwitch()), so I just put a TODO.
In any case, the fix is out of scope of this pr.

ebuchman · 2017-02-24T22:22:24Z

rpc/client/httpclient.go

+	// used to maintain counts of actively listened events
+	// so we can properly subscribe/unsubscribe
+	// FIXME: thread-safety???
+	// FIXME: reuse code from go-events???


hmmm - why are we replicating all this code?

Because it is impossible to reuse code from go-event. All private (package local). I tried... then I did it. As above, I am down to improve, but changes to go-event are out of scope. Otherwise this PR will never get finished.

Signed-off-by: Thane Thomson <connect@thanethomson.com>

ethanfrey requested review from ebuchman and melekes February 22, 2017 19:05

melekes reviewed Feb 23, 2017

View reviewed changes

ethanfrey force-pushed the feature/416-client-interface branch from 9bcf3a3 to 7fe9ff5 Compare February 23, 2017 13:17

ebuchman approved these changes Feb 23, 2017

View reviewed changes

ethanfrey added 17 commits February 24, 2017 17:05

collision merge of light-client code

d971416

Clean up tests, remove panics

2c75c9d

Add dependencies, pull out HTTPClient test code

c9d36cd

test info

5ea3f24

Full test coverage for rpc client

64feaa0

Added missing pkg/errors dependency

a0bdae4

Moved httpclient into subpackage

bf1ee89

Begin implementation of local client

7c26be3

Extracted Clients into a consistent interface, fixed type issue in ht…

ce044db

…tp.Client

Provide mock interfaces for calling abci app over tendermint rpc

df172fa

MockClient for real abci app

0905332

Add MockStatus, WaitForHeight helper

70f19e8

Clean up package names

26f4b5c

Fix race condition in websocket event listening

f7f7cf5

Reworked WaitForHeight to avoid race conditions

d92a5b1

Make all client tests safe to run in parallel

42a9b84

cleanup

cd9ee9d

ethanfrey added 4 commits February 24, 2017 17:05

Move common code to merkleeyes/testutil

9693795

Fix up checktx/delivertx in broadcastTx(A)sync

202146e

Combine local and http into client package, unify tests with table-dr…

931af6a

…iven tests

Client: DumpConsensusState, not DialSeeds. Cleanup

98450ee

ethanfrey force-pushed the feature/416-client-interface branch from ee9671b to 98450ee Compare February 24, 2017 16:10

ethanfrey added 4 commits February 24, 2017 17:51

Expose and test EventSwitch in client.Local

9be3064

Expose EventSwitch on top of websocket client

175bb32

Clean up event switch add helper function

6282fad

Client embeds EventSwitch, client.HTTP properly un/subscribes events …

4fead23

…over websocket

ebuchman approved these changes Feb 24, 2017

View reviewed changes

fix typo

d56cb2a

ebuchman merged commit d56cb2a into develop Feb 26, 2017

ebuchman mentioned this pull request Feb 26, 2017

Release v0.9.0 #401

Closed

ebuchman deleted the feature/416-client-interface branch April 19, 2017 02:35

faddat referenced this pull request in notional-labs/tendermint Apr 3, 2023

ci: Fix CodeQL YAML (#418)

331fc78

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Conversation

ethanfrey commented Feb 22, 2017

Uh oh!

ethanfrey commented Feb 22, 2017

Uh oh!

codecov-io commented Feb 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

melekes commented Feb 22, 2017

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!

ethanfrey Feb 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

melekes commented Feb 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ethanfrey commented Feb 23, 2017

Uh oh!

ethanfrey commented Feb 23, 2017

Uh oh!

melekes commented Feb 23, 2017

Uh oh!

ebuchman left a comment

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!

ebuchman Feb 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ethanfrey commented Feb 23, 2017

codecov-io commented Feb 22, 2017 •

edited

Loading

ethanfrey Feb 23, 2017 •

edited

Loading

melekes commented Feb 23, 2017 •

edited

Loading

ebuchman Feb 23, 2017 •

edited

Loading