Conversation
Breaking API Changes (Intentional)Breaking API changes detected and declared in the PR title. semver-checks output |
3f16269 to
3e39b64
Compare
louise-poole
left a comment
There was a problem hiding this comment.
This likely can be simplified more if the tycho-simulation is updated to take a config file directly. So far looks good though.
docs/guides/server-configuration.md
Outdated
| ## Blocklist config | ||
|
|
||
| Exclude specific components from routing, useful for components with known simulation issues (e.g., [rebasing tokens on UniswapV3 pools](https://docs.uniswap.org/concepts/protocol/integration-issues)): | ||
| By default, Fynd loads `blocklist.toml` from the working directory. The shipped default excludes components with known simulation issues (e.g., [rebasing tokens on UniswapV3 pools](https://docs.uniswap.org/concepts/protocol/integration-issues)). Override with `--blocklist-config`: |
There was a problem hiding this comment.
Maybe the default blocklist file should move to the tycho-simulation repo? As the reason it's blocklisted is because of simulation issues and not Fynd-specific issues.
There was a problem hiding this comment.
Do we want to always blocklist the pools defined here? even for solver users (not Fynd)? Or are these issues only for Fynd? I think I'm missing a bit of context here
There was a problem hiding this comment.
I have asked Thales and he mentioned, that the most common use cases are:
- this pool is faulty, so we want to to quickly remove this pool from the calculations
- user wants to avoid going through this pool - maybe they give fees to someone they don’t like, or this pool has a security flaw or doesn’t comply to any user’s internal compliance
d99168e to
471ff7b
Compare
dianacarvalho1
left a comment
There was a problem hiding this comment.
I'm quite confused 😬 Did you also have a look at this PR #166 ?
| self.config | ||
| .blocklisted_components | ||
| .clone(), |
There was a problem hiding this comment.
and shouldn't it take a file path? 😵💫 I'm quite confused tbh
There was a problem hiding this comment.
This add components to blocklist in the ComponentFilter and it takes an iterator of the component ids.
If it makes it easier to follow, I can add components to blocklist on the ProtocolStreamBuilder level, it is basically the same.
| /// Replaces the set of component addresses that are excluded from routing. | ||
| pub fn blacklisted_components(mut self, components: HashSet<String>) -> Self { | ||
| self.blacklisted_components = components; | ||
| /// Sets component IDs to exclude from the Tycho stream. |
There was a problem hiding this comment.
why are we keeping this here? Can you please explain the motivation in the PR description?
There was a problem hiding this comment.
reviewing this is quite confusing.. we accept a file in one place and read it and then in another place we accept already a hashset? I'm really not following 😕 Can you explain please?
There was a problem hiding this comment.
blocklisted_components() on FyndBuilder is the builder setter that lets callers configure which components to exclude. In build(), the value is passed to TychoFeedConfig (here), which forwards it to TychoFeed and that's where the actual filtering happens.
There was a problem hiding this comment.
aaaahhh ok then AG! I was just very confused when reviewing 😵💫
I did see it and as far as I understand it was an adhoc change, that would be replaced with a change in this PR |
dianacarvalho1
left a comment
There was a problem hiding this comment.
Looks good to me! let's just not merge this before we merge all the other related PRs and change the cargo.toml here!
eeeb226 to
d5b93c6
Compare
|
Since we now provide a blocklist file, do we still need |
ebe8789 to
6418f36
Compare
The blocklist filter itself lives in tycho-indexer, but blocklist.toml stays in fynd because it defines which pools fynd user considers problematic.