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

Add CoinOS connector #263

Closed
rolznz opened this issue Dec 17, 2024 · 20 comments · Fixed by #281
Closed

Add CoinOS connector #263

rolznz opened this issue Dec 17, 2024 · 20 comments · Fixed by #281

Comments

@rolznz
Copy link
Collaborator

rolznz commented Dec 17, 2024

No description provided.

@Dayvvo
Copy link
Contributor

Dayvvo commented Dec 31, 2024

Hey @rolznz I'd like to give this a go. If anyone isn't working on it yet

@rolznz
Copy link
Collaborator Author

rolznz commented Jan 1, 2025

@Dayvvo I don't think anyone is, please go ahead!

@Dayvvo
Copy link
Contributor

Dayvvo commented Jan 10, 2025

Hey @rolznz I went though the code in the past week, looking at how existing connectors are setup. I was able to contact the guys at coinos and they'd like to have a connect option like Alby's which has the best UX on bitcoin-connect. Looking at the setup it looks like I'd have to create a corresponding issue on getAlby/js-sdk in order o add config options for a sign in api like the one done in NWCClient.ts here.

Will create a PR when I have it all figured out. Let me know what you think!

@rolznz
Copy link
Collaborator Author

rolznz commented Jan 11, 2025

@Dayvvo awesome that you contacted them. We are currently planning an open protocol for connecting to wallets which should support deep links like the existing Alby one. We should have some updates here in the next couple of weeks, if you are able to wait.

Instead of hardcoding authorization URLs like it is done currently in the JS SDK that you linked, it would be possible to find the authorization URL just by typing the lightning address of your CoinOS/Alby/etc. wallet.

@Dayvvo
Copy link
Contributor

Dayvvo commented Jan 17, 2025

Hey @rolznz thanks for this info. would it be possible to get a bit more context on how this would work? We were kinda looking at the convenience of the current alby login where a click redirects to the alby login and user doesn't need to copy and paste a string.

I'm not exactly sure how it works but from what you said I'm getting that it seems like a 2 step process where the user will have to copy and paste his lightning address and then get redirected to a login screen which he/she may have to enter login credentials again. Is that how the flow would work?

@rolznz
Copy link
Collaborator Author

rolznz commented Jan 17, 2025

@Dayvvo I agree and prefer the original flow. It seems like we will go with this one rather than the lightning address idea. The reason why we were considering this is because we also want to support self-hosted hubs which might not have a publicly accessible URL, but we have found a different way to do this.

Our current documentation for deeplink parameters are here: https://github.com/getAlby/hub/?tab=readme-ov-file#client-created-secret - it would be great to create a spec around this. There are two ways the connection url (relay and wallet pubkey) can be returned (via the return_to property or via a message from an auth popup)

One essential thing is that the clientside (here bitcoin connect) generates the secret key, and only gives the pubkey when doing the login.

We will have to update the JS SDK to return the wallet service pubkey since at least in the hub, it's now different for every app. I will make a PR for this soon.

Please let me know if you have any questions! I will update here when I have PRs and more information.

@rolznz
Copy link
Collaborator Author

rolznz commented Jan 18, 2025

@Dayvvo I've started a PR in Alby JS SDK to simplify executing the auth flow here: getAlby/js-sdk#297

CoinOS would need to do a few things:

  1. Implement an authorization page where the user can approve or decline the new connection (and optionally implement parameters can be passed to configure the new connection - I am not sure if CoinOS plans to have multiple connections though)
  2. Once the new connection is authorized, dispatch success events from the authorization page so that Bitcoin Connect receives the new connection string.
    const nwcEvent = new CustomEvent("nwc:success", {
      detail: {
        nostrWalletConnectUrl,
      },
    });
    window.dispatchEvent(nwcEvent);

    // notify the opener of the successful connection
    if (window.opener) {
      window.opener.postMessage(
        {
          type: "nwc:success",
          nostrWalletConnectUrl,
        },
        "*"
      );
    }

@rolznz
Copy link
Collaborator Author

rolznz commented Jan 18, 2025

The CoinOS connector should be able to be simple like the old Alby Connector (bc-alby-nwc-connector.ts), with no page needed.

its _onClick method could look like:

  protected async _onClick() {
    const providerConfig = store.getState().bitcoinConnectConfig.providerConfig;
    const nwcClient = await nwc.NWCClient.fromAuthorizationUrl(
      'https://coinos.io/apps/new',
      {
        ...(providerConfig?.nwc?.authorizationUrlOptions || {}),
        name: this._appName,
      }
    );

    await store.getState().connect({
      nwcUrl: nwcClient.nostrWalletConnectUrl,
      connectorName: 'CoinOS',
      connectorType: 'nwc.coinos',
    });
  }

@Dayvvo
Copy link
Contributor

Dayvvo commented Jan 20, 2025

Alright. So to clarify this snippet here

const nwcEvent = new CustomEvent("nwc:success", { detail: { nostrWalletConnectUrl, }, }); window.dispatchEvent(nwcEvent);

is to trigger a successful login for internal logic handing that in the auth api page of a provider like coinos and not to send a message to the opener. that part is handled by the window.opener. postMessage in the later part of the code, correct?

Also this is much clearer to me. tbh I have been through the code that facillitates the alby login flow and from what I saw it seemed like the secret was being generated by the client and the walletpubkey and relay url are also hardcoded in the js-sdk code. So basically the nwc string was not being returned from the opened window that authorizes a user. I was really confused about how that worked to authorize a specific user.
Honestly, I still wouldn't mind some clarification in case I missed something, if you don't mind

but this new flow seems to return the NWC string from the provider api page that is Opened which seems much simpler. I will notify the guys at coinos about this. Also let me know what you think @rolznz. Happy to hear your input or further explanation about what I missed

@rolznz
Copy link
Collaborator Author

rolznz commented Jan 21, 2025

Alright. So to clarify this snippet here

const nwcEvent = new CustomEvent("nwc:success", { detail: { nostrWalletConnectUrl, }, }); window.dispatchEvent(nwcEvent);

is to trigger a successful login for internal logic handing that in the auth api page of a provider like coinos and not to send a message to the opener. that part is handled by the window.opener. postMessage in the later part of the code, correct?

The CustomEvent is for when the auth is handled in a webview (e.g. on native mobile apps). The postMessage is for when the auth is handled in a popup (what Bitcoin Connect does). So, these two are for separate flows.

But the intent is the same - to send information back to the app that initiated the auth flow, so that it receives the relay URL and the wallet service pubkey. In combination with the app key which is generated by the app itself, all the parameters necessary to initiate a NWC connection are available.

Also this is much clearer to me. tbh I have been through the code that facillitates the alby login flow and from what I saw it seemed like the secret was being generated by the client and the walletpubkey and relay url are also hardcoded in the js-sdk code. So basically the nwc string was not being returned from the opened window that authorizes a user. I was really confused about how that worked to authorize a specific user.
Honestly, I still wouldn't mind some clarification in case I missed something, if you don't mind

Note: the secret key is still generated locally, this is so that the secret does not have to be sent over the internet. But if that does not fit with coinOS, they can still return a secret of their own and return it in the event/message above. The app should discard the one it generated and instead use the one generated by coinOS.

@Dayvvo
Copy link
Contributor

Dayvvo commented Jan 24, 2025

Alright. So to clarify this snippet here
const nwcEvent = new CustomEvent("nwc:success", { detail: { nostrWalletConnectUrl, }, }); window.dispatchEvent(nwcEvent);
is to trigger a successful login for internal logic handing that in the auth api page of a provider like coinos and not to send a message to the opener. that part is handled by the window.opener. postMessage in the later part of the code, correct?

The CustomEvent is for when the auth is handled in a webview (e.g. on native mobile apps). The postMessage is for when the auth is handled in a popup (what Bitcoin Connect does). So, these two are for separate flows.

But the intent is the same - to send information back to the app that initiated the auth flow, so that it receives the relay URL and the wallet service pubkey. In combination with the app key which is generated by the app itself, all the parameters necessary to initiate a NWC connection are available.

Also this is much clearer to me. tbh I have been through the code that facillitates the alby login flow and from what I saw it seemed like the secret was being generated by the client and the walletpubkey and relay url are also hardcoded in the js-sdk code. So basically the nwc string was not being returned from the opened window that authorizes a user. I was really confused about how that worked to authorize a specific user.
Honestly, I still wouldn't mind some clarification in case I missed something, if you don't mind

Note: the secret key is still generated locally, this is so that the secret does not have to be sent over the internet. But if that does not fit with coinOS, they can still return a secret of their own and return it in the event/message above. The app should discard the one it generated and instead use the one generated by coinOS.

Hey @rolznz so it could be done. The guys at coinos are willing to implement whatever flow works best for everyone and gives good UX.

I'm basically asking because I am bit confused as to how it works in tandem with the nip47 spec. After I looked at the alby implementation I actually had the intention to ask this question before we got to conversing about this apporoach.

From the spec I have come to understand that the secret is generated by the wallet to be connected, along with other parameters. I was under the impression that the generated secret that is used to decrypt messages sent to the relay is how the relay confirms the validity of the strings.

So how does messages to the relay with a locally generated secret work. is this in agreement with nip47 spec or another implelmentation? a Bit of an explanation will be greatly appreciated @rolznz

@asoltys
Copy link

asoltys commented Jan 24, 2025

Hi guys thanks for working on this, I'm excited to get Coinos added to bitcoin connect!

I'm willing to redo things on our end a bit so that we can support accepting app secrets and have multiple connections under a single Coinos account.

I'll update our nostr settings page to let people see all their connections and manage limits/budgets for each one.

@rolznz
Copy link
Collaborator Author

rolznz commented Jan 27, 2025

I'm basically asking because I am bit confused as to how it works in tandem with the nip47 spec. After I looked at the alby implementation I actually had the intention to ask this question before we got to conversing about this apporoach.

From the spec I have come to understand that the secret is generated by the wallet to be connected, along with other parameters. I was under the impression that the generated secret that is used to decrypt messages sent to the relay is how the relay confirms the validity of the strings.

Hi, there is no spec yet to cover the deep-link flow. Alby Hub has documented the properties it supports here: https://github.com/getAlby/hub/?tab=readme-ov-file#client-created-secret but we would love to add this to NIP-47 (along with the messaging I mentioned so the wallet service can send the relay and wallet service pubkey back to the app) once CoinOS supports it, so we can confirm it's the best possible flow for different wallet services. Awesome to hear CoinOS will support multiple connections @asoltys

So how does messages to the relay with a locally generated secret work. is this in agreement with nip47 spec or another implelmentation? a Bit of an explanation will be greatly appreciated @rolznz

This is not part of the spec - the mutiny guys did a PR here which could be included as part of the above changes. nostr-protocol/nips#792

To be clear, the communication does not change. It's just where the app secret is generated. It's more secure for the app secret to never be communicated, which is why we think the app should generate its own secret and only give its pubkey to the wallet service. @asoltys does this work on the CoinOS side? in Alby Hub we do not store app secret keys, only app pubkeys. (Edit: the PR above still allows for the wallet service to override the secret if necessary)

@rolznz
Copy link
Collaborator Author

rolznz commented Jan 31, 2025

Update: we changed the nwc success events to explicitly pass the relay URL and wallet service pubkey, rather than a NWC URL (with no secret).

This makes it more clear that the app private key is app-generated and never communicated with the wallet service. This means wallet services MUST support the client-created app secret flow in order to support the deeplink flow we are proposing for bitcoin connect.

    // dispatch a success event which can be listened to by the opener or by the app that embedded the webview
    // this gives those apps the chance to know the user has enabled the connection
    const nwcEvent = new CustomEvent("nwc:success", {
      detail: {
        relayUrl: createAppResponse.relayUrl,
        walletPubkey: createAppResponse.walletPubkey,
      },
    });
    window.dispatchEvent(nwcEvent);

    // notify the opener of the successful connection
    if (window.opener) {
      window.opener.postMessage(
        {
          type: "nwc:success",
          relayUrl: createAppResponse.relayUrl,
          walletPubkey: createAppResponse.walletPubkey,
        },
        "*"
      );
    }

Alby Hub PR: getAlby/hub#1009

@rolznz
Copy link
Collaborator Author

rolznz commented Feb 19, 2025

@Dayvvo This flow has been implemented for Alby Cloud now (PR here: #267)

@rolznz
Copy link
Collaborator Author

rolznz commented Feb 28, 2025

@asoltys @Dayvvo awesome that this was added in coinos. We are just finalizing the 1-click connection flow for mobile/self-hosted wallets and then we will prepare a new release.

Just one issue I noticed is that the max_amount is received as coinos as sats rather than millisats. We just fixed this in Alby Hub so it's consistent with the NIP-47 spec (millisats is always the unit). can this be updated inside coinos?

Example: this should be 100k sats https://coinos.io/apps/new?name=Test&max_amount=100000000

@asoltys
Copy link

asoltys commented Feb 28, 2025

Yep I'll change it tomorrow

@asoltys
Copy link

asoltys commented Feb 28, 2025

Fixed! coinos/coinos-ui@8f2d058

@rolznz
Copy link
Collaborator Author

rolznz commented Mar 1, 2025

Thanks @asoltys !

@rolznz
Copy link
Collaborator Author

rolznz commented Mar 3, 2025

I've made a PR to update the NWC spec to include the parameters and general flow for initiating a connection with a client-created secret. In case either of you have time I would love a review: nostr-protocol/nips#1818

@asoltys I saw you also recently created a Android app, so the Nostr flow I am proposing (and/or the linked "NWC Deep Links" one) might be of interest.

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 a pull request may close this issue.

3 participants