-
Notifications
You must be signed in to change notification settings - Fork 286
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
fix(quorum/api): use Web3 HTTP Provider by default #66
fix(quorum/api): use Web3 HTTP Provider by default #66
Conversation
It is deprecated because it does not support subscriptions, but we do not use that feature anyway and just need the examples to work so it seems like an okay trade-off to use HTTP instead of WS. Signed-off-by: Peter Somogyvari <[email protected]>
@denis-yu-glotov FYI: CI won't pass on this until we merge the other fixes in. |
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.
Hi! We also need to change Quorum docker-compose file here or how did you think to manage it?
const option = `ws://${config.web3.host[`${process.platform}`]}:${config.web3.port}`; | ||
const web3 = new Web3(option); | ||
|
||
const rpcApiHost = `http://${config.web3.host[`${process.platform}`]}:${config.web3.rpcPort}`; |
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.
need to add this rpcPort
to config. Or why not just leave it as port
?
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.
need to add this
rpcPort
to config. Or why not just leave it asport
?
I wanted to keep it separate from the WS API / port. Naming is a little confusing because in the end both HTTP and the WS are using the same JSON RPC API under the hood just with different transport protocols (HTTP, WS).
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.
ok, then you need to add this rpcPort
to config/config
, no?
Signed-off-by: Peter Somogyvari <[email protected]>
Signed-off-by: Peter Somogyvari <[email protected]>
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, just please note the remaining comments
@@ -54,6 +54,12 @@ x-quorum-def: | |||
--nodiscover \ | |||
--verbosity 5 \ | |||
--networkid $${NETWORK_ID} \ | |||
--rpc \ | |||
--rpcaddr=0.0.0.0 \ | |||
--rpcvhosts=node1 \ |
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 use "*" here, same as in cors. Will it work with node1
value?
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.
Thank you!
node1
: docker-compose guarantees that the host will resolve to the container's IP based on what you name the container in the compose .yml file (also tested it so I know for a fact that it works).
Could've put *
but I figured if I can make it work with tighter controls then might as well use the least privilege best practice.
const option = `ws://${config.web3.host[`${process.platform}`]}:${config.web3.port}`; | ||
const web3 = new Web3(option); | ||
|
||
const rpcApiHost = `http://${config.web3.host[`${process.platform}`]}:${config.web3.rpcPort}`; |
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.
ok, then you need to add this rpcPort
to config/config
, no?
Signed-off-by: Peter Somogyvari <[email protected]>
Ooops, yes. :-) |
) * fix(quorum/api): use Web3 HTTP Provider by default It is deprecated because it does not support subscriptions, but we do not use that feature anyway and just need the examples to work so it seems like an okay trade-off to use HTTP instead of WS. Signed-off-by: Peter Somogyvari <[email protected]> * fix(quorum/platform): RPC API in docker-compose Signed-off-by: Peter Somogyvari <[email protected]> * fix(quorum/platform): opens port 8545 on node1 Signed-off-by: Peter Somogyvari <[email protected]> * fix(quorum/platform): ports in config.js, .env Signed-off-by: Peter Somogyvari <[email protected]>
Code Fixes, and new command added to create all credentials for data transfer
It is deprecated because it does not support
subscriptions, but we do not use that feature anyway
and just need the examples to work so it seems like
an okay trade-off to use HTTP instead of WS.
Signed-off-by: Peter Somogyvari [email protected]