Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Access] Add implementation BlockProvider #6636

Merged

Conversation

UlyanaAndrukhiv
Copy link
Contributor

@UlyanaAndrukhiv UlyanaAndrukhiv commented Nov 12, 2024

Closes: #6584 , #6585

Context

In this PR implemented :

  • DataProviderFactory which is responsible for creating data providers based on the requested topic.
  • DataProvider which is the interface abstracts of the actual data provider used by the WebSocketCollector.
  • BaseDataProvider and BaseDataProviderImpl - the interface and the concrete implementation which holds common objects for the provider.
  • BlocksDataProvider which is responsible for providing blocks.
  • BlockHeadersDataProvider which is responsible for providing block headers.
  • BlockDigestsDataProvider which is responsible for providing block digests.

Added unit tests to cover this functionality.

@codecov-commenter
Copy link

codecov-commenter commented Nov 12, 2024

Codecov Report

Attention: Patch coverage is 61.43791% with 118 lines in your changes missing coverage. Please review.

Project coverage is 41.05%. Comparing base (85913ad) to head (f8668ef).

Files with missing lines Patch % Lines
...ckets/data_providers/mock/data_provider_factory.go 0.00% 28 Missing ⚠️
...st/websockets/data_providers/mock/data_provider.go 0.00% 24 Missing ⚠️
engine/access/subscription/util.go 0.00% 18 Missing ⚠️
engine/access/rest/websockets/controller.go 0.00% 13 Missing ⚠️
access/handler.go 0.00% 10 Missing ⚠️
engine/access/state_stream/backend/handler.go 18.18% 9 Missing ⚠️
.../rest/websockets/data_providers/blocks_provider.go 91.78% 4 Missing and 2 partials ⚠️
engine/access/rest/http/request/get_transaction.go 0.00% 2 Missing ⚠️
...ss/rest/websockets/data_providers/base_provider.go 86.66% 2 Missing ⚠️
...e/access/rest/websockets/data_providers/factory.go 88.88% 2 Missing ⚠️
... and 4 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6636      +/-   ##
==========================================
- Coverage   41.05%   41.05%   -0.01%     
==========================================
  Files        2071     2076       +5     
  Lines      183431   183653     +222     
==========================================
+ Hits        75303    75393      +90     
- Misses     101823   101962     +139     
+ Partials     6305     6298       -7     
Flag Coverage Δ
unittests 41.05% <61.43%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@illia-malachyn
Copy link
Contributor

Please, wrap data_provider.Run() in a separate goroutine.

LGTM

Copy link
Contributor

@peterargue peterargue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mostly smaller comments. I think you should expand on the data provider tests to include some happy path testing. I also think there's currently a gap that covers more end-to-end functional testing. That can come in a separate PR, but wondering if we will need more control than is possible using integration tests alone. what do you think?

engine/access/rest/websockets/controller.go Outdated Show resolved Hide resolved
engine/access/rest/websockets/controller.go Show resolved Hide resolved
engine/access/rest/websockets/controller.go Outdated Show resolved Hide resolved
engine/access/rest/websockets/data_providers/factory.go Outdated Show resolved Hide resolved
type BlockMessageResponse struct {
// The sealed or finalized blocks according to the block status
// in the request.
Block *flow.Block `json:"block"`
Copy link
Contributor

@jribbink jribbink Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these response objects finalized?

Should this be a dedicated DTO similar to what exists when using REST (see here)? Casing and naming on REST endpoints is different than what these models will serialize to if directly marshalled into JSON.

Consistency aside, I'm also not sure that these models will serialize "properly" to JSON out-of-box. On the REST endpoints, large numbers are serialized to JSON strings instead of numbers which prevents integer overflow on JS clients. Without a custom JSON parser, JS clients have a maximum safe precision of 53 bits.

Copy link
Contributor

@Guitarheroua Guitarheroua Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Jordan! Yes, this is exactly what we missed. Thank you for pointing out this issue. As I see it, the solution could involve reusing the same converters and models as those for the REST API, or creating new converters similar to the ones we currently have for the REST API. We will discuss this and I come back to you with concrete answer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. the WS endpoint should return data in the same format as the REST endpoints.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have created a new issue to address this #6775

Copy link
Contributor

@peterargue peterargue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good. we still need to address the response format issue, but that can be done in a follow up PR

Copy link
Contributor

@peterargue peterargue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good. we still need to address the response format issue, but that can be done in a follow up PR

Co-authored-by: Peter Argue <89119817+peterargue@users.noreply.github.com>
@peterargue peterargue added this pull request to the merge queue Dec 3, 2024
Merged via the queue into onflow:master with commit 533123a Dec 3, 2024
55 checks passed
@peterargue peterargue deleted the UlyanaAndrukhiv/6585-block-data-provider branch December 3, 2024 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Access] Implement DataProvider interface and factory of data providers
7 participants