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

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

Open
6 of 13 tasks
Hosuke opened this issue Jun 20, 2024 · 3 comments
Open
6 of 13 tasks
Assignees
Labels
enhancement New feature or request in review Assignee is currently reviewing the PR

Comments

@Hosuke
Copy link
Collaborator

Hosuke commented Jun 20, 2024

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
  1. 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
Copy link
Member

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:

  • dex_aggregators_base_trades
  • unions all project views

then in dex_aggregator_trades:

  • reads from base trades above, joins to prices.usd to obtain amount_usd

does this sound reasonable?

@jeff-dude jeff-dude self-assigned this Aug 29, 2024
@jeff-dude jeff-dude added the in review Assignee is currently reviewing the PR label Aug 29, 2024
@jeff-dude
Copy link
Member

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
Copy link
Collaborator Author

Hosuke commented Aug 30, 2024

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.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request in review Assignee is currently reviewing the PR
Projects
None yet
Development

No branches or pull requests

2 participants