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.

Sign up for GitHub

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

Jump to bottom

Transaction loading revamp #11306

Open
marekrjpolak opened this issue Feb 22, 2024 · 4 comments · May be fixed by #11355
Open

Transaction loading revamp #11306

marekrjpolak opened this issue Feb 22, 2024 · 4 comments · May be fixed by #11355
Assignees
@jvaclavik @vojtatranta @marekrjpolak
Labels
transactions Transaction history page

Comments

@marekrjpolak
Copy link
Contributor

In order to be able to implement some transaction list improvements (e.g. filter out Ehtereum scam transactions or #7005), we have to decouple transaction pagination in Suite UI from api call pagination. The steps should be approximately following:

  • remove pagination in TransactionList and make it infinitely scrolling list instead (with some clever UI maybe?)
    • fetch new transaction batch when needed
    • component virtualization/recyclation must be resolved first as the list can be quite long
    • page concept (page param in fetchTransactionsThunks) can be still preserved under the hood, but only directly following pages will be ever accessed
    • must work with tx filtering
  • load all the account transactions eagerly instead of on demand
    • good for local-first, offline mode etc.
    • the same thing happens when filtering or exporting transactions
    • it can happen right after discovery (Asynchronously load whole transaction history #7003), but it definitely shouldn't slow it down, so think about it as a second non-blocking stage of discovery?

With this, it should be possible to hide some transactions as they wouldn't need to correspond to selected page, and we won't have to deal with expected undefined's in transactionReducer arrays because no page will ever be skipped.
@marekrjpolak marekrjpolak added the transactions Transaction history page label Feb 22, 2024
@MiroslavProchazka MiroslavProchazka moved this to 🎯 To do in Suite Desktop Feb 24, 2024
@MiroslavProchazka MiroslavProchazka moved this from 🎯 To do to 🏃‍♀️ In progress in Suite Desktop Feb 27, 2024
@MiroslavProchazka MiroslavProchazka moved this from 🏃‍♀️ In progress to 🚫 Blocked in Suite Desktop Apr 4, 2024
@peter-sanderson
Copy link
Contributor

peter-sanderson commented May 7, 2024 •
edited
Loading

Proof of concept done by Marek: #11355

@peter-sanderson peter-sanderson linked a pull request May 7, 2024 that will close this issue
Draft
@peter-sanderson peter-sanderson self-assigned this May 7, 2024
@peter-sanderson peter-sanderson removed their assignment Jul 8, 2024
@tomasklim tomasklim mentioned this issue Aug 2, 2024
Closed
@vojtatranta vojtatranta self-assigned this Jan 27, 2025
@vojtatranta
Copy link
Contributor

vojtatranta commented Jan 27, 2025 •
edited
Loading

Windsurf AI gave me this overlook of item rendering states. Not sure how accurate it is, but it is better than nothing.

Transaction UI Elements and Conditions

UI Element TransactionItem CoinjoinBatchItem Conditions/Notes
Type Icon ✓ ✓ TransactionItem: varies by tx type (send/receive/self/failed)
CoinjoinBatch: always "joint" type
Timestamp ✓ ✓ Both show transaction time
TransactionItem: per transaction
CoinjoinBatch: shows latest tx time
Amount (Crypto) ✓ ✓ TransactionItem: per transaction
CoinjoinBatch: sum of all transactions in batch
Amount (Fiat) ✓ ✓ TransactionItem: current rate
CoinjoinBatch: historical rates for each tx
Fee ✓ - Only for sent transactions
Multiple fees shown for multi-target txs
RBF Button ✓ - Shown when:
- transaction.rbfParams exists
- network supports RBF
- no deadline exists
Warning Card ✓ - Shows when isPending=true
Phishing Warning ✓ - When transaction is flagged as phishing
Blurs all transaction details
Target Addresses ✓ - Shows recipient addresses
Default limit of visible targets
Expandable for more targets
Collapsible View - ✓ Shows individual coinjoin rounds
Each round shows its own amount
Transaction Count - ✓ Total number of transactions in batch
Multiple Transactions Shows as separate items Groups related txs TransactionItem: Multiple targets shown in expandable list
CoinjoinBatch: All related txs grouped in one collapsible item
Staking ✓ - If the transaction was staked instantly / unstaked
Phishing TODO TODO Levels of phishing - really scame, maybe scam)
Sent to my account TODO TODO If the transaction was sent to one of "my" accounts

Special Layouts

TransactionItem:

  • Single Row Layout (1 target):
    • Crypto amount in same row as heading
    • Fiat amount with address in second row
  • Multi Row Layout (multiple targets):
    • Default shows first N targets
    • "Show more" button for additional targets
    • Each target shows its own amount

CoinjoinBatchItem:

  • Always uses collapsible layout
  • Batch summary shows total amounts
  • Expandable to show individual rounds:
    • Each round has its own row
    • Shows round number and amount
    • Historical

@vojtatranta
Copy link
Contributor

vojtatranta commented Jan 28, 2025 •
edited
Loading

I checked the PR made by @marekrjpolak #11355
TL;DR: I love it (great fetching API).

There are things that I don't understand / can't decide with my current knowledge (=no knowledge):

  • what's the motivation / core of the unification of the staking and transaction unification - currently done with different lists
  • the pagination is removed completely, making imcompatible (and unmergable basically) in the current state
  • remove Transaction Actions component - and "replaced by" its content, I don't know the motivation here.

I know that @Nodonisko has been changing things around regarding fetching, making in nicer in Sept 2024. I am not sure why Marek's changes weren't used.

My proposal:

  • Definitely use the useFetchTransactions hook, while giving it an ability to fetch by page (backwards-compatbile)
  • keep the TransactionActions - don't know the reason of the removal
  • keep the current "ununified" stake and transactions - for compatibility (up for discussion, I lack the context)

@vojtatranta
Copy link
Contributor

vojtatranta commented Jan 28, 2025 •
edited
Loading

Noticed: the scroll restore doesn't work properly, when clicking on the unloaded page, it gets scrolled way up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jvaclavik jvaclavik

@vojtatranta vojtatranta

@marekrjpolak marekrjpolak

Labels
transactions Transaction history page
Projects
Suite Desktop
Status: 🚫 Blocked
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

TransactionList infinite scroll trezor/trezor-suite
4 participants
@jvaclavik @vojtatranta @marekrjpolak @peter-sanderson