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

feat: utility to calculate quote amounts and costs #210

Merged
merged 7 commits into from
Jun 6, 2024

Conversation

shoom3301
Copy link
Contributor

This is a refactored and improved version of:

  1. https://github.com/cowprotocol/cowswap/blob/0132c63db04a605826df6086480613faacacff97/apps/cowswap-frontend/src/legacy/state/swap/TradeGp.ts
  2. https://github.com/cowprotocol/cowswap/blob/f0bf8cbc57f4983de31929b848bd544d0a47294a/apps/cowswap-frontend/src/legacy/state/swap/extension.ts

Goals of the refactoring:

  1. Reduce count of inputs and abstractions. The only source of data should be OrderParameters which is a DTO from /quote API.
  2. Replace *withFee / *withoutFee with *afterFees / *beforeFees. Depending on sell/buy order we substract/add network costs to the amounts and it's not super clear what with/without means.
  3. Differentiate network costs and partner fees. Currently, network costs are named as feeAmount in OrderParameters which is not super clear.
  4. Get rid of Uniswap abstractions in favor of native bigint.
  5. Share and reuse the utility in CoW Swap and Explorer (at least).

@shoom3301 shoom3301 requested review from mfw78 and a team May 29, 2024 15:38
@shoom3301 shoom3301 self-assigned this May 29, 2024
@coveralls
Copy link
Collaborator

coveralls commented May 29, 2024

Coverage Status

coverage: 79.56% (+0.8%) from 78.809%
when pulling 60dcedf on feat/quote-and-amounts-costs
into 16155d1 on main.

@shoom3301 shoom3301 force-pushed the feat/quote-and-amounts-costs branch from ce4738f to 7a1d600 Compare May 29, 2024 15:40
src/order-book/types.ts Outdated Show resolved Hide resolved
@shoom3301 shoom3301 force-pushed the feat/quote-and-amounts-costs branch from 7a1d600 to 143c42b Compare May 31, 2024 12:34
Copy link

socket-security bot commented May 31, 2024

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

@cowprotocol cowprotocol deleted a comment from socket-security bot Jun 3, 2024
Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Soft approve. I agree with leandro that the price being a number is error prone. As a financial app is better to not have rounding issues

src/order-book/types.ts Outdated Show resolved Hide resolved
@alfetopito
Copy link
Contributor

Soft approve. I agree with leandro that the price being a number is error prone. As a financial app is better to not have rounding issues

Approved, but keep in mind that even after removing the price from the response, the calculation still uses native number for deriving the returned values.

@mfw78 mfw78 removed their request for review June 6, 2024 04:44
@shoom3301 shoom3301 merged commit 38ee550 into main Jun 6, 2024
9 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jun 6, 2024
@alfetopito alfetopito deleted the feat/quote-and-amounts-costs branch June 6, 2024 14:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants