feat: implement stablesats for galoy connector #2730 #3020

riccardobl · 2024-02-08T00:26:28Z

Describe the changes you have made in this PR

This is my submission for the galoy bounty GaloyMoney/blink#3950 and feature request #2730
The PR adds support for stable sats in the galoy connector.

Allow users to select their account during account addition
Change the connector to support fiat-denominated account configs
Change the requests to the GraphQL endpoint so they use the right wallet

Let me know if i missed something

Link this PR to an issue [optional]

Implements #2730

Type of change

feat: New feature (non-breaking change which adds functionality)

Screenshots of the changes [optional]

How has this been tested?

Connect a Blink account
When prompted select the USD (Stablesats) wallet
Send and receive stablesats to and from another lightning wallet

Checklist

reneaaron · 2024-02-09T09:37:25Z

@openoms May I ask you for a quick review? I am also planning to test / review it during the next few days.

openoms · 2024-02-10T20:31:39Z

Thank you for the submission @riccardobl

The wallet choice on the connector screen looks good and is working, I got my Stablesats balance display correctly.

There are two main things would need to be changed to deal with Stablesats correctly:

The amount of the transactions need to be displayed in USD as the main unit because with Stablesats the USD value is stable and the sats number is not (~$0.01 suggest the other way around).

Going next to the receive screen the Amount needs to be denominated in USD cents.

The Stablesats wallet has USD cent as the base unit so smaller than 1 cent sat values are not valid.
When I input 11 sats the actual invoice created is for 21 sats (1 USD cent).
Using satoshis here results in inaccuracies, would need to input Amount in USD ...

See the examples of these distinctions in the Blink wallet app or here: https://dev.blink.sv/api/usd-ln-receive#generate-a-stablesats-invoice

riccardobl · 2024-02-10T23:16:58Z

@openoms To address the points as requested, this PR needs to change several things in the alby core, i've already prepared a draft commit that implements these changes 4e5d52d , I'd like a feedback from the Alby team, @reneaaron, regarding the overall concept of these changes, if we reach a consensus, I'll refine the commit to align with the code quality of the repository.

These are the screenshot after the last commit:

riccardobl · 2024-02-11T12:41:32Z

I've cleaned and refactored the proposal with the goal of reducing the number of changes required to the Alby code.

The points that are touched outside of the galoy connector are:

Define a balance field currencyPrecision that goes together with currency and represents the number of decimals for values, it defaults as 0 for sats and is set to 2 by the galoy connector
Add a configurable step field to DuelCurrencyField to allow floating point inputs
Use getFormattedInCurrency instead of getFormattedSats to display the values
Disable fiat conversion when currency != BTC, it is not clear to me if fiat2fiat conversion is wanted or worth to implement
Change "Amount in satoshi" with "Amount ($CURRENCY)" when currency != BTC

I've forced pushed the refactored commit to the pr branch: 95291ac

openoms · 2024-02-12T18:17:21Z

The history, balance and the invoice is now correctly denominated in USD, all working well. The Stablesats implementation is complete from my point of view.

reneaaron

Thanks very much for this PR! 🙌

While providing a first-class USD experience for users is a great goal to strive for, it poses quite some problems that might not seem obvious in the first place and add a lot of complexity:

The concept of sats is deeply embedded in the wallet and outside APIs (such as WebLN). Changing e.g. the receive screen to use USD as the primary currency will only work for when a user creates an invoice manually, however it will break contracts with WebLN as these APIs only knows sats and no fiat currencies. (see webln.makeInvoice() for instance)

I wonder how we should deal with this problem 🤔 Possibly you could also add an API on Galoy side that allows to create USD invoices by using sats as the unit? (and convert it internally using the current exchange rate)

FYI: We are about to improve the invoice creation for fiat currencies in #2511 which would improve the experience for stablesats accounts as users would be able to enter USD amounts, too. (the converted sats amount could be submitted to the API then)

For now I would prefer to keep the changes related to different UI components (ReceiveInvoice, TransactionTable, TransactionModal, etc) to an absolute minimum. I assume users will know they connected their USD denominated wallet and I am afraid changing primary units all across the application will add quite a lot of complexity when applied to all affected parts (e.g. budgets, etc)

src/app/screens/connectors/ConnectGaloy/index.tsx

src/extension/background-script/connectors/galoy.ts

riccardobl · 2024-02-14T09:25:51Z

I wonder how we should deal with this problem 🤔 Possibly you could also add an API on Galoy side that allows to create USD invoices by using sats as the unit? (and convert it internally using the current exchange rate)

I think the vast majority of use cases are fine with doing the sats -> usd conversion in the galoy connector before creating the invoice, like i did in the first commit (then removed since it wasn't needed after changing the receive page with usd based input), i can revert back to this behavior and then #2511 will provide the usd input as asked by @openoms

For now I would prefer to keep the changes related to different UI components (ReceiveInvoice, TransactionTable, TransactionModal, etc) to an absolute minimum

Since there is already the concept of currencies for balances, would you be fine with keeping the change only for transactions? If the connector uses sats internally, it should be just a visual change.
If having transaction.totalAmount being USD denominated is a problem or introduces inconsistencies, i can add a new field "transaction.displayAmount" that, if set, overrides the totalAmount graphically in the UI so that connectors can display whatever they want in the transaction amount, while keeping it sat denominated internally.

Would one of these two options qualify as a valid solution?

openoms · 2024-02-16T19:02:34Z

Thank you for the review @reneaaron!

Having the ability to create invoices priced in USD like discussed in #2511 could work as the main problem is the different accuracy of sats vs cents resulting in rounding on conversion and inconsistency in the amounts (noted in my first comment). #2511 (with the USD invoice creation) would become the dependency of this PR.

The fixed USDcent denomination would need to be kept for the transaction history, would be happy with either of solutions offered by @riccardobl as the Alby maintainers see fit.

reneaaron

Just did another round of testing & reviews, great progress and clean implementation so far! 👌

The send page currently doesn't work as it's checked against the USD balance: 🤔

webln.makeInvoice currently leads to quite strange behavior due to different rates being used for the calculation

makeInvoice(210000) led to an invoice of 209742 sats. Do you know if there is a way to use the galoy exchange rates or denominate an invoice in sats for the USD account?

You can use this page for testing:
https://webln.twentyuno.net/invoice

reneaaron · 2024-02-26T15:31:15Z

src/extension/background-script/connectors/galoy.ts

+    let amountSats = Number(args.amount);
+    if (currency !== "BTC") {
+      const rate = await getCurrencyRateWithCache(CURRENCIES[currency]);
+      amountSats = this.toFiatInt(amountSats * rate, currency);


toFiatInt() is a bit misleading here as it's converting from fiat to bitcoin, right?

Maybe fiatToSats() is better here?

This is actually transforming a float $ value to an int cents value. So maybe something like this would be better:
toFiatCents
fromFiatCents

src/extension/background-script/connectors/galoy.ts

riccardobl · 2024-02-26T17:56:20Z

The send page currently doesn't work as it's checked against the USD balance: 🤔

this seems like a bug in alby not in this pr necessarily

…d usd invoices, fix naming

riccardobl · 2024-02-26T18:01:01Z

makeInvoice(210000) led to an invoice of 209742 sats. Do you know if there is a way to use the galoy exchange rates or denominate an invoice in sats for the USD account?

It should be fixed in the last commit, i've changed the code to use lnUsdInvoiceBtcDenominatedCreateOnBehalfOfRecipient that is either a new api or i didn't noticed it before.

reneaaron

tACK

src/app/screens/Home/AllowanceView/index.tsx

reneaaron · 2024-03-14T10:02:34Z

I think this is a great start! 💯 Thanks everyone involved! We're now prioritizing #2511 to improve the integration further.

@riccardobl You have done a great job in here, thanks for your detailed analysis and thought-through suggestions on GitHub & Discord. Really appreciate your structured way of working! 💪 If you are up for it, we'd love to get your help on #2511 as well.

riccardobl · 2024-03-15T17:20:23Z

I think this is a great start! 💯 Thanks everyone involved! We're now prioritizing #2511 to improve the integration further.

@riccardobl You have done a great job in here, thanks for your detailed analysis and thought-through suggestions on GitHub & Discord. Really appreciate your structured way of working! 💪 If you are up for it, we'd love to get your help on #2511 as well.

Thank you, it was a pleasure to contribute to the project.
I'll start looking into #2511 and see if i can open a draft for a possible implementation

openoms · 2024-03-18T17:13:21Z

Tested in the main branch now and the changes work as expected, ticking our bounty requirements which now can be considered complete: GaloyMoney/blink#3950 (reply in thread)

Thank you very much for the work @riccardobl and the reviews @reneaaron!

Looking forward to working together further on maintaining and improving this integration as already proposed in #2511

reneaaron added the next-release To be included in the next release label Feb 8, 2024

riccardobl force-pushed the stablegaloy branch 2 times, most recently from 4e5d52d to 95291ac Compare February 11, 2024 12:39

reneaaron reviewed Feb 13, 2024

View reviewed changes

riccardobl added a commit to riccardobl/lightning-browser-extension that referenced this pull request Feb 20, 2024

feat: implement stablesats for galoy connector getAlby#2730 getAlby#3020

3f16ef5

riccardobl force-pushed the stablegaloy branch from 7905aec to b7cad44 Compare February 22, 2024 20:40

feat: implement stablesats for galoy connector getAlby#2730

e91d466

riccardobl force-pushed the stablegaloy branch from b7cad44 to e91d466 Compare February 22, 2024 20:43

reneaaron requested changes Feb 26, 2024

View reviewed changes

fix: galoy connector defaults to BTC currency, create sats denominate…

6712709

…d usd invoices, fix naming

fix: disable amount check for non btc balances

cd0a67c

riccardobl force-pushed the stablegaloy branch from 5209d2d to cd0a67c Compare March 6, 2024 10:52

riccardobl requested a review from reneaaron March 6, 2024 10:53

reneaaron and others added 5 commits March 8, 2024 09:48

Merge branch 'master' into stablegaloy

5a6f8d5

Merge branch 'master' into stablegaloy

9b563d5

Merge branch 'master' into stablegaloy

60e1fcd

fix: remove obsolete code

9d3e8de

fix: create helper for parsing invoices

8ae5ed3

reneaaron approved these changes Mar 14, 2024

View reviewed changes

src/app/screens/Home/AllowanceView/index.tsx Outdated Show resolved Hide resolved

Merge branch 'master' into stablegaloy

a44c81c

Merge branch 'master' into stablegaloy

a23a25e

reneaaron enabled auto-merge (squash) March 14, 2024 10:03

reneaaron merged commit 186ec4b into getAlby:master Mar 14, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement stablesats for galoy connector #2730 #3020

feat: implement stablesats for galoy connector #2730 #3020

riccardobl commented Feb 8, 2024 •

edited

reneaaron commented Feb 9, 2024 •

edited

openoms commented Feb 10, 2024

riccardobl commented Feb 10, 2024 •

edited

riccardobl commented Feb 11, 2024 •

edited

openoms commented Feb 12, 2024 •

edited

reneaaron left a comment

riccardobl commented Feb 14, 2024 •

edited

openoms commented Feb 16, 2024

reneaaron left a comment

reneaaron Feb 26, 2024

riccardobl Feb 26, 2024

riccardobl commented Feb 26, 2024

riccardobl commented Feb 26, 2024 •

edited

reneaaron left a comment

reneaaron commented Mar 14, 2024

riccardobl commented Mar 15, 2024

openoms commented Mar 18, 2024

feat: implement stablesats for galoy connector #2730 #3020

feat: implement stablesats for galoy connector #2730 #3020

Conversation

riccardobl commented Feb 8, 2024 • edited

Describe the changes you have made in this PR

Link this PR to an issue [optional]

Type of change

Screenshots of the changes [optional]

How has this been tested?

Checklist

reneaaron commented Feb 9, 2024 • edited

openoms commented Feb 10, 2024

riccardobl commented Feb 10, 2024 • edited

riccardobl commented Feb 11, 2024 • edited

openoms commented Feb 12, 2024 • edited

reneaaron left a comment

Choose a reason for hiding this comment

riccardobl commented Feb 14, 2024 • edited

openoms commented Feb 16, 2024

reneaaron left a comment

Choose a reason for hiding this comment

reneaaron Feb 26, 2024

Choose a reason for hiding this comment

riccardobl Feb 26, 2024

Choose a reason for hiding this comment

riccardobl commented Feb 26, 2024

riccardobl commented Feb 26, 2024 • edited

reneaaron left a comment

Choose a reason for hiding this comment

reneaaron commented Mar 14, 2024

riccardobl commented Mar 15, 2024

openoms commented Mar 18, 2024

riccardobl commented Feb 8, 2024 •

edited

reneaaron commented Feb 9, 2024 •

edited

riccardobl commented Feb 10, 2024 •

edited

riccardobl commented Feb 11, 2024 •

edited

openoms commented Feb 12, 2024 •

edited

riccardobl commented Feb 14, 2024 •

edited

riccardobl commented Feb 26, 2024 •

edited