Add univ3 router api #143
Add univ3 router api #143
Conversation
| pub fn new(client: Client, base: Url) -> Self { | ||
| Self { | ||
| client, | ||
| estimate: base.join("estimate").unwrap(), |
There was a problem hiding this comment.
nit: The current implementation of the uniswap router server interprets all requests as estimation requests. Should "/estimate" maybe be dropped here to avoid confusion?
Or should the "/estimate" endpoint be added to the uniswap router server?
There was a problem hiding this comment.
I documented that the endpoint is /estimate in the router's readme. Technically it replies this way to all requests but I want to have the option to add other routes in the future.
| #[tokio::test] | ||
| #[ignore] | ||
| async fn real() { | ||
| let api = Api::new(Default::default(), "http://localhost:8080".parse().unwrap()); |
There was a problem hiding this comment.
I guess for local setups it would be nice if the default port of the uni router would not conflict with the default port of the order book.
There was a problem hiding this comment.
I don't want to change the default port of other standalone projects to match this one. When you're running the test here you're probably not running the orderbook right now anyway.
| #[serde(with = "serde_with::rust::display_fromstr")] | ||
| pub gas: u64, | ||
| #[serde(deserialize_with = "deserialize_prefixed_hex")] | ||
| pub call_data: Vec<u8>, |
There was a problem hiding this comment.
nit: IMO using Bytes seems more appropriate than Vec<u8> but we are using both throughout the code base so I don't have a strong opinion here.
Would be nice to make it consistent in the code base eventually, though.
There was a problem hiding this comment.
I disagree with the use of Bytes but haven't consistently mentioned it in reviews. Bytes is is a "random" type from the web3 crate. No where do they document what it is used for and how it is serialized. So we shouldn't be relying on that. To me Vec is already bytes and how it is serialized is what you specify with the serde tag just like we do with the u64 above.
| use reqwest::{Client, Url}; | ||
| use serde::{Deserialize, Serialize}; | ||
|
|
||
| #[derive(Debug, Copy, Clone, Serialize)] |
There was a problem hiding this comment.
nit: #[serde(rename_all = "camelCase")]
There was a problem hiding this comment.
For this one I felt giving the enum values explicitly was a bit easier to understand.
Api for https://github.com/cowprotocol/univ3-api which will be used as a solver.
Test Plan
CI