Skip to content
This repository has been archived by the owner on Jan 20, 2020. It is now read-only.

Simplify OrderbookSync constructor signature #177

Merged
merged 3 commits into from
Dec 25, 2017
Merged

Simplify OrderbookSync constructor signature #177

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9cc86d1
Add checkAuth() utility function to check if auth object is valid
rmm5t Dec 25, 2017
11cf861
Simplify OrderbookSync constructor to take auth object
rmm5t Dec 25, 2017
09be1e8
Add OrderbookSync test w/ AuthenticationClient for backwards compat
rmm5t Dec 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions lib/clients/websocket.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,8 @@ class WebsocketClient extends EventEmitter {
super();
this.productIDs = Utils.determineProductIDs(productIDs);
this.websocketURI = websocketURI;
if (auth && !(auth.secret && auth.key && auth.passphrase)) {
throw new Error(
'Invalid or incomplete authentication credentials. You should either provide all of the secret, key and passphrase fields, or leave auth null'
);
}
this.channels = channels;
this.auth = auth || {};
this.auth = Utils.checkAuth(auth);
this.heartbeat = heartbeat;
this.connect();
}
Expand Down
21 changes: 16 additions & 5 deletions lib/orderbook_sync.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,25 +1,36 @@
const WebsocketClient = require('./clients/websocket.js');
const AuthenticatedClient = require('./clients/authenticated.js');
const PublicClient = require('./clients/public.js');
const Orderbook = require('./orderbook.js');
const Utils = require('./utilities.js');

// Orderbook syncing
class OrderbookSync extends WebsocketClient {
constructor(
productIDs,
apiURI = 'https://api.gdax.com',
websocketURI = 'wss://ws-feed.gdax.com',
authenticatedClient = null,
auth = null,
{ heartbeat = false } = {}
) {
super(productIDs, websocketURI, authenticatedClient, { heartbeat });
super(productIDs, websocketURI, auth, { heartbeat });
this.apiURI = apiURI;
this.authenticatedClient = authenticatedClient;
this.auth = Utils.checkAuth(auth);

this._queues = {}; // []
this._sequences = {}; // -1
this._public_clients = {};
this.books = {};

if (this.auth.secret) {
this._authenticatedClient = new AuthenticatedClient(
Copy link
Contributor

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 of AuthenticatedClient?

Copy link
Contributor Author

@rmm5t rmm5t Dec 25, 2017 •
edited
Loading

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, the apiURI argument passed to the OrderbookSync constructor would then be ignored, and you'd end up with unintended consequences of talking to the wrong endpoint.

this.auth.key,
this.auth.secret,
this.auth.passphrase,
this.apiURI
);
}

this.productIDs.forEach(productID => {
this._queues[productID] = [];
this._sequences[productID] = -2;
Expand Down Expand Up @@ -55,8 +66,8 @@ class OrderbookSync extends WebsocketClient {
const bookLevel = 3;
const args = { level: bookLevel };

if (this.authenticatedClient) {
this.authenticatedClient
if (this._authenticatedClient) {
this._authenticatedClient
.getProductOrderBook(args, productID)
.then(onData.bind(this))
.catch(onError.bind(this));
Expand Down
10 changes: 10 additions & 0 deletions lib/utilities.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,16 @@ function determineProductIDs(productIDs) {
return [productIDs];
}

function checkAuth(auth) {
if (auth && !(auth.secret && auth.key && auth.passphrase)) {
throw new Error(
'Invalid or incomplete authentication credentials. You should either provide all of the secret, key and passphrase fields, or leave auth null'
);
}
return auth || {};
}

module.exports = {
determineProductIDs,
checkAuth,
};
19 changes: 6 additions & 13 deletions tests/orderbook_sync.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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' }
);
});

Expand Down Expand Up @@ -89,7 +84,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)
Expand All @@ -99,12 +94,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, {
Expand Down Expand Up @@ -147,18 +141,17 @@ suite('OrderbookSync', () => {
});
});

test('emits an error event on error (with AuthenticatedClient)', done => {
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 AuthenticatedClient properties and the backwards compatibility that comes with it is just a nice and elegant side effect . Backwards compatibility guarantees can be good sometimes, but they can also hold things back long-term.

Nonetheless, with that off my chest, I'm happy to add an extra test with AuthenticatedClient if would make you feel more comfortable. I'm just not sure this is the correct place to test that AuthenticatedClient quacks like an auth payload object of { key, secret, passphrase }.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An extra test using an instance of AuthenticatedClient was just added via 09be1e8.

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', () =>
Expand Down