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

Balances page fails to load (rate limits) #104

Open
ghost opened this issue Jul 13, 2021 · 9 comments
Open

Balances page fails to load (rate limits) #104

ghost opened this issue Jul 13, 2021 · 9 comments
Labels
help wanted Extra attention is needed

Comments

@ghost
Copy link

ghost commented Jul 13, 2021

I did some investigation into the Balances page, which is always broken for me and slams the RPC node until it rate limits you.

Problem: A single load of the "Balances" page from my main wallet, hit the projectserum public rpc node about 400 times in 2 seconds before getting rate limited. These are 95% individual calls to getAccountInfo, should be replaced with getMultipleAccounts asap. It actually hit ~600 requests in the total page load, but the limiting started at ~400 in 2s.

Addressing this could, in my opinion, immensely improve UX. And maybe reduce load on projectserum rpc's by an order of magnitude. The orders page has similar behavior.
image

Potential solution: Batch all these single getAccountInfo calls from the Balances page into multiple getMultipleAccounts calls, instead.

@armaniferrante armaniferrante added the help wanted Extra attention is needed label Jul 13, 2021
@ghost
Copy link
Author

ghost commented Jul 14, 2021

Discussed with someone who took a look (@cryptogosu) - apparently solana's web3.js doesn't have getMultipleAccounts directly implemented.

However, was told the following: You can do connection._rpcRequest to use any endpoint not built in on the solana web3js

So this should still be doable.

@armaniferrante
Copy link
Contributor

armaniferrante commented Jul 14, 2021

Discussed with someone who took a look (@cryptogosu) - apparently solana's web3.js doesn't have getMultipleAccounts directly implemented.

However, was told the following: You can do connection._rpcRequest to use any endpoint not built in on the solana web3js

So this should still be doable.

If you're lazy, Anchor also has this via anchor.utils.rpc.getMultipleAccounts. So one could either use that or copy pasta the implementation from there.

@jacobcreech
Copy link

Looks like

Discussed with someone who took a look (@cryptogosu) - apparently solana's web3.js doesn't have getMultipleAccounts directly implemented.
However, was told the following: You can do connection._rpcRequest to use any endpoint not built in on the solana web3js
So this should still be doable.

If you're lazy, Anchor also has this via anchor.utils.rpc.getMultipleAccounts. So one could either use that or copy pasta the implementation from there.

Found an actual implementation already here https://github.com/project-serum/serum-dex-ui/blob/master/src/utils/send.tsx#L931

Working on this, should be pretty simple. Problem is, I keep getting rate limited during testing 🤷

jacobcreech added a commit to jacobcreech/serum-dex-ui that referenced this issue Jul 14, 2021
@jacobcreech
Copy link

Updated Balances page to use getMultipleAccounts. However, I'm still seeing a lot of getAccountInfo calls from something else on the page, even before I connect my wallet. I will investigate to understand and mitigate. For now, I will open the PR because getMultipleAccounts should at least lower the amount of calls.

jacobcreech@a595edf

@ghost
Copy link
Author

ghost commented Jul 14, 2021

Dug very deep.

These lines (https://github.com/project-serum/serum-dex-ui/blob/master/src/utils/markets.tsx#L48-L70) need to be modified to use getMultipleAccounts, then deserialize into the Market object from serum-ts.

Or, if we can only deserialize the Market struct using serum-ts, would need a helper method in there to do the mass call and deserialize each, returning into the Collection

jacobcreech added a commit to jacobcreech/serum-dex-ui that referenced this issue Jul 14, 2021
jacobcreech added a commit to jacobcreech/serum-dex-ui that referenced this issue Jul 16, 2021
useAllMints would fail if there were >100 mint accounts to search. Batching them on retrieval will allow you to have >100 mint accounts. Related to project-serum#104
@ghost
Copy link
Author

ghost commented Jul 26, 2021

@cryptogosu Tested the latest changes. The load seems to work good, but the rows in the table are re-arranging themselves every few seconds in seems.

@jacobcreech
Copy link

@SkynetCapital Added ordering of balances by descending order so that order is guaranteed. Whenever the page re-renders, it will go grab the mints/accounts, which are not guaranteed to be in the same order. This should fix the balances switching on every render

@ghost
Copy link
Author

ghost commented Jul 28, 2021

@cryptogosu Awesome, the sorting seems to have fixed it. @armaniferrante should be good for another QA to review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants
@armaniferrante @jacobcreech and others