-
Notifications
You must be signed in to change notification settings - Fork 180
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] Make WebSocket responses from data providers consistent with Access REST API responses #6802
[Access] Make WebSocket responses from data providers consistent with Access REST API responses #6802
Conversation
…The-K-R-O-K/flow-go into UlyanaAndrukhiv/6775-websocket-responses
…-events-data-provider
…The-K-R-O-K/flow-go into UlyanaAndrukhiv/6775-websocket-responses
…The-K-R-O-K/flow-go into UlyanaAndrukhiv/6775-websocket-responses
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6802 +/- ##
========================================
Coverage 41.12% 41.12%
========================================
Files 2109 2116 +7
Lines 185513 185719 +206
========================================
+ Hits 76286 76377 +91
- Misses 102817 102931 +114
- Partials 6410 6411 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
…:The-K-R-O-K/flow-go into UlyanaAndrukhiv/6775-websocket-responses
subscription.HandleResponse(p.send, func(b *flow.Block) (interface{}, error) { | ||
var block commonmodels.Block | ||
|
||
executionResult, err := p.getExecutionResult(b) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you remind me why this is needed? I would expect the flow.Block
object to already contain the correct execution results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a discussion with Yurii @durkmurder about execution results in flow.Block
and the execution results from the flow.Block
Payload
cannot be used because they are not the execution result for this specific block.
Since the REST API
block response is expected the execution result specific to this block, I retrieve it from storage
by block ID
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussion result:
- Remove of
expand
as an argument for Websocketsblock
subscription. - Do not include the
execution result
into block response. - Always return the
payload
in block response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…github.com:The-K-R-O-K/flow-go into UlyanaAndrukhiv/6775-websocket-responses
…github.com:The-K-R-O-K/flow-go into UlyanaAndrukhiv/6775-websocket-responses
…xecution result from response, added payload to result
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Closes: #6775
Context
This pull request updates the
WebSocket
data providers to utilize the same models and response converters used by theAccess REST API
. By aligning the data processing logic, responses fromWebSocket
andREST API
endpoints will now have consistent structure and content. Updated and added new unit tests to validate shared model usage.