Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NIP-47] Add versioning and migrate to NIP-44. #1531

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jklein24
Copy link

NIP-04 is deprecated and no longer recommended for use. Authors of NIP-44 have confirmed that it can act as a drop-in replacement for NIP-04 for NWC's use case.

Versioning for NIP-47 allows the protocol to evolve and even make breaking changes while maintaining backwards-compatibility where needed. Versions are represented as <major>.<minor> (e.g. 1.3). Major version bumps imply breaking changes that are incompatible with the previous major version. Minor version bumps, however, only include non-breaking feature additions or improvements which can maintain full compatibility with the previous minor version.

@vitorpamplona
Copy link
Collaborator

vitorpamplona commented Oct 10, 2024

Ack

FYI, NIP-46 (#1248) is just dynamically figuring out if the encryption is nip-04 or nip-44.

fun isNIP04(ciphertext: String): Boolean {
    val l = ciphertext.length
    if (l < 28) return false
    return ciphertext[l - 28] == '?' && 
           ciphertext[l - 27] == 'i' && 
           ciphertext[l - 26] == 'v' && 
           ciphertext[l - 25] == '='
}

The migration over there has three steps:

  1. Everybody decrypts both nip 04 and nip 44
  2. Everybody starts encrypting with NIP44 in their own timeline.
  3. Everybody drops support for encrypting and decrypting with nip04.

@jklein24
Copy link
Author

Ack

FYI, NIP-46 (#1248) is just dynamically figuring out if the encryption is nip-04 or nip-44.

fun isNIP04(ciphertext: String): Boolean {
    val l = ciphertext.length
    if (l < 28) return false
    return ciphertext[l - 28] == '?' && 
           ciphertext[l - 27] == 'i' && 
           ciphertext[l - 26] == 'v' && 
           ciphertext[l - 25] == '='
}

The migration over there has three steps:

  1. Everybody decrypts both nip 04 and nip 44
  2. Everybody starts encrypting with NIP44 in their own timeline.
  3. Everybody drops support for encrypting and decrypting with nip04.

Thanks, this also makes sense as the alternative path for NIP44 migration, and is certainly simpler. My main reason for not going this route right now is so that connections can use NIP44 right away if both sides indicate support, without needing to wait for a migration or try encrypting with NIP44 first, getting an error, and then re-encrypting with NIP-04.

I also think in general a versioning scheme will be useful for NWC for future breaking changes if they're needed, but I recognize that it adds complexity.

@rolznz
Copy link

rolznz commented Oct 16, 2024

Linking the notifications PR which ideally is merged (it has multiple implementations using it already) and has implications for versioning.

Because there is no communication from the client application to the wallet service to listen to notifications (the client application is simply subscribing to the relay), I think the simplest way would be to update the notifications PR to use a different notification event kind for version 1.0, and start including the tag in the notification event. That way, version v0.0 apps will not break by trying to decode v1.0 notifications.

I do not know how useful the version tag will be on notifications though, or if another breaking change in the future would require another change to the notification event kind, since the wallet service might still want to support the previous version.

Copy link

@ekzyis ekzyis left a comment

Choose a reason for hiding this comment

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

Looks good!

The v tag leaks metadata about connections but it also allows us to track adoption rate of new versions and thus when we can start dropping old versions.

Is this implemented in any wallet service yet with which I could test against as a client?

### Info event

First, the **wallet service** adds a `v` tag to its `info` event containing a list of versions it supports. This list should be a space-separated list in decreasing order with the format
`<major>.<minor>`. There should be one entry in the list for each major version supported by the wallet, where the minor version is the highest minor version for that major version. For example,
Copy link

Choose a reason for hiding this comment

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

What about patch versions? I think clients would also be interested in knowing if the service has the latest patches.

Copy link
Author

Choose a reason for hiding this comment

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

My take was the same as @bumi mentioned with regards to patch versions. If they don't really change compatibility, and versions don't bump often, I think patch versions will add more complexity then benefit.

@bumi
Copy link

bumi commented Oct 26, 2024

lgtm, thanks!
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. (thus I do not care about patches here)

Alby plans to implement this with NIP44

@jklein24
Copy link
Author

Looks good!

The v tag leaks metadata about connections but it also allows us to track adoption rate of new versions and thus when we can start dropping old versions.

This is a really good callout, thanks for pointing it out. The adoption rate tracking is nice though. I think this is a reasonable tradeoff.

Is this implemented in any wallet service yet with which I could test against as a client?

Currently only in a demo with the uma-nwc-server docker image, but it sounds like it'll be in alby hub in the not-so-distant future too :-)

@jklein24
Copy link
Author

Linking the notifications PR which ideally is merged (it has multiple implementations using it already) and has implications for versioning.

Because there is no communication from the client application to the wallet service to listen to notifications (the client application is simply subscribing to the relay), I think the simplest way would be to update the notifications PR to use a different notification event kind for version 1.0, and start including the tag in the notification event. That way, version v0.0 apps will not break by trying to decode v1.0 notifications.

I do not know how useful the version tag will be on notifications though, or if another breaking change in the future would require another change to the notification event kind, since the wallet service might still want to support the previous version.

Thanks for flagging this! I defer to you on what's best for notifications and how versioning could be relevant for them. What you've suggested with a new notification event kind sounds somewhat reasonable to me for forward-compatibility, but I could see how it'd be a bit of an annoying migration flow :-/

@ekzyis
Copy link

ekzyis commented Oct 28, 2024

This is a really good callout, thanks for pointing it out. The adoption rate tracking is nice though. I think this is a reasonable tradeoff.

I realized that it actually doesn't leak anything about connections since the requests and responses use ephemeral events (or are at least supposed to) so we can't track adoption rate of clients. We can only track adoption of wallet services by filtering for kind:13194. I think knowing which wallet service supports which version is unrelated to privacy unlike being able to distinguish individual connections from each other.

Was just something I noticed and wanted to mention, it wasn't (and still isn't) a real concern of mine.

I'll make sure that SN supports NIP-44 for NWC soon to see if I have any other feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants