balancer / balancer-v3-monorepo Goto Github PK

Would avoid enums here. If a pool dev wants to introduce a new kind, they have to edit the IBasePool which is ugly. Also, as discussed, proportional is the universally implemented join type, which is value 2 in the enum definition, meaning that the pool would then need to revert for value 0 and 1. This is a type of requirement that is difficult to enforce, and if not properly implemented creates down stream challenges for integrations.

#87 (comment)

Having to understand internal balance, not to mention passing in the FundsManagement struct on every swap, is a burden to users. It's not on the UI, so retail customers don't use it. And we've seen very little use of this feature overall, despite believing it is an underrated feature, and major differentiator.

One option is to get rid of it entirely. If we do keep it, we can at least streamline the path that 99.9% of users take by removing it from the common swap interface.

If swaps don’t use internal balance, we can remove the FundsManagement struct, and just pass sender/recipient addresses. Then users don’t have to figure out what it means, or deal with structs.

We can always have an “advanced” interface with internal balance put back in, if we still want to support that (e.g., for aggregators/relayers). Alternatively, if we decide to remove internal balance, it can just go away entirely.

We’d also talked about streamlining userData, since there are only so many “common” operations. We could have an “advanced” swap that would only be used by new pool types, that could pass arbitrary data as we have it now.

Considerations:

• Autowrap ETH --> this can be moved to a relayer. Discuss. Could also be added back later.
• Join returns protocol fees to be paid. Remove.
• Arguments: we shouldn't need the token addresses anymore. This is because tokens will be immutable at the Vault level.

Review API and arguments (also compare to known protocols, and propose to the rest of the teams in Balancer).

Based on #38.

The Vault should be able to hold the code for (all) BPT accounting, including minting / burning functionality.

https://github.com/ylv-io/multi-token is a great starting place for a PoC.

Needs discussion: how to handle events. The general consensus is that emitting the transfers, approvals, etc. (just) at the vault level is OK, but we should triple check.

Migrating pools is a faulty scenario, but it has happened in the past and it fragments liquidity as it forces the users to move the funds to newer pools.

In general this is a tedious and inefficient process that should be avoided.

A possibility to address this is to have a built-in mechanism that allows automatic liquidity migration to newer pools. There are a lot of nuances to be discussed, such as what are the guarantees given to users, liabilities for governance?, how to actually do it in case it makes sense to proceed.

In V2 we use an enum with a single function.

Other protocols use different functions for given in and given out. We should review what's the preference ecosystem-wide.

Note that this a relatively minor detail; we can start with a base version and then refactor.

https://soliditydeveloper.com/erc721-permit

Working with the updated version (compatible with 0.8)

We'll go with a straightforward approach: take a fee in the tokens, on every swap.
We'll not accrue debt between joins / exits nor pay in BPT.

The swap fee should not imbalance the pools at any time; it should just be accounted separately.

We can start by transferring the fees elsewhere and measure the gas costs; warm storage access should make this cheaper than what it was by V2 launch. If needed, we can optimize in a second iteration.

From the brainstorming doc:

"Protocol fees over the course of v2 got increasingly more complex. So much so that it started introducing extra values that needed to be tracked offchain for an arber / aggregator to calculate swap amounts. I’d argue the loss of volume from the worse dev UX probably outweighed any additional protocol fees gained."

However we eventually decide to handle protocol fees, I think it’s safe to say we’re not going back to having the Vault transfer them in join/exit hooks, so we can just remove those now. Then we don’t need to have the pools return hard-coded zero arrays.

If we want to totally redesign them, we can do a PR to remove the current method, and then another one to add the new method.

Assuming we're still going with an external contract with a pointer in the Vault, we can also build in the "ProtocolFeeWithdrawer" functionality, and possibly even parts of the fee type id system.

We want the pools to receive token balances as if all tokens had the same 18 decimals, upscaling before the operations and downscaling back to the actual decimals afterwards.

In V2 the decimal difference is stored in the pools as immutable values. For now we can start with a straightforward approach that reads the decimals from storage, and optimize later.

We will want to combine the decimal difference with the rate providers eventually. We can have all of this in a contract deployed by the pool (or the pool factory) that has token addresses, decimals and rate providers in immutable storage. For this approach we'll need:

• Constant init code by getting the parameters from msg.sender in the constructor
• Compute immutable contract address keccak256( 0xff ++ address ++ salt ++ keccak256(init_code))[12:]

See:

• https://github.com/Uniswap/v3-core/blob/main/contracts/UniswapV3PoolDeployer.sol
• https://github.com/Uniswap/v3-core/blob/main/contracts/UniswapV3Pool.sol

Comment

TODO list, to review after getting the code to a functional state in main:

• userData in regular interfaces.
• Be explicit about the fact that userData is optional.
• Vault getter
• Mint / Burn (ref)

Transient API and composable pools may intensify certain types of attacks, but they are not the inherent problem. To ensure safety, we should:

• Exercise extreme caution regarding rounding and precision. Ensure they can’t be exploited by preventing absurdly large or small numbers.
• Limit trading to reasonable amounts, especially since the Vault will account for the scaling decimal of tokens.
• Never let the pool supply drop to zero. In general, avoid resetting any significant value back to zero.
• Ensure that the Vault and pool states never fall out of sync. Ideally, we’d store all significant state information in the Vault and commit it in one go (stateless pools)
• Treat BPT 'differently'? Don't allow e.g. inflating the supply in the middle of a tx.

Note: in this context it is not enough to e.g. mint a small amount of BPT to ensure the balance never goes below a certain amount.
You can always trade on debt with transient accounting, even without holding the tokens in the first place, given that everything's settled at the end.

Permit2

Instead of storing the 'pause' state in the pool, we could store individual 'pool paused' states in the Vault upon registration (probably factory registration; in the end that'd give us the same behavior that we have in V2).

This way we could keep the pause behavior consistent across pools. If the pools need to enforce anything extra while paused, they can read the state from the vault.

With the introduction of Ethers V6, we could further polish and tighten our own definitions here and there.

See #45 (comment) for context.

First approach: measure the difference in rate between any operation, and take a cut.

We'll need to introduce rate providers and scaling factors (#98) to accomplish this.

As per initial research by @ylv-io, the three candidates are:

• Balancer V2 math (with the new custom errors and Solidity 0.8.x)
• PBR math
• Solmate

We need to review our requirements once more to make a decision.

Some notes about PBR math, which looked promising:

• It needs Solc 0.8.19, which is not supported by Hardhat yet. See also NomicFoundation/hardhat#3713.
• I did a quick test with 0.8.19 with TemporarilyPausable and both the console log and the stack traces seem to work correctly. Those are the two things noted in Hardhat's notes for using unsupported versions. In other words, this doesn't seem to be a hard blocker.

Analogous to #39, but should be done afterwards; it will have more setup to test, since to exit we first need to join.

Similar to #38, but applied to ERC721.

For now this shall be the initial foundation for potential CL support at the Vault level.
This should help us lay a path forward after the MVP.

These are a great complication (and attack vector). Do we need them for Linear Pool rebalancing? Can we redesign asset managers / design a Vault with built-in composability (i.e., BPT awareness, and support for single-token BPT swaps) so that we don't need Linear Pools at all?

The mechanism to disable the queries is currently a one-way trigger; they cannot be re-enabled.

While this is not a fatal issue because queries can still be implemented elsewhere, it'd be good to be able to re-enable them (probably behind a timelock).

Ref: https://github.com/balancer/balancer-v3-monorepo/pull/82/files#r1299498871.

We don’t size from EnumerableMap because now pools have fixed token size and we are limited to 4 tokens. Most likely we can remove this extra SLOAD.

https://www.notion.so/V2-Pain-Points-a66a412d7a4644b8854467c45aabef37?pvs=4

As we've already started doing, now that we're on 0.8, we can upgrade the implementations of our Open Zeppelin knock-offs, in the process avoiding certain fateful gas optimizations.

Retain general Vault non-reentrancy as blanket protection, but ensure that we scrupulously follow checks/effects/interactions along all code paths - especially if we are still supporting native ETH wrapping/unwrapping (up for discussion, I guess).

Looks like 0.8.18 is the latest Solidity version supported by hardhat.

In the Vault, we’re mainly affected by things like:

• Stricter casting. You can’t say bytes32(uint256(address))) - has to be bytes32(uint256(uint160(address))) now.
• Stricter int vs. uint processing. You can’t say int256(-amount) (where amount is a unit256). Per the docs, to negate a uint256, you have to ensure amount > 0, then say: int256(type(uint256).max - amount + 1)
• We can get rid of `pragma experimental ABIEncoderV2 everywhere.

Brought by #85.

They'll need some minor tweaks to account for the new error codes.

Double check if lastChangedBlock is required or not, and implement if necessary.

If it is indeed necessary, we might change it with a timestamp instead of a block.

From the brainstorming doc:

"After POS, built in AMM oracles are the wrong approach and the space will move quickly towards zkproofs of historical ethereum state."

This is mainly just removing lastChangeBlock from the join/exit hooks.

However, one big implication for the BalanceAllocation library is that we no longer need those 32 bits for the block, which was the origin of the 112 bit maximum balance: 112 * 2 + 32 = 256.

Now, we can make cash and managed balances 128 bits each. (Of course, if we remove asset managers, we no longer need to encode balances, and don’t need that library at all.)

Fetch code for pool registry (as is in V2).

Pool IDs and different specializations are not required anymore.
It's highly likely that we are going for the general specialization (unless 2+ tokens are not required anymore).

Let's see if we can be more strict with the docs.

From the brainstorming doc:

"It’s clear that flashloan fees have already raced to zero so there is no protocol fee upside. Flashloans are used less for arbs these days and is in the case of V2 you can only arb external protocols due to reentrancy checks. This can be fixed, but imo not worth the tradeoff of complexity around vault balances and reentrancy. Also, it really sucks to see when your protocol liquidity is used to attack others like Euler. Yes those attacks will always be there for capitalized attackers, but it doesn’t mean we have to help."

Bring token registry from V2. To close this ticket, the vault should be able to register pools with tokens.

Requirements and expected changes:

• Register tokens and pools at the same time.
  • Use the format from the pool specialization that remains.
• Implement getPoolTokenInfo & getPoolTokens.
  • Now that tokens are immutable, consider a function that just gets the balances.
  • lastChangedBlock should not be necessary anymore. Let's start without it.
• Deregister tokens is no longer needed.

Storage determination is tied to issues #14 and #8 . Do we want to keep EnumeratedSet, etc.? Update them with reference to the latest Open Zeppelin implementation? Revisit the reasons for introducing them.

For standard and simple libraries that we can use without modifications, we can rely on Open Zeppelin directly instead of copying the code and maintaining it here.

If they still don't support custom error codes, we can use them to get started and review them later; OZ might release a new version before we need to go public.

For those that we've already ported without modifications (e.g. ReentrancyGuard) we can clean them up later.

Base version for one pool; given in + given out.

There is some consensus about this.
The problem is that UniV3 CL is not composable / chainable like the operations we have in Balancer V2.

What kind of support do we want to give this at the Vault level?

Maybe include them in the multitoken standard?

How would this work, and how would this be different from regular operations?

See https://github.com/balancer/balancer-v3-monorepo/pull/82/files#r1299498871.

Timelock authorizer would be great to be able to re-enable it if we consider it's worth it.

I'd like to suggest moving away from a monorepo for v3. One thing that I found frustrating as someone frequently consuming the v2 monorepo was trying to conceptualize which pieces of code were set in stone vs changing, and then how that mapped to live deployments. I.e. a common pattern of mine is seeing an address on Etherscan and wanting to instead view the code on github or locally to review. That reverse mapping is really difficult in the monorepo setup. If we want devs to build on top of the protocol, it has to be clear that there aren't constant shifting sands that may bubble up the stack to projects built on top.

It'd be a really nice property to have a vault repo that once deployed the code never changes and anyone looking at the github repo doesn't have to think about whether the code matches a specific deployment.

This probably means using git submodules (which I'm not the biggest fan of), but I think the tradeoffs might make sense for this

On the general theme of simplicity, and reflecting the lesser importance of gas costs now, with recent EIPs plus the rise of L2s, this will make the interface much easier, and save a ton of bytecode besides.

The only reason we introduced Pool ID was so we could get the specialization without an additional storage read. And the specialization system was mainly for gas optimization. We can get rid of pool IDs entirely, and just identify the pool by the address. (This means we can't have a pool ID with multiple addresses, which I believe we wanted for some reason early on: we have never actually needed this flexibility.)

These are closely related and have to be done together, since the current pool ID encodes the specialization. Instead, we can just use the pool address as the “id.” In this case:

• We don’t need VaultHelpers, or any functions that encode or extract things from pool IDs.
• We don’t need to pass around byte32s in the external pool interfaces; those arguments can just be addresses. (In some cases, if we already know the pool address somehow, we can even remove arguments.)
• Pool registration is simpler, and no longer needs any arguments.
• We no longer need three separate ...PoolsBalance contracts, but can just use GeneralPoolsBalance, also removing all associated logic.

Pool factories should use CREATE2, so that their address can be calculated before its creation.

The salt should probably include the chain ID and a nonce somewhere to make unique addresses. E.g. salt = hash(chainId, nonce++);.

We're going to keep the same Authorizer for V3 (with an open question of whether it's literally the same instance, or just the same code).

Another question is whether we want to make actionIds more granular by including the chainId. It wouldn't change anything with the authorizer itself, which just operates on opaque bytes32 actionIds.