feat: Adds the trending endpoint for nfts #62
Conversation
* feat: add rankings endpoints with data already process * feat: add the rankings component and tests * fix: packages versions so it builds * fix: fix test description
There was a problem hiding this comment.
I just reviewed the logic and it looks okay to me, except for the fact the this is returning NFTs when i think we should return Items. The sale list is being reduced and accumulated by item id: https://github.com/decentraland/nft-server/pull/62/files#diff-1252b400db58108e959ce30a4956c10e6cd01c8034662e8cd550f6a69083ff2eR45 and then the itemId is used to find the correspondng NFts: https://github.com/decentraland/nft-server/pull/62/files#diff-1252b400db58108e959ce30a4956c10e6cd01c8034662e8cd550f6a69083ff2eR45
I think this will be misleading, because if an item sells for instance 1000 times (via mints), once aggregated the contractAddress + itemId record will have a value of 1000, and then when doing the find it will get one of the 1000 nfts and return that one. So on the homepage you will see one specific NFT that got sold once, and might not even be on sale now, instead of the Item that has all the sales and where you as a user can buy it again (mint it). So I think instead of using the nftComponent we should use instead the itemComponent and filter out sales of nfts that don't have an itemId (like the case of LANDs or names) since those sales even tho could show up by volume they might not be on sale anymore, and it's of no interest for the user browsing the trending section.
src/ports/trending/component.ts
Outdated
| // Fetch all the nfts from those 24hs sales | ||
| const nftResults = ( | ||
| // Fetch all the items from those 24hs sales | ||
| const items = ( | ||
| await Promise.all( | ||
| sales.map((sale) => |
There was a problem hiding this comment.
I think this is going to include repeated items in the list, and do unnecessary fetchs. For instance if there are 20 sales in the last 24hs for the the same item of the same collection there are going to be 20 fetchs for that same item.
If we moved the computation of the trendingSales record that's below this block to happen first, then we could use the keys of that record to fetch the required items, like this:
// First compute the trending sales record
const trendingSales = ...
// Then use the keys to fetch the items
const items = Promise.all(
Object.keys(trendingSales).map(key => {
const [contractAddress, itemId] = key.split('-')
return itemsComponent.fetch({ contractAddress, itemId })
})
).recuce(a, b) => a.concat(b), []) // flattenThere was a problem hiding this comment.
Ohh nice catch! yeah, it can definitely be moved after to avoid those extra fetches. Updated!
src/ports/trending/component.ts
Outdated
|
|
||
| // Get trending sales by amount of sales | ||
| const trendingSales = sales.reduce((acc, sale) => { | ||
| const key = `${sale.contractAddress}-${sale.itemId}` |
There was a problem hiding this comment.
Watch out here: the sale.itemId could be undefined for NFTs that are not part of a collection (parcels, estates, names, and L1 wearables). Those should be skipped from the trendingSales record, ie:
const trendingSales = sales.reduce((acc, sale) => {
if (sale.itemId) {
const key = `${sale.contractAddress}-${sale.itemId}`
acc[key] = (acc[key] || 0) + 1
}
return acc
}, {} as Record<string, number>)There was a problem hiding this comment.
yes! updated to avoid that unnecessary fetch in case they don't have itemId.
src/ports/trendings/component.ts
Outdated
| @@ -72,6 +72,8 @@ export function createTrendingsComponent( | |||
| // It will iterate over sales which is already ordered by volume as it was used as the SortBy parameter. | |||
| const trendingByVolume: Item[] = sales | |||
| .map((sale) => { | |||
| // console.log('sale.contractAddress: ', sale.contractAddress) | |||
| // console.log('sale.itemId: ', sale.itemId) | |||
There was a problem hiding this comment.
should we remove these comments?
There was a problem hiding this comment.
yes! 🤦 removed!
Closes #55