Add `HTTP` and `WebSocket` handlers for `JSON-RPC` server #11

m-Peter · 2023-11-22T17:26:31Z

Closes: #1

Description

go run ./cmd/server/main.go

The above command, when run on the project's root directory, will start the RPC server, send some HTTP requests and print out the response.

It will also start the indexer, which uses the Event Streaming API, for these 2 events:

A.7e60df042a9c0868.FlowToken.TokensWithdrawn
A.7e60df042a9c0868.FlowToken.TokensDeposited
The main reason for using these two events is just to mock out the indexer's functionality.

For production, we will use the following two events:

flow.evm.BlockExecuted
flow.evm.TransactionExecuted

To see the interaction of the the server/indexer/storage in action, we can simply submit some curl requests:

curl -XPOST 'localhost:8545/rpc' --header 'Content-Type: application/json' --data-raw '{"jsonrpc":"2.0","id":1,"method":"eth_blockNumber","params": []}'
{"jsonrpc":"2.0","id":1,"result":"0x8156973"}

curl -XPOST 'localhost:8545/rpc' --header 'Content-Type: application/json' --data-raw '{"jsonrpc":"2.0","id":1,"method":"eth_blockNumber","params": []}'
{"jsonrpc":"2.0","id":1,"result":"0x8156986"}

We can see that the result for eth_blockNumber is changing, based on the block height from the events.

For contributor use:

Targeted PR against master branch
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the standards mentioned here.
Updated relevant documentation
Re-reviewed Files changed in the Github PR explorer
Added appropriate labels

…serve on

storage/store.go

peterargue · 2023-12-18T21:32:25Z

indexer/client.go

@@ -0,0 +1,188 @@
+package indexer


looks like this is copy/pasted from https://github.com/peterargue/execdata-client/blob/main/client/client.go

while I'm not personally offended by it, this is generally something to avoid with open source projects since it can invite licensing challenges.

FWIW, a more robust version of this logic will be included in the flow-go-sdk soon which is probably a better option for this project:
onflow/flow-go-sdk#417

looks like this is copy/pasted from https://github.com/peterargue/execdata-client/blob/main/client/client.go

while I'm not personally offended by it, this is generally something to avoid with open source projects since it can invite licensing challenges.

My sincere apologies 🙏 I had no intention to shadow your work or even present it as mine. I have mentioned the source of this code in the flow-evm discord channel and on the regular syncs we have with the team working on Flow EVM. That's why I asked Jerome to add you as a reviewer 😇

My first approach was to add your repository as a module, but it simply wouldn't work, as this PR needs specific versions for go-ethereum and flow-go, e.g.:

github.com/ethereum/go-ethereum v1.12.0 github.com/onflow/flow-go v0.32.4-0.20231122162232-0ba81ff8d241

My main purpose was to use the code only for creating a PoC with the Event Streaming API, until proper support was added in flow-go-sdk. As soon as onflow/flow-go-sdk#417 is included in a release, I will pull it in.

All good, and I'm not worried about it from a credit perspective. I wrote that library within my capacity at flow, so it's no problem.

I'm just calling it out because we need to be careful that code included in the onflow org is following proper licensing practices, otherwise we could get into some legal issues. In this case, one option would be to fork the repo and make whatever changes were needed, or submitting a PR back to the original repo.

That's a fair point 👍
I started out that way (peterargue/execdata-client#3), I just got carried out after hours of troubleshooting 😅

@peterargue I have opened a PR on the upstream repository: peterargue/execdata-client#4. When it gets merged, I can remove all the relevant code from this PR.

I merged that flow-go-sdk PR into master, so you can import it now

Perfect 💯
I have completely replaced the indexer code with the functionality offered from flow-go-sdk in 8545330.

peterargue · 2023-12-18T21:35:16Z

cmd/server/main.go

+		log.Fatalf("could not subscribe to execution data: %v", err)
+	}
+
+	for {


you should check in this loop that responses were received for all block heights, otherwise it's possible that a response was missed.

How would that check look like? 🤔

To share some more context, we want to keep track of the 2 following service events (a.k.a core events):

sub, err = execClient.SubscribeEvents( ctx, flow.ZeroID, height, indexer.EventFilter{ EventTypes: []string{ "flow.evm.BlockExecuted", "flow.evm.TransactionExecuted", }, }, )

and build some sort of state around the payloads of these 2 events.

These 2 events will start being emitted from a certain block height and onwards. That block height would most likely be the height in which the EVM contract (and associated code) is deployed/introduced on testnet & mainnet. For this case we can have a utility script for back-filling.

My assumption here is that SubscribeEvents will publish a response for every block that matches these EventFilter criteria. Of course, not every block will contain such events. In the context of this Flow EVM Gateway, the latest block height is the block height which contained a flow.evm.BlockExecuted, we are not interested in regular Flow blocks.

Could you elaborate on which case it is possible to miss a response which matched the EventFilter criteria?

The SubscribeEvents endpoint has a heartbeatInterval request parameter:
https://github.com/onflow/flow/blob/b7ed2c3e452b5e44561399afc6b5e868e8957117/protobuf/flow/executiondata/executiondata.proto#L140-L148

This is used to set how often the backend will return a response message. If you set it to 1, it will return a response for every block. If the block has no events, the list is empty. If you check the block height from each message increases sequentially, you can verify that you received all messages, and thus all blocks were searched.

The API is currently missing an important sequence number field, which would let you get the same guarantee with larger heartbeatIntervals without receiving a response for every block.

Given missing any events may corrupt your index, I'd recommend using the 1 block heartbeat for now.

Thanks for the detailed explanation, I was not aware of the heartbeatInterval request parameter.
I have added this parameter and the relevant check in 2b36693.

cmd/server/main.go

sideninja · 2023-12-19T16:07:41Z

api/api.go

@@ -201,13 +278,30 @@ func (s *BlockChainAPI) GetTransactionReceipt(
 	ctx context.Context,
 	hash common.Hash,
 ) (map[string]interface{}, error) {
-	return map[string]interface{}{}, nil
+	receipt := map[string]interface{}{}


The general point on this mocking of data. I would suggest you define an interface for the indexer (as discussed on our call), which you then implement one version as mock indexer that has these values. You should then use the indexer interface in the API so you don't get carried away with API containing any additional logic.

Do you intend to address this comment in this PR or in the future PR? If not in this PR add a TODO.

The PR has already grown quite large, so I will tackle this in a future PR. Added a TODO: 6173b96

sideninja · 2023-12-19T16:09:32Z

api/api_test.go

+		)
+	})
+
+	t.Run("CreateAccessList", func(t *testing.T) {


What I mentioned above about having a indexer (or multiple modules) defined as interface and then used in the API, would also allow for better testing, as you would use a test mock (maybe using tools like mockery) with which you can then specify the values and assert the returned values. This way, like you made it now, it will soon break all the tests as you heavily rely on the mocked values in the API itself, which you will soon start replacing with real values.

Agreed, this was mainly for my convenience, to see what kind of return values these methods require, and to possibly use these later on as fixtures.

indexer/utils.go

sideninja

A suggestion for future PRs is to try to keep them more specific, I would break this one into 3, one to add mock data to the APi, the other to add indexer, and the third to add storage. This way it will be easier to review.

m-Peter · 2023-12-19T16:43:28Z

A suggestion for future PRs is to try to keep them more specific, I would break this one into 3, one to add mock data to the APi, the other to add indexer, and the third to add storage. This way it will be easier to review.

Makes sense, thanks for the suggestion 🙏

… response is received for each block on the indexer

api/api.go

cmd/server/main.go

storage/store.go

peterargue · 2023-12-20T23:19:06Z

indexer/client.go

@@ -0,0 +1,188 @@
+package indexer


I merged that flow-go-sdk PR into master, so you can import it now

cmd/server/main.go

peterargue · 2023-12-22T14:00:17Z

storage/store.go

+	"sync"
+)
+
+type Store struct {


should this also store the data from events?

Yes, exactly, that's the idea behind it. However, I have to try this out with the Emulator, because the following events:

flow.evm.BlockExecuted,

flow.evm.TransactionExecuted

are not currently available in neither testnet nor mainnet.

ramtinms

Looks good to me.

sideninja

I left some comments, and I followed up on some from last time that weren't addressed, more specifically the interface suggestion.
Also, I wonder if we got to a conclusion on what to do about attribution/licensing of go-ethereum code? My advice is to include attributions as per their license, maybe in a comment on the top of the file, or just generally in the license/readme file.

sideninja · 2024-01-05T12:06:32Z

.github/workflows/ci.yml

@@ -0,0 +1,27 @@
+name: CI


Ideally, in the future try to keep these kinds of changes in separate PRs so they are more focused.

Makes sense 👍 Will do.

sideninja · 2024-01-05T12:08:16Z

api/api.go

+
+// this is added to resolve the issue with chainhash ambiguous import,
+// the code is not used, but it's needed to force go.mod specify and retain chainhash version
+// workaround for issue: https://github.com/golang/go/issues/27899


add a TODO that once go-ethereum is updated this can go away.

Good catch 👍
I removed it altogether in eb14c1e, since I am using the latest go-ethereum version, and that's going to be used in flow-go as well.

sideninja · 2024-01-05T12:14:57Z

api/api.go

-	return hexutil.Uint64(65848272)
+	latestBlockHeight, err := api.Store.LatestBlockHeight(context.Background())
+	if err != nil {
+		return hexutil.Uint64(0)


I wonder if it's worth panicking at this point or if nothing else logging an error, the way it's written now it might cause issues that will go unnoticed. If logging is not yet part of this PR maybe add TODO but also consider panic.

Good idea 👍
I should definitely add a logger instance to BlockChainAPI.
For now, I have added a TODO and a panic in df9553c

sideninja · 2024-01-05T12:16:25Z

api/api.go

@@ -201,13 +278,30 @@ func (s *BlockChainAPI) GetTransactionReceipt(
 	ctx context.Context,
 	hash common.Hash,
 ) (map[string]interface{}, error) {
-	return map[string]interface{}{}, nil
+	receipt := map[string]interface{}{}


Do you intend to address this comment in this PR or in the future PR? If not in this PR add a TODO.

sideninja · 2024-01-05T12:19:30Z

api/server_test.go

+	"github.com/stretchr/testify/assert"
+)
+
+var requests = []string{


it would be better to extract this to fixture files, you can create request.json and then use embed to embed it.

Nice idea 👍
Fixed in e893a6f

sideninja · 2024-01-05T12:46:23Z

cmd/server/main.go

+	fmt.Printf("Latest Block Height: %d\n", latestBlockHeader.Height)
+	fmt.Printf("Latest Block ID: %s\n", latestBlockHeader.ID)
+
+	data, errChan, initErr := flowClient.SubscribeEventsByBlockHeight(


you could just use the reconnect function so you don't duplicate the code here

Good idea 👍
Updated in 328de2f

sideninja · 2024-01-05T12:46:36Z

cmd/server/main.go

+	if err != nil {
+		panic(err)
+	}
+	fmt.Printf("Latest Block Height: %d\n", latestBlockHeader.Height)


I suggest you add a logging library and replace fmt outputs.

Updated in f55dc7b

sideninja · 2024-01-05T12:50:32Z

cmd/server/main.go

+				if ctx.Err() != nil {
+					return // graceful shutdown
+				}
+				fmt.Println("subscription closed - reconnecting")


I think the reconnect logic should have some timeout before reconnecting, otherwise a very vicious cycle can happen due to some issues in establishing connection. Also I would error log the connection failure so alarms can be set up in the future.

That's a good idea, I have added a simple time.Sleep for now, and a TODO to add a proper library such as: https://github.com/sethvargo/go-retry .
Updated in 24cab8e

sideninja · 2024-01-05T12:52:46Z

cmd/server/main.go

-		`{"jsonrpc":"2.0","id":4,"method":"eth_getBalance","params": ["0x407d73d8a49eeb85d32cf465507dd71d507100c1"]}`,
-		`{"jsonrpc":"2.0","id":5,"method":"eth_getBlockTransactionCountByNumber","params": ["0x4E4ee"]}`,
+
+	latestBlockHeader, err := flowClient.GetLatestBlockHeader(ctx, true)


add a todo that the height from which the indexer starts should be retrieved from storage, not be equal to latest height

Good point 👍
Added in f55dc7b
We should probably have a command-line flag for this as well.

sideninja · 2024-01-05T12:53:53Z

storage/store.go

+type Store struct {
+	mu sync.RWMutex
+	// highest block height
+	blockHeight uint64


a comment suggests a better naming: latestHeight

Good point 👍
Updated in ed60231

… NewStore constructor

Also add a comment on the starting height to be used for the indexer

…nterfaces

m-Peter · 2024-01-06T18:43:39Z

Added disclaimer regarding httpServer in fc18d38

Add HTTP and WebSocket handlers for JSON-RPC server

a32d801

m-Peter added the Feature label Nov 22, 2023

m-Peter self-assigned this Nov 22, 2023

m-Peter added 4 commits November 22, 2023 20:30

Fetch chainID from flow-go repository

57f4553

Add a basic indexing functionality

ab6e342

Add description for commented-out code

03c6a9a

Use hexutil.Bytes as type for input on SendRawTransaction rpc method

f291025

m-Peter marked this pull request as ready for review December 18, 2023 11:33

m-Peter added the Improvement label Dec 18, 2023

m-Peter force-pushed the add-http-and-web-socket-handlers branch 2 times, most recently from 914d550 to 7aa8509 Compare December 18, 2023 11:45

m-Peter added 2 commits December 18, 2023 14:34

Add tests for JSON-RPC HTTP handler & BlockChainAPI service

0ddb302

Add a basic Makefile and CI workflow for GitHub

4251feb

m-Peter force-pushed the add-http-and-web-socket-handlers branch from 7aa8509 to 4251feb Compare December 18, 2023 12:34

Run JSON-RPC HTTP handler on port 8545 which most clients default to …

9f9375f

…serve on

m-Peter requested review from ramtinms and sideninja December 18, 2023 18:43

Test more JSON-RPC methods and some mock data

00f6a43

franklywatson requested a review from peterargue December 18, 2023 19:14

peterargue reviewed Dec 18, 2023

View reviewed changes

m-Peter added 3 commits December 19, 2023 10:23

Use empty value for sync.RWMutex when initializing the Store

78b1e39

Acquire read lock in LatestBlockHeight

de90c62

Add reconnect functionality in indexer

4d47834

m-Peter requested a review from peterargue December 19, 2023 10:35

sideninja reviewed Dec 19, 2023

View reviewed changes

indexer/utils.go Outdated Show resolved Hide resolved

sideninja reviewed Dec 19, 2023

View reviewed changes

Add a heartbeatInterval parameter to SubscribeEvents and check that a…

2b36693

… response is received for each block on the indexer

ramtinms reviewed Dec 21, 2023

View reviewed changes

api/api.go Outdated Show resolved Hide resolved

api/api.go Show resolved Hide resolved

api/api.go Outdated Show resolved Hide resolved

cmd/server/main.go Outdated Show resolved Hide resolved

Introduce a basic Config type for the BlockChainAPI

42be5cd

m-Peter requested a review from ramtinms December 22, 2023 10:38

Add eth RPC methods for getting uncles

130b167

peterargue reviewed Dec 22, 2023

View reviewed changes

Base indexer functionality on flow-go-sdk's BaseClient

8545330

m-Peter requested a review from peterargue December 22, 2023 16:07

Add check-tidy and ci recipes in Makefile and use in CI workflow

974e834

m-Peter requested a review from sideninja January 4, 2024 18:21

ramtinms approved these changes Jan 4, 2024

View reviewed changes

m-Peter mentioned this pull request Jan 4, 2024

Enable state streaming API over gRPC onflow/flow-emulator#547

Merged

6 tasks

sideninja reviewed Jan 5, 2024

View reviewed changes

m-Peter added 4 commits January 6, 2024 17:57

Add TODO for including more options in Config

b8b8ac9

Rename blockHeight to latestHeight and add a TODO for requiring it in…

ed60231

… NewStore constructor

Replace fmt.Println with a proper logger

f55dc7b

Also add a comment on the starting height to be used for the indexer

Add a time.Sleep before reconnecting the indexer

24cab8e

m-Peter force-pushed the add-http-and-web-socket-handlers branch from b7fa216 to 24cab8e Compare January 6, 2024 17:03

m-Peter added 6 commits January 6, 2024 19:20

Create dedicated fixtures for eth JSON-RPC requests & responses

e893a6f

Remove workaround for chainhash ambiguous import

eb14c1e

Add a panic in BlockNumber() when retrieval from storage fails

df9553c

Refactor the reconnect method to be able to re-use it

328de2f

Add disclaimer about the source of inspiration for the httpServer

fc18d38

Add TODO for getting rid of mock data in favor of storage & indexer i…

6173b96

…nterfaces

m-Peter requested a review from sideninja January 6, 2024 18:43

Run make test recipe with -cover flag

aaf3ba3

sideninja approved these changes Jan 8, 2024

View reviewed changes

m-Peter mentioned this pull request Jan 10, 2024

FLIP 243: Flow EVM Gateway onflow/flips#235

Open

m-Peter merged commit 8ffa586 into onflow:main Jan 10, 2024
1 check passed

m-Peter deleted the add-http-and-web-socket-handlers branch January 10, 2024 10:36

Add HTTP and WebSocket handlers for JSON-RPC server #11

Add HTTP and WebSocket handlers for JSON-RPC server #11

Conversation

m-Peter commented Nov 22, 2023 • edited Loading

Description

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

peterargue Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

m-Peter Dec 19, 2023 • edited Loading

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

sideninja left a comment

Choose a reason for hiding this comment

m-Peter commented Dec 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-Peter Dec 22, 2023 • edited Loading

Choose a reason for hiding this comment

ramtinms left a comment

Choose a reason for hiding this comment

sideninja 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

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

m-Peter commented Jan 6, 2024

Add `HTTP` and `WebSocket` handlers for `JSON-RPC` server #11

Add `HTTP` and `WebSocket` handlers for `JSON-RPC` server #11

m-Peter commented Nov 22, 2023 •

edited

Loading

peterargue Dec 19, 2023 •

edited

Loading

m-Peter Dec 19, 2023 •

edited

Loading

m-Peter Dec 22, 2023 •

edited

Loading