Lightning fiat #2574

Beerosagos · 2024-02-26T09:59:54Z

This is a first PR to start adding Fiat conversion support for lightning.
Fiat conversions for LN send/receive will be addressed in a separate PR.

frontends/web/src/api/lightning.ts

Beerosagos · 2024-02-26T10:11:06Z

Requested reviews from both @benma and @thisconnect for backend/frontend 😇 🙏 thanks a lot

thisconnect

tested a bit, looks very nice. added some comments.

frontends/web/src/api/lightning.ts

thisconnect · 2024-02-26T11:28:01Z

frontends/web/src/routes/account/summary/accountssummary.tsx

@@ -217,7 +216,7 @@ export function AccountsSummary({ accounts, devices }: TProps) {
                  totalBalancePerCoin={ balancePerCoin ? balancePerCoin[keystore.rootFingerprint] : undefined}
                  totalBalance={ accountsTotalBalance ? accountsTotalBalance[keystore.rootFingerprint] : undefined}
                  balances={balances}
-                  lightningNodeState={(lightningConfig.accounts.length && lightningConfig.accounts[0].rootFingerprint === keystore.rootFingerprint) ? nodeState : undefined}
+                  lightningBalance={(lightningConfig.accounts.length && lightningConfig.accounts[0].rootFingerprint === keystore.rootFingerprint) ? lightningBalance : undefined}


This component doesn't really need lightningConfig and lightningBalance as those are only used in SummaryBalance component, wdyt about moving as much s possible into SummaryBalance?

you'll probably have to pass keystore.rootFingerprint as well somehow for the condition.

yeah, I moved the LN logic inside the component and refactored a bit to only pass it the keystore struct, instead of various fields belonging to it.

frontends/web/src/api/lightning.ts

benma · 2024-02-26T13:06:56Z

backend/lightning/handlers.go

+func (lightning *Lightning) GetBalance(_ *http.Request) interface{} {
+	balance, err := lightning.Balance()
+	if err != nil {
+		return responseDto{Success: false, ErrorMessage: err.Error()}


what does Dto mean? man i really dislike abbreviations lol

It's Data Transfer Object. It's quite used as an abbreviation, afaik.

benma · 2024-02-26T13:11:26Z

backend/lightning/lightning.go

@@ -154,13 +167,51 @@ func (lightning *Lightning) Deactivate() error {
 	return nil
 }

+// AssertActive returns an error if the lightning service not has been activated.
+func (lightning *Lightning) AssertActive() error {


assert is the wrong word, as assert in all contexts I know (in Python, C, ...), it crashes/panics/exists.

CheckActive or ValidateActive would be better.

benma · 2024-02-26T13:11:47Z

backend/lightning/lightning.go

+}
+
+// Balance returns the balance of the lightning account, together with its conversions to fiat.
+func (lightning *Lightning) Balance() (interface{}, error) {


don't return interface{} but a concrete type

this function should really return *accounts.Balance, like btc/account.go and eth/account.go, and the handler should convert to AccountBalance.

Reason: AccountBalance is frontend focused, and it's the handler's job to convert input/output from/to the frontend.

Agree 👍 done.

Only now I see that the handlers are methods on the same struct :O but the split was still good, as we will hopefully refactor this later and make a clean split 😄

Yeah, we should definitely refactor the lightning handlers to belong to the handlers package, as done in the other places, but didn't want to do it now 😬 So that we can also use handlers.formatAmountAsJSON()

benma · 2024-02-26T13:12:03Z

backend/lightning/lightning.go

+// Balance returns the balance of the lightning account, together with its conversions to fiat.
+func (lightning *Lightning) Balance() (interface{}, error) {
+	if err := lightning.AssertActive(); err != nil {
+		return coin.FormattedAmount{}, err


return nil, err

benma · 2024-02-26T13:12:09Z

backend/lightning/lightning.go

+
+	nodeInfo, err := lightning.sdkService.NodeInfo()
+	if err != nil {
+		return coin.FormattedAmount{}, err


return nil, err

benma · 2024-02-26T13:13:20Z

backend/lightning/lightning.go

+		return coin.FormattedAmount{}, err
+	}
+
+	amount := coin.NewAmount(big.NewInt(int64(nodeInfo.ChannelsBalanceMsat / 1000)))


there is coin.NewAmountFromInt64()

Nice! thanks

benma · 2024-02-26T13:17:32Z

backend/accounts/account.go

@@ -32,6 +32,14 @@ type AddressList struct {
 	Addresses  []Address
 }

+// AccountBalance includes the total and incoming balance of an account.
+type AccountBalance struct {


there is already a Balance struct (in balance.go). making this ultra confusing.

Maybe move this to balance.go too and name it FormattedAccountBalance.

FormattedAmount is needed inside the lightning package. Moving it into the `coin` package allow us to import it there too. Same for AccountBalance, which is explicitly declared inside `accounts`.

Adds a new backend endpoint that allows to fetch the node balance and the relative fiat conversions.

Beerosagos · 2024-02-26T16:08:40Z

Rebased and addressed comments. @benma @thisconnect PTAL 🙏

benma

Thanks, utACK

thisconnect

tested LGTM 👍

Beerosagos force-pushed the lightning-fiat branch from 907a464 to 8481603 Compare February 26, 2024 10:05

Beerosagos requested a review from thisconnect February 26, 2024 10:07

Beerosagos commented Feb 26, 2024

View reviewed changes

frontends/web/src/api/lightning.ts Outdated Show resolved Hide resolved

Beerosagos marked this pull request as ready for review February 26, 2024 10:10

Beerosagos requested a review from benma February 26, 2024 10:10

thisconnect reviewed Feb 26, 2024

View reviewed changes

benma requested changes Feb 26, 2024

View reviewed changes

Beerosagos added 2 commits February 26, 2024 15:41

backend/handlers: refactor balance related structs

ce60ecd

FormattedAmount is needed inside the lightning package. Moving it into the `coin` package allow us to import it there too. Same for AccountBalance, which is explicitly declared inside `accounts`.

backend/lightning: add lightning balance endpoint

40976c9

Adds a new backend endpoint that allows to fetch the node balance and the relative fiat conversions.

Beerosagos force-pushed the lightning-fiat branch from 8481603 to 40976c9 Compare February 26, 2024 16:02

benma approved these changes Feb 26, 2024

View reviewed changes

thisconnect approved these changes Feb 27, 2024

View reviewed changes

Beerosagos merged commit 83836fe into BitBoxSwiss:staging-ln Feb 27, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lightning fiat #2574

Lightning fiat #2574

Beerosagos commented Feb 26, 2024 •

edited

Loading

Beerosagos commented Feb 26, 2024

thisconnect left a comment

thisconnect Feb 26, 2024

Beerosagos Feb 26, 2024

benma Feb 26, 2024

Beerosagos Feb 26, 2024

benma Feb 26, 2024

benma Feb 26, 2024

benma Feb 26, 2024

Beerosagos Feb 26, 2024

benma Feb 26, 2024

Beerosagos Feb 26, 2024

benma Feb 26, 2024

benma Feb 26, 2024

benma Feb 26, 2024

Beerosagos Feb 26, 2024

benma Feb 26, 2024

Beerosagos commented Feb 26, 2024

benma left a comment

thisconnect left a comment

Lightning fiat #2574

Lightning fiat #2574

Conversation

Beerosagos commented Feb 26, 2024 • edited Loading

Beerosagos commented Feb 26, 2024

thisconnect left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Beerosagos commented Feb 26, 2024

benma left a comment

Choose a reason for hiding this comment

thisconnect left a comment

Choose a reason for hiding this comment

Beerosagos commented Feb 26, 2024 •

edited

Loading