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

Adjust the sql query for total surplus #3090

Merged
merged 6 commits into from
Nov 5, 2024
Merged

Conversation

sunce86
Copy link
Contributor

@sunce86 sunce86 commented Oct 28, 2024

Description

Fixes #3089

Changes

Includes orders from "jit_orders" to calculate the total surplus.

  • Updated sql query.

How to test

Executed manually the new query against prod db. Two things are verified:

  1. https://explorer.cow.fi/address/0xbf8868b754a77e90ea68ffc0b5b10a7c729457e1 now returns non-zero surplus.
  2. Regular owners surplus remains the same, so this change did not break existing functionality.

Performance should be good since "jit_orders" table also has index on owners.

@sunce86 sunce86 self-assigned this Oct 28, 2024
@sunce86 sunce86 requested a review from a team as a code owner October 28, 2024 14:35
@sunce86 sunce86 added the bug Something isn't working label Oct 28, 2024
Copy link
Contributor

@MartinquaXD MartinquaXD left a comment

Choose a reason for hiding this comment

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

Change looks alright but agree with @squadgazzz's comment.

Performance should be good since "jit_orders" table also has index on owners.

Can you please compare the performance of the surplus query for an account that has many regular orders by manually executing the old and the new query?
Would be a shame if we pessimise the performance of the common use case by adding support for jit orders.

@sunce86
Copy link
Contributor Author

sunce86 commented Nov 4, 2024

Change looks alright but agree with @squadgazzz's comment.

Performance should be good since "jit_orders" table also has index on owners.

Can you please compare the performance of the surplus query for an account that has many regular orders by manually executing the old and the new query? Would be a shame if we pessimise the performance of the common use case by adding support for jit orders.

I compared it previously with a random owner and it was comparative. But for the sake of mind, I searched for owners with most entries in "orders" table and used them to test the performance.

With owners that have 10-20k entries, this version of code introduces the least performance hit, practically negligible.

@sunce86 sunce86 enabled auto-merge (squash) November 5, 2024 10:14
@sunce86 sunce86 merged commit 8273cd8 into main Nov 5, 2024
11 checks passed
@sunce86 sunce86 deleted the jit-orders-contribute-surplus branch November 5, 2024 10:19
@github-actions github-actions bot locked and limited conversation to collaborators Nov 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: JIT orders not considered in total surplus endpoint
4 participants