Balancer Event Handler for WeightedPool2TokenFactory #717
Conversation
There was a problem hiding this comment.
Would it make sense to have two even handler classes (one for WeightedTwoToken pools and another one for WeightedGeneral pools)? This way we might make the class generic on the Event type (to reduce code duplication) and only implement the conversion from WeightedTwoTokenEvent to RegisteredWeightedPool and WeightedGeneralEvent to RegisteredWeightedPool. This way the event handler itself wouldn't have to take the event type as an argument any more (and do custom logic in the deletion/insertion of events)
For the consumer that wants to get all RegisteredWeightedPools, we could have an aggregator fetching all RegisteredWeightedPool from the two event processing classes.
Just a thought, will leave it up to you and @nlordell to decide on a design. It might also be worth while thinking ahead of how different pools will eventually map to Liquidity types that we pass in to the solver. Maybe that influences the choice of data structures?
shared/src/balancer/event_handler.rs
Outdated
| #[derive(Clone, Debug, Default, Eq, PartialEq, Hash)] | ||
| #[derive(Clone, Copy, Debug, Eq, PartialEq, Hash)] | ||
| pub enum PoolType { | ||
| WeightedGeneral, |
There was a problem hiding this comment.
What is the difference in how the services will handle a general vs a weighted two token pool? Asking, because it's not yet clear to me why we need to differentiate between the two (I assume they have different methods for swapping of fetching balances?)
There was a problem hiding this comment.
In this particular case, both types are the same as (far as I know). However, we can be certain that the other pool types will need to be differentiated for, at least, exactly the reason you mentioned (i.e. possible different transaction encoding and pool arithmetic).
In this PR, I partially kept things separate like this mostly in preparation for what it to come. However, it is very likely that both WeightedPool and WeightedPool2Tokens don't actually need to be separated.
There was a problem hiding this comment.
How is the pool arithmetic different between general weighted pools and weighted two token pools? For swapping, I was under the impression that all we need is the poolId and the desired amounts.
There was a problem hiding this comment.
I believe I said they are not different at all.
In this particular case, both types are the same
WeightedPool and WeightedPool2Tokens don't actually need to be separated.
What I was trying to convey is that the arithmetic might be different for pools of other types.
This pool type distinction is merely introduced here in preparation for the other pool types to come.
There was a problem hiding this comment.
I see (sorry for misreading). My general feeling is that these other types might look much different (e.g. they may not have weights on them) and so they might have to be a different struct altogether? Maybe it would be helpful to draw up just the data model draft for how we want to represent all balancer pools eventually?
There was a problem hiding this comment.
Yes, certainly we will want to do this. However, we have decided that at the particular moment, we will remove the pool type since these two pools don't require it and only to reintroduce this when it is absolutely necessary. With this in mind, I will sketch up a description of how each are different/handled internally when the time comes.
There was a problem hiding this comment.
From a cursory glance at the Vault code, they behave the same from the caller's point of view. The difference is only done to enable internal optimiations (2Token vs MinSwapInfo pools). THis likely means we can consider them the exact same from the Baseline solver point of view.
There was a problem hiding this comment.
I think ultimately we would want something like:
enum BalancerPool {
Weighted(WeightedPool),
Stable(StablePool),
// ...
}
There was a problem hiding this comment.
There are only two (ish) pool types WeightedPool (including TwoTokens) and StablePool.
The transaction encoding is likely the same for both since we perform swaps via the Vault contract
While the arithmetic is different for each since Stable Pools use StableMath library while Weighted Pools uses WeightedMath.
This means we will need to introduce a PoolType enum (as @nlordell suggested above) down the road (i.e. when we include StablePools) that the price estimating will need to distinguish between.
To elaborate a bit more on Stable Pools and how they differ from our current WeightedPool struct:
Stable Pools do not have weights, but they do have an amplificationParameter (which is not part of the weighted pools) that is passed though the methods _onSwapGivenIn and _onSwapGivenOut. This amplification parameter changes on everyblock and can be fetched via a public view method getAmplificationParameter.
Specifically, these methods call into
StableMath._calcOutGivenIn(currentAmp, balances, indexIn, indexOut, swapRequest.amount)
We note that generically getAmplificationParameter could be evaluated from the value passed into the constructor and the current block height, but there are some manual overrides to the value so we probably shouldn't attempt to compute its current value manually.
With this in mind I propose that
pub struct RegisteredStablePool {
pub pool_id: H256,
pub pool_address: H160,
pub tokens: Vec<H160>,
pub scaling_exponents: Vec<u8>,
block_created: u64,
}Which is essentially the same as RegisteredWeightedPool without the weights.
Our PoolRegistry Would stay similar except that We would replace the values of the pools hashmap with BalancerPool where
enum BalancerPool {
Weighted(WeightedPool),
Stable(StablePool),
}
The PoolFetching, which is responsible for getting pool reserves, would then also need to get the current Amplification value whenever it is faced with a StablePool.
There was a problem hiding this comment.
Created an issue and linked it here to track the collected information #733
|
I have reworked the implementation here having one |
There was a problem hiding this comment.
Looks great!
Just requesting changes as I believe that the use of HashSet for a collection of RegisteredWeightedPool is slightly incorrect. I think the same applies for:
gp-v2-services/shared/src/balancer/event_handler.rs
Lines 148 to 171 in e77938e
Which I missed when reviewing #698 (here, the pools are already unique by pool ID so there is no need to collect them in a HashSet and a Vec makes more sense).
…instead of HashSet
| BalancerV2WeightedPoolFactoryContract(weighted_pool_factory), | ||
| PoolStorage::new(Box::new(PoolDataFetcher { | ||
| web3: web3.clone(), | ||
| token_info_fetcher: token_info_fetcher.clone(), |
There was a problem hiding this comment.
Is cloning the correct way to pass the token info fetcher into both event handlers?
There was a problem hiding this comment.
It is behind an Arc (i.e. Atomically Reference Counted pointer), so cloning is a very cheap operation (it just increases the reference count - i.e. the number of "owners" to the inner data).
It is the correct way of sharing an Arc.
There was a problem hiding this comment.
Cool, now I am just waiting on @fleupold's final thoughts and I would like to ship-it.
Disclaimer
Not yet satisfied with the amount of code duplication and looking for some assistance. I believe the reason for the duplication stems from this issue in ethcontract-rs, but I may be wrong. Maybe we could have some sort of
enum ContractRouterthat can distinguish between the two different factories and somehow reduce this redundancy...Change Summary
WeightedPool2TokensFactorycontract artifactsBalancerEventUpdaterto stream events from both factories and store all pools accordingly.Test Plan
You can try this out yourself in another branch (cf #710) by running
Results are as follows: