[BUG] 1inch-LOP introduces duplicates into dex.trades

0xBoxer commented 3 days ago

Description

1inch-LOP models are introducing duplicates into dex.trades. Swap events that belong to other protocols are decoded as being 1inch LOP swaps with an incorrect evt_index number.

Example queries:

single example: https://dune.com/queries/4306571 multiple examples: https://dune.com/queries/4306524

Expected behavior

1inch-LOP model should not pick up these trades

Impacted model(s)

dex.trades
oneinch.lop

Possible solution

turns out that articifcially creating the event_index here is the problem

https://github.com/duneanalytics/spellbook/blob/90c8c376ee62a7c0ef187b88fdf55e4da79a601c/dbt_subprojects/dex/models/_projects/oneinch/oneinch_lop_own_trades.sql#L39

jeff-dude commented 3 days ago

@grkhr @max-morrow what do you think of this issue?

grkhr commented 3 days ago

These are not duplicates. In the first example everything is correct as in this TX a lot of 1inch limit orders have been filled (=our own liquidity).

The issue is in evt_index logic. We don't emit trade params in OrderFilled event, so we take them from traces, and there is no way to properly match subtrace with event index (e.g. join the trade from first example with the corresponding event), so we do row_number() over(partition by tx_hash order by call_trace_address) as evt_index to align with table's unique key.

In my honest opinion, evt_index shouldn't present in dex.trades at all, as it's assuming that the dex project will emit trade event, but generally speaking it doesn't have to. But in current architecture it's all we can do with evt_index.

I can only suggest adding trace_address to dex.trades and unique key

0xBoxer commented 1 day ago

fair assessment, adding trace_address seems logically consistent to me.

@jeff-dude I guess to not run into the null <> null problem we will need to deploy a surrogate key that combines a bunch of different factors together and can't be null

duneanalytics / spellbook