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.

Sign up for GitHub

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

Jump to bottom

feat(1115): flipping nfts #88

Closed
wants to merge 11 commits into from
Closed

feat(1115): flipping nfts #88

wants to merge 11 commits into from

Conversation

KngZhi
Copy link
Contributor

@KngZhi KngZhi commented May 23, 2022

  • feat: model of flipping nft
  • feat: resolver

vikiival
vikiival requested changes May 23, 2022
View reviewed changes
src/server-extension/query/nft.ts Outdated Show resolved Hide resolved
src/server-extension/query/nft.ts Outdated Show resolved Hide resolved
src/server-extension/resolvers/flippingNfts.ts Outdated Show resolved Hide resolved
vikiival
vikiival reviewed May 24, 2022
View reviewed changes
Copy link
Member

@vikiival vikiival left a comment

Choose a reason for hiding this comment

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

noice

@KngZhi KngZhi marked this pull request as ready for review May 24, 2022 07:00
@KngZhi KngZhi mentioned this pull request May 26, 2022
Closed
17 tasks
@KngZhi KngZhi requested a review from vikiival May 27, 2022 06:45
@KngZhi
Copy link
Contributor Author

KngZhi commented May 27, 2022

https://github.com/kodadot/ideas/issues/7

@KngZhi KngZhi changed the title 1619 flipping nfts feat(1619): flipping nfts May 27, 2022
@vikiival vikiival changed the title feat(1619): flipping nfts feat(1115): flipping nfts May 27, 2022
vikiival
vikiival requested changes May 27, 2022
View reviewed changes
src/server-extension/model/nft.model.ts Outdated Show resolved Hide resolved
src/server-extension/model/nft.model.ts
import { Field, ObjectType } from 'type-graphql';

@ObjectType()
export class FlippingNFT {
Copy link
Member

Choose a reason for hiding this comment

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

may a bit better name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I should change all those Flipping to MostTraded?

src/server-extension/query/nft.ts Show resolved Hide resolved
src/server-extension/resolvers/flippingNfts.ts Outdated Show resolved Hide resolved
@vikiival
Copy link
Member

As I have nothing specific for this PR.
I honestly think that this should be cached the same way as spotlight or series.

The computation on the DB is overkill.
I can spawn @eldargab

@eldargab
Copy link

I'm lacking context here. Generally plain database queries are of course better unless it becomes expensive.
May be @ma-shulgin can provide a better input.

@vikiival
Copy link
Member

Hey @KngZhi we had some issues with a subsquid node that was unable to process. With your PR I have a feeling that we can buy a ⚰️ for the processor.

What are the next steps:

  • It can't go out as a resolver.
  • so preferrably I would make a caching and a new entity (similar stuff what has been done in typo in FAQ nft-gallery#87)

@KngZhi
Copy link
Contributor Author

KngZhi commented Jun 1, 2022

okay, I would alter that ASAP.

@KngZhi
Copy link
Contributor Author

KngZhi commented Jun 2, 2022 •
edited
Loading

Hey @vikiival, after I checked with kodadot/nft-gallery#87 I kind of feel it is unable to do so.

Some fields have relied on the previous query results, there are two sequenced queries.

This was referenced Jun 2, 2022
Open
Merged
@petersopko
Copy link
Contributor

@KngZhi what's the status of this PR? we have now two PRs (this one and kodadot/nft-gallery#3063) which are 3 weeks old and still open 😢

@vikiival
Copy link
Member

I wont merge this as resolver.

We are already running out of memory and this is simply overkill

@petersopko
Copy link
Contributor

based on @vikiival comments, my understanding here is that this should be done as extension of cache.ts not as separate resolvers. let's close this one since it's sitting here for a while, and reopen with extended cache whenever you feel ready to do that @KngZhi

@petersopko petersopko closed this Jul 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vikiival vikiival vikiival requested changes

Assignees
No one assigned
Labels
blocked
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@KngZhi @vikiival @eldargab @petersopko