Add support for event streaming API #417
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #417 +/- ##
==========================================
+ Coverage 61.25% 62.57% +1.32%
==========================================
Files 26 26
Lines 3324 3658 +334
==========================================
+ Hits 2036 2289 +253
- Misses 1115 1180 +65
- Partials 173 189 +16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
access/client.go
Outdated
| GetExecutionDataByBlockID(ctx context.Context, blockID flow.Identifier) (*flow.ExecutionData, error) | ||
|
|
||
| // SubscribeExecutionData subscribes to execution data updates starting at the given block ID or height. | ||
| SubscribeExecutionData(ctx context.Context, startBlockID flow.Identifier, startHeight uint64) (<-chan flow.ExecutionDataStreamResponse, <-chan error, error) |
There was a problem hiding this comment.
Might be a good idea to break these into two functions: ByHeight and ById
There was a problem hiding this comment.
Currently the rpc accepts either height/blockID which is a bit of an outlier for the Access API. All of the existing methods match the rpc. @sideninja @nvdtf what's your opinion on this?
- Keep it consistent with the rpc (keep it as-is in this PR)
- Keep it consistent with the other methods (add
*ByBlockID,*ByHeightversions - Align the protobuf with 2. If we go this route, I vote we do 1, then make those changes separately.
There was a problem hiding this comment.
I think 2 is good enough. To me, accepting two fields and relying on either one is not good developer experience. Go SDK as the SDK can break this into two functions and call the RPC properly.
I like 3 as well, but don't want to push extra work for minor improvements.
| close: func() error { return conn.Close() }, | ||
| jsonOptions: []json.Option{json.WithAllowUnstructuredStaticTypes(true)}, | ||
| rpcClient: grpcClient, | ||
| executionDataClient: execDataClient, |
There was a problem hiding this comment.
the execution data API doesn't necessarily need to be run on the same interface as the access api. It's probably a good idea to allow specifying a different address/connection, maybe as a setting that overrides this original value.
|
FYI: we moved the Looks like you're already mapping the import so there's probably no changes needed to this PR besides pulling in the latest version |
peterargue
requested review from
janezpodhostnik,
sideninja and
chasefleming
as code owners
|
@sideninja, @nvdtf mind taking a look? once this is ready, I can take a stab at adding it into the flow-cli |
Closes: #378
Note that this feature is still in development in access nodes. I'll update the examples before merging when it becomes available on all nodes.