-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Some fixes for OpenSea, Seaport, LooksRare and X2Y2 #1536
Some fixes for OpenSea, Seaport, LooksRare and X2Y2 #1536
Conversation
Thanks so much for these great updates @hildobby ! Your proposal introduces I wonder if we should use an update statement for this that updates the @dsalv can you please look into this? |
Removed join on erc20 transfers for now as it results in duplication of buy events and duplicate `unique_trade_id` in some cases. fyi @hildobby an example of a tx that results in duplication is [0x04f019f90a5afea8bdc72dddccdf5f7fb5ec5cdce69877497ad35bdabe07f9c5](https://etherscan.io/tx/0x04f019f90a5afea8bdc72dddccdf5f7fb5ec5cdce69877497ad35bdabe07f9c5#eventlog) #1536 may very well be affected by this issue also.
@masquot looks like you had open concerns here. if you'd like me to look into it, please assign me to the PR & share a few updated thoughts on where you'd like to see this go |
@hildobby please merge the dune main branch into this branch. i tried, but i'm getting merge conflicts that conflict with other recent merge changes & i'm not 100% sure the direction needed |
Done @jeff-dude ✅ |
please see the dbt slim ci job, it's failing due to issues with columns in select statement in opensea |
@hildobby the job is still failing. i highly recommend running the dbt commands locally and finalizing. run a |
I run Also, I just reran |
definitely understand the pain points on compile/run/testing at the moment, but we will try to continue to enhance it. this PR involves multiple models, so we have to ensure we check them all. for instance, i cloned locally & ran
you can see it error out on a typo in a column name. this is what the gh action is spitting out for us |
I see that there tests are failing with opensea v1, but I am pretty sure this isn't caused by my PR but is a problem from the original abstraction, please do lmk if my assumption is incorrect! |
i'm seeing a few things:
|
Ah awesome ty!
Issued a new PR to fix it and I also checked on dune.com, runs fine now!
That doesn't seem to be caused by this PR, I'm already seeing the duplicate in current nft.trades table: https://dune.com/queries/1434604?d=1 |
you're right -- we currently exclude looksrare in production as it's been failing for this issue. once you submit new PR to fix |
Awesome, here is the fix PR for LooksRare @jeff-dude: #1824 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we're set here -- we'll look to merge and deploy
Brief comments on the purpose of your changes:
Buyer is sometimes the address of an aggregator, fixed that for OpenSea, Seaport, LooksRare & X2Y2 (This query should ideally always return nothing: https://dune.com/queries/1249102).
Also for LooksRare: fixed sometimes missing
token_standard
and changed NULLs to 0 for royalties/marketplaceplace fees & simplifiednumber_of_items
.FYI: Probably needs a full rerun of all those to apply the fixes retroactively :)
For Dune Engine V2
I've checked that:
General checks:
lowercase_snake_cased
Join logic:
Incremental logic: