NIP46 - update NIP44 calls

[monlovesmango]

is it too late to update the NIP44 methods in NIP46? I removed getKey and made encrypt and decrypt take arrays of params so encryption and decryption can happen in bulk.

these changes reflect the decisions that were made in the NIP07 PR #940

https://github.com/monlovesmango/nips/blob/NIP46-update-NIP44-calls/46.md

[monlovesmango]

@alexgleason can you expand on how you envision batching to be implemented for nip46? I am open to what you are suggesting but would like something a little more concrete to evaluate.

cc @staab @vitorpamplona @arthurfranca @erskingardner @fiatjaf

[alexgleason]

Sure, here's what I propose:

1. Leave kind 24133 alone, but deprecate it. Leave the structure of RPC calls alone.
2. Introduce a kind 24135. It has the exact same semantics and structure as the old kind, but encrypted in its content is an array of RPC calls/responses.

That's it.

Here's an example request:

{
  "kind": 24135,
  "pubkey": "eff37350d839ce3707332348af4549a96051bd695d3223af4aabce4993531d86",
  "content": nip44([
    {
      "id": "1",
      "method": "nip44_decrypt",
      "params": [<third_party_pubkey>, "01abcdef..."]
    },
    {
      "id": "2",
      "method": "nip44_decrypt",
      "params": [<third_party_pubkey>, "09dcdf..."]
    },
    {
      "id": "3",
      "method": "sign_event",
      "params": [json_stringified(<{
          content: "Hello, I'm signing remotely",
          pubkey: "fa984bd7dbb282f07e16e7ae87b26a2a7b9b90b7246a44771f0cf5ae58018f52",
          // ...the rest of the event data
      }>)]
    },
  ]),
  "tags": [["p", "fa984bd7dbb282f07e16e7ae87b26a2a7b9b90b7246a44771f0cf5ae58018f52"]], // p-tags the remote user pubkey
}

Example response:

{
  "kind": 24135,
  "pubkey": "fa984bd7dbb282f07e16e7ae87b26a2a7b9b90b7246a44771f0cf5ae58018f52",
  "content": nip44([
    {
      "id": "1",
      "method": "nip44_decrypt",
      "success": true,
      "data": ["hello world"]
    },
    {
      "id": "2",
      "method": "nip44_decrypt",
      "success": false,
      "error": "Malformed encryption data"
    },
    {
      "id": "3",
      "method": "sign_event",
      "success": true,
      "data": [<signed-event>]
    },
  ]),
  "tags": [["p", "eff37350d839ce3707332348af4549a96051bd695d3223af4aabce4993531d86"]], // p-tags the remote user pubkey
}

The results don't have to be structured exactly this way, it should just give you an idea. I went with a result type similar to zod's safeParse, which is well understood and I think it's very good. But the main point is just that you put a list in, get a list out. Everything else is just semantic.

[monlovesmango]

I like this a lot @alexgleason , thank you. let me update this pr.

[fiatjaf]

I don't see how batching improves anything. What do we gain? Time checking the signatures of the encompassing event?

[alexgleason]

@fiatjaf NIP-46 has overhead of encryption / network transfer / decryption / encryption / network transfer / decryption - in both directions.

Instead of trying to solve this on the individual function level, it makes more sense to solve it at the event level and keep the individual RPC calls atomic.

[vitorpamplona]

I don't see how batching improves anything. What do we gain? Time checking the signatures of the encompassing event?

Mostly the latency in decryption calls of multiple messages in chats.

Some relays might also block too many new events in the same second, thinking it is spam (e.g. when decrypting 10+ messages in the same chat screens).

[fiatjaf]

Batching stuff will still give you overhead of encrypting a bigger blob, decrypting it, sending a big event and so on. The only thing you gain is that you only have to sign and check one signature instead of many.

Also this batching stuff is only for loading past chat history? Why not store these messages locally after they were decrypted once?

[fiatjaf]

Mostly the latency in decryption calls of multiple messages in chats.

What about the latency of sending a big event with a ton of requests?

Some relays might also block too many new events in the same second

Aren't relays going to also block big encrypted events containing 10+ request/responses in them?

[fiatjaf]

I think we can either accept the inefficiency of one request per decryption and then cache these things locally as an optimization or we can implement getConversationKey() and be maximally efficient. And both approaches could be done simultaneously.

The batching stuff feels very inelegant to me and a bad solution doesn't matter if it's in the same request or in multiple batched requests in the same event.

[alexgleason]

@fiatjaf The question is not whether to batch or not. The question is, if we were to support batching, should it be done on an individual function level or at the whole event level? All the problems you describe are problems with the previously proposed nip44_decrypt function that would accept any number of arguments.

[vitorpamplona]

Yep, all of these ideas have tradeoffs. I kinda want to wait for a client to code one of these and give us some hard data on how much gain they actually provide.

[fiatjaf]

All the problems you describe are problems with the previously proposed nip44_decrypt function that would accept any number of arguments.

Yes, they are. And we gain nothing here but then we add a breaking way of doing NIP-46 requests immediately after we had all agreed that we wouldn't break NIP-46 in @erskingardner's PR from last week? This is insane to me.

If we are going to break things let's at least break it right and use his proposal.

[alexgleason]

Okay. Then add a new top-level RPC call called "batch" which batches multiple calls.

I proposed a new event kind to preserve backwards-compat. But if we want to force it to work the old way, it can look like this instead.

This to me is worse than just adding a new event kind, though? The idea about "breaking things" being a huge existential crisis is so overstated. Okay, so just make this a new NIP instead, and then that will help people with it psychologically?

{
  "kind": 24133,
  "pubkey": "eff37350d839ce3707332348af4549a96051bd695d3223af4aabce4993531d86",
  "content": nip44({
    "id": "0",
    "method": "batch",
    "params": [
      {
        "id": "1",
        "method": "nip44_decrypt",
        "params": [<third_party_pubkey>, "01abcdef..."]
      },
      {
        "id": "2",
        "method": "nip44_decrypt",
        "params": [<third_party_pubkey>, "09dcdf..."]
      },
      {
        "id": "3",
        "method": "sign_event",
        "params": [json_stringified(<{
            content: "Hello, I'm signing remotely",
            pubkey: "fa984bd7dbb282f07e16e7ae87b26a2a7b9b90b7246a44771f0cf5ae58018f52",
            // ...the rest of the event data
        }>)]
      },
    ]
  }),
  "tags": [["p", "fa984bd7dbb282f07e16e7ae87b26a2a7b9b90b7246a44771f0cf5ae58018f52"]], // p-tags the remote user pubkey
}

[fiatjaf]

This to me is worse than just adding a new event kind, though? The idea about "breaking things" being a huge existential crisis is so overstated. Okay, so just make this a new NIP instead, and then that will help people with it psychologically?

No, it won't help to create a new "method" or a new NIP. It is not a psychological issue, it is a compatibility issue. I will ask that you read #1047 (comment) because I have made my point way too many times already in the past week.

[monlovesmango]

rebased and removed all batching stuff, just too much disagreement. pulling an @alexgleason :)

agree with what fiatjaf is saying about having duplicate standards and there doesn't seem to be a good way to implement batching that everyone will be happy with. this now also aligns with #1063 which is makes me happy.

I am coming to the realization that theres not really a difference in network latency between batched calls and atomic calls if you send all your atomic calls concurrently rather than sequentially.

[staab]

Batching can be added in the future in a compatible way. If clients want to batch, they can check if the method exists, and use it or fall back. I'm happy with this.

[staab]

I'm happy with this.

By "this" I mean not batching. I still don't like conversation keys.

[erskingardner]

The idea about "breaking things" being a huge existential crisis is so overstated

Just wanted to state that I wholeheartedly agree with this. This certainly won't always be the case, so we should embrace it while we can in order to clean/improve/change things when we learn through using them that there is a better way.

All the same, happy to have the basic methods back in the PR.

[fiatjaf]

There is still disagreement about get_conversation_key.

[monlovesmango]

I still don't like conversation keys.

me neither.

[vitorpamplona]

Let's get rid of nip44_conversation_key and merge this. We can always add this method in the future if there is enough data to satisfy the need for it.

PS: I am seeing the need for a more generic deterministic key derivation from the private key (which is what the conversation key does behind the scenes). We could have deriveAndSign(derivationParams, ...), deriveAndNip44Encrypt(derivationParams, ...), deriveAndNip44Decrypt(derivationParams, ...). Private Zaps is one of the use cases for this.

[monlovesmango]

done, just reverted it back to "potential future addition".

[alexgleason]

Just remove it entirely. There's no point putting future plans in a nip. Either it's defined or it isn't.

[staab]

I agree, #1065