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

Await Token Info in Parallel #868

Closed
bh2smith opened this issue Jun 23, 2020 · 4 comments · Fixed by #872
Closed

Await Token Info in Parallel #868

bh2smith opened this issue Jun 23, 2020 · 4 comments · Fixed by #872

Comments

@bh2smith
Copy link
Contributor

This method is currently inefficient and slow when used to fetch many tokens at once. This could be easily improved by sending out EVM queries in parallel (i.e. with await Promise.all).

for (const id of tokenIds) {
const tokenAddress = await exchange.tokenIdToAddressMap(id);
let tokenInfo;
try {
const tokenInstance = await ERC20.at(tokenAddress);
const symbol = await tokenInstance.symbol();
const decimals = (await tokenInstance.decimals()).toNumber();
tokenInfo = {
id: id,
symbol: symbol,
decimals: decimals,
address: tokenAddress,
};
} catch (err) {
// This generic try-catch is essentially a TokenNotFoundError
// Could occur when the given ID slot is not occupied by a registered token on the exhchange
// or if the code registered at address occupied by a token slot is not that of and ERC20 token
tokenInfo = {
id: id,
address: tokenAddress,
};
}
tokenObjects.set(id, tokenInfo);

While improving this, it would also be nice to refactor this to use the batch exchange viewer contract changes that were introduced in #860

@nlordell
Copy link
Contributor

I would suggest using https://web3js.readthedocs.io/en/v1.2.9/include_package-core.html?highlight=batch#batchrequest

This would send all the EVM calls to the node in a single batch request, which would (in theory) allow the node to better optimize the access to blockchain data and improve performance even further.

@bh2smith
Copy link
Contributor Author

Very nice suggestion! Thanks @nlordell. This also means we now have many other places we can improve with this!

@bh2smith
Copy link
Contributor Author

@nlordell - Not sure that we have access to the web3 artifact from within this method. Is this going to be more involved that originally expected? Do I really want to pass this into the function...?

@nlordell
Copy link
Contributor

Not sure that we have access to the web3 artifact from within this method

I think you have access to the underlying Web3 contract instance with instance.contract.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants