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

feat(suite-native): make token selectors network agnostic #14633

Merged
merged 1 commit into from
Oct 1, 2024

Conversation

vytick
Copy link
Contributor

@vytick vytick commented Sep 30, 2024

This PR does not touch any UI so far, only contains changes for working with tokens.

  • change isCoinWithTokens to work with more token enabled networks
  • make txns work with token enabled networks
  • change ethereum token selectors to token selectors and make it network agnostic

Related Issue

Part of #14196

Screenshots:

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-09-30.at.22.24.56.mp4

@vytick vytick added the mobile Suite Lite issues and PRs label Sep 30, 2024
@vytick vytick requested a review from a team as a code owner September 30, 2024 20:30
@vytick vytick force-pushed the feat/native/polygon-tokens branch 2 times, most recently from e5a5b55 to 6a6f55a Compare September 30, 2024 21:16
Copy link
Contributor

@Nodonisko Nodonisko left a comment

Choose a reason for hiding this comment

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

just few nits

{ size: 500 },
);

export const selectAccountOrTokenAccountTransactions = (
Copy link
Contributor

@Nodonisko Nodonisko Oct 1, 2024

Choose a reason for hiding this comment

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

Name is strange, it returns only transactions. Something like selectAccountTokenTransactions is more clear

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It returns either all txns for whole account (eg. eth + tokens)or only for some token (usdc on eth)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isTransactionWithTokenTransfers(transaction)),
),
) as WalletAccountTransaction[],
{ size: 500 },
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think there will be 500 accounts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

),
),
) as WalletAccountTransaction[],
{ size: 500 },
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need such a big number, something like 50 will be also fine there because there won't more than one mounted instance of component that uses this selector

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Will change. I kept the number from renamed and moved ethereum selector.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Nodonisko
Copy link
Contributor

Also E2E seems to fail

@vytick
Copy link
Contributor Author

vytick commented Oct 1, 2024

/rebase

Copy link

github-actions bot commented Oct 1, 2024

@trezor-ci trezor-ci force-pushed the feat/native/polygon-tokens branch from e8dde21 to 1c2056e Compare October 1, 2024 20:49
@vytick vytick merged commit b903214 into develop Oct 1, 2024
48 of 88 checks passed
@vytick vytick deleted the feat/native/polygon-tokens branch October 1, 2024 21:38
@vytick
Copy link
Contributor Author

vytick commented Oct 2, 2024

@STew790 Please test, that everything works as before:

  • with disabled poygon and disabled bnb only eth has tokens and nothing else
  • eth has correctly token txns toggle and nothing else (unless enabled pol/bnb) has it
  • nothing breaks with tokens and transactions

@STew790
Copy link
Contributor

STew790 commented Oct 9, 2024

QA OK

  • Only ETH has tokens (I don't see them anywhere else) with BNB and Polygon disabled
  • ETH has toggle in transactions for tokens
  • tokens and other transactions LGTM

Info
24.9.2 b718af4

@STew790 STew790 added the QA OK Issue passed QA without any blocker label Oct 9, 2024
@vytick vytick mentioned this pull request Oct 14, 2024
3 tasks
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 QA OK Issue passed QA without any blocker
Projects
Status: ✅ Approved
Development

Successfully merging this pull request may close these issues.

3 participants