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

refactor: multi-wallet endpoints #956

Merged
merged 19 commits into from Jan 15, 2019

Conversation

@alexbarnsley
Copy link
Member

commented Jan 11, 2019

Proposed changes

This implements the use of the multi-wallet search endpoints. These are currently only available on devnet, but it will automatically switch between the search endpoint and the "1 request per wallet" endpoints, depending on if it's available. It does this check on each profile change, by doing a spoof search request to see if it can search for many at once.

This has also been incorporated into the ledger wallet loading, so it batch loads 10 wallets, then gets the data for all 10, and keeps doing so until it hits a cold wallet.

This needs extensive testing on mainnet and devnet since it is important changes to how we get data

Types of changes

  • Refactoring (improve a current implementation without adding a new feature or fixing a bug)

Checklist

  • I have read the CONTRIBUTING documentation
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
@codecov-io

This comment has been minimized.

Copy link

commented Jan 11, 2019

Codecov Report

Merging #956 into develop will increase coverage by 1.44%.
The diff coverage is 81.39%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #956      +/-   ##
===========================================
+ Coverage    37.36%   38.81%   +1.44%     
===========================================
  Files          196      196              
  Lines         4742     4833      +91     
  Branches       920      942      +22     
===========================================
+ Hits          1772     1876     +104     
+ Misses        2851     2838      -13     
  Partials       119      119
Impacted Files Coverage Δ
src/renderer/App.vue 0% <ø> (ø) ⬆️
src/renderer/services/synchronizer/ledger.js 0% <0%> (ø) ⬆️
src/renderer/services/synchronizer.js 20.63% <0%> (-0.68%) ⬇️
src/renderer/services/synchronizer/wallets.js 66.23% <100%> (+4.58%) ⬆️
...rer/components/Dashboard/DashboardTransactions.vue 94.73% <100%> (+77.08%) ⬆️
src/renderer/services/client.js 67.14% <71.42%> (+0.93%) ⬆️
src/renderer/store/modules/ledger.js 76.4% <93.75%> (+16.8%) ⬆️
src/renderer/components/Wallet/WalletAddress.vue 53.84% <0%> (-6.87%) ⬇️
src/renderer/store/modules/session.js 30% <0%> (-1.12%) ⬇️
... and 3 more

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 16fb96a...3dfbe4a. Read the comment docs.

@alexbarnsley alexbarnsley changed the title refactor: mutli-wallet endpoints refactor: multi-wallet endpoints Jan 11, 2019

alexbarnsley and others added some commits Jan 14, 2019

@j-a-m-l

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2019

@alexbarnsley in a brief test I didn't detect any problem. Any suggestion about most bug-prone things to try?

@alexbarnsley

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2019

@j-a-m-l I also couldn't reproduce any issues. But I'd say the most bug prone is ledger usage with many wallets. It will still be a problem on a mainnet profile but should be faster on devnet. My tests loaded 100 ledger wallets in 35 seconds on devnet. Took 50 seconds on mainnet.

But I guess as long as everything works, it's all good

@j-a-m-l j-a-m-l referenced this pull request Jan 14, 2019
@luciorubeens
Copy link
Member

left a comment

I could not test this ledger performance, but about the multi-wallet request it's great. I did a test using 5 wallets, 60 seconds on the wallet transactions page:

refactor/mutli-wallet-endpoints = 13 requests/107 KB
develop = 44 requests/214 KB

But I noticed that two requests are made at the same time, for transactions/search and wallets/<address>/transactions. I think this is related to this watch https://github.com/ArkEcosystem/desktop-wallet/blob/refactor/mutli-wallet-endpoints/src/renderer/components/Wallet/WalletTransactions/WalletTransactions.vue#L76

@alexbarnsley

This comment has been minimized.

Copy link
Member Author

commented Jan 15, 2019

@luciorubeens great, thanks! Interesting, possibly something we can review. The stuff in the watch is mostly for checking for new transactions and updating the confirmation times. However, that may not be necessary when we start caching all transactions for wallets 🤔

@alexbarnsley alexbarnsley merged commit 7326f3b into develop Jan 15, 2019

1 check passed

ci/circleci: test-node-9 Your tests passed on CircleCI!
Details

@alexbarnsley alexbarnsley deleted the refactor/mutli-wallet-endpoints branch Jan 15, 2019

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