Create solve competition api #188
Conversation
vkgnosis
force-pushed
the
competition-route-impl
branch
from
ad3c03e
to
8a16fee
Compare
|
|
||
| pub fn post( | ||
| handler: Arc<SolverCompetition>, | ||
| expected_auth: String, |
There was a problem hiding this comment.
Instead of adding auth code - we can just disallow POST solver_competition at the ingress level. We already connect to the orderbook using a cluster-local URL (so doesn't go through the ingress) and makes it so we don't need to manage secrets.
There was a problem hiding this comment.
Not sure. Doing it only at the ingress level has the downside that an accidental misconfiguration could expose it. And we need to deal with another kubernetes configuration layer that I'm not familiar with. On the other hand it does make the code simpler and we don't want to keep this code for a long time anyway.
There was a problem hiding this comment.
Personally, I would prefer it at an ingress level (I tend to prefer having generic access control configured at an ingress level as Nginx does a really good job with this). Also this avoids having to add another command line argument and plumbing it through to the API all the while making running the orderbook and solver locally for testing much easier.
an accidental misconfiguration could expose it.
To me this isn't a super strong argument since the damage that can be done isn't that big (show incorrect information in the UI while waiting for the order to trade or lie to external solvers who will have incorrect data for some batches and will likely notice something is up).
That being said, code is already there, simple enough, and will hopefully go away in the near future, so I am fine with including it in the PR.
| // These types are going to be updated once the openapi documentation gets | ||
| // merged. Consider them a placeholder. | ||
|
|
||
| pub type SolverCompetitionResponse = HashMap<String, SolverSettlement>; |
There was a problem hiding this comment.
Shouldn't this be a Vec<(String, SolverSettlement)> if we want to allow multiple settlements from the same solver (which, AFAIU, we currently do right - because of settlement merging?).
There was a problem hiding this comment.
Like I wrote above there, this is a placeholder until we merge the Openapi PR. Then I will make sure the types are right.
There was a problem hiding this comment.
LGTM - only comment is that we should use a Vec<SolverSettlement> instead of a map so that:
- We can order by objective value 😄 (if you get the JSON its just a nicety that makes immediately seeing the winner and where you rank among the other settlements quicker).
- Allows for multiple settlements per solver. We technically have this with the merged settlements - although just uploading the "best" of the merged settlements also makes a lot of sense to me.
|
|
||
| #[derive(Clone, Debug, Default, Deserialize, Serialize, PartialEq)] | ||
| pub struct SolverSettlement { | ||
| objective_value: f64, |
There was a problem hiding this comment.
If we use a Vec instead of a HashMap we should include solver_name: String here.
|
Removed the authorization code. Planning on merging this and testing the ingress config on staging. |
vkgnosis
force-pushed
the
competition-route-impl
branch
from
ae4462d
to
54741f1
Compare
crates/orderbook/Cargo.toml
Outdated
| @@ -34,13 +35,13 @@ num = "0.4" | |||
| primitive-types = { version = "0.10", features = ["fp-conversion"] } | |||
| prometheus = "0.13" | |||
| prometheus-metric-storage = { git = "https://github.com/cowprotocol/prometheus-metric-storage" , tag = "v0.4.0" } | |||
| rand = "0.8" | |||
There was a problem hiding this comment.
I think this is no longer needed.
| rand = "0.8" |
vkgnosis
force-pushed
the
competition-route-impl
branch
from
335331d
to
3e2a73b
Compare
Codecov Report
@@ Coverage Diff @@
## main #188 +/- ##
==========================================
+ Coverage 64.58% 64.66% +0.08%
==========================================
Files 186 190 +4
Lines 38635 38741 +106
==========================================
+ Hits 24952 25053 +101
- Misses 13683 13688 +5 |
Part of #127 .
This is waiting on #185 to know the exact interface.
Next PR will make use of the post endpoint in the driver.
Test Plan
new unit tests