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

[0xBoxer]

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

spellbook/dbt_subprojects/dex/models/_projects/oneinch/oneinch_lop_own_trades.sql

Line 39 in 90c8c37

, row_number() over(partition by tx_hash order by call_trace_address) as evt_index

[jeff-dude]

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

[grkhr]

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]

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