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

nft_mints macro #4608

Closed
wants to merge 26 commits into from
Closed

Conversation

tomfutago
Copy link
Contributor

No description provided.

@dune-eng
Copy link

Workflow run id 6524957814 approved.

@dune-eng
Copy link

Workflow run id 6524957757 approved.

@dune-eng
Copy link

Workflow run id 6525120080 approved.

@dune-eng
Copy link

Workflow run id 6525120166 approved.

@dune-eng
Copy link

Workflow run id 6525196578 approved.

@dune-eng
Copy link

Workflow run id 6525196574 approved.

@dune-eng
Copy link

Workflow run id 6525534819 approved.

@dune-eng
Copy link

Workflow run id 6525534816 approved.

@dune-eng
Copy link

Workflow run id 6525610143 approved.

@dune-eng
Copy link

Workflow run id 6525610132 approved.

@dune-eng
Copy link

Workflow run id 6525727086 approved.

@dune-eng
Copy link

Workflow run id 6525727197 approved.

@dune-eng
Copy link

Workflow run id 6525781261 approved.

@dune-eng
Copy link

Workflow run id 6525781262 approved.

@dune-eng
Copy link

Workflow run id 6525892697 approved.

@dune-eng
Copy link

Workflow run id 6525892678 approved.

@dune-eng
Copy link

Workflow run id 6544776524 approved.

@dune-eng
Copy link

Workflow run id 6544776776 approved.

@dune-eng
Copy link

Workflow run id 6576846071 approved.

@dune-eng
Copy link

Workflow run id 6596525175 approved.

@dune-eng
Copy link

Workflow run id 6596525181 approved.

@dune-eng
Copy link

Workflow run id 6596556236 approved.

@dune-eng
Copy link

Workflow run id 6596556245 approved.

@dune-eng
Copy link

Workflow run id 6596788301 approved.

@dune-eng
Copy link

Workflow run id 6596788421 approved.

@dune-eng
Copy link

Workflow run id 6596927633 approved.

@dune-eng
Copy link

Workflow run id 6596927631 approved.

@dune-eng
Copy link

Workflow run id 6598008875 approved.

@dune-eng
Copy link

Workflow run id 6598008800 approved.

@tomfutago
Copy link
Contributor Author

Goal: create nft.mints macro initially based on existing nft_ethereum_native_mints spell and then refactored to avoid long list of left joins + aggs (with another long list of group by columns) all in one final query - and then window function on top of it..

Notes:

  • there are a number of columns with hardcoded defaults - left them as they were, as they seem to replicate the same column list as nft.trades - not sure tho if there’s a plan to create macro/spell to combine the two?
  • similarly, seller column always defaults to burn address atm..
  • amount_raw, amount_original and amount_usd aggregated in 2 separate cte's per native or erc20 cost for the mint, which is meant to improve performance - still would be great if it was possible to replace traces with more performant table/spell - ideas welcome 🙏
  • extended uniqueness by adding currency_contract and the reason for it are cases with more than 1 erc20 transfer on nft mint. maybe not the best example, but that’s what happens when Uniswap V3 Positions NFT-V1 are minted and liquidity added to the pool
  • temporarily removed nft_optimism_native_mints spell from global nft_mints as it uses modified version of original nft_ethereum_native_mints spell, so needs to be looked at separately. and also, back to the note on the amounts.. : it sums them up, which for different erc20 tokens is incorrect imo. And then that row_number on the final query only takes highest amount_usd per mint 🤔

@jeff-dude linking this back to #4637

@tomfutago tomfutago marked this pull request as ready for review October 21, 2023 15:22
@jeff-dude jeff-dude requested a review from 0xRobin October 23, 2023 13:48
@jeff-dude jeff-dude added ready-for-review this PR development is complete, please review dbt: nft covers the NFT dbt subproject labels Oct 23, 2023
@jeff-dude
Copy link
Member

asking @0xRobin to take a quick look, as i know he's deep in the nft redesign efforts and this may be on the roadmap

Copy link
Collaborator

@0xRobin 0xRobin left a comment

Choose a reason for hiding this comment

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

I'll take a closer look at this this week!
Can you explain a bit more why you've chosen to exclude optimism and magiceden from the overall nft.mints model?

}}


{% set native_mints = [
ref('nft_ethereum_native_mints')
,ref('nft_optimism_native_mints')
Copy link
Collaborator

Choose a reason for hiding this comment

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

temporarily removed nft_optimism_native_mints spell from global nft_mints as it uses modified version of original nft_ethereum_native_mints spell, so needs to be looked at separately. and also, back to the note on the amounts.. : it sums them up, which for different erc20 tokens is incorrect imo. And then that row_number on the final query only takes highest amount_usd per mint 🤔

While I agree with your concerns with about the optimism model, we should not just take them out of nft.mints because of this.

@tomfutago
Copy link
Contributor Author

Thanks @0xRobin for looking into this! I should have properly clarify: this is still WIP and my initial attempt at adding this macro - just wanted to get some initial feedback before I go too deep while potentially going in completely wrong direction. Both exclusions are only temporary - my plan is to revisit them after getting the feedback 🙏

Hope that makes sense 🤞 can expand if/when needed.

@jeff-dude jeff-dude added WIP work in progress and removed ready-for-review this PR development is complete, please review labels Nov 6, 2023
@jeff-dude
Copy link
Member

closing due to #4819

@jeff-dude jeff-dude closed this Dec 6, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
dbt: nft covers the NFT dbt subproject WIP work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants