Feat/get notifications history

[devceline]

Changes

• Implement getNoticationHistory per spec.
• Resolves Implement getNotificationHistory  #89
• Remove getMessageHistory and deleteNotifyMessage
• This is all working towards the [EPIC] Add support for history against the new spec #87 epic

To achieve this

• New event handlers were put in place in the engine, mimicking the ones in the client. on,off, once, etc.
• These are in place to go against the current pub-sub architecture in place, to provide "blocking" functions that resolve once a response is given
• This is to achieve the spec which takes in params and returns the results in the same function
• This system will be reused by many other functions, including the refactored subscribe down the line.

Tested

Tested via new integration test that communicates with notify server

How does this new flow work?

[enesozturk]

Looks good in general, left some feedback

[enesozturk]

Suggested change

	const issuedAt = Math.round(Date.now() / 1000);
	const issuedAtInSeconds = Math.round(Date.now() / 1000);

When we work with dates, or format them into different periods (hrs, seconds, milliseconds, etc.). It's better to select names that indicate what the value is. This will improve further maintenance and also will allow the reviewer to see possible issues on the next lines if he/she knows what the the time period of the variable is in mind

[enesozturk]

Suggested change

	act: "notify_get_notifications",
	action: "notify_get_notifications",

I don't know if this is the right name but I wanted to point out that this field and other fields in this object might be better to have more readable field names. When I read the code, I didn't understand if these values were correctly set without knowing the codebase as a reviewer. I believe we should prever readable field names too.

[devceline]

The fields unfortunately have to be compliant with the spec linked above. See the "Authentication" page for details. The reason for the short, 3 letter names is to cut down redundant data on transport, since this all gets encoded into a JWT.

[bkrem]

As mentioned below in my other comment this is to be spec compliant: https://github.com/WalletConnect/walletconnect-specs/pull/183/files#diff-02dc8b9dd052b54a577b55ecf0d2b3ad2d58f1df0a0be7ba388fa721ffcbc48eR137

Would have to be agreed at the x-platform level to use longer form but I believe this is the convention for JWT claims

[enesozturk]

Not urgent at this time but since we're using this string in different places and there are different types of values to pass to this function. It might be better to keep these values in a tuple. This would help to maintain them and prevent from possible issues in the future.

type NotifyMessage = {
 GetNotificationResponse: "notify_get_notifications_response",
}

and while using:

this.emit(NotifyMessage.GetNotificationResponse, {...})

[devceline]

This honestly is probably a worthwhile refactor to reduce on the crazy amount of hardcoded strings, but this is not the PR for it. I'll create an issue for this

[enesozturk]

Ref to previous naming comment. Better naming don't require comments as a return

[enesozturk]

Ref to previous naming comment. Better naming don't require comments as a return

[bkrem]

These fields are concretely defined in the spec with these namings, so deviating from them just creates unnecessary mental overhead and remapping, see: https://github.com/WalletConnect/walletconnect-specs/pull/183/files#diff-02dc8b9dd052b54a577b55ecf0d2b3ad2d58f1df0a0be7ba388fa721ffcbc48eR137

[enesozturk]

Oh didn't know about it, make sense then, thanks

[bkrem]

As mentioned below in my other comment this is to be spec compliant: https://github.com/WalletConnect/walletconnect-specs/pull/183/files#diff-02dc8b9dd052b54a577b55ecf0d2b3ad2d58f1df0a0be7ba388fa721ffcbc48eR137

Would have to be agreed at the x-platform level to use longer form but I believe this is the convention for JWT claims