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

feat add holdings by user #2751

Merged
merged 25 commits into from
Apr 6, 2022
Merged

feat add holdings by user #2751

merged 25 commits into from
Apr 6, 2022

Conversation

Jarsen136
Copy link
Contributor

Thank you for your contribution to the KodaDot NFT gallery.
👇 _ Let's make a quick check before the contribution.

PR type

  • Bugfix
  • Feature
  • Refactoring

What's new?

Before submitting Pull Request, please make sure:

  • My contribution builds clean without any errors or warnings
  • I've merged recent default branch -- main and I've no conflicts
  • I've tried to respect high code quality standards
  • I've didn't break any original functionality
  • I've posted a screenshot of demonstrated change in this PR

Optional

  • I've tested it at </rmrk/collection/26902bc2f7c20c546a-1FVG7>
  • I've tested PR on mobile and everything seems works
  • I found edge cases
  • I've written some unit tests 🧪

Had issue bounty label?

  • Fill up your KSM address: Payout

Community participation

Screenshot

  • My fix has changed something on UI; a screenshot is best to understand changes for others.

image

@netlify
Copy link

netlify bot commented Apr 3, 2022

Deploy Preview for koda-nuxt ready!

Name Link
🔨 Latest commit 099c904
🔍 Latest deploy log https://app.netlify.com/sites/koda-nuxt/deploys/624da6dc23e01d0008a78e2c
😎 Deploy Preview https://deploy-preview-2751--koda-nuxt.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@yangwao
Copy link
Member

yangwao commented Apr 3, 2022

@yangwao
Copy link
Member

yangwao commented Apr 3, 2022

@yangwao
Copy link
Member

yangwao commented Apr 3, 2022

Otherwise is seems works nice https://deploy-preview-2751--koda-nuxt.netlify.app/rmrk/u/FAZBj1G1H1Ls4nKn8ru7F31qku4Fhgzcce3D56hgTfJAj4Q?tab=holdings

just wanted inspect on some collectors tho!
:)

@Jarsen136
Copy link
Contributor Author

Jarsen136 commented Apr 3, 2022

can't see https://deploy-preview-2751--koda-nuxt.netlify.app/rmrk/u/FXUxw9Xp98gTzrU2z63D2G92kcx1oznd6xYa8WdqRgTcEDo it redirect me to collected...

same for https://deploy-preview-2751--koda-nuxt.netlify.app/rmrk/u/JCHzJjQAbgk96DsGq69JgwPLXnc3Ab6UfGjtxYZ5eLV2vFW?tab=collected

I bet it has something to do if the user who did not create, won't see holdings?

I've also encountered several addresses where this is the case. I'm not sure if it's because the user is set to not allow access to the profile page. The interface for requesting collections list returned empty. And it works well for other addresses.

image

but maybe it's fixed in #2725 ?

#2725 solves the occasional problem of displaying empty lists for every address. Not the same problem as mentioned above

@yangwao
Copy link
Member

yangwao commented Apr 3, 2022

I'm not sure if it's because the user is set to not allow access to the profile page.

I guess we don't have any setting like that 🤔

@yangwao yangwao requested a review from roiLeo April 3, 2022 17:48
@yangwao
Copy link
Member

yangwao commented Apr 3, 2022

wait https://deploy-preview-2751--koda-nuxt.netlify.app/rmrk/u/CuHWHNcBt3ASMVSJmcJyiBWGxxiWLyjYoYbGjfhL4ovoeSd?tab=holdings&page=1

just noticed it shows only NFTs which were minted by my address?

@Jarsen136
Copy link
Contributor Author

wait https://deploy-preview-2751--koda-nuxt.netlify.app/rmrk/u/CuHWHNcBt3ASMVSJmcJyiBWGxxiWLyjYoYbGjfhL4ovoeSd?tab=holdings&page=1

just noticed it shows only NFTs which were minted by my address?

Yup, now I have fixed it and it can show all nfts owned by users.

@Jarsen136
Copy link
Contributor Author

Jarsen136 commented Apr 4, 2022

However, I have to use two queries including nftListOwned(just added) and allCollectionSaleEvents, to get all holding information. On addresses with a large number of NFTs, performance will be poor and it may cause a server-side error.

The best solution is to combine them into one request.
Specifically, it is to enable the nftListOwned interface to return the result of the allCollectionSaleEvents interface.
Maybe need some help from @vikiival @roiLeo

query nftListOwned(
  $account: String!,
  $first: Int,
  $offset: Int,
  $orderBy: NftEntitiesOrderBy = BLOCK_NUMBER_DESC
) {
  nFTEntities(
    filter: {
      currentOwner: { equalTo: $account }
      name: { notLike: "%Kanaria%" }
      burned: { distinctFrom: true }
    }
    orderBy: [$orderBy, BLOCK_NUMBER_DESC]
    first: $first
    offset: $offset
  ) {
    totalCount
    nodes {
      ...nft
    }
  }
}
query allCollectionSaleEvents($ids: [ID!], $and: [EventWhereInput!]) {
  events(where: { nft: { collection: { id_in: $ids } }, AND: $and }) {
    meta
    interaction
    blockNumber
    timestamp
    caller
    id
    nft {
      id
      name
      collection {
        id
        name
      }
    }
  }
}

components/rmrk/Gallery/Holder/Holder.vue Outdated Show resolved Hide resolved
components/rmrk/Gallery/Holder/Holder.vue Outdated Show resolved Hide resolved
components/rmrk/Gallery/Holder/Holder.vue Outdated Show resolved Hide resolved
components/rmrk/Gallery/Holder/Holder.vue Outdated Show resolved Hide resolved
components/rmrk/Gallery/Holder/Holder.vue Outdated Show resolved Hide resolved
components/rmrk/Gallery/Holding.vue Outdated Show resolved Hide resolved
components/rmrk/Gallery/Holding.vue Outdated Show resolved Hide resolved
@yangwao
Copy link
Member

yangwao commented Apr 4, 2022

However, I have to use two queries including nftListOwned(just added) and allCollectionSaleEvents, to get all holding information. On addresses with a large number of NFTs, performance will be poor and it may cause a server-side error.

We can do special magic and cache some queries which loads superior fast, same technique we are using in spotlight and series-insight iirc, as it's huge query.

@yangwao
Copy link
Member

yangwao commented Apr 4, 2022

Testcase

https://deploy-preview-2751--koda-nuxt.netlify.app/rmrk/u/FXUxw9Xp98gTzrU2z63D2G92kcx1oznd6xYa8WdqRgTcEDo?tab=holdings

Oh, I see an issue with big queries now. Seems giving me 504 which is gateway timeout, I'll ping the Subsquid squad so let's see how they can help us on this track

image

image

@Jarsen136
Copy link
Contributor Author

We can do special magic and cache some queries which loads superior fast, same technique we are using in spotlight and series-insight iirc, as it's huge query.

Yes, you are right. And I think the quickest solution is to enable the nftListOwned interface to return events for specific nfts. It can obviously solve the problem of too large response data.

@vikiival
Copy link
Member

vikiival commented Apr 4, 2022

Specifically, it is to enable the nftListOwned interface to return the result of the allCollectionSaleEvents interface.

Not sure what do you mean.

@Jarsen136
Copy link
Contributor Author

Jarsen136 commented Apr 4, 2022

Specifically, it is to enable the nftListOwned interface to return the result of the allCollectionSaleEvents interface.

Not sure what do you mean.

Hey, @vikiival Let me expand more:

Current process:
nftListOwned -> receive specific nfts list-> generate collectionId list -> allCollectionSaleEvents -> receive all events list of specific collections

What process I expected:
nftListOwned -> receive the events list of specific nfts
And the structure of events should be the same as what allCollectionSaleEvents return : )

    meta
    interaction
    blockNumber
    timestamp
    caller
    id
    nft {
      id
      name
    }

@vikiival
Copy link
Member

vikiival commented Apr 4, 2022

#2739 (comment)

@vikiival
Copy link
Member

vikiival commented Apr 4, 2022

Here you go sir

query collectionHoldingsByAccount($id: String!) {
  collectionEntities(where: {nfts_some: {currentOwner_eq: $id}}) {
    id
    name
    nfts(where: {currentOwner_eq: $id}) {
      id
      price
      updatedAt
      bought: events(where: {caller_eq: $id, interaction_eq: BUY}, orderBy: timestamp_DESC, limit: 1) {
        id
        meta
      }
    }
  }
}

@vikiival
Copy link
Member

vikiival commented Apr 4, 2022

feel free to test here

query variables:

{
  "id": "Fksmad33PFxhrQXNYPPJozgWrv82zuFLvXK7Rh8m1xQhe98"
}

In case of last actitivity you can extend the query with

      lastUpdate: events(orderBy: timestamp_DESC, limit: 1) {
        id
        interaction
        blockNumber
        timestamp
        meta
      }
    }

@Jarsen136
Copy link
Contributor Author

A hovercard could be added as well as now it doesn't work :|

Can make a separate issue or make it here with extra bounty, up to you.

image

okey, i will make it here.

@yangwao
Copy link
Member

yangwao commented Apr 6, 2022

Oh, I've just noticed by using it,
would be nice if when I

  • go from collection holders tab
  • Click on the user in the table
  • would redirect me to the holdings tab of the user directly

Small tweak which makes contextual sense :)

@Jarsen136
Copy link
Contributor Author

Jarsen136 commented Apr 6, 2022

A hovercard could be added as well as now it doesn't work :|

Can make a separate issue or make it here with extra bounty, up to you.

image

I'm not sure what kind of card do you would like to add : ) as we only have the user card component. Could you give me an example? @yangwao

image

Or we can make a separate issue to make it and build a new card component? Would be great if assign me.

@Jarsen136
Copy link
Contributor Author

Jarsen136 commented Apr 6, 2022

Oh, I've just noticed by using it,
would be nice if when I

go from collection holders tab
Click on the user in the table
would redirect me to the holdings tab of the user directly
Small tweak which makes contextual sense :)

I have finished it.
And I also fix the bug mentioned at #2753, so that it can directly jump to the holdings tab when clicking on the user name.

@Jarsen136 Jarsen136 requested a review from roiLeo April 6, 2022 12:37
@yangwao
Copy link
Member

yangwao commented Apr 6, 2022

@Jarsen136
Copy link
Contributor Author

@yangwao
Copy link
Member

yangwao commented Apr 6, 2022

Once @roiLeo has no objections, let's merge it!

@roiLeo roiLeo self-requested a review April 6, 2022 14:44
Copy link
Contributor

@roiLeo roiLeo left a comment

Choose a reason for hiding this comment

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

code is error free, we might need to replace all any type later

@Jarsen136
Copy link
Contributor Author

code is error free, we might need to replace all any type later

Ok, I will replace these 'any' type later in this issue #2785

@yangwao
Copy link
Member

yangwao commented Apr 6, 2022

pay 300 usd

@yangwao yangwao merged commit cdc0f68 into kodadot:main Apr 6, 2022
@yangwao
Copy link
Member

yangwao commented Apr 6, 2022

interesting lol
Screenshot 2022-04-06 at 17 04 48

@yangwao
Copy link
Member

yangwao commented Apr 6, 2022

pay 300 usd

@yangwao
Copy link
Member

yangwao commented Apr 6, 2022

😍 Perfect, I’ve sent the payout
💵 $300 @ 174.83 USD/KSM ~ 1.716 $KSM
🧗 Caiv9TbPz68q5dC8EcHu5xKYPRnremimGzqmEejDFNpWWLG
🔗 0xe7d1ae5e9be3a1a257fbc19c2f3fd346a70c26f9e64fb2e6b853b4cd935d161f

🪅 Let’s grab another issue and get rewarded!
🪄 github.com/kodadot/nft-gallery/issues

@yangwao yangwao added the paid pull-request has been paid label Apr 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
paid pull-request has been paid
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Holdings by User
5 participants