-
Notifications
You must be signed in to change notification settings - Fork 315
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
chore: sunset unused ChildAccount type #6524
Conversation
The latest updates on your projects. Learn more about Vercel for Git βοΈ
4 Ignored Deployments
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
/** super type that is either a token or a child account */ | ||
export type SubAccount = TokenAccount | ChildAccount; | ||
/** | ||
* deprecated use TokenAccount instead |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use jsdoc tag to highlight it in IDE * @deprecated Use TokenAccount instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
@@ -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 }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why force cast with a new type? The removed one should imply that the account
cannot be cast, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean also casting from ChildAccount
(which is a specific type) to TokenAccount
is quite weird as it is not the same type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm scared we someday need to use child account for family accounts with currency subAccounts, if it works i'd keep it
@hedi-edelbloute to port what we discussed in our DM earlier: |
@@ -50,7 +50,7 @@ const fields: Field[] = [ | |||
{ | |||
title: "Operation Fees", | |||
cell: (account, parentAccount, op) => | |||
["TokenAccount", "ChildAccount"].includes(account.type) | |||
["TokenAccount"].includes(account.type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could be
["TokenAccount"].includes(account.type) | |
account.type === "TokenAccount" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nitpicking ;)
account.type !== "ChildAccount" && account.spendableBalance | ||
? account.spendableBalance | ||
: account.balance; | ||
const nested = ["TokenAccount"].includes(account.type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is simpler no?
const nested = account.type === "TokenAccount"
account.type !== "ChildAccount" && account.spendableBalance | ||
? account.spendableBalance | ||
: account.balance; | ||
const balance = account.spendableBalance ? account.spendableBalance : account.balance; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simpler, no ?
const balance = account.spendableBalance || account.balance
@@ -50,7 +50,7 @@ const fields: Field[] = [ | |||
{ | |||
title: "Operation Fees", | |||
cell: (account, parentAccount, op) => | |||
["TokenAccount", "ChildAccount"].includes(account.type) | |||
["TokenAccount"].includes(account.type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't use ===
?
f69225c
f69225c
to
897134a
Compare
No dependency changes detected. Learn more about Socket for GitHub βοΈ π No dependency changes detected in pull request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
π
β Checklist
npx changeset
was attached. (not needed)π 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 toTokenAccount
.β Context
π§ Checklist for the PR Reviewers