-
Notifications
You must be signed in to change notification settings - Fork 7
Allow instantiation with existing ws #49
base: master
Are you sure you want to change the base?
Conversation
* Very experimental
* added test coverage * pass in raw socket to plugin
09d85e0
to
1aaed04
Compare
@@ -120,6 +123,58 @@ describe('BtpPlugin', function () { | |||
}) | |||
}) | |||
|
|||
describe('can pass in websocket connection', function () { | |||
|
|||
beforeEach(async function () { |
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.
The rest of this project is 2-space indented.
@@ -132,6 +132,9 @@ export interface IlpPluginBtpConstructorOptions { | |||
port: number, | |||
secret: string | |||
}, | |||
raw?: { |
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 reason for this extra layer of indirection?
Also, from the option alone it isn't clear whether the created plugin will act as a client or server. Maybe it should be server_socket
(in the top-level options), or listener.socket
.
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 reason for this extra layer of indirection?
Good suggestion. I think listener.socket
makes the most sense.
Also, from the option alone it isn't clear whether the created plugin will act as a client or server.
The plugin is not going to act as a WS server because it is created around an existing ws socket so it's not listening for new connections.
BUT it will handle incoming auth messages (i.e. it's a "BTP server" in as much as the difference between a client and server is that one performs auth with the other even though they may actually be peers)
This is a general problem (I think) with the architecture of the plugin in that it mixes the transport (websockets) with the protocol (BTP).
I tried a more decoupled approach with ilp-transport
, interested to hear what you think of that and if we could move to something similar with BTP: https://github.com/adrianhopebailie/ilp-transport
This change is required by interledgerjs/ilp-connector#476
It allows an AccountProvider in the connector to create new instances of the plugin from sockets that are created by a WS server running outside the plugin.