Mark subscribe() and unsubscribe() as async

[hjoelr]

This PR is intended to solve the issue described in #661. Basically, because subscribe() called sendMessage() which was an async function without awaiting it, subscribe would immediately return process flow back to the calling code. If the calling code happened to call disconnect()--which sets this.socket to undefined--when the call to sendMessage() returns, subscribe() would reference a non-existent socket object when sending the subscribe command.

[codecov-commenter]

Codecov Report

Merging #678 (2195b2f) into main (d48e45e) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #678   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           19        19           
  Lines          531       531           
  Branches        39        39           
=========================================
  Hits           531       531           

Flag	Coverage Δ
unittests	100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/client/WebSocketClient.ts	100.00% <100.00%> (ø)

[hjoelr]

@bennycode This is ready for review. There is one additional question I have and would like your input. Here's the code in question:

coinbase-pro-node/src/client/WebSocketClient.ts

Lines 483 to 502 in d48e45e

	async sendMessage(message: WebSocketRequest): Promise<void> {
	if (!this.socket) {
	throw new Error(`Failed to send message of type "${message.type}": You need to connect to the WebSocket first.`);
	}
	/**
	* Authentication will result in a couple of benefits:
	* 1. Messages where you're one of the parties are expanded and have more useful fields
	* 2. You will receive private messages, such as lifecycle information about stop orders you placed
	* @see https://docs.pro.coinbase.com/#subscribe
	*/
	const signature = await this.signRequest({
	httpMethod: 'GET',
	payload: '',
	requestPath: `${UserAPI.URL.USERS}/self/verify`,
	});
	Object.assign(message, signature);
	this.socket.send(JSON.stringify(message));
	}

In the sendMessage() function, we check for a non-existent socket object at the beginning of the call. This is most likely necessary for signRequest to work. However, if someone happens to call subscribe() without await, call flow would return back to the original caller while signRequest is being awaited. If the original caller happened to immediately call disconnect(), the socket object would be set to undefined. Then when signRequest eventually returns, this code runs:

coinbase-pro-node/src/client/WebSocketClient.ts

Line 501 in d48e45e

this.socket.send(JSON.stringify(message));

This obviously results in the error that we are calling a function on an undefined object.

What I'm wondering is if we should check for an undefined socket object both before AND after the call to signRequest() function? We would at least at that point be able to throw a meaningful exception.

I did in fact try this using this code:

async sendMessage(message: WebSocketRequest): Promise<void> {
    const noSocketError = `Failed to send message of type "${message.type}": You need to connect to the WebSocket first.`;
    if (!this.socket) {
      throw new Error(noSocketError);
    }

    /**
     * Authentication will result in a couple of benefits:
     * 1. Messages where you're one of the parties are expanded and have more useful fields
     * 2. You will receive private messages, such as lifecycle information about stop orders you placed
     * @see https://docs.pro.coinbase.com/#subscribe
     */
    const signature = await this.signRequest({
      httpMethod: 'GET',
      payload: '',
      requestPath: `${UserAPI.URL.USERS}/self/verify`,
    });
    Object.assign(message, signature);

    /*
     * We retest here because someone may have called this function without awaiting it and then then
     * when signRequest() is awaited process flow returns to calling code which could call disconnect().
     * Process flow would eventually return from signRequest() and there would no longer be a socket
     * object even though there was a socket object at the beginning of this function call.
     */
    if (!this.socket) {
      throw new Error(noSocketError);
    }

    this.socket.send(JSON.stringify(message));
  }

However, code coverage complained about not hitting the second throw line (right before the call to send()). I can't add a test with just ws.subscribe() without async because the linter will not allow that. So I'm a little stuck in being able to test it and get the linter and coverage to be happy. Anyway, I'm interested in your thoughts on this.

[hjoelr]

@bennycode I thought about this some more and realized that I was thinking the signRequest() call required the socket object. However, that is not the case. (It uses the REST API for time call, not WebSocket.) So, I just moved the test for undefined socket after the signRequest() call. This fixes the issue if someone calls sendMessage() without await and then immediately calls disconnect().

[bennycode]

Thanks for your great efforts @hjoelr. Taking care of properly awaiting disconnects helped with upgrading to 'ws' v8: 58b522a

I released your changes with coinbase-pro-node v4.0.0 (major bump) because the signatures of subscribe and unsubscribe have changed.

Thanks a lot!