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

fix(trading-sdk): fix order amounts calculations #231

Merged
merged 1 commit into from
Jan 22, 2025

Conversation

shoom3301
Copy link
Contributor

@shoom3301 shoom3301 commented Jan 21, 2025

Fixes first issue from the doc.

This change fixes both sell and buy orders.
It also fixes "Private Key example not working" issue.

Basically, I reverted this commit: 718b212

Unfortunately, I don't remember why I did this :(
But obviuosly this logic is wrong, because it adds fees + slippage twice:

  • once in swapParamsToLimitOrderParams
  • and another one in getOrderToSign (getQuoteAmountsAndCosts call)

I compared the result with CoW Swap and it matches 100%.
How to test that:

  1. Copy quoteResponseMock from unit test
  2. Go to apps/cowswap-frontend/src/api/cowProtocol/api.ts in CoW Swap
  3. Return the value of copied mock from getQuote() function
  4. Open CoW Swap and setup a trade correspondingly to the mock
  5. Init order signing
  6. Compare sell and but amounts in the wallet with values in unit tests

@shoom3301 shoom3301 requested a review from a team January 21, 2025 14:28
@shoom3301 shoom3301 self-assigned this Jan 21, 2025
@shoom3301 shoom3301 force-pushed the fix/trading-sdk-amounts branch from ccdbe70 to ad70522 Compare January 21, 2025 14:35
@coveralls
Copy link
Collaborator

coveralls commented Jan 21, 2025

Coverage Status

coverage: 78.463% (+0.09%) from 78.378%
when pulling ad70522 on fix/trading-sdk-amounts
into c379396 on main.

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.

Cool, I will test in the script after publishing to NPM. If you want earlier validation, I could debug it locally.

Let me know

...originalFetch,
default: fetchMock,
}
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this soimething we want to mock always or just in this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The mock is specific for sell/buy orders, so I don't think we can use it by default

@shoom3301
Copy link
Contributor Author

Cool, I will test in the script after publishing to NPM

@anxolin appreciate it 🙏

@shoom3301 shoom3301 merged commit 47ea798 into main Jan 22, 2025
9 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jan 22, 2025
@alfetopito alfetopito deleted the fix/trading-sdk-amounts branch January 23, 2025 13:02
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.

3 participants