chore: sunset unused ChildAccount type #6524

gre · 2024-03-22T14:59:36Z

✅ Checklist

npx changeset was attached. (not needed)
Covered by automatic tests. if it builds, it should be good.
Impact of the changes: none (unused)

📝 Description

We introduced the concept of ChildAccount back in 2020 on the first implementation of Tezos which had this concept of KT Accounts. These not being strictly a token, but a "sub account of tezos", it made sense at the time to diverge from the Token Account.
Since the JS rework of tezos (2021?) as well as Tezos' sunset of kt accounts (delegation now directly happens on the main account), we no longer needed to support KT Accounts. However that type was still forgotten in our type model and became tech debt.

We can now remove it, this makes possible simplifications in the future, you can notably observe that the type SubAccount is now strictly equivalent to TokenAccount.

❓ Context

JIRA or GitHub link: https://ledgerhq.atlassian.net/browse/LIVE-11884

🧐 Checklist for the PR Reviewers

The code aligns with the requirements described in the linked JIRA or GitHub issue.
The PR description clearly documents the changes made and explains any technical trade-offs or design decisions.
There are no undocumented trade-offs, technical debt, or maintainability issues.
The PR has been tested thoroughly, and any potential edge cases have been considered and handled.
Any new dependencies have been justified and documented.
Performance considerations have been taken into account. (changes have been profiled or benchmarked if necessary)

vercel · 2024-03-22T14:59:45Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
web-tools	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 28, 2024 4:53pm

4 Ignored Deployments

Name	Status	Preview	Updated (UTC)
ledger-live-docs	⬜️ Ignored (Inspect)	Visit Preview	Mar 28, 2024 4:53pm
ledger-live-github-bot	⬜️ Ignored (Inspect)	Visit Preview	Mar 28, 2024 4:53pm
native-ui-storybook	⬜️ Ignored (Inspect)	Visit Preview	Mar 28, 2024 4:53pm
react-ui-storybook	⬜️ Ignored (Inspect)	Visit Preview	Mar 28, 2024 4:53pm

sprohaszka-ledger

Thanks for lighten the model :D
I am not fully sure about the WalletAPI part. I'd let @Justkant give you his opinion.

For the rest, I rely on you about Tezos and Solana update (I don't have any knowledge about those chains)

sprohaszka-ledger · 2024-03-26T10:56:22Z

libs/ledgerjs/packages/types-live/src/account.ts

-/** super type that is either a token or a child account */
-export type SubAccount = TokenAccount | ChildAccount;
+/**
+ * deprecated use TokenAccount instead


Use jsdoc tag to highlight it in IDE * @deprecated Use TokenAccount instead

@sprohaszka-ledger unfortunately can't do this due to a bug with documentation that will completely break the README doc generation. i lost half a day trying to understand what was wrong on this. I think @hedi-edelbloute is aware of this problem as it arise in the past too. very unfortunate.

documentationjs/documentation#1596

libs/coin-framework/src/account/helpers.test.ts

sprohaszka-ledger · 2024-03-26T11:26:39Z

apps/ledger-live-mobile/src/screens/Accounts/AccountRow.tsx

@@ -46,7 +46,7 @@ const AccountRow = ({
 useEnv("HIDE_EMPTY_TOKEN_ACCOUNTS");
 const currency = getAccountCurrency(account);
 const parentAccount = useSelector((state: State) =>
- parentAccountSelector(state, { account: account as ChildAccount }),
+ parentAccountSelector(state, { account: account as TokenAccount }),


Why force cast with a new type? The removed one should imply that the account cannot be cast, no?

i'm not familiar with this specific code.
I will create a ticket on Jira to investigate why we need this casting / if it can be fixed, but I think out of scope of the type simplification of this PR I think.
The flaw is in the existing code of develop, it's just that here the new ChildAccount is now TokenAccount, it seems like we loosely expect account is a token account here and we need this cast to not break the typing.

I mean also casting from ChildAccount (which is a specific type) to TokenAccount is quite weird as it is not the same type.

ooh I see what you mean. i think it was a mistake in the past indeed, instead of ChildAccount we meant to do SubAccount.
because the parentAccountSelector takes a SubAccount out there. and the ChildAccount was type matching to be a SubAccount, the problem is that, indeed, casting hide the type issue in this place.

It isn't typed well at this selector side, it could accept an AccountLike so we get rid of this casting

export const parentAccountSelector = createSelector( accountsSelector, (_: State, { account }: { account?: SubAccount }) => (account ? account.parentId : null), (accounts, accountId) => accounts.find(a => a.id === accountId), );

i'm pushing a fix.

(side note: the usecase for such a selector is very weird, we shouldn't have to infer back the parent, it should be provided in the first place... but i'm not going to look at this now)

hedi-edelbloute

I'm scared we someday need to use child account for family accounts with currency subAccounts, if it works i'd keep it

gre · 2024-03-26T16:26:37Z

@hedi-edelbloute to port what we discussed in our DM earlier:
unfortunately we just forgot this tech debt in the past, ChildAccount being sunset since 2021, i'm almost certain that our UI have not survived the successive rework and will not support out of the box this idea from the past. We are likely to have many subtle broken parts if we think we can start reusing this and it's why it's dangerous and that we want to get rid of (similar to freshAddresses being wrong / empty for many implementations, because it's not used by anything). So, it probably would be easier to just reintroduce the concept whenever you re-need it in the future, but i think we have to sunset what is unused first.
More wider thought on the topic, but I think a better way for the future will be to flatten our accounts as much as possible and not try to introduce too much nesting. so many we also will need to double think about the idea to reintroduce this pattern. We have numerous discussion on the fact nesting created us many complexity on our current stack.

mcayuelas-ledger · 2024-03-28T15:14:39Z

libs/ledger-live-common/src/csvExport.ts

@@ -50,7 +50,7 @@ const fields: Field[] = [
 {
 title: "Operation Fees",
 cell: (account, parentAccount, op) =>
- ["TokenAccount", "ChildAccount"].includes(account.type)
+ ["TokenAccount"].includes(account.type)


could be

Suggested change

["TokenAccount"].includes(account.type)

account.type === "TokenAccount"

sprohaszka-ledger

Some nitpicking ;)

sprohaszka-ledger · 2024-03-28T15:15:36Z

apps/ledger-live-desktop/src/renderer/components/SelectAccount.tsx

- account.type !== "ChildAccount" && account.spendableBalance
- ? account.spendableBalance
- : account.balance;
+ const nested = ["TokenAccount"].includes(account.type);


This is simpler no?

const nested = account.type === "TokenAccount"

sprohaszka-ledger · 2024-03-28T15:17:58Z

apps/ledger-live-desktop/src/renderer/screens/exchange/Swap2/Form/TargetAccountDrawer/index.tsx

- account.type !== "ChildAccount" && account.spendableBalance
- ? account.spendableBalance
- : account.balance;
+ const balance = account.spendableBalance ? account.spendableBalance : account.balance;


Simpler, no ?

const balance = account.spendableBalance || account.balance

sprohaszka-ledger · 2024-03-28T15:21:26Z

libs/ledger-live-common/src/csvExport.ts

@@ -50,7 +50,7 @@ const fields: Field[] = [
 {
 title: "Operation Fees",
 cell: (account, parentAccount, op) =>
- ["TokenAccount", "ChildAccount"].includes(account.type)
+ ["TokenAccount"].includes(account.type)


Why don't use === ?

socket-security · 2024-03-28T16:33:08Z

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

sprohaszka-ledger

👍

gre requested review from a team as code owners March 22, 2024 14:59

github-actions bot assigned gre Mar 22, 2024

live-github-bot bot added desktop Has changes in LLD mobile Has changes in LLM common Has changes in live-common ledgerjs Has changes in the ledgerjs open source libs cli labels Mar 22, 2024

gre marked this pull request as draft March 22, 2024 15:10

gre force-pushed the live-11884 branch from dd6442c to 334e222 Compare March 22, 2024 15:17

vercel bot had a problem deploying to Preview – web-tools March 22, 2024 15:19 Failure

gre force-pushed the live-11884 branch from 334e222 to f1b4c7f Compare March 22, 2024 15:22

vercel bot had a problem deploying to Preview – web-tools March 22, 2024 15:25 Failure

gre force-pushed the live-11884 branch from f1b4c7f to 385d0af Compare March 22, 2024 15:27

vercel bot had a problem deploying to Preview – web-tools March 22, 2024 15:29 Failure

gre force-pushed the live-11884 branch from 385d0af to c358ebe Compare March 22, 2024 15:50

vercel bot deployed to Preview – web-tools March 22, 2024 15:54 View deployment

gre force-pushed the live-11884 branch from c358ebe to f218195 Compare March 22, 2024 15:59

vercel bot had a problem deploying to Preview – web-tools March 22, 2024 16:00 Failure

gre force-pushed the live-11884 branch from f218195 to 06a2dc6 Compare March 22, 2024 16:05

gre marked this pull request as ready for review March 22, 2024 16:06

gre requested a review from a team as a code owner March 22, 2024 16:06

vercel bot deployed to Preview – web-tools March 22, 2024 16:10 View deployment

gre force-pushed the live-11884 branch from 06a2dc6 to 804971b Compare March 22, 2024 17:11

vercel bot deployed to Preview – web-tools March 22, 2024 17:15 View deployment

sprohaszka-ledger previously approved these changes Mar 26, 2024

View reviewed changes

hedi-edelbloute reviewed Mar 26, 2024

View reviewed changes

gre force-pushed the live-11884 branch from 804971b to eaaa450 Compare March 28, 2024 14:01

gre marked this pull request as ready for review March 28, 2024 14:01

Justkant previously approved these changes Mar 28, 2024

View reviewed changes

vercel bot deployed to Preview – web-tools March 28, 2024 14:07 View deployment

gre dismissed Justkant’s stale review via fba3799 March 28, 2024 14:15

gre force-pushed the live-11884 branch from eaaa450 to fba3799 Compare March 28, 2024 14:15

vercel bot deployed to Preview – web-tools March 28, 2024 14:17 View deployment

hedi-edelbloute previously approved these changes Mar 28, 2024

View reviewed changes

mcayuelas-ledger reviewed Mar 28, 2024

View reviewed changes

sprohaszka-ledger previously approved these changes Mar 28, 2024

View reviewed changes

gre dismissed stale reviews from sprohaszka-ledger and hedi-edelbloute via f69225c March 28, 2024 16:25

gre force-pushed the live-11884 branch 2 times, most recently from f69225c to 897134a Compare March 28, 2024 16:30

vercel bot deployed to Preview – web-tools March 28, 2024 16:40 View deployment

chore: sunset unused ChildAccount type

4f05432

gre force-pushed the live-11884 branch from 897134a to 4f05432 Compare March 28, 2024 16:43

vercel bot deployed to Preview – web-tools March 28, 2024 16:45 View deployment

gre requested review from mcayuelas-ledger and sprohaszka-ledger March 28, 2024 17:19

hedi-edelbloute approved these changes Mar 29, 2024

View reviewed changes

Justkant approved these changes Mar 29, 2024

View reviewed changes

sprohaszka-ledger approved these changes Mar 29, 2024

View reviewed changes

CremaFR approved these changes Mar 29, 2024

View reviewed changes

mcayuelas-ledger approved these changes Mar 29, 2024

View reviewed changes

gre merged commit 7cbe2f4 into develop Mar 29, 2024
56 of 59 checks passed

gre deleted the live-11884 branch March 29, 2024 10:17

gre mentioned this pull request May 3, 2024

chore: decoupled {unit, starred, name} fields away from Account model #6796

Merged

3 tasks

sprohaszka-ledger mentioned this pull request May 14, 2024

chore: remove SubAccount type usage #6854

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: sunset unused ChildAccount type #6524

chore: sunset unused ChildAccount type #6524

gre commented Mar 22, 2024

vercel bot commented Mar 22, 2024 •

edited

sprohaszka-ledger left a comment •

edited

sprohaszka-ledger Mar 26, 2024

gre Mar 26, 2024 •

edited

sprohaszka-ledger Mar 26, 2024

gre Mar 26, 2024

sprohaszka-ledger Mar 26, 2024

gre Mar 28, 2024 •

edited

hedi-edelbloute left a comment

gre commented Mar 26, 2024 •

edited

mcayuelas-ledger Mar 28, 2024

sprohaszka-ledger left a comment

sprohaszka-ledger Mar 28, 2024

sprohaszka-ledger Mar 28, 2024

sprohaszka-ledger Mar 28, 2024

socket-security bot commented Mar 28, 2024 •

edited

sprohaszka-ledger left a comment

	["TokenAccount"].includes(account.type)
	account.type === "TokenAccount"

chore: sunset unused ChildAccount type #6524

chore: sunset unused ChildAccount type #6524

Conversation

gre commented Mar 22, 2024

✅ Checklist

📝 Description

❓ Context

🧐 Checklist for the PR Reviewers

vercel bot commented Mar 22, 2024 • edited

sprohaszka-ledger left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gre Mar 26, 2024 • edited

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

gre Mar 28, 2024 • edited

Choose a reason for hiding this comment

hedi-edelbloute left a comment

Choose a reason for hiding this comment

gre commented Mar 26, 2024 • edited

Choose a reason for hiding this comment

sprohaszka-ledger 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

socket-security bot commented Mar 28, 2024 • edited

sprohaszka-ledger left a comment

Choose a reason for hiding this comment

vercel bot commented Mar 22, 2024 •

edited

sprohaszka-ledger left a comment •

edited

gre Mar 26, 2024 •

edited

gre Mar 28, 2024 •

edited

gre commented Mar 26, 2024 •

edited

socket-security bot commented Mar 28, 2024 •

edited