Initial implementation of unix socket IPC support [WIP]

[axic]

Fixes #180.

[axic]

Very rudimentary at the moment, but works.

[MicahZoltu]

Really the only thing blocking a merge (given my terribly limited understanding of this project or IPCs in general) is the missing newline on the failure response, and possibly inclusion of the parsing error in the response.

[MicahZoltu]

Comment should probably say via IPC rather than via http. Copy paste issue I suspect?

[MicahZoltu]

Down on line 181 there is a comment suggesting that some clients expect a trailing new line. Should that be applied here as well?

[MicahZoltu]

Consider including the error in the response so the user can have an easier time understanding what their problem was:

socket.write('{"error":"Bad request","message":"' + e + '"}\n')

[MicahZoltu]

Recommend making this return the underlying server. This is how the HTTP TestRPC server is implemented and I believe consistency is important.

[MicahZoltu]

In particular, the way you have done it here means someone cant' call .close() on the returned object. You could just add a passthrough close method, but that doesn't feel like the right solution.

[MicahZoltu]

My thought is something like this to maintain consistency with the HTTP server implementation. I'm not a fan of this style of monkeypatching functions to change from synchronous to asynch, but consistency is important here.

  var underlyingServerListen = server.listen;
  server.listen = function (ipcPath, callback) {
    underlyingServerListen(ipcPath);
    server.provider.manager.waitForInitialization(function(error, accounts) {
      callback(error, accounts);
    });
  };

  return server;

[MicahZoltu]

Hmmm, this pattern doesn't match up well with Web Sockets as they don't have a "listen" step. Constructing the WebSocket server binds to the port immediately and the options must be passed into the constructor. If we accept that the pattern won't fit for all 3 transports, is it worth bothering with consistency? Perhaps all 3 should bind to their respective ports on construction and the "listen" method can be changed to "start" or something and would just initialize the backing provider.

[MicahZoltu]

For consistency, server.provider should be set to provider, like the HTTP one.

[MicahZoltu]

Sadly, I don't believe this will work. From what I can tell, Ethereum JSON-RPC over IPC doesn't define any message boundary or enveloping. JSON-RPC (the general one) assumes that the transport defines message boundaries but unlike HTTP and WS, IPC is just a byte stream so it is up to the protocol to define message boundaries. I believe this is why the newline is required by some clients, though I have to wonder if those clients will do-the-right-thing if a message has newlines in it.

Best option I can come up with to correctly handle this is to have a streaming JSON parser that streams until it receives a complete single object. Once it does, it would call that a single message and then start streaming until it has received another object.

I'm open to better ideas, I am doing some related work and would love a better solution. :/

[tcoulter]

@MicahZoltu Hey, I'm finally getting back to the TestRPC (and the Truffle team has more staff now, thank goodness). Can I get you to evaluate your above comments and see if they hold true? I don't know enough about IPC either to try this out, but I will put more time into it if you believe the above is still not coded correctly. Appreciate it!

[MicahZoltu]

I believe the comments still apply, though at this point my real recommendation is to not support IPC at all and instead only support WS and HTTP.

I think the inclusion of IPC support, which pushes message enveloping to the protocol, in Geth and Parity was a mistake. With a JavaScript based node any performance benefits of IPC over WS are going to be so small in comparison to the fact that you are running your node in JavaScript that there is really no compelling reason to use IPC. The one caveat would be if you want to use TestRPC to test your dApp's IPC code. However, I think that in general people should stop using IPC in their dApps unless they have a really compelling reason to do so (like you need those extra nanoseconds). I haven't seen any particularly compelling arguments for IPC over WS at this point, which is why I would push to discourage its usage across the ecosystem.

[benjamincburns]

Closing this as the logic has moved to trufflesuite/ganache-core now. If this is still relevant, please reopen as a PR against the develop branch there.

Also, @tcoulter, we should likely discuss @MicahZoltu's last comment above.

[axic]

It is very much relevant given every tool has moved on to IPC and are going to (or have) shut down HTTP support. Though I do not have any resources to work on this.

[MicahZoltu]

Tools are preferencing WS over HTTP, which I can get behind. I'm unaware of any tools moving towards IPC though. Do you have some examples? I know Geth and Parity initially implemented IPC (and due to the lack of spec implemented it differently) but both now support WS which is "fast enough" for just about every use case I have been able to come up with and it provides message boundaries (unlike IPC) which means authoring tooling against it is significantly simpler (no need for a streaming JSON parser that can handle a stream of non-delimited JSON objects).

[axic]

Should have written IPC + WebSockets, with heavier weight on WebSockets. Either way, regular HTTP is not really preferred going forward.

[benjamincburns]

@axic no worries if you don't have the resources - it's on my list.

[benjamincburns]

Provided this still merges cleanly we should probably just pull the ipc branch into ganache-core and work on it from there

[adipascu]

This would be a cool feature to have, I am interested in having Ganache as a drop-in replacement for my local geth/parity setup.