feat: Implement BadTokenMonitor #3153
Conversation
|
CLA Assistant Lite bot: I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request |
There was a problem hiding this comment.
We already started working on this internally (PR pending) which matches more what we imagined so this PR will probably not get merged but we appreciate your contribution a lot.
In any case I left a review to give some feedback that's hopefully helpful to you.
| @@ -72,6 +72,7 @@ pub const TABLES: &[&str] = &[ | |||
| "auction_participants", | |||
| "app_data", | |||
| "jit_orders", | |||
| "bad_tokens" | |||
There was a problem hiding this comment.
I'd like to avoid persisting bad tokens in the DB. At least from the start we should collect the heuristics only based on an in-memory cache.
This trades code complexity for accuracy after restarts. Also so far the driver does not depend on any DB at all which would be an additional piece of infra external teams would have to maintain.
| @@ -57,6 +60,8 @@ impl Api { | |||
| app = routes::metrics(app); | |||
| app = routes::healthz(app); | |||
|
|
|||
| let mut bad_token_monitors = BadTokenMonitor::collect_from_path("./src/infra/bad_token/configs", &solvers); | |||
There was a problem hiding this comment.
Things like this should be passed via arguments to the program.
Instead of introducing another file this could be part of the existing driver config file (each solver could have their own section with supported tokens).
| eth: self.eth.clone(), | ||
| liquidity: self.liquidity.clone(), | ||
| bad_token_monitor: { | ||
| let mut monitor = bad_token_monitors.remove(&solver.address()).unwrap(); |
There was a problem hiding this comment.
This function already returns a Result so having error handling instead of .unwrap() would be very easy.
| timespan = 3 | ||
|
|
||
|
|
||
| [[tokens.supported]] |
There was a problem hiding this comment.
Why are there multiple entries for tokens.supported? A list should be defined in a single array, I think.
Also it might be nicer to have a mapping token => supported_enum to avoid error cases where the same token is part of both lists.
|
|
||
| pub struct ConfigFile { | ||
| solver: Option<String>, | ||
| tokens: Option<TomlValue>, |
There was a problem hiding this comment.
Raw TomlValues should be avoided given that we could use serde to parse the file into proper rust types.
| tokens: Option<TomlValue>, | ||
| timespan: Option<u32>, | ||
| heuristic: Option<TomlValue>, | ||
| mode: Option<String>, |
There was a problem hiding this comment.
mode should be parsed into an enum right away
| solver: Address, | ||
| supported_tokens: Vec<TokenAddress>, | ||
| unsupported_tokens: Vec<TokenAddress>, | ||
| timespan: u32, // needs to be discussed |
There was a problem hiding this comment.
Seems like this should rather be a Duration.
| } | ||
| } | ||
|
|
||
| pub struct ConfigFile { |
There was a problem hiding this comment.
Seems like the names might have gotten mixed up?
ConfigFile does not implement Deserialize but Config does.
| deny_list: HashSet<TokenAddress>, | ||
| explicit_deny_list: BTreeSet<TokenAddress>, | ||
| explicit_allow_list: BTreeSet<TokenAddress>, |
There was a problem hiding this comment.
What's the reason that some lookups are BTree based and some hash based?
| if threshold.threshold <= threshold_state.count + 1 { | ||
| self.deny_list.insert(token.to_owned()) | ||
| } |
There was a problem hiding this comment.
Unclear how the heuristic is supposed to work. Overall having doc comments (///) on all the relevant types would be helpful.
I'd imagine the heuristic to keep track of how often a token is involved in a solution that failed to simulate. Here it looks like it would just iterate over all the tokens in the auction without any indication whether they were good or not.
|
Thank you for giving this much feedback. Other than the design choice of not persisting in the postgres db, I've had my reasons and justifications for every thing else. I wanted to contribute to get attention and to show my skills as I am in need of a job. But what seems to me that, in this repository most of the issues are either internally discussed or internally solved. Not much from what I can pick up on. If you a have job opening, I am open to get some help. |
|
Thank you for taking a shot. We have a Let me know if you are willing to take another shot with other tasks as well. I am willing to guide you through execution. Also, you can find more hiring info here: https://cow.fi/careers |
|
@sunce86 thank you for replying. Yes, I am willing to try other tasks. Please guide me through them. And yes, I knew about the careers page, but for that I don't have the relevant on-paper experience even though I have the necessary skills for it. So, I figured, the only way to prove my skills was through open source contributions. |
|
@usagi32 Thank you so much for taking the initiative! It's always great to see enthusiasm and proactivity from people interested in joining our team. Regarding our current job opening, as you mentioned, it does require a certain level of seniority. That said, let's connect on LinkedIn (you can find my profile on my page) and stay in touch for future opportunities. I'd be happy to discuss things further if you're open to exploring possibilities down the line! |
addresses #3094 and #3092
There will be some mistakes as I did not test this code but this should be suffice for initial implementation.