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

rpc/client/http: drop endpoint arg from New and add WSOptions #6176

Merged
merged 11 commits into from
Feb 25, 2021

Conversation

melekes
Copy link
Contributor

@melekes melekes commented Feb 24, 2021

also

  • replace MaxReconnectAttempts, ReadWait, WriteWait and PingPeriod options with WSOptions in WSClient (rpc/jsonrpc/client/ws_client.go).
  • set default write wait to 10s for WSClient(rpc/jsonrpc/client/ws_client.go)
  • unexpose WSEvents(rpc/client/http.go)

Closes #6162

@codecov
Copy link

codecov bot commented Feb 24, 2021

Codecov Report

Merging #6176 (6b9b170) into master (4557211) will increase coverage by 0.19%.
The diff coverage is 68.42%.

@@            Coverage Diff             @@
##           master    #6176      +/-   ##
==========================================
+ Coverage   60.67%   60.86%   +0.19%     
==========================================
  Files         276      276              
  Lines       25716    25712       -4     
==========================================
+ Hits        15602    15650      +48     
+ Misses       8486     8447      -39     
+ Partials     1628     1615      -13     
Impacted Files Coverage Δ
cmd/tendermint/commands/light.go 16.55% <0.00%> (ø)
rpc/jsonrpc/client/http_json_client.go 10.85% <0.00%> (ø)
statesync/stateprovider.go 0.00% <0.00%> (ø)
rpc/jsonrpc/client/ws_client.go 63.08% <92.30%> (+4.82%) ⬆️
light/provider/http/http.go 52.12% <100.00%> (ø)
privval/signer_endpoint.go 72.72% <0.00%> (-3.04%) ⬇️
p2p/pex/pex_reactor.go 76.48% <0.00%> (-0.60%) ⬇️
proxy/multi_app_conn.go 48.05% <0.00%> (ø)
consensus/state.go 66.60% <0.00%> (+0.09%) ⬆️
p2p/conn/connection.go 79.06% <0.00%> (+0.55%) ⬆️
... and 11 more

@melekes
Copy link
Contributor Author

melekes commented Feb 24, 2021

@hydrogen18 how does this new API look to you? (re #6162)

@melekes melekes marked this pull request as ready for review February 24, 2021 13:22
@melekes melekes self-assigned this Feb 24, 2021
@hydrogen18
Copy link

@melekes Ok wow this is larger than I anticipated but it gives an easy way to pass in the options for the web socket connection so this is great! Thank you

Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

LGTM 👍 - also nice that you cleaned up a few things here and there. I just have a few comments that you might want to address but other than that if everyone else is happy with the solution then we should be good to merge

rpc/jsonrpc/client/ws_client.go Outdated Show resolved Hide resolved
rpc/jsonrpc/client/ws_client.go Outdated Show resolved Hide resolved
Copy link
Contributor

@alexanderbez alexanderbez 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. Are we sure we updated all RPC/websocket documentation with the new API? Also, do we not need to update the changelog with the new breaking API changes?

@melekes melekes added the S:automerge Automatically merge PR when requirements pass label Feb 25, 2021
@mergify mergify bot merged commit e9e5026 into master Feb 25, 2021
@mergify mergify bot deleted the anton/6162-ws-client branch February 25, 2021 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S:automerge Automatically merge PR when requirements pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The websocket RPC client doesn't have a way to configure the ping period
4 participants