Wen Account Selector: Fill in the rest of the account selector data, adjust the wallet tab to only display current account #502

Shadowfiend · 2021-12-02T03:54:56Z

This PR fils in the remaining data in the account selector, as
well as adjusting the wallet and single asset tabs to only display
balances and activity related to the currently-selected account. In
the process, almost all of the combined activity-related code in the
accounts slice is removed, since it was meant to populate this view,
which was never meant to display data across accounts.

The other large refactor here was the introduction of the
accountsSelectors file, following @henryboldi's example with the
activitiesSelectors file. Both of these are exposed at the top level
via a re-exporting background/redux-slices/selectors package that
exposes all internal selectors for the UI to use, which has allowed
some cleanup in typing dependencies to take place (in particular,
allowing the selectors to reference the full state without having to
resort to redeclaring it to avoid circular dependencies).

A few additional tweaks include trying to resolve avatars where
possible, changing the fixed Trezor header to display read-only mode
(for now), and not displaying a name when an account has no associated
ENS name. Also, the single asset page no longer displays placeholder
“this asset is on Arbitrum” language, which was confusing users and is
rooted in unimplemented functionality. These elements are hidden for
now.

accountSelectors now holds selectAccountAndTimestampedActivities, but just as important selectors are imported directly from the top-level `redux-slices/selectors` package instead of going to the particular subpackage for simplicity. In the process, use a RootState defined at the top level of redux-slices that can be used in the selectors package, since there is no longer a circular dependencies concern.

In addition to the already-functioning address and ENS name in the list, this commit adds the populating of correct per-address totals, updates the account grouping to indicate that the addresses are read-only, and replaces the bolded name with the shortened address in cases where there is no ENS name to work with. It also populates the correct avatar, when available. A new selectAccountTotals selector is introduced that does per-account totaling to make this possible. There is likely an opportunity for refactoring of some of the code that is similar with selectAccountAndTimestampedActivities.

This selector allows for the selection of just the currently selected account's balances, both with a total in the main currency value and per asset owned by that particular account.

These two components were operating on the combined asset data from all accounts, but were always meant to only display the current account's information. Activities/transactions are currently still shown across accounts.

To do this, WalletActivityList now takes an array of activities to display from its caller rather than selecting it directly. The Wallet and SingleAsset components use selectCurrentAccountActivitiesWithTimestamps to pull current activity, and SingleAsset further filters to the asset it is displaying.

This isn't supported yet, and displaying it is confusing to real users. Coming S∞n™.

Use the number of accounts in the account slice to determine if the wallet has any accounts, and update the accounts balance selector to exit early if there is no current account.

This ensures that adding a checksum address doesn't result in two redux entries, only one of which will actually receive data.

These were rendered at-size, and are now rendered scaled to their containing box.

Shadowfiend · 2021-12-03T04:31:56Z

There's an issue remaining with the back button on the single asset page, but it's present on main due to the onboarding flow changes that made it position: fixed. I should have flagged that, position: fixed in a shared component is always a recipe for disaster down the line haha.

I tried to fix it up but ran into some walls… @henryboldi could use some help with it, maybe as a separate PR?

henryboldi · 2021-12-03T04:57:04Z

I should have flagged that, position: fixed in a shared component is always a recipe for disaster down the line haha. I tried to fix it up but ran into some walls… @henryboldi could use some help with it, maybe as a separate PR?

Totally my bad! Yes, I'll open up a separate PR with a fix @Shadowfiend

henryboldi

This is chef's kiss! A few questions and comments though,

After adding a new account, the wallet should probably switch to it
The avatar border should be rounded

How about hiding this "add address" button? It could be confusing

How do we feel about hiding the notifications tab in this PR?

henryboldi · 2021-12-03T22:09:33Z

background/redux-slices/selectors/accountsSelectors.ts

+  }
+)
+
+export type AccountTotal = {


The AccountTotal naming is a tad confusing to me. To someone unfamiliar with the codebase, it might give the impression that it's some sort of total amount

It's the account info + it's total balance, which is in line with how we've done eg AccountNetwork.

Would love a better name though--I don't love that pattern, but I couldn't come up with anything great after giving it some thought.

Either way a jsdoc couldn't hurt.

Shadowfiend · 2021-12-04T01:36:55Z

Yes 2. Yes but not doing it for this release in the interest of time 3. Yes 4. Yes.

It shall rise again.

Existing avatars are rounded at the image level, but ENS avatars may not be, so the border rounding is now applied at the browser/element level.

The add flow is currently via Add Wallet, while Add address is meant for adding within an account. Thinking through how these pieces will interact will read-only wallets will happen later, so for now hide the (always-disabled-anyway) Add address button.

This also obviates the need to try to auto-select the first account for wallet initialization.

The key error interferes with a linter error, but we can skip the linter error to save our developer console.

Shadowfiend · 2021-12-04T03:19:44Z

Ended up doing all of them + fixing a React console error. Notably, wanting to select the newly-added account led to also removing some hacky code that was in use for initial wallet startup 🎉

mhluongo

Non-controversial, LGTM!

mhluongo · 2021-12-04T03:35:03Z

background/index.ts

@@ -5,11 +5,13 @@ import { AnyAction } from "@reduxjs/toolkit"

 import Main from "./main"
 import { encodeJSON, decodeJSON } from "./lib/utils"
-import logger from "./lib/logger"
+
+import { RootState } from "./redux-slices"


Nice to be done with the ReturnType

mhluongo · 2021-12-04T03:44:21Z

background/redux-slices/index.ts

+
+export default mainReducer
+
+export type RootState = ReturnType<typeof mainReducer>


It's overall better, otherwise we'd have to update every time we added a slice 🤷 DRY and all.

henryboldi · 2021-12-06T19:09:15Z

Should this close #363?

Shadowfiend · 2021-12-07T12:50:20Z

I'm interested in your take on that. I didn't set out to refactor the accounts slice as a goal, so there may still be some work to do if it's considered from that perspective.

Shadowfiend requested a review from henryboldi December 2, 2021 03:54

Base automatically changed from wen-token-data to fix-fairy December 2, 2021 15:43

Base automatically changed from fix-fairy to main December 2, 2021 16:14

Shadowfiend force-pushed the wen-account-selector branch from 4e4b32d to 9372d7a Compare December 2, 2021 16:34

Shadowfiend force-pushed the wen-account-selector branch from 9372d7a to 2f290f7 Compare December 3, 2021 02:51

Shadowfiend added 9 commits December 2, 2021 22:07

Introduce selectCurrentAccountBalances selector

1195995

This selector allows for the selection of just the currently selected account's balances, both with a total in the main currency value and per asset owned by that particular account.

Update Wallet and SingleAsset to show current account assets

4492979

These two components were operating on the combined asset data from all accounts, but were always meant to only display the current account's information. Activities/transactions are currently still shown across accounts.

Hide Arbitrum/move placeholder on SingleAsset page

f63cb32

This isn't supported yet, and displaying it is confusing to real users. Coming S∞n™.

Fix handling of empty wallet state

ef6777d

Use the number of accounts in the account slice to determine if the wallet has any accounts, and update the accounts balance selector to exit early if there is no current account.

Normalize new addresses from the UI

3fcc47f

This ensures that adding a checksum address doesn't result in two redux entries, only one of which will actually receive data.

Clear some useless code in account selector

2603676

Fix overly-large avatar images in the top menu

e3879fb

These were rendered at-size, and are now rendered scaled to their containing box.

Shadowfiend force-pushed the wen-account-selector branch from 2f290f7 to e3879fb Compare December 3, 2021 04:30

Shadowfiend marked this pull request as ready for review December 3, 2021 04:34

henryboldi suggested changes Dec 3, 2021

View reviewed changes

Shadowfiend added 5 commits December 3, 2021 21:59

Hide notifications panel for now

e873dc7

It shall rise again.

Round all avatar edges in account selector list

35db9ad

Existing avatars are rounded at the image level, but ENS avatars may not be, so the border rounding is now applied at the browser/element level.

Set selected account after adding a new one

dd00c2e

This also obviates the need to try to auto-select the first account for wallet initialization.

Fix a React key error and class spelling in SharedProgressIndicator

78d0606

The key error interferes with a linter error, but we can skip the linter error to save our developer console.

mhluongo approved these changes Dec 4, 2021

View reviewed changes

mhluongo merged commit 28eb79e into main Dec 4, 2021

mhluongo deleted the wen-account-selector branch December 4, 2021 04:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wen Account Selector: Fill in the rest of the account selector data, adjust the wallet tab to only display current account #502

Wen Account Selector: Fill in the rest of the account selector data, adjust the wallet tab to only display current account #502

Shadowfiend commented Dec 2, 2021 •

edited

Loading

Shadowfiend commented Dec 3, 2021

henryboldi commented Dec 3, 2021

henryboldi left a comment •

edited

Loading

henryboldi Dec 3, 2021

Shadowfiend Dec 4, 2021

Shadowfiend commented Dec 4, 2021

Shadowfiend commented Dec 4, 2021

mhluongo left a comment

mhluongo Dec 4, 2021

mhluongo Dec 4, 2021

Shadowfiend Dec 4, 2021

henryboldi commented Dec 6, 2021

Shadowfiend commented Dec 7, 2021


		export default mainReducer

		export type RootState = ReturnType<typeof mainReducer>

Wen Account Selector: Fill in the rest of the account selector data, adjust the wallet tab to only display current account #502

Wen Account Selector: Fill in the rest of the account selector data, adjust the wallet tab to only display current account #502

Conversation

Shadowfiend commented Dec 2, 2021 • edited Loading

Shadowfiend commented Dec 3, 2021

henryboldi commented Dec 3, 2021

henryboldi left a comment • edited Loading

Choose a reason for hiding this comment

henryboldi Dec 3, 2021

Choose a reason for hiding this comment

Shadowfiend Dec 4, 2021

Choose a reason for hiding this comment

Shadowfiend commented Dec 4, 2021

Shadowfiend commented Dec 4, 2021

mhluongo left a comment

Choose a reason for hiding this comment

mhluongo Dec 4, 2021

Choose a reason for hiding this comment

mhluongo Dec 4, 2021

Choose a reason for hiding this comment

Shadowfiend Dec 4, 2021

Choose a reason for hiding this comment

henryboldi commented Dec 6, 2021

Shadowfiend commented Dec 7, 2021

Shadowfiend commented Dec 2, 2021 •

edited

Loading

henryboldi left a comment •

edited

Loading