-
Notifications
You must be signed in to change notification settings - Fork 316
Simplify OrderbookSync constructor signature #177
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,18 +31,13 @@ suite('OrderbookSync', () => { | |
}); | ||
}); | ||
|
||
test('passes authentication details to websocket (with AuthenticatedClient)', done => { | ||
test('passes authentication details to websocket', done => { | ||
const server = testserver(++port, () => { | ||
const authClient = new Gdax.AuthenticatedClient( | ||
'suchkey', | ||
'suchsecret', | ||
'muchpassphrase' | ||
); | ||
new Gdax.OrderbookSync( | ||
'BTC-USD', | ||
EXCHANGE_API_URL, | ||
'ws://localhost:' + port, | ||
authClient | ||
{ key: 'suchkey', secret: 'suchsecret', passphrase: 'muchpassphrase' } | ||
); | ||
}); | ||
|
||
|
@@ -59,6 +54,29 @@ suite('OrderbookSync', () => { | |
}); | ||
}); | ||
|
||
test('passes authentication details to websocket (via AuthenticationClient for backwards compatibility)', done => { | ||
const server = testserver(++port, () => { | ||
new Gdax.OrderbookSync( | ||
'BTC-USD', | ||
EXCHANGE_API_URL, | ||
'ws://localhost:' + port, | ||
new Gdax.AuthenticatedClient('mykey', 'mysecret', 'mypassphrase') | ||
); | ||
}); | ||
|
||
server.on('connection', socket => { | ||
socket.on('message', data => { | ||
const msg = JSON.parse(data); | ||
assert.equal(msg.type, 'subscribe'); | ||
assert.equal(msg.key, 'mykey'); | ||
assert.equal(msg.passphrase, 'mypassphrase'); | ||
|
||
server.close(); | ||
done(); | ||
}); | ||
}); | ||
}); | ||
|
||
test('emits a message event', done => { | ||
nock(EXCHANGE_API_URL) | ||
.get('/products/BTC-USD/book?level=3') | ||
|
@@ -89,7 +107,7 @@ suite('OrderbookSync', () => { | |
}); | ||
}); | ||
|
||
test('emits a message event (with AuthenticatedClient)', done => { | ||
test('emits a message event (with auth)', done => { | ||
nock(EXCHANGE_API_URL) | ||
.get('/products/BTC-USD/book?level=3') | ||
.times(2) | ||
|
@@ -99,12 +117,11 @@ suite('OrderbookSync', () => { | |
}); | ||
|
||
const server = testserver(++port, () => { | ||
const authClient = new Gdax.AuthenticatedClient('key', 'secret', 'pass'); | ||
const orderbookSync = new Gdax.OrderbookSync( | ||
'BTC-USD', | ||
EXCHANGE_API_URL, | ||
'ws://localhost:' + port, | ||
authClient | ||
{ key: 'key', secret: 'secret', passphrase: 'pass' } | ||
); | ||
orderbookSync.on('message', data => { | ||
assert.deepEqual(data, { | ||
|
@@ -147,18 +164,17 @@ suite('OrderbookSync', () => { | |
}); | ||
}); | ||
|
||
test('emits an error event on error (with AuthenticatedClient)', done => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could one of these still use an authenticated client, to ensure we don't break backwards compatibility? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personally, I don't think we should have tests for that, because we're moving forward with the API. The benefit of previously changing the Nonetheless, with that off my chest, I'm happy to add an extra test with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An extra test using an instance of |
||
test('emits an error event on error (with auth)', done => { | ||
nock(EXCHANGE_API_URL) | ||
.get('/products/BTC-USD/book?level=3') | ||
.replyWithError('whoops'); | ||
|
||
const server = testserver(++port, () => { | ||
const authClient = new Gdax.AuthenticatedClient('key', 'secret', 'pass'); | ||
const orderbookSync = new Gdax.OrderbookSync( | ||
'BTC-USD', | ||
EXCHANGE_API_URL, | ||
'ws://localhost:' + port, | ||
authClient | ||
{ key: 'key', secret: 'secret', passphrase: 'pass' } | ||
); | ||
|
||
orderbookSync.on('message', () => | ||
|
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.
Can this default to
auth
if it already is an instance ofAuthenticatedClient
?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 vote no...for the same reasons as above. We should not be making side-effect assumptions about the passed object, aside from the fact that it quacks like a
{ key, secret, passphrase }
object. Otherwise, the project opens itself to a world of forwards compatibility issues and limitations down the road.i.e. "Optimizations" like this only yield future maintenance problems. There's almost zero overhead to having an extra
AuthenticatedClient
instance.e.g. If we did reuse a passed-in
AuthenticationClient
, theapiURI
argument passed to theOrderbookSync
constructor would then be ignored, and you'd end up with unintended consequences of talking to the wrong endpoint.