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 highest sale to series insight #54

Merged
merged 7 commits into from
Apr 28, 2022
Merged

Conversation

KngZhi
Copy link
Contributor

@KngZhi KngZhi commented Apr 17, 2022

  • fix: ambiguous name column error block squid processor
  • feat: add highest sale to series insight

Related Issue: issues-877

Hey @vikiival, I get two potential ways to find out the highest sale, could spot me for picking the right one. I'm still confused a bit about the meaning of the scheme.

  1. the current code using same query statement of collection max buy
  2. using the logic as floor_price, SQL should be MAX(NULLIF(ne.price, 0)) as highest_sale

Thanks a lot!! And by the time, if there is any issue or refactor should be made, pls don't hesitate to point it out.

This is my first time making a contribution on rubick, excited 😄.

@KngZhi KngZhi mentioned this pull request Apr 17, 2022
2 tasks
@KngZhi KngZhi changed the title add highest sale to series insight feat: add highest sale to series insight Apr 18, 2022
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.

  • I don't see highest_sale in series cached query
  • I would add highest_sale in OrderBy series resolver

the rest seems to me correct

edit:
would be top if we can remove burned nft from the queries
WHERE ne.burned != 'false' (related with #47)

@KngZhi
Copy link
Contributor Author

KngZhi commented Apr 20, 2022

@roiLeo Thanks!!! updated.

Edit:
hmm, I see, let me revise that.
Maybe fix both series and spotlight in one commit by the same query condition.

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.

why do I see highest_price in db migrations?
do you think it's better to remove burned nft in another PR?

src/server-extension/resolvers/series.ts Outdated Show resolved Hide resolved
@KngZhi
Copy link
Contributor Author

KngZhi commented Apr 20, 2022

why do I see highest_price in db migrations?

Since we add a new column, I am thinking that we should update DB migrations.
Don't know if I understand correctly?

do you think it's better to remove burned nft in another PR?

Yes, cause I also want to do some refactoring, and extract those same queries(both in cache and resolvers) into one file.

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.

keep it as highest sale (not highest price)

db/migrations/1650183454615-Data.js Outdated Show resolved Hide resolved
@KngZhi KngZhi requested review from vikiival and roiLeo April 21, 2022 04:08
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.

I don't see the highest_sale used in the SQL queries

@KngZhi
Copy link
Contributor Author

KngZhi commented Apr 27, 2022

@vikiival vikiival merged commit c3f62d4 into kodadot:main Apr 28, 2022
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.

3 participants