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] Update websockets event streaming to return JSON-CDC encoded events #5048

Merged
merged 3 commits into from
Nov 23, 2023

Conversation

peterargue
Copy link
Contributor

Closes: #5047

A regression was introduced that cause the events returned from the websockets event streaming endpoint to be CCF encoded. This PR updates the handler to encode to JSON-CDC.

@peterargue peterargue self-assigned this Nov 22, 2023
@codecov-commenter
Copy link

codecov-commenter commented Nov 22, 2023

Codecov Report

Attention: 569 lines in your changes are missing coverage. Please review.

Comparison is base (cafb1a6) 56.02% compared to head (a368dde) 56.29%.
Report is 469 commits behind head on master.

Files Patch % Lines
module/metrics/execution.go 0.00% 71 Missing ⚠️
fvm/evm/stdlib/contract.go 82.70% 33 Missing and 27 partials ⚠️
network/p2p/test/fixtures.go 27.11% 43 Missing ⚠️
engine/execution/storehouse/register_store.go 70.42% 32 Missing and 10 partials ⚠️
engine/execution/rpc/engine.go 66.66% 33 Missing and 4 partials ⚠️
network/netconf/flags.go 0.00% 34 Missing ⚠️
engine/execution/storehouse/register_engine.go 0.00% 26 Missing ⚠️
network/p2p/p2pnode/gossipSubAdapter.go 41.86% 24 Missing and 1 partial ⚠️
fvm/bootstrap.go 17.39% 18 Missing and 1 partial ⚠️
...e/execution/storehouse/in_memory_register_store.go 91.90% 13 Missing and 4 partials ⚠️
... and 32 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5048      +/-   ##
==========================================
+ Coverage   56.02%   56.29%   +0.26%     
==========================================
  Files         965      975      +10     
  Lines       89671    90848    +1177     
==========================================
+ Hits        50241    51145     +904     
- Misses      35678    35894     +216     
- Partials     3752     3809      +57     
Flag Coverage Δ
unittests 56.29% <68.40%> (+0.26%) ⬆️

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.

Copy link
Contributor

@Guitarheroua Guitarheroua 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 for me.

wsController.wsErrorHandler(err)
return
}
events[i].Payload = payload
Copy link
Member

Choose a reason for hiding this comment

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

This is a event that is only updated with the Payload field, and the rest fields are all empty?

Copy link
Contributor Author

@peterargue peterargue Nov 22, 2023

Choose a reason for hiding this comment

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

good catch. I'm not actually sure why tests were passing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

they passed because the handler was overwriting the expected results data. fixed in 5eb72b7

@peterargue peterargue enabled auto-merge November 22, 2023 23:46
@peterargue peterargue disabled auto-merge November 22, 2023 23:46
@peterargue peterargue enabled auto-merge November 23, 2023 00:05
@peterargue peterargue added this pull request to the merge queue Nov 23, 2023
Merged via the queue into master with commit 642635d Nov 23, 2023
54 checks passed
@peterargue peterargue deleted the petera/5047-fix-wsrest-events-encoding branch November 23, 2023 00:38
peterargue added a commit that referenced this pull request Nov 23, 2023
…coding

[Access] Update websockets event streaming to return JSON-CDC encoded events
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] WebSockets event streaming returns CCF encoded events
4 participants