-
Notifications
You must be signed in to change notification settings - Fork 93
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
Enabling connections with websocket client #222
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #222 +/- ##
==========================================
- Coverage 99.70% 98.86% -0.85%
==========================================
Files 77 78 +1
Lines 4477 4562 +85
==========================================
+ Hits 4464 4510 +46
- Misses 13 52 +39 ☔ View full report in Codecov by Sentry. |
spec/eth/client_spec.rb
Outdated
@@ -425,4 +437,8 @@ | |||
expect(geth_ipc.is_valid_signature(contract, hashed, signature)).to be true | |||
end | |||
end | |||
|
|||
describe ".send" do | |||
pending("using the mock websocket server") |
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.
looking good so far. let me know when you are ready so I can review it.
📝 This test is failing on Ubuntu only. under investigation.
|
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.
Great, left a few comments!
lib/eth/client/ws.rb
Outdated
# | ||
# @param payload [Hash] the RPC request parameters. | ||
# @return [Integer] Number of bytes sent by this method. | ||
def send(payload) |
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.
We recently renamed send
to send_request
#201
def send(payload) | |
def send_request(payload) |
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.
Thanks for the info, helpful. I fixed this here: fcaf5db
lib/eth/client/ws.rb
Outdated
|
||
@ws.on :close do |e| | ||
puts "-- websocket close (#{e.inspect})" | ||
exit 1 |
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.
we should not exit here as we are only providing a library and not an application
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.
exit 1
has been deleted.
lib/eth/client/ws.rb
Outdated
@ws = WebSocket::Client::Simple.connect @host | ||
|
||
@ws.on :message do |msg| | ||
puts ">> #{msg.data}" |
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.
let's not have stray puts
in the code
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.
puts
method has been removed.
spec/eth/client_spec.rb
Outdated
|
||
sleep 0.001 | ||
geth_dev_ws.send(payload) | ||
sleep 0.001 |
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.
is there a way to avoid sleeping here?
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'll look into ways to avoid using sleeping.
Co-authored-by: Afri <[email protected]>
Co-authored-by: Afri <[email protected]>
@q9f Thanks for the review. I've just taken in the fixes, please look at it again! |
tests fail due to shanghai (or lack thereof) |
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.
Is there a way we can abstract the way websockets work away from the client?
I.e., I would like to be able just to call chain_id
and get a result.
lg = Logger.new(STDOUT, level: Logger::FATAL)
ws = Client.create("ws://127.0.0.1:8546", logger: lg)
ws.chain_id
# TypeError: no implicit conversion of Integer into String
# from /usr/lib/ruby/3.0.0/json/common.rb:216:in `initialize'
ws.default_account
# TypeError: no implicit conversion of Integer into String
# from /usr/lib/ruby/3.0.0/json/common.rb:216:in `initialize'
Do you think this is possible?
subject(:geth_ipc) { Client.create geth_ipc_path } | ||
subject(:geth_http) { Client.create geth_http_path } | ||
subject(:geth_http_authed) { Client.create geth_http_authed_path } | ||
|
||
# it expects an $INFURA_TOKEN in environment | ||
let(:infura_api) { "https://mainnet.infura.io/v3/#{ENV["INFURA_TOKEN"]}" } | ||
subject(:infura_mainnet) { Client.create infura_api } | ||
let(:logger) { Logger.new(STDOUT, level: Logger::FATAL) } | ||
subject(:geth_dev_ipc) { Client.create geth_dev_ipc_path } | ||
subject(:geth_dev_http) { Client.create geth_dev_http_path } |
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.
where do these two lines come from?
|
||
describe ".send" do | ||
it "should set up the WebSocket connection" do | ||
expect(geth_dev_ws.instance_variable_get("@ws")).to be_instance_of(WebSocket::Client::Simple::Client) |
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.
shall we expose @ws
with a getter?
break if geth_dev_ws.instance_variable_get("@ws").open? || (Time.now - start_time > 3) | ||
end | ||
|
||
geth_dev_ws.send_request(payload) |
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.
while this is fine for a test, it would be good if it supports all APIs out of the box (instead of using send_request
)
# Wait for the response to be received | ||
start_time = Time.now | ||
loop do | ||
break if received_data || (Time.now - start_time > 3) |
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.
It might be good to add a TIMEOUT to the client, so we don't have to deal with this externally
expect(received_data["result"]).to start_with("0x") | ||
|
||
contract = Eth::Contract.from_file(file: "spec/fixtures/contracts/dummy.sol") | ||
geth_http.deploy_and_wait(contract) |
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.
geth_http.deploy_and_wait(contract) | |
geth_dev_ws.deploy_and_wait(contract) |
Okay, as a result, it appears that the number of bytes is returned.This is because the send method returns the number of bytes sent.Unlike HTTP, WebSocket is stateful in nature, so I may need to wait for the response before parsing it. I will think about it and try to implement it. 😄 From: /Users/kurotaki/ghq/github.com/q9f/eth.rb/lib/eth/client.rb:487 Eth::Client#send_command:
479: def send_command(command, args)
480: args << "latest" if ["eth_getBalance", "eth_call"].include? command
481: payload = {
482: jsonrpc: "2.0",
483: method: command,
484: params: marshal(args),
485: id: next_id,
486: }
=> 487: binding.pry
488: output = JSON.parse(send_request(payload.to_json))
489: raise IOError, output["error"]["message"] unless output["error"].nil?
490: output
491: end
[1] pry(#<Eth::Client::Ws>)> payload
=> {:jsonrpc=>"2.0", :method=>"eth_chainId", :params=>[], :id=>1}
[2] pry(#<Eth::Client::Ws>)> output = JSON.parse(send_request(payload.to_json))
TypeError: no implicit conversion of Integer into String
from /Users/kurotaki/.rbenv/versions/3.2.0/lib/ruby/3.2.0/json/common.rb:216:in `initialize'
[3] pry(#<Eth::Client::Ws>)> send_request(payload.to_json)
=> 79 |
I'm back! 😄 |
Enabling connections via Websocket. Connect using only Ruby libraries.
fix: #86
Sample procedure
Eth::Client::Ws.new
Output
cli.send
Output