feat: add nip 44 and versioning support #273
Conversation
| @@ -895,6 +927,7 @@ export class NWCClient { | |||
| resultValidator: (result: T) => boolean, | |||
| ): Promise<(T & { dTag: string })[]> { | |||
| await this._checkConnected(); | |||
| await this._checkCompatibility(); | |||
There was a problem hiding this comment.
why did you choose to check compatibility here?
what about once in the constructor? then we also know what notification kind to subscribe to?
There was a problem hiding this comment.
It's an async function (fetching wallet service info), so I thought of doing the call before making a request
then we also know what notification kind to subscribe to?
We are doing that right? We store the version in this.version/this.supportedVersion and use that before passing the kind in subscribeNotifications
| } | ||
| } | ||
|
|
||
| private selectHighestCompatibleVersion( |
There was a problem hiding this comment.
It is nice, but I am wondering if such a complex algorithm is needed?
What about just:
if the wallet supports version 1.0, use it. Otherwise use 0.0.
As Bumi mentioned:
I do not expect many versions (and rather bigger changes like the switch to nip-44) because too many versions (of a spec) lead to incompatibilities which must be avoided.
There was a problem hiding this comment.
Hmmm, I was also thinking about this, but then thought to add this as this is an sdk and can face any possibility... (a weird case where the wallet service info gives "0.5" or "1.1" for example)
@im-adithya this is great! 🚀 please let me know when it's ready for testing. |
|
Done, ready for testing! |
Ready for review
nostr-toolsv2 as nip44 library doesn't match the latest spec (and hence gives errors as it doesn't match with the Hub's nip-44)nip-44folder so we can test it, in the meantime I'll work on a PR to shift tonostr-toolsv2 so we can remove it.