duneanalytics / spellbook

SQL views for Dune
Other
1.16k stars 1.09k forks source link

[Enhancement] Refactor `amount_usd` Calculation in `dex_aggregators.trades` #6215

Open Hosuke opened 3 months ago

Hosuke commented 3 months ago

Description

The current implementation of amount_usd calculation in dex_aggregators.trades requires a refactor to improve clarity and maintainability. I propose categorizing the aggregators into three distinct classes based on their amount_usd calculation methods:

  1. Direct USD Value from Contracts: Aggregators that obtain the usd_value directly from the smart contract. These aggregators rely on the contract to provide the precise USD value of the trades.

  2. Custom amount_usd Calculation: Aggregators that have their own method for calculating amount_usd. These methods might involve custom logic or external data sources to determine the USD value of the trades.

    cow_protocol_trades
    bebop_trades
  3. Conversion from amount_raw to amount_usd: Aggregators that first calculate a raw amount (amount_raw) and then convert it to amount_usd. This conversion typically involves using exchange rates or other conversion factors to determine the final USD value.

    paraswap_trades
    lifi_trades
    yield_yak_trades
    unidex_optimism_trades
    firebird_finance_optimism_trades

Aggregators List:

jeff-dude commented 1 month ago

finally getting to review this 🙏

i'm looking at #6229 first. from what i see, we are removing prices.usd joins and amount_usd from chain-level spells which read from source decoded tables. this is a good first step. however, it appears next we are adding the amount_usd logic back in at the cross-chain, project-level view?

i think we want to push it down the lineage further. the crosschain project views would also not use amount_usd. we want to introduce a new spell in between all these project views and the final dex_aggregator_trades:

then in dex_aggregator_trades:

does this sound reasonable?

jeff-dude commented 1 month ago

with the above in mind, if we want to use this design, we would want one PR per project to remove amount_usd, then a PR to do final dex_aggregator_base_trades and dex_aggregator_trades, so that we can merge them all together and ensure end result matches current

Hosuke commented 1 month ago

Let me check if we can remove amount_usd from all aggregators. (Those with PRs are feasible, but I am not sure for the rest.)