feat: account names #284

jgresham · 2022-02-12T15:11:05Z

Implements #263
Needs cleanup once overall UI flow and functionality is approved.

account_names1.mp4

account_names2.mp4

delaaxe · 2022-02-14T10:30:57Z

Thanks for you contribution John, this is great! Small nitpick: it seems that when toggling edit mode, everything that's below the name shifts vertically by 1px. Possible to keep it all fixed?

Also, let me know when you think it's ready so I can review the code.

jgresham · 2022-02-15T15:14:11Z

Thanks! Will tried to get it cleaned up for review in the next day or two!

jgresham · 2022-02-19T02:14:27Z

@delaaxe fixed the 1px UI shifting bug and cleaned up the PR.

wallet/account naming seems mixed but there is an issue for that #311

note: this is separate from starknet's cli --account=my_account

packages/extension/src/ui/components/Account/AccountListItem.tsx

delaaxe · 2022-02-21T08:35:54Z

packages/extension/src/ui/components/Account/AccountListItem.tsx

  accountNumber: number
  address: string
  status: WalletStatus
  isDeleteable?: boolean
 }

 export const AccountListItem: FC<AccountListProps> = ({
+  accountName,
  accountNumber,


accountNumber shouldn't be needed anymore with this change

delaaxe · 2022-02-21T08:36:36Z

packages/extension/src/ui/components/Account/AccountSubheader.tsx

@@ -34,18 +34,33 @@ interface AccountSubheaderProps {
  networkId: string
  status: WalletStatus
  accountNumber: number


same here and also everywhere below

delaaxe · 2022-02-21T08:37:40Z

packages/extension/src/ui/components/Account/AccountSubheader.tsx

 }) => (
  <>
-    <div>
-      <AccountName>{getAccountName(accountNumber)}</AccountName>
+    <div style={{ display: "flex", flexDirection: "column" }}>


Try avoiding inline styles as much as possible

delaaxe · 2022-02-21T08:46:17Z

packages/extension/src/ui/states/account.ts

-        ...state.wallets,
-        [newWallet.address]: newWallet,
+export const useAccount = create<AccountStore>(
+  persist(


shouldn't it be cleaner to do a separate store that's persisted for account names and potentially other data, maybe AccountDetails or AccountMetadata, instead of wrapping this whole store just to make one field persistent?

I think it could be cleaner and easier to reason about where the data is coming from. I kept in account store because I thought it would be less changes elsewhere. Few thoughts:

Pros for separate stores:

Keep the data coming from recovery.ts recover() (account store before these changes) separate from any user input/metadata

More future proof if there are plans for user's to add more account details.

Cons for separate stores:

UI components would need to use multiple stores to show all the data for an account

delaaxe · 2022-02-21T10:32:30Z

packages/extension/src/ui/states/account.ts

+  return selectedWallet ? wallets[selectedWallet] : undefined
+}
+
+const calcAccountNumber = (


please avoid abbreviations and acronyms as much as possible

delaaxe · 2022-02-21T10:34:35Z

packages/extension/src/ui/states/account.ts

+  return (
+    Object.keys(wallets).findIndex((address) => address === accountAddress) + 1
+  )
+}


i think this should be more along the lines of:

index = 1 while getDefaultAccountName(index) not in accountNames: index++ return getDefaultAccountName(index)

I don't think this would work with the structure of accountNames being:
https://github.com/argentlabs/argent-x/pull/284/files#diff-7b1dbae5fd43e7fa9c15da5204f37abd705b208adc0be83178e26bcd3816c32dR48-R54

accountNames: { ...state.accountNames, [networkId]: { ...state.accountNames[networkId], [address]: name, }, },

packages/extension/src/ui/utils/wallets.ts

delaaxe · 2022-02-21T10:37:14Z

packages/extension/src/ui/utils/recovery.ts

@@ -34,6 +34,7 @@ export const recover = async ({
      .map(({ address, network }) => new Wallet(address, network))
      .reduce((acc, wallet) => ({ ...acc, [wallet.address]: wallet }), {})

+    setAccountNamesFromBackup(wallets)


this function wouldn't be needed if there was a single source of truth for account names

correct, but would need to add an additional "useStore" to UI components.

delaaxe · 2022-02-21T10:38:03Z

packages/extension/src/ui/Wallet.ts

@@ -11,13 +11,20 @@ const ArgentCompiledContractJson: CompiledContract = json.parse(
 export class Wallet {
  address: string
  networkId: string
+  name?: string


this field shouldn't exist on this class

I'm not aware how Wallet and Account is used with starknet. Is this class better named Account?

delaaxe · 2022-02-22T12:04:50Z

we'll try to include this in a minor release for the end of week, so if you don't have time to go through the changes, I hope you won't mind if I do them myself!

jgresham · 2022-02-22T15:56:53Z

Good to hear. Yeah go for it! I don't have any unpushed work.

delaaxe · 2022-02-23T13:40:54Z

@jgresham I've made the changes. Do you see anything that could be wrong or that I might've missed before I merge? Thanks again for your help

jgresham · 2022-02-23T16:58:57Z

packages/extension/src/ui/states/accountMetadata.ts

+    useAccountMetadata.getState().accountNames[account.networkId] || {},
+  )
+  let index = 1
+  while (names.includes(`Account ${index}`)) {


I think this method of determining the default account name has a bug. On first open of my wallet, all accounts had "Account 1" as the name. Also, my opinion is that the n-th account should be named Account n, regardless of the other account names.

jgresham · 2022-02-23T17:13:40Z

packages/extension/src/ui/states/accountMetadata.ts

+  for (const [address, account] of Object.entries(accounts)) {
+    const { networkId } = account
+    if (!accountNames?.[networkId]?.[address]) {
+      const name = getDefaultAccountName(account)


names may need to be passed in here as the state isn't updated for each iteration in getDefaultAccountName

delaaxe · 2022-02-23T17:31:21Z

@jgresham just reverted it to the way you did it before. seeing any more issues?

jgresham · 2022-02-23T18:03:23Z

@jgresham just reverted it to the way you did it before. seeing any more issues?

I don't see any new commits?

delaaxe · 2022-02-23T21:32:32Z

@jgresham just reverted it to the way you did it before. seeing any more issues?

I don't see any new commits?

sorry I pushed to wrong branch. should be ok now

jgresham · 2022-02-24T02:11:52Z

@delaaxe just tested it out. looks good to me!

feat: account names

514efc2

add account name to ConfirmScreen. PR cleanup

44498b6

jgresham changed the title ~~(WIP) feat: account names~~ feat: account names Feb 19, 2022

jgresham added 4 commits February 19, 2022 11:31

remove repeat code in EditableHeader. no spellcheck for account name.

3fdf5e3

rename localstorage key from accountNames to account for account store

0caf71c

fix:remove wallets from local storage. only save account names.

b8aee51

fix: after account is deployed, set name to default account name

bfc7273

delaaxe suggested changes Feb 21, 2022

View reviewed changes

delaaxe added 2 commits February 21, 2022 14:52

Merge branch 'main' into pr/284

049101a

Merge branch 'main' into pr/284

ee08121

delaaxe added 3 commits February 23, 2022 08:49

Update packages/extension/src/ui/components/Account/AccountListItem.tsx

5e6902a

Update packages/extension/src/ui/utils/wallets.ts

2cb7308

refactor: move persisted account state to own store

8ae5133

delaaxe force-pushed the issue_#263_name_account branch from c3162d4 to 8ae5133 Compare February 23, 2022 13:36

jgresham commented Feb 23, 2022

View reviewed changes

fix(ui): revert default account name to index

b35f967

delaaxe approved these changes Feb 24, 2022

View reviewed changes

delaaxe merged commit 7e59127 into argentlabs:main Feb 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: account names #284

feat: account names #284

jgresham commented Feb 12, 2022

delaaxe commented Feb 14, 2022

jgresham commented Feb 15, 2022

jgresham commented Feb 19, 2022 •

edited

Loading

delaaxe Feb 21, 2022

delaaxe Feb 21, 2022

delaaxe Feb 21, 2022

delaaxe Feb 21, 2022

jgresham Feb 23, 2022

delaaxe Feb 21, 2022

delaaxe Feb 21, 2022

jgresham Feb 23, 2022

delaaxe Feb 21, 2022

jgresham Feb 23, 2022

delaaxe Feb 21, 2022

jgresham Feb 23, 2022

delaaxe commented Feb 22, 2022

jgresham commented Feb 22, 2022

delaaxe commented Feb 23, 2022

jgresham Feb 23, 2022

jgresham Feb 23, 2022

delaaxe commented Feb 23, 2022

jgresham commented Feb 23, 2022

delaaxe commented Feb 23, 2022

jgresham commented Feb 24, 2022

feat: account names #284

feat: account names #284

Conversation

jgresham commented Feb 12, 2022

delaaxe commented Feb 14, 2022

jgresham commented Feb 15, 2022

jgresham commented Feb 19, 2022 • edited Loading

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

delaaxe commented Feb 22, 2022

jgresham commented Feb 22, 2022

delaaxe commented Feb 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delaaxe commented Feb 23, 2022

jgresham commented Feb 23, 2022

delaaxe commented Feb 23, 2022

jgresham commented Feb 24, 2022

jgresham commented Feb 19, 2022 •

edited

Loading