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

IPC #572

Merged
merged 21 commits into from
Mar 29, 2018
Merged

IPC #572

merged 21 commits into from
Mar 29, 2018

Conversation

faboweb
Copy link
Collaborator

@faboweb faboweb commented Mar 23, 2018

Thank you a lot for contributing to Cosmos Voyager!

Issue

closes: #375

Missing:

  • rewrite tests

❤️ Thank you!

@faboweb faboweb requested review from NodeGuy and nylira as code owners March 23, 2018 12:37
@faboweb faboweb mentioned this pull request Mar 23, 2018
2 tasks
@NodeGuy
Copy link
Contributor

NodeGuy commented Mar 23, 2018

Again, I don't feel yet like I have anything meaningful to add.

@codecov
Copy link

codecov bot commented Mar 28, 2018

Codecov Report

Merging #572 into develop will increase coverage by 0.92%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop     #572      +/-   ##
===========================================
+ Coverage    83.69%   84.62%   +0.92%     
===========================================
  Files           87       87              
  Lines         1460     1450      -10     
  Branches        67       67              
===========================================
+ Hits          1222     1227       +5     
+ Misses         226      211      -15     
  Partials        12       12
Impacted Files Coverage Δ
app/src/renderer/connectors/rpcWrapperMock.js 100% <ø> (ø) ⬆️
app/src/renderer/vuex/modules/node.js 87.8% <100%> (ø) ⬆️
app/src/renderer/main.js 89.28% <100%> (ø) ⬆️
app/src/renderer/connectors/rpcWrapper.js 100% <100%> (ø) ⬆️
app/src/main/index.js 93.9% <100%> (+17.99%) ⬆️
app/src/renderer/connectors/node.js 100% <100%> (ø) ⬆️

@faboweb faboweb changed the title WIP: IPC IPC Mar 28, 2018
This was referenced Mar 29, 2018
@@ -2,17 +2,17 @@
const RestClient = require('./lcdClient.js')
const mockedRestClient = require('./lcdClientMock.js')
const RpcWrapper = require('./rpcWrapper.js')
const mockedRpcWrapper = require('./rpcWrapperMock.js')
const MockedRpcWrapper = require('./rpcWrapperMock.js')
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is our standard for capital vs camel casing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I used capital here, as it is a constructor-like function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In general only classes and constants should have capital letters

module.exports = function (nodeIP, relayPort, mocked = false) {
const RELAY_SERVER = 'http://localhost:' + relayPort
module.exports = function (nodeIP, lcdPort, mocked = false) {
const LCD_SERVER = 'http://localhost:' + lcdPort
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@@ -24,7 +24,6 @@ module.exports = function (nodeIP, relayPort, mocked = false) {
}
// TODO: eventually, get all data from light-client connection instead of RPC

connector.setMocked(mocked)
connector.initRPC(nodeIP)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why don't we need to call initRPC anymore?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

init is called only after the main process signals, that it has connected to a node

})
let lcdPort = getQueryParameter('lcd_port')
console.log('Expecting lcd-server on port:', lcdPort)
const node = Node('localhost', lcdPort)
Copy link
Collaborator

Choose a reason for hiding this comment

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

isn't this defaulting to a localhost now instead of the actual node we want to connect to?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is confusing I will change this.

const RestClient = require('./lcdClient.js')

module.exports = function (nodeIP, relayPort, lcdPort) {
const RELAY_SERVER = 'http://localhost:' + relayPort
Copy link
Collaborator

Choose a reason for hiding this comment

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

is RELAY_SERVER what we call the mock server?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was dead code. I deleted the file.

let newRpc = new RpcClient(`ws://${nodeIP}`)
node.rpcOpen = true
node.rpcConnecting = false
// we need to check immediately if he connection fails. later we will not be able to check this error
Copy link
Collaborator

Choose a reason for hiding this comment

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

"if the connection"

})
// TODO: eventually, get all data from light-client connection instead of RPC

// node.rpcConnect(nodeIP)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this commented out?

jbibla
jbibla previously approved these changes Mar 29, 2018
@faboweb faboweb merged commit a1e6596 into develop Mar 29, 2018
@faboweb faboweb deleted the fabo/375-ipc branch May 29, 2018 13:54
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.

Secure web-view node-process communication
3 participants