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: cache wallet page transactions #1269

Open
wants to merge 11 commits into
base: develop
from

Conversation

Projects
None yet
6 participants
@j-a-m-l
Copy link
Contributor

commented May 30, 2019

This PR introduces a simplistic, but effective, way to cache transactions (resolves #894).

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Refactor
  • Performance
  • Tests
  • Build
  • Documentation
  • Code style update
  • Continuous Integration
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes
  • No

Does this PR release a new version?

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

  • It's submitted to the develop branch, not the master branch
  • All tests are passing
  • New/updated tests are included

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)
@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

commented May 30, 2019

The ci/circleci: test-node-11 job is failing as of a34613b62cfe15f5cfe4350efb7ed9e9b824f9d4. Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@codecov-io

This comment has been minimized.

Copy link

commented May 30, 2019

Codecov Report

Merging #1269 into develop will increase coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop   #1269      +/-   ##
==========================================
+ Coverage    39.88%   39.9%   +0.01%     
==========================================
  Files          229     229              
  Lines         6345    6367      +22     
  Branches      1253    1253              
==========================================
+ Hits          2531    2541      +10     
- Misses        3592    3601       +9     
- Partials       222     225       +3
Impacted Files Coverage Δ
src/renderer/store/modules/transaction.js 43.52% <0%> (-8.59%) ⬇️
...s/Wallet/WalletTransactions/WalletTransactions.vue 25% <0%> (+7.14%) ⬆️
src/renderer/components/Network/NetworkModal.vue 29.13% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b53de6...9da50a0. Read the comment docs.

@luciorubeens
Copy link
Member

left a comment

I'm randomly getting the new transactions banner:

Captura de Tela 2019-05-30 às 13 26 39

Some thoughts:

  • I think storing each page does not make sense, just the last 10 (or whatever the number of rows per page) transactions and the order is enough;
  • Only transactions of imported wallets/contacts should be stored;
  • It could have an expiration date, since initialization depends on rehydration.
@j-a-m-l

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

uhmm, @luciorubeens could you try to capture under which circumstances do you see that banner? I've never see it (it works well when I sen a transfer).

About storing only the last transactions and only store wallets / contacts, my idea was making navigation smoother, so users don't have to wait to navigate to / from pages that have been already visited. That specially useful under bad network conditions due user connection or due using a slow peer.

About the expiration date, what do you propose? flushing those old transactions at the beginning? checking the date every time that they are accessed?

@zillionn

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2019

IMO you should cache all txs under one key (wallet address or public key) and use that cached object to show/order/paginate the txs. So if I go on the next page, you should check the cached object if it contains 50 txs older than oldest on the current page, otherwise query the API. You can use /api/transactions/search to retrieve 50 txs from timestamp < oldest tx on the current page.

And btw ordering of txs just doesn't work.

Screenshot 2019-06-02 at 18 52 15

Ordering by Sender, Recipient and Smartbridge:
Screenshot 2019-06-02 at 18 53 48

By Date:
page 1:
Screenshot 2019-06-02 at 18 56 41
page 2:
Screenshot 2019-06-02 at 18 56 48

IMO you should only order the txs on the current page on the fly, same way the explorer is ordering them, without calling the API.

@j-a-m-l

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

Thanks, @zillionn. I'll fix the ordering on this PR.

About sorting the transactions of the current page only, we can't do it because we'd lose a lot of functionality: for instance, users would need to navigate between lots of pages to see the first one, instead of just ordering them ascendantely. In the explorer that use cases are not as important.

And about aggregating transactions by address (instead of using the address, but the pagination too) the problems with that approach are:

  • when you visit an address with thousands of transactions, you've to download all of them to sort them
  • depending on the order parameter, you'd retrieve different sets of transactions, so you wouldn't be able to know how to sort them with other order parameters or which other not-yet-received transactions should be included

Keep in mind that this PR focus on making the navigation faster, not on having a copy of the entire data of an address. That could be a waste of space in several scenarios.

@zillionn

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

I guess you don't understand me, do it as you decide.

@j-a-m-l

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

I've re-read it and, @zillionn, your approach looks like a good one until you start thinking about edge cases, but if you think that the current implementation could be improved, I'd love to hear your feedback.

@dated

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

i'd go so far as to say that remote sorting is the way to go, even on the explorer. i'll speak to @ItsANameToo about it when i'm back.

@zillionn

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

I'll just say that you don't have to download all the txs, you only download/cache the last 50 (limit per page) txs. If you change the order, you update the cache with the new 50 txs. But most importantly, when I hit Refresh, you should pull only the txs newer than the last in the cache - 99% of the cases that'll return 0 txs and not like the current implementation when you pull 50 txs on every refresh. IMO this will dramatically speed up the app.

@j-a-m-l

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

Caching the last 50 would cover only 5 pages, so it could be useful, but it's limited.

About the refresh button, using the search endpoint to fetch only the latest could be a good optimization in some cases. Probably we could keep this PR approach and, later, dig into that other kind of enhancements.

@j-a-m-l

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

The sorting problem is produced by core: ArkEcosystem/core#2649

j-a-m-l added some commits Jun 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.