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

Optimize query on TokenEntity #7462

Closed
1 task done
vikiival opened this issue Oct 2, 2023 · 12 comments · Fixed by #7641
Closed
1 task done

Optimize query on TokenEntity #7462

vikiival opened this issue Oct 2, 2023 · 12 comments · Fixed by #7641
Assignees
Labels
bug Something isn't working

Comments

@vikiival
Copy link
Member

vikiival commented Oct 2, 2023

What happened?

One of the reasons why #7419 on AHP was caused is this huge and scary query.
It does so many joins that squid database decided to give up.

Screenshot 2023-10-02 at 12 59 10

query tokenListWithSearch($first: Int!, $offset: Int, $denyList: [String!], $search: [NFTEntityWhereInput!], $orderBy: [TokenEntityOrderByInput!]) {
  tokenEntities: tokenEntities(
    orderBy: $orderBy
    limit: $first
    offset: $offset
    where: {nfts_some: {burned_eq: false, issuer_not_in: $denyList, metadata_not_eq: "", AND: $search}}
  ) {
    id
    blockNumber
    hash
    image
    media
    name
    updatedAt
    createdAt
    count
    nfts(
      where: {burned_eq: false, issuer_not_in: $denyList, metadata_not_eq: "", AND: $search}
    ) {
      ...nft
      ...nftDetails
      royalty
      recipient
      collection {
        id
        name
        floor
        __typename
      }
      meta {
        ...baseMeta
        __typename
      }
      __typename
    }
    __typename
  }
  tokenEntitiesConnection(
    where: {nfts_some: {burned_eq: false, issuer_not_in: $denyList, metadata_not_eq: "", AND: $search}}
    orderBy: blockNumber_DESC
  ) {
    totalCount
    __typename
  }
}

fragment nft on NFTEntity {
  id
  metadata
  issuer
  currentOwner
  blockNumber
  burned
  __typename
}

fragment nftDetails on NFTEntity {
  name
  sn
  price
  __typename
}

fragment baseMeta on MetadataEntity {
  id
  image
  animationUrl
  name
  description
  __typename
}
{
	"search": [
		{}
	],
	"first": 40,
	"offset": 1,
	"denyList": [
		"128qRiVjxU3TuT37tg7AX99zwqfPtj2t4nDKUv9Dvi5wzxuF",
		"155HWw3J9jyYphMm5is4vp9Bzj7ZRRd6HEzCPdWd8cq97KfT"
	],
	"orderBy": [
		"blockNumber_DESC"
	]
}

Please reproduce in steps

Expected Behavior

The task:

Delete unnecessary information from the query so I does not make too many complicated joins.

Blocked by Questions on @daiagi

  1. Do we really need to query all available NFTs?
  2. Does all nfts need all metadata?

What browsers are you seeing the problem on?

No response

At which address did you encounter bug?

No response

Are you logged in?

None

Which wallet you are using?

No response

At which chain did you encounter bug?

No response

Screenshots

photo_2023-10-02 12 59 21

Relevant log output

No response

Payment link for reward

https://kodadot.xyz/ksm/transfer/?target=%3CMy_Kusama_Address_check_https://github.com/kodadot/nft-gallery/blob/main/CONTRIBUTING.md#creating-your-ksm-address%3E

Code of Conduct

  • I agree to follow this project's Code of Conduct
@vikiival vikiival added bug Something isn't working S-blocked-✋ labels Oct 2, 2023
@daiagi
Copy link
Contributor

daiagi commented Oct 2, 2023

@vikiival
if you agree, I'll undo the change on the FE that uses TokenEntity i.e. revert back to using plain old NFTEntity
and reimplement TokenEntity again

more data should be on tokenEntity, to avoid needing to query lots of nft data on the FE

@vikiival
Copy link
Member Author

vikiival commented Oct 2, 2023

more data should be on tokenEntity, to avoid needing to query lots of nft data on the FE

Yes. Which data do we need?

@daiagi
Copy link
Contributor

daiagi commented Oct 3, 2023

we need

id
name
image
media
count (unburnt)
blockNumber (most recent NFT) - for sort
updatedAt - for sort
price (of cheapest nft) - for sort and "buy now" filter
sn - for sort
owners - for "own" filter
collection: {
id
name
floor price
}

nftsForListing: {
id
owner
}
nftForBuying {
id
price

}

Gaps :

count (unburnt) - currently returning total count (burned included)
price (of cheapest nft)
sn
owners: string[]

collection floor price, rn getting it requires another query over nfts

Currently possible query

query TokenEntites($userid: String) {
  tokenEntities(orderBy: updatedAt_DESC, limit:50,  ) {
    id
    name
    image
    media
    count
    updatedAt
    blockNumber
    
    nftForBuying: nfts(
      where: {
        currentOwner_not_eq: $userid,
        price_gt: "0",
        burned_eq: false},
        limit: 1,
      orderBy: price_ASC) 
    {
      id
      price
    }
    nftsForListing: nfts(
      where: {
        currentOwner_eq: $userid,
        price_eq: "0",
        burned_eq: false
      } )
    {
      id
      currentOwner
      
    }
    collection {
      floorPrice :nfts(where: {price_gt: "0"} ,orderBy: price_ASC, limit:1) {
        price
      }
      name
      id
    }

  }
}

This was referenced Oct 3, 2023
@vikiival
Copy link
Member Author

vikiival commented Oct 3, 2023

id, name, image, media

count (unburnt)

blockNumber (most recent NFT) - for sort

updatedAt - for sort

price (of cheapest nft) - for sort and "buy now" filter

sn - for sort

This filter does not make sense imo as there is nothing I can represent as serial number.

owners - for "own" filter

Rather cached resolver

collection

So that means TokenEntity is expecting same image under one collection right?

nftsForListing

Like count or why?

nftForBuying

same as above

Gaps :

owners: string[]
collection floor price, rn getting it requires another query over nfts

@daiagi
Copy link
Contributor

daiagi commented Oct 3, 2023

This filter does not make sense imo as there is nothing I can represent as serial number.

ok, but that will mean we will need to drop sort by sequence

So that means TokenEntity is expecting same image under one collection right?

correct, all of the nfts under one collection that share same ipfs URL are a single TokenEntity

Like count or why?

no. list of id's
image

this btn will add all these ids's into listing cart
it's either this or query for all of the nft's under token entity and let the FE sort out which is what
right?

same as above

same for the buy now action button. need to know which nft id to buy (the cheapest)

@vikiival
Copy link
Member Author

vikiival commented Oct 3, 2023

no. list of id's

Can be lazy loaded on demand.
Do not see value to overfetch the client for that.

correct, all of the nfts under one collection that share same ipfs URL are a single TokenEntity

Collection Is already present, same as blockNumber and updated at

https://github.com/kodadot/stick/blob/f870effae7141dde9668e94dfe771f6fdd7ff596/schema.graphql#L32

@kodabot
Copy link
Collaborator

kodabot commented Oct 11, 2023

ASSIGNED - @daiagi 🔒 LOCKED -> Friday, October 13th 2023, 04:09:35 UTC -> 36 hours

@daiagi
Copy link
Contributor

daiagi commented Oct 11, 2023

new query will look something like this

query tokenListWithSearch(
  $first: Int!
  $offset: Int
  $orderBy: [String!] = ["blockNumber_DESC"]
  $price_gte: Float
  $price_gt: Float
  $price_lte: Float
  $owner: String
  $denyList: [String!]
) {
  tokenEntities: tokenEntityList(
    owner: $owner
    denyList: $denyList
    orderBy: $orderBy
    limit: $first
    offset: $offset
    price_gte: $price_gte
    price_gt: $price_gt
    price_lte: $price_lte
  ) {
    id
    name
    image
    media
    metadata
    supply
    cheapest {
      id
      price
      currentOwner
    }
    collection {
      id
      name
    }
    meta {
      id
      image
      animationUrl
      description
    }
  }
    tokenEntityCount(
    owner: $owner
    denyList: $denyList
    price_gte: $price_gte
    price_gt: $price_gt
    price_lte: $price_lte
  ) {
    totalCount
  }

}

kodadot/stick#110

and I am currently integrating the front end using local machine indexer + graphql server

PR will come soon

@vikiival
Copy link
Member Author

Released Stick v5 and Speck v5 with this change

@daiagi
Copy link
Contributor

daiagi commented Oct 12, 2023

Thanks viki
I'm about a day away from releasing the pr for front end

@kodabot
Copy link
Collaborator

kodabot commented Oct 13, 2023

ASSIGNMENT EXPIRED - @daiagi has been unassigned.

@vikiival
Copy link
Member Author

New release of v5.0.1 in kodadot/stick#115 and kodadot/stick#116

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants