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

fix: Blink connector to use the API key from the Blink Dashboard #2887

Merged
merged 12 commits into from
Dec 11, 2023

Conversation

openoms
Copy link
Contributor

@openoms openoms commented Nov 24, 2023

Fix the problem that the current method to connect the Blink wallet does not work.
Change the header to use the new method for the API key
Add instructions to the connector page about generating the API key.
Remove translations referencing the deprecated method of log in through the discontinued web wallet.
Use the staging endpoint of a galoy_staging_ API key is used.
Fix the checkPayment function

Fixes #2881

  • fix: Bug fix (non-breaking change which fixes an issue)

Screenshots:

image

Screenshot from 2023-11-24 17-57-32

image

image

How has this been tested?

Tested manually through building the extension, connected to the Galoy staging and Blink mainnet.
Generating an invoice, checking if received and sending to LN invoice functions are working.

Known remaining issues (to be addressed separately):

  • getTransactions() is not yet supported with the currently used account: Galoy
  • default Stablesats wallet is not supported for generating invoices
  • cannot pay a signet LN invoice (lntbs..): Unknown coin bech32 prefix

Checklist

  • Self-review of changed code
  • Manual testing
  • Update Docs & Guides (built in to the connector page)

@openoms openoms force-pushed the fix/-use-X-API-KEY-header-for-Blink branch from d55ff92 to 1c7aa64 Compare November 24, 2023 17:10
@rolznz
Copy link
Contributor

rolznz commented Nov 26, 2023

Hi @openoms

I tried this out and managed to successfully pay and receive lightning invoices. But I seem to have no history:

image

Also, what happens if the user's token is invalid (e.g. an old jwt token or a new API token that has expired, especially since the default is 90 days now). Do they just receive an error message and have to delete their extension account and add a new one? (This becomes more problematic as users have to migrate e.g. their master key / nostr key. For example, the Alby connector will allow you to re-login if your auth token has expired)

@openoms openoms force-pushed the fix/-use-X-API-KEY-header-for-Blink branch from e31b93d to 6ed7201 Compare November 28, 2023 15:04
@openoms
Copy link
Contributor Author

openoms commented Nov 28, 2023

Thank you for the feedback!
Rebased to integrate the latest changes in master.

Implemented and tested the transaction history now (returns the last 100 transactions max):
image

In Blink there are two balances: BTC and Stablesats (synthetic USD)
Changed that there is only the BTC wallet is used as Alby is expecting the balances in sats and Stablesats is denominated in USDcents. Using a Stablesats wallet would need bigger changes involving other parts of Alby as well. Would prefer to discuss this separately.

Agree that it is important to be able to relogin to the same account so I am testing the workflow of the token expiration next.

Note: since the connector is also used for Bitcoin Jungle checked for compatibility. Bitcoin Jungle only has BTC denomination: https://github.com/Bitcoin-Jungle/galoy/blob/3c31dec0728b0ab3484ee5cecbdf2daadc5467a4/src/graphql/main/schema.graphql#L857 so the changes should not affect it. However I cannot test as the wallet does not expose the authentication token any more.

edit: had an old connection with Bitcoin Jungle so could extract the key and testing.

@openoms
Copy link
Contributor Author

openoms commented Dec 4, 2023

After discussing the various possibilities of dealing with the expired API key came to the conclusion of simply creating a longer (or non expiring) API key would be the best trade-off.

If the user would have other services (like Nostr) connected to the account they should can still migrate with the Master Key.

The Blink Dashboard will have the option to select a non-expiring key as well so added the relevant notes to the connector screen:

image

Please let me know if there are more changes expected.

@rolznz
Copy link
Contributor

rolznz commented Dec 4, 2023

@openoms I tried to test it again using the account I created before and I get this now:
image

I am pretty sure I made the API key with the default expiry.

Now I tried to login to my blink account again (I used phone number to sign up) but I cannot seem to login using that form any more. Can you help me out?

image

@openoms
Copy link
Contributor Author

openoms commented Dec 4, 2023

@rolznz was your old account logged in with and API key (blink_..)? It does not work any more with ory_.... In case of the API key you will be able to check if it is expired in the dashboard.

You should be able to log in to the dashboard with the number again if you click on Sign in with phone. If you are logged in on a phone or in the dashboard you can add an email address which then you can use to log in as well.

I have run a couple new rounds of tests manually and still working.

@rolznz
Copy link
Contributor

rolznz commented Dec 5, 2023

@rolznz was your old account logged in with and API key (blink_..)? It does not work any more with ory_.... In case of the API key you will be able to check if it is expired in the dashboard.

You should be able to log in to the dashboard with the number again if you click on Sign in with phone. If you are logged in on a phone or in the dashboard you can add an email address which then you can use to log in as well.

I have run a couple new rounds of tests manually and still working.

@openoms I only created the API key when testing this PR, so it should still be valid.

But I cannot login with my phone number (starts with +666) as the input at https://dashboard.blink.sv/ only seems to allows logging in with email. My phone number doesn't pass the regex validation. Without entering anything in the field, it asks me to fill it out before continuing.

How do I login with phone number?

Copy link
Contributor

@rolznz rolznz left a comment

Choose a reason for hiding this comment

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

tACK

@reneaaron
Copy link
Contributor

Thanks for your PR @openoms! Just tested it and worked like a charm!

I noticed one minor detail:
Incoming transactions don't seem to yield the memo (that's why every entry in the transaction list just shows "received"). Do you know why that could be the case? 🤔

Existing users that use the Blink with the current implementation will see errors after the upgrade:
image

I don't see an easy path to migrate them. I guess it shouldn't be too many users though...

@reneaaron
Copy link
Contributor

Changed that there is only the BTC wallet is used as Alby is expecting the balances in sats and Stablesats is denominated in USDcents. Using a Stablesats wallet would need bigger changes involving other parts of Alby as well. Would prefer to discuss this separately.

+1 for tackling that in a separate PR, we already have the foundation to support fiat-denominated accounts (was introduced quite some time back for the Kollider connector). However, in Alby they would have 3 separate accounts for that (BTC, USD, EUR) as we don't have any hierarchy for wallets yet. (users select which currency they want to use at the time of connecting their account)

If you want to get a first idea of how that could work take a look at the Kollider connector:
https://github.com/getAlby/lightning-browser-extension/blob/master/src/extension/background-script/connectors/kollider.ts#L65

Happy to collaborate and answer any questions you might have. 🙌

@openoms
Copy link
Contributor Author

openoms commented Dec 5, 2023

Thank you for the review @reneaaron!

I also found an outstanding edge case where the checkPayment doesn't work if the user sets Stablesats as their default wallet. Will fix this in the next commit and look at the option to preserve the ory_... tokens.

The memo issue is known and needs a fix in the backend: GaloyMoney/blink#2449 which when done should show up correctly in Alby.

Thank you for the clue about following the Kollider connector for Stablesats, will do that and will ask the related question separately.

@openoms
Copy link
Contributor Author

openoms commented Dec 7, 2023

Now the connector is tested to be able to preserve the previously connected Blink and Bitcoin Jungle accounts with different auth tokens.

There has been a change in the Blink Dashboard to offer API keys with no expiry by default:
image

This has been reflected in the connect page:
image

Tested receiving, checkPayment, sending and history for all accounts again.

@openoms
Copy link
Contributor Author

openoms commented Dec 8, 2023

As a last cosmetic change removed the now inappropriate alpha warning and displaying a note about using only the BTC wallet for now (as the scope of the PR):
image

@openoms openoms force-pushed the fix/-use-X-API-KEY-header-for-Blink branch from 5356be0 to dc7e961 Compare December 11, 2023 10:07
@openoms
Copy link
Contributor Author

openoms commented Dec 11, 2023

Rebased on the latest master, awaiting the checks.

@reneaaron
Copy link
Contributor

@openoms Just did a final round of testing and the legacy accounts now work as well! Thanks for the PR! 💯

@reneaaron reneaaron merged commit 2e31bcf into getAlby:master Dec 11, 2023
7 of 8 checks passed
@rolznz
Copy link
Contributor

rolznz commented Dec 20, 2023

@openoms I didn't notice in this PR (these lines were not changed) but @bumi mentioned sendPayment in this connector does not return the preimage. Can this be added?

Also I noticed there it also will return without a preimage if the payment is in "PENDING" state. But lightning payments should technically be instant in most cases, right? or is this for HODL invoices?

image

CC @reneaaron

@rolznz
Copy link
Contributor

rolznz commented Dec 20, 2023

Update: I saw Rene made an issue here: #2946

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.

[Feature] Switch to the new API key scheme for Blink wallet
3 participants