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

fix: Improve NFT fetch performance on rentals #126

Merged
merged 3 commits into from
Aug 24, 2022

Conversation

LautaroPetaccio
Copy link
Contributor

@LautaroPetaccio LautaroPetaccio commented Aug 22, 2022

This PR improves the NFT fetching procedure when rentals are requests by using id_in.
This PR also fixes the should fetch function of the rentals source to check the isLand parameter when there's no category set.

const tokenIdsOfRentals = rentals.map((rental) => rental.nftId)
const nftResultsOfRentals = await nfts.fetchByTokenIds(tokenIdsOfRentals)
const nftResultsOfRentalsById = Object.fromEntries(
nftResultsOfRentals.map((nft) => [nft.nft.id, nft])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a heads up, this is looping the nftResultsOfRentals twice (one for the map and another for the fromEntries) and creating N arrays + 1 object. Using a .reduce would loop only once and create just 1 object.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is meant just for a single page of 20 elements might not make a difference but for 1000 results it could.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to a reduce!

@LautaroPetaccio LautaroPetaccio merged commit d164d03 into master Aug 24, 2022
@LautaroPetaccio LautaroPetaccio deleted the fix/improve-nft-fetch-performance-on-rentals branch August 24, 2022 14:24
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 this pull request may close these issues.

2 participants