Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Investigate "found pools that don't correspond to any known Balancer pool factory " #343

Closed
vkgnosis opened this issue Jul 6, 2022 · 19 comments · Fixed by #371
Closed

Investigate "found pools that don't correspond to any known Balancer pool factory " #343

vkgnosis opened this issue Jul 6, 2022 · 19 comments · Fixed by #371

Comments

@vkgnosis
Copy link
Contributor

vkgnosis commented Jul 6, 2022 •
edited
Loading

We are currently logging this warning:

WARN shared::sources::balancer_v2::pool_fetching: found pools that don't correspond to any known Balancer pool factory total_count=3
unused_pools=
{
0x8df6efec5547e31b0eb7d1291b511ff8a2bf987c: RegisteredPools
    {
        fetched_block_number: 15088180,
        pools: [
            PoolData { pool_type: Stable, id: 0x178e029173417b1f9c8bc16dcec6f697bc32374600000000000000000000025d, address: 0x178e029173417b1f9c8bc16dcec6f697bc323746, factory: 0x8df6efec5547e31b0eb7d1291b511ff8a2bf987c, swap_enabled: true, tokens: [Token { address: 0x586aa273f262909eef8fa02d90ab65f5015e0516, decimals: 18, weight: None }, Token { address: 0x6b175474e89094c44da98b954eedeac495271d0f, decimals: 18, weight: None }, Token { address: 0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48, decimals: 6, weight: None }]},
            PoolData { pool_type: Stable, id: 0x2d011adf89f0576c9b722c28269fcb5d50c2d17900020000000000000000024d, address: 0x2d011adf89f0576c9b722c28269fcb5d50c2d179, factory: 0x8df6efec5547e31b0eb7d1291b511ff8a2bf987c, swap_enabled: true, tokens: [Token { address: 0x5c6ee304399dbdb9c8ef030ab642b10820db8f56, decimals: 18, weight: None }, Token { address: 0xf24d8651578a55b0c119b9910759a351a3458895, decimals: 18, weight: None }] },
            PoolData { pool_type: Stable, id: 0x3dd0843a028c86e0b760b1a76929d1c5ef93a2dd000200000000000000000249, address: 0x3dd0843a028c86e0b760b1a76929d1c5ef93a2dd, factory: 0x8df6efec5547e31b0eb7d1291b511ff8a2bf987c, swap_enabled: true, tokens: [Token { address: 0x5c6ee304399dbdb9c8ef030ab642b10820db8f56, decimals: 18, weight: None }, Token { address: 0x616e8bfa43f920657b3497dbf40d6b1a02d4608d, decimals: 18, weight: None }] }]
    }
}

This looks weird to me because the pool type is Stable but the factory address is not the one we have in the repository. It seems like someone else created a new StablePoolFactory. This does not match how we model balancer in our code where we expect only one StablePoolFactory.

Found a reference to this here balancer/balancer-subgraph-v2@e23515e.
@nlordell
Copy link
Contributor

nlordell commented Jul 6, 2022

The code should be setup in a way where you can have multiple factories for similar things (see WeightedPoolFactory and WeighedTwoTokenPoolFactory). It might be something easy to solve (although, now sure how impactful it will be).

@markusbkoch
Copy link

@nlordell is right. We may sometimes deploy new factories for pools of the same type. In this case, 0x8df6efec5547e31b0eb7d1291b511ff8a2bf987c is a new stable pool factory that deprecated the previous one.

Note that the previous one can't be "shut down", so people can still deploy pools with it. But due to the known issues we'll disincentivize its use to the extent possible (eg no new pools deployed by the old factory should be expected to be added to the Balancer UI).

@nlordell
Copy link
Contributor

nlordell commented Jul 6, 2022

@markusbkoch - do you know why the previous one was deprecated and what were the issues?

@markusbkoch
Copy link

Numerical issues when one of the token depegs. The invariant calculation computation wouldn't converge and the pool would get stuck. Didn't happen to any of ours, but it did to one on Beethoven.

@vkgnosis
Copy link
Contributor Author

I'm wondering if the math we have implemented for the v1 stable pools still holds for v2. Since you're mentioning the invariant computation which I also see referenced in in our code here it probably does not. Also relevant for our Quasimodo solver.

(Not really directed at anyone, just thinking out loud for reference. Also CC @bh2smith because you did most of the balancer implementation and we talked about this issue on Slack. Feel free to ignore the ping since you're not working on the backend atm.)

@markusbkoch
Copy link

I'm wondering if the math we have implemented for the v1 stable pools still holds for v2

Good point, we'll look into it and get back to you soon.

@EndymionJkb
Copy link

It is the same algorithm: in that sense the "math" didn't change. We did change some details of the implementation to exactly match Curve's code (and also the Vyper compiler). These changes make it converge over a wider range, and should avoid any issues like we saw on Beethoven after the UST de-peg.

In particular, we calculate a D_P value (formerly P_D) that has higher initial precision, and then adjust the quotient below to use it. Also, we dropped the directional rounding, as Vyper just does truncation.

The latest code (same deployed for v2) is at: https://github.com/balancer-labs/balancer-v2-monorepo/blob/9eb7e44a4e9ebbadfe3c6242a086118298cadc9f/pkg/pool-stable-phantom/contracts/StableMath.sol#L57

@bh2smith
Copy link
Contributor

So, we have implemented stable math here:

services/crates/shared/src/sources/balancer_v2/swap/stable_math.rs

Lines 19 to 84 in f7e53c3

/// https://github.com/balancer-labs/balancer-v2-monorepo/blob/ad1442113b26ec22081c2047e2ec95355a7f12ba/pkg/pool-stable/contracts/StableMath.sol#L49-L105
fn calculate_invariant(
amplification_parameter: U256,
balances: &[Bfp],
round_up: bool,
) -> Result<U256, Error> {
let mut sum = U256::zero();
let num_tokens_usize = balances.len();
for balance_i in balances.iter() {
sum = sum.badd(balance_i.as_uint256())?;
}
if sum.is_zero() {
return Ok(sum);
}
let mut invariant = sum;
let num_tokens = U256::from(num_tokens_usize);
let amp_times_total = amplification_parameter.bmul(num_tokens)?;
for _ in 0..255 {
// If balances were empty, we would have returned on sum.is_zero()
let mut p_d = balances[0].as_uint256().bmul(num_tokens)?;
for balance in &balances[1..] {
// P_D = Math.div(Math.mul(Math.mul(P_D, balances[j]), numTokens), invariant, roundUp);
p_d = rounded_div(
p_d.bmul(balance.as_uint256())?.bmul(num_tokens)?,
invariant,
round_up,
)?
}
let prev_invariant = invariant;
invariant = rounded_div(
// invariant = Math.div(
// Math.mul(Math.mul(numTokens, invariant), invariant).add(
// Math.div(Math.mul(Math.mul(ampTimesTotal, sum), P_D), _AMP_PRECISION, roundUp)
// ),
num_tokens
.bmul(invariant)?
.bmul(invariant)?
.badd(rounded_div(
amp_times_total.bmul(sum)?.bmul(p_d)?,
*AMP_PRECISION,
round_up,
)?)?,
// Math.mul(numTokens + 1, invariant).add(
// // No need to use checked arithmetic for the amp precision, the amp is guaranteed to be at least 1
// Math.div(Math.mul(ampTimesTotal - _AMP_PRECISION, P_D), _AMP_PRECISION, !roundUp)
// ),
(num_tokens.badd(1.into())?)
.bmul(invariant)?
.badd(rounded_div(
(amp_times_total.bsub(*AMP_PRECISION)?).bmul(p_d)?,
*AMP_PRECISION,
!round_up,
)?)?,
round_up,
)?;
match convergence_criteria(invariant, prev_invariant) {
None => continue,
Some(invariant) => return Ok(invariant),
}
}
Err(Error::StableInvariantDidntConverge)
}

It appears that calculate_invariant has been changed (d_p instead of p_d). I will have to look a bit closer and see what (if anything needs to be changed on our side). Do you happen to have any new unit tests that were introduced to demonstrate the difference between the old and the new implementation? This would help determine if anything has actually changed or if its just semantics.

@bh2smith
Copy link
Contributor

bh2smith commented Jul 14, 2022 •
edited
Loading

Ok, so I am looking at the DIFF between the new an old contract implementations and I am seeing the following weird difference:

You have changed calculateInvariant so that it always rounds down (cf https://github.com/balancer-labs/balancer-v2-monorepo/blob/9eb7e44a4e9ebbadfe3c6242a086118298cadc9f/pkg/pool-stable-phantom/contracts/StableMath.sol#L71)

However, I also see that both _calcOutGivenIn and _calcInGivenOut (cf here and here) have this new comment "// The invariant should be rounded up." and the invariant is now being passed into the methods. If the calculateInvariantmethod has been changed to only support rounding down, how can we ever expect to pass in an invariant which is expected to be rounded up? It seems contradictory.

For reference I am looking at the DIFF of

/// OLD: https://raw.githubusercontent.com/balancer-labs/balancer-v2-monorepo/ad1442113b26ec22081c2047e2ec95355a7f12ba/pkg/pool-stable/contracts/StableMath.sol

/// NEW: https://raw.githubusercontent.com/balancer-labs/balancer-v2-monorepo/9eb7e44a4e9ebbadfe3c6242a086118298cadc9f/pkg/pool-stable-phantom/contracts/StableMath.sol

I am also unable to tell why invariant is now passed into the calcXgivenY instead of being computed internally. I suspect this is a gas optimization of some sort.

@bh2smith bh2smith mentioned this issue Jul 14, 2022
Merged
@EndymionJkb
Copy link

EndymionJkb commented Jul 16, 2022 via email

@bh2smith
Copy link
Contributor

bh2smith commented Jul 16, 2022 •
edited
Loading

Yes; that was a stale comment (already fixed in repo by #1488. Thanks!

So which comment was fixed, could you link to the actual change so I can see?

Have there been any new unit tests introduced upon making these changes? Is there a specific PR I could look at that shows exactly what changed from one version of the contract to the other? Generally I find that these balancer contract projects change the entire directory structure every time you change a contract. Makes tracking the diff and keeping up pretty challenging.

Update:

Here is the change - https://github.com/balancer-labs/balancer-v2-monorepo/pull/1488/files

and here is the relevant PR containing updated unit tests I have been asking about:
balancer/balancer-v2-monorepo#1330

@EndymionJkb
Copy link

EndymionJkb commented Jul 17, 2022 via email

@bh2smith
Copy link
Contributor

Thanks @EndymionJkb - I have made the analogous changes to our implementation of calculate_invariant

#368

At the moment only one of our unit tests is failing (something that was not expected to converge before, that is converging now) - which aligns with the purpose of the change, to "extend the convergence range".

My only question now is whether we now need to support both implementations (one for all new stable pools and one for all old stable pools) or have the old pools been upgraded to refer to the new implementation?

@EndymionJkb
Copy link

EndymionJkb commented Jul 17, 2022 via email

@bh2smith
Copy link
Contributor

bh2smith commented Jul 18, 2022 •
edited
Loading

This is all fantastic news. It sounds a bit like our backend services will still need to support the possibility that older versions of the pool are being used. Since this is the case, then we would need some way of identifying which stable pool uses what invariant calculation. Is this something we can determine automatically from the pool itself, or something we would have to hardcode in our project? I can imagine something like a boolean flag on the pool's attributes like uses_deprecated_arithmetic, just wondering if the pool contract itself has some indication which invariant it is using? Perhaps this can be determined from the factory contract that creates the pool:

All stable pools from the

  • old factory use the old arithmetic
  • new factory use the new arithmetic

Is this a legit statement?

If so, can you confirm that these are the Stable Pool Factories?

Old: 0xc66ba2b6595d3613ccab350c886ace23866ede24
New: 0x8df6efec5547e31b0eb7d1291b511ff8a2bf987c

@bh2smith bh2smith mentioned this issue Jul 18, 2022
Merged
@EndymionJkb
Copy link

EndymionJkb commented Jul 18, 2022 via email

@bh2smith
Copy link
Contributor

The reason is that we index the events emitted by the factory contracts (namely PoolCreated) in order to have a complete set Balancer Pools. We also emulate the contract methods calc_out_given_in and calc_in_given_out. I was worried that with the upgrade in StablePool contract that we may have to continue to support both implementations of these methods, but am very happy to hear that

All "versions" will produce the same value

which means we can simply update the invariant calculation and start indexing pools from the newly deployed factory.

FYI, I did notice, however, that the new contract factory has a new event being emitted. Namely FactoryDisabled which is emitted with no additional parameters.

Thanks for these clarifications. Will make everything much easier now.

@EndymionJkb
Copy link

EndymionJkb commented Jul 18, 2022 via email

@bh2smith
Copy link
Contributor

The PRs related to this issue have been merged and will soon be available in our staging environment.

So, within the next few minutes, we should be able to

  1. place an order involving tokens contained within these pools on https://barn.cowswap.exchange/#/swap?chain=mainnet
  2. If one of these pools has deep enough liquidity, we should see that it is filled via one of these pools.

Note that one of those pools involves a BPT (aka LP token) which is not a likely candidate to select as a tradable token. However, we may find that since there exists a pool for it, it does become tradable. I suspect this will not be the case however, since the pool containing this BP token does not also contain one of our "base-tokens" In any case, these pools should now be entered into the ring for competitive prices. I will try and force a trade using one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Vendor Stable Pool V2 Artifacts cowprotocol/services
5 participants
@EndymionJkb @nlordell @bh2smith @markusbkoch @vkgnosis