frontend: update 'My portfolio' with lightning account #2703

strmci · 2024-05-07T10:49:52Z

This commit adds functionality to display Lightning account
in 'My portfolio' in cases where it is active but not associated
with a connected or remembered wallet, or when all mainnet
accounts in the connected wallet are disabled.

The total coins table is updated to include the Lightning account,
and the Lightning configuration now also contains the keystore
name. Hide chart in my portfolio when only lightning account is
enabled.

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

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

Beerosagos · 2024-05-20T13:54:32Z

Here I found the keystore (c6d..) repeated twice, during my test

Beerosagos · 2024-05-20T13:28:53Z

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

+  const hasLightningFromOtherKeystore = (
+    lightningConfig.accounts.length !== 0
+    && (
+      accountsByKeystore.length === 0


I think that you can avoid checking the length, it should be handled by the some condition anyway, right?

Yes, this check is redundant here, removed.

Beerosagos · 2024-05-20T13:31:51Z

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

+    )
+  );
+
+  const showTotalCoins = accountsByKeystore.length > 1 || (hasLightningFromOtherKeystore && accountsByKeystore.length === 1);


Nit: what about doing smth like

let keystores = accountsByKeystore.length; if (hasLightningFromOtherKeystore) { keystores += 1; }

and check for keystores > 1 below?

I feel like it would be more readable

I updated this to use keystores

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

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

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

strmci · 2024-06-24T15:50:18Z

@Beerosagos
I cherry-picked the total coins refactor from master and updated this PR.

Here I found the keystore (c6d..) repeated twice, during my test

Also, this should be resolved now.

PTAL

thisconnect · 2024-06-25T06:09:47Z

There was some refactoring on master, maybe consider merge master->staging-ln before this?

Beerosagos · 2024-06-25T07:24:35Z

There was some refactoring on master, maybe consider merge master->staging-ln before this?

Yes, I agree 👍

Beerosagos · 2024-06-26T15:33:11Z

merge master > staging-ln PR: #2782

strmci · 2024-07-03T11:26:31Z

@Beerosagos rebased
PTAL

Beerosagos

Left a couple comments.

Also, I noticed that if the only active account is the lightning one, then the chart in the account summary is empty. Not sure what we want to do with this, the easier is probably to hide it, at least for now. wdyt @thisconnect ?

Beerosagos · 2024-07-04T09:57:49Z

backend/handlers/handlers.go

+			return nil, err
+		}
+		totalCoinsBalances[coin.CodeBTC] = lightningBalance.Available().BigInt()
+		sortedCoins = append(sortedCoins, coin.CodeBTC)


This potentially breaks the coin sorting, which should be derived by the accounts sorting. The LN balance handling should be done together with the BTC accounts balance

Beerosagos · 2024-07-04T10:17:40Z

backend/config/lightning.go

@@ -26,6 +26,8 @@ type LightningAccountConfig struct {
 	Mnemonic string `json:"mnemonic"`
 	// RootFingerprint is fingerprint of the keystore that generated the entropy.
 	RootFingerprint jsonp.HexBytes `json:"rootFingerprint"`
+	// KeyStoreName is name of the keystore.


I realized that storing the keystore name could lead to issues when the device name is changed by the user.
I didn't look deeply into this, but I think that the proper way of doing it could be to get the persisted keystore info and retrieve the name there (see backend.Config().AccountsConfig()), but we may need to actively persist the current keystore when enabling the LN account).

cc @benma wdyt?

thisconnect · 2024-07-16T09:35:22Z

Also, I noticed that if the only active account is the lightning one, then the chart in the account summary is empty. Not sure what we want to do with this, the easier is probably to hide it, at least for now. wdyt @thisconnect ?

ack to hiding the chart for now if there is only lightning. But it would be nice in a future PR to have the chart also is there if there is only the lightining account.

This commit adds functionality to display Lightning account in 'My portfolio' in cases where it is active but not associated with a connected or remembered wallet, or when all mainnet accounts in the connected wallet are disabled. The total coins table is updated to include the Lightning account, and the Lightning configuration now also contains the keystore name. Hide chart in my portfolio when only lightning account is enabled.

strmci · 2024-07-30T16:29:16Z

@Beerosagos

as discussed, moved the addition of the lightning balance after the accounts iteration and added Bitcoin to the beginning of sortedCoins if it is not already there
hid the chart in my portfolio if only the lightning account is active
updated to use Config().AccountsConfig().LookupKeystore to get the lightning account keystore name

Currently, we use the isAmbiguousName function to determine the table title for watch-only accounts. We could apply a similar approach for lightning account from other keystore. For now, if there’s an active lightning account from another keystore, the table title will be displayed as "keystoreName (rootFingerprint)".

PTAL

Beerosagos · 2024-08-01T14:05:01Z

backend/handlers/handlers.go

+		keystoreName := ""
+		lightningKeystore, err := handlers.backend.Config().AccountsConfig().LookupKeystore(account.RootFingerprint)
+		if err == nil {
+			keystoreName = lightningKeystore.Name


This is an interesting solution, but I think it has a defect: if the keystore device is renamed, the config in the frontend is not updated bc it is served by the provider. That means the keystore name won't be updated until the app is restarted. I think it would be better to add a backend endpoint keystore-name that given a rootfingerprint looks for a keystore in the config and returns the name.

There is also a more latent bug, which depends on the watch-only implementation: when the device is renamed, the keystore name in the config file is not updated until the device is disconnected and reconnected again. But this deserves its own PR as it is out of scope here, imo.

cc @benma

Beerosagos · 2024-08-01T14:39:05Z

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

@@ -158,3 +158,55 @@ export const SummaryBalance = ({
    </div>
  );
 };
+
+
+export function LightningBalance() {


There is some code duplication here, I think that it would be nice to move the logic to subscribe and fetch the LN balance inside accountsummary.tsx and pass it to the LightningBalance and SummaryBalance components. wdyt?

Beerosagos · 2024-08-01T14:41:57Z

Also, I noticed that when the chart is hidden, the week/month/.. filter buttons are still visible (disabled). They should be hidden too, I think.

Beerosagos · 2024-08-01T14:43:49Z

Currently, we use the isAmbiguousName function to determine the table title for watch-only accounts. We could apply a similar approach for lightning account from other keystore. For now, if there’s an active lightning account from another keystore, the table title will be displayed as "keystoreName (rootFingerprint)".

Yeah, I agree 👍

strmci force-pushed the lightning_to_my_portfolio branch 2 times, most recently from 4a1bd10 to 3db7734 Compare May 8, 2024 12:52

strmci requested a review from thisconnect May 8, 2024 13:15

thisconnect reviewed May 8, 2024

View reviewed changes

frontends/web/src/routes/account/summary/accountssummary.tsx Outdated Show resolved Hide resolved

strmci force-pushed the lightning_to_my_portfolio branch 2 times, most recently from 1dd6252 to 571341f Compare May 10, 2024 06:43

thisconnect reviewed May 10, 2024

View reviewed changes

frontends/web/src/routes/account/summary/accountssummary.tsx Show resolved Hide resolved

thisconnect requested a review from Beerosagos May 10, 2024 07:05

thisconnect requested changes May 10, 2024

View reviewed changes

strmci force-pushed the lightning_to_my_portfolio branch from 571341f to 7e6de8d Compare May 10, 2024 07:44

thisconnect reviewed May 10, 2024

View reviewed changes

frontends/web/src/routes/account/summary/coinbalance.tsx Outdated Show resolved Hide resolved

Beerosagos requested changes May 20, 2024

View reviewed changes

thisconnect requested changes May 21, 2024

View reviewed changes

frontends/web/src/routes/account/summary/summarybalance.tsx Outdated Show resolved Hide resolved

frontends/web/src/routes/account/summary/coinbalance.tsx Outdated Show resolved Hide resolved

strmci force-pushed the lightning_to_my_portfolio branch 2 times, most recently from 547408d to 2e0ac00 Compare June 24, 2024 15:42

strmci force-pushed the lightning_to_my_portfolio branch 3 times, most recently from e11b223 to 3aed2f7 Compare July 3, 2024 10:27

Beerosagos requested changes Jul 4, 2024

View reviewed changes

strmci force-pushed the lightning_to_my_portfolio branch 4 times, most recently from 0853c5d to 5b11da2 Compare July 30, 2024 15:39

strmci force-pushed the lightning_to_my_portfolio branch from 5b11da2 to a81ef45 Compare July 30, 2024 16:21

Beerosagos requested changes Aug 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

frontend: update 'My portfolio' with lightning account #2703

frontend: update 'My portfolio' with lightning account #2703

strmci commented May 7, 2024 •

edited

Loading

Beerosagos commented May 20, 2024

Beerosagos May 20, 2024

strmci Jun 24, 2024

Beerosagos May 20, 2024

strmci Jun 24, 2024

strmci commented Jun 24, 2024 •

edited

Loading

thisconnect commented Jun 25, 2024 •

edited

Loading

Beerosagos commented Jun 25, 2024

Beerosagos commented Jun 26, 2024

strmci commented Jul 3, 2024 •

edited

Loading

Beerosagos left a comment

Beerosagos Jul 4, 2024

Beerosagos Jul 4, 2024

thisconnect commented Jul 16, 2024

strmci commented Jul 30, 2024 •

edited

Loading

Beerosagos Aug 1, 2024

Beerosagos Aug 1, 2024

Beerosagos commented Aug 1, 2024

Beerosagos commented Aug 1, 2024

frontend: update 'My portfolio' with lightning account #2703

Are you sure you want to change the base?

frontend: update 'My portfolio' with lightning account #2703

Conversation

strmci commented May 7, 2024 • edited Loading

Beerosagos commented May 20, 2024

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

strmci commented Jun 24, 2024 • edited Loading

thisconnect commented Jun 25, 2024 • edited Loading

Beerosagos commented Jun 25, 2024

Beerosagos commented Jun 26, 2024

strmci commented Jul 3, 2024 • edited Loading

Beerosagos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thisconnect commented Jul 16, 2024

strmci commented Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Beerosagos commented Aug 1, 2024

Beerosagos commented Aug 1, 2024

strmci commented May 7, 2024 •

edited

Loading

strmci commented Jun 24, 2024 •

edited

Loading

thisconnect commented Jun 25, 2024 •

edited

Loading

strmci commented Jul 3, 2024 •

edited

Loading

strmci commented Jul 30, 2024 •

edited

Loading