Skip to content
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

Store also first empty accounts after discovery #12521

Merged
merged 3 commits into from
May 31, 2024

Conversation

matejkriz
Copy link
Member

@matejkriz matejkriz commented May 22, 2024

Description

By creating invisible empty discovered accounts, user can see it once he sent some TX there.

We don't repeat discovery on every Trezor connect yet and with view-only feature it can happen more often, that user will send some TX on new account and will never see it in Suite Lite until disable view-only and reconnect.

As opposed to Suite Desktop, in mobile there is a batched discovery so it can happen that there are 2 invisible empty accounts for some coins.

This is not a final solution at all, just a small improvement as a first step – discovery resume/continue needs to be implemented #11562

Acceptance criteria (notes for QA)

  • If user receive some funds on first empty account, it should appear in Suite Lite automatically.
  • If user receive some funds on the very first account of some coin, that wasn't visible in Suite Lite, it should appear automatically.
  • User should be able to add new account/coin until reaching 10 accounts per type limit and only 1 empty account limit (it's not possible to add new account if the previous one is empty and already visible)
  • It could also affect visibility of empty accounts on desktop, would be nice to double check adding new coin and account there too.

@matejkriz matejkriz added the mobile Suite Lite issues and PRs label May 22, 2024
@matejkriz matejkriz requested a review from a team as a code owner May 22, 2024 16:03
@matejkriz matejkriz changed the title feat(suite-native): create hidden accounts for first empty accounts during discovery Store also first empty accounts after discovery May 22, 2024
@mnuky mnuky added this to the Mobile - View-Only milestone May 23, 2024
Copy link
Contributor

@vytick vytick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The empty accounts are visible in My Assets and Receive tabs.

In case we hide them there, make sure that Add account works. Because now add new account wont add anything, since there is an empty account for each asset.

How should it work if we add first empty account? Possibly discover the next one if under limit of 10?

@matejkriz matejkriz self-assigned this May 24, 2024
@vytick vytick force-pushed the feat/discovery-including-first-empty branch from b2c61f5 to 416db6e Compare May 24, 2024 14:45
@vytick
Copy link
Contributor

vytick commented May 24, 2024

One more thought. Does this also solve situation where user in Suite adds new account of network that was not previously discovered on Lite app and receives txn there? Or in that case he wont see it?

@matejkriz matejkriz marked this pull request as draft May 28, 2024 09:28
@matejkriz
Copy link
Member Author

matejkriz commented May 28, 2024

It is WIP, because the newly added account is not visible on Assets/Recieve screens until you perform search. I guess it's a memoization issue, need to investigate more.

It was really a memoization issue caused by usage of selectEthereumAccountsTokensWithFiatRates selector, probably related to dai-shi/proxy-memoize#96. As a workaround, I removed the filter for tokens with fiat rates completely, as I believe it has to change anyway - we have to make tokens without fiat rates accessible somehow later.

I'll also prepare update of proxy-memoize, but it has caused us a lot of problems before, so I'll prepare it in a special PR for testing.

@matejkriz
Copy link
Member Author

One more thought. Does this also solve situation where user in Suite adds new account of network that was not previously discovered on Lite app and receives txn there? Or in that case he wont see it?

It should solve this case too, needs to be tested.

@matejkriz matejkriz force-pushed the feat/discovery-including-first-empty branch from 85614d8 to 0790dbc Compare May 28, 2024 15:02
@matejkriz matejkriz marked this pull request as ready for review May 29, 2024 14:34
@matejkriz matejkriz marked this pull request as draft May 29, 2024 14:54
@matejkriz
Copy link
Member Author

Back to draft, empty wallet is not handled yet.

@matejkriz
Copy link
Member Author

Back to draft, empty wallet is not handled yet.

fixed in e99f18e

@matejkriz matejkriz marked this pull request as ready for review May 29, 2024 15:30
Comment on lines +303 to +310
const account =
firstHiddenEmptyAccount ||
(await dispatch(
addAndDiscoverNetworkAccountThunk({
network,
deviceState: device.state,
}),
).unwrap());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Temporary solution, I would expect removal of addAndDiscoverNetworkAccountThunk as part of #11562

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to discover another hidden empty account here even in case this one is made visible?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, this probably does not make a sense unless the account is not empty.

But we can check regularly, that there exists at least one empty account of each coin-accountType pair and if not, run discovery until we find it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is basically how it wokrks on desktop, but let's leave it please for another PR, this one is already bigger than expected 🙈

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for sure :)

Copy link
Contributor

@vytick vytick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job. I’ve tested it and everything works as expected. 👌

…uring discovery

- By creating invisible empty discovered accounts, user can see it once he sent some TX there.
- We don't repeat discovery on every Trezor connect yet and with view-only feature it can happen more often, that user will send some TX on new account and will never see it in Suite Lite until disable view-only and reconnect.
- As opposed to Suite Desktop, in mobile there is a batched discovery so it can happen that there are 2 invisible empty accounts for some coins.
@matejkriz matejkriz force-pushed the feat/discovery-including-first-empty branch from e99f18e to 60a4364 Compare May 30, 2024 15:38
@matejkriz matejkriz merged commit 3caaef4 into develop May 31, 2024
27 checks passed
@matejkriz matejkriz deleted the feat/discovery-including-first-empty branch May 31, 2024 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mobile Suite Lite issues and PRs
Projects
Status: 🤝 Needs QA
Development

Successfully merging this pull request may close these issues.

None yet

4 participants