feat(api): Add notifications API

[corollari]

This PR integrates the notifications API discussed in #511

Regarding the Notifications class, I haven't implemented an api like api.neoscan.getBalance("http://www.neoscan-testnet.io/test_net/v1/", address) because the notifications api is stateful (needs to keep track of the websockets connections to be able to close them later) while neoscan is largely not.

[snowypowers]

1. I would avoid the use of the word Provider here as there is a specific interface for it. Reusing the name would be confusing. Consider not calling it a Provider in the docs and moving it under a different folder.

2. Any reason why we are not adding support for the rest of the endpoints or is that coming too?

3. Please try to separate the code and documentation. Docs can come later when we commit the code. If there are big changes, its gonna be a headache trying to reread the docs every time something changes.

4. You are mixing spaces and tabs. Check your IDE.

[corollari]

I would avoid the use of the word Provider here as there is a specific interface for it. Reusing the name would be confusing. Consider not calling it a Provider in the docs and moving it under a different folder.

Makes sense.

Any reason why we are not adding support for the rest of the endpoints or is that coming too?

Looking at the evolution and usage that this kind of system has had on ethereum it seems that users mostly just have two use-cases:

• Listen for events in their own smart contract
• Activate a callback when a transaction is consolidated in a block

Because of this I think that the other endpoints should be integrated in different ways (directly into the API that broadcasts transactions for example) but I believe that this other part is more controversial and wanted to get the first PR simple, that's why for now I'm only integrating a single endpoint.

Please try to separate the code and documentation. Docs can come later when we commit the code. If there are big changes, its gonna be a headache trying to reread the docs every time something changes.

Okay.

[corollari]

I believe I've now addressed all the concerns except those related to api design.

[snowypowers]

To me, its a matter of functionality for api design. I think it is really weird that I have to unsubscribe all my callbacks if i ever want to unsub. Even if we are talking about common developer patterns, the unsubscribe call has always been scoped to a single subscription and not to a single topic.

How about a mix of both designs. We can provide both unsubscribe(reference) and unsubscribeAll(contractHash) methods on the client. I dont think that we have to put the unsubscribe call on the returned reference if you prefer everything on the client.

[corollari]

I see, what kind of object were you thinking to use as reference on the unsubscribe calls?

[snowypowers]

I was thinking of just an empty object at the moment. You may consider adding maybe an identifier (maybe human readable). This paves the way for us to transform this into a class with methods if we ever need to go down the route where we want the unsubscribe/pause function on the reference.

[corollari]

So something that would look like this:

let object = notificationsProvider.subscribe('0xabc')
notificationsProvider.unsubscribe(object)

?

[snowypowers]

yeah

[corollari]

I'm currently busy with exams so I'll put this PR on hiatus and come back to it in a few weeks.

[corollari]

I've changed the API to work in the way you initially proposed:

let subscription = notifications.subscribe(hash, console.log)
subscription.unsubscribe()

[snowypowers]

Implementation is pretty solid, just some cleanup notes and adding on more tests.

[snowypowers]

Optional, a cleaner flow would be to first pull out the ContractSubscriptions object first and just do a push.

const contractSubscriptions = this.subscriptions.get(contract) ?? createWebsocketForContract(contract);
contractSubscriptions.callbacks.push(callback);

[corollari]

Done in 50387ee

[snowypowers]

we will have to make it clear that the null string is a special subscription that listens to all contracts and unsubscribeContract() !== unsubscribeAll().

[corollari]

Changed the comments with ca90ea9 to try to make that clearer, thoughts?

[snowypowers]

this inner variable feels redundant when we are not throwing. The savings is pretty small for the cost of having a possible wrong state.

[corollari]

What alternative do you think would work best? Throwing exceptions?

[snowypowers]

I meant that you could just remove the alive check and just call unsubscribeFunction since it doesn't fail.

[corollari]

I see what you are saying, but that could result in unexpected behaviour in the following case:

let s1 = notifications.subscribe("0x...", console.log)
let s2 = notifications.subscribe("0x...", console.log)
s1.unsubscribe()
s1.unsubscribe() //s2 gets unsubscribed

With that said, this is a sort of an extreme edge case so we may want to ignore it.

[snowypowers]

ah thats interesting then the issue here is that we are storing the reference to the callback as the unique identifier (instead of something that is unique to the subscription). This solves the problem by ensuring each subscription can only at most remove one callback. A little roundabout but i guess it works.

[corollari]

I've changed how that works (now it works by keeping a unique identifier, like you suggested) to reduce the edge cases and the complexity, see #514 (comment)

[snowypowers]

add a test for this edge case.

[snowypowers]

testcase

[snowypowers]

testcase

[corollari]

I'm currently reworking how the class synchronises the state of the subscriptions object with the individual subscriptions by storing callbacks along a unique identifier, therefore making it much easier to check whether the subscription has been removed before.

I'd thought about this previously but decided against it because I thought that looping over an array of callbacks (the most frequent action due to the fact that it's done every time a new event is received) would be faster compared to iterating over some other data structure such as the values of a map. Later, your comment made me consider that I might be wrong, so I ran a performance benchmark and, contrary to my expectations, it turns out that iterating over the values of a map only results in a slowdown of <2%, a performance hit that, in my opinion, is worth the simpler implementation, which results in less edge cases and removes the need to keep two components synchronized.

[corollari]

Just added a bunch of tests, let me know if I'm missing anything.

[corollari]

With the addition of the last 2 tests, the coverage right now is sitting at 100%.

[snowypowers]

finally done! Thanks for the contribution!