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

chore: Unique ID for /notify endpoint #2002

Closed
1 task
sunce86 opened this issue Oct 20, 2023 · 1 comment · Fixed by #2003
Closed
1 task

chore: Unique ID for /notify endpoint #2002

sunce86 opened this issue Oct 20, 2023 · 1 comment · Fixed by #2003
Labels
E:3.1 Driver Colocation See https://github.com/cowprotocol/pm/issues/14 for details

Comments

@sunce86
Copy link
Contributor

sunce86 commented Oct 20, 2023

Background

Inspired by the comment #1980 (comment), I thought about the best way to uniquely identify solutions received by solver engine, taking into consideration what is currently required/supported by the external solvers, but also, what would be useful considering the new driver implementation (now & in the future). Obviously, using only auction_id to identify notification is not enough because:

  1. We can forsee we will send notifications for driver's quote endpoint solutions also, and for those calls auction does not have defined auction id.
  2. Driver can even merge multiple solutions received from solver engines and create new artificial solutions that solver engines not even know about.

Details

Solver engines should generate id for each solution they send to the driver. Driver send the notification that has:

1. Option<auction_Id>
Still useful and needed to support the old notification module. Optional since we might want to send notification for the quote endpoint also.

2. NonEmptyVec<SolutionId(u64)>
Can contain multiple ids to represent the merged solution.

3. Additional data
Different for each specific notification, or even empty

For now, solver engines would generate the solution ids internally. Additionally, we can propose to add additional field id to SettledBatchAuctionModel for external solvers to use to identify solutions however suits them.

Acceptance criteria

  • For a single solve or quote call, solver engines should return solutions with unique IDs. There should be implemented check for this and appropriate observe and notify calls in case of error.
@sunce86 sunce86 added the E:3.1 Driver Colocation See https://github.com/cowprotocol/pm/issues/14 for details label Oct 20, 2023
@fleupold
Copy link
Contributor

We can forsee we will send notifications for driver's quote endpoint solutions also, and for those calls auction does not have defined auction id.

I think quotes should have some form of ID so that solvers can identify them. Otherwise I don't think it makes sense to receive a notification about something that you cannot map back to a concrete response (we send many quote requests per seconds).

As for the solution ID, I think this should be included in the driver <> solver engine communication (since solver engine can send multiple solutions). I would drop it in the solver engine <> external solver communication (since external solvers only ever send a single solution).

For driver generated/merged solutions I wouldn't send a notification (since solvers have nothing to do with that).

sunce86 added a commit that referenced this issue Oct 24, 2023
# Description
Fixes #2002

Now, with each notification, we are sending both auction_id and
solution_id which should be enough for all debugging purposes.

Solution ID is represented as `Vec<u64>` since it can contain multiple
solutions, as in case of merged settlements.
sunce86 added a commit that referenced this issue Oct 26, 2023
# Description
Implements (3) from #1891

Note that important consequence of the decision to not [send
notifications for merged
solutions](#2002 (comment))
is that submission result will not be sent for merged solutions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E:3.1 Driver Colocation See https://github.com/cowprotocol/pm/issues/14 for details
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants