Skip to content

PendingSignerRequest.signerPubkey is unreliable #744

Description

@nogringo

PendingSignerRequest.signerPubkey is unreliable

Labels: bug

Problem

signerPubkey is the only thing attributing a pending request to an account. It is
not trustworthy today, which blocks any multi-account aggregation.

Empty during login (NIP-46 and NIP-07)

packages/ndk/lib/data_layer/repositories/signers/nip46_event_signer.dart:132

signerPubkey: cachedPublicKey ?? '',

cachedPublicKey is only assigned once getPublicKeyAsync() resolves (line 268),
and that call goes through remoteRequest itself. So the first requests, connect
and get_public_key, carry an empty string. That is exactly when the UI most needs
to say who it is waiting on.

packages/ndk_flutter/lib/signers/src/nip07_event_signer_web.dart:59 has the
identical pattern, with cachedPublicKey assigned at line 213 inside a
_trackRequest call.

Related: #237, which covers the sync/async getPublicKey design that forces this
caching in the first place.

npub vs hex (NIP-55)

packages/ndk_flutter/lib/data_layer/repositories/signers/nip55_event_signer.dart:84
uses the raw constructor-injected publicKey, which may be an npub. Line 49 decodes
it to hex for _currentUser, but neither signerPubkey nor getPublicKey() (line
147) do.

Accounts.addAccount keys accounts on signer.getPublicKey(), so a NIP-55 account
can sit under an npub key while a NIP-46 account sits under hex. An aggregated map
would then hold two entries for what may be the same identity.

Ambiguous semantics

The doc comment says "The public key of the signer (the account making the request)".
For a bunker those are two different keys: the user pubkey and
connection.remotePubkey. The field currently holds the former. This needs to be
decided explicitly, and possibly split into two fields, if the UI is ever to show
which signer a request is waiting on.

Suggested fix

  • NIP-46 and NIP-07: populate the pubkey as early as possible instead of ?? ''.
  • NIP-55: normalize to hex.
  • Entity: pin down the semantics in the doc comment.

Breaking change to decide

Normalizing NIP-55 to hex changes what getPublicKey() returns, therefore the key
the account is registered under in Accounts.accounts. Existing callers doing
hasAccount(npub) or switchAccount(pubkey: npub) would break.

The alternative is to normalize in Accounts.addAccount, which fixes every signer at
once but has a wider blast radius.

Preference: the narrow fix in NIP-55 plus a changelog entry, since NIP-55 specifies
hex ("All pubkeys in this NIP are in hex format") and the npub form is already an
anomaly there.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions