Many components are shared across contracts, a prime example of this being the fixed point arithmetic library. There's also instances where code reuse would be good, such as Trading Stragies and Trade Scripts (were applicable, e.g. Constant Weighted Product Pools).

The Vault is currently very much in flux and still following patterns from the v1 code, such as inheriting from BConst, BNum, etc. Once we finish removing Trading Strategies from the Vault it would be good to finish stripping of all this extra contracts from the Vault, and start reusing.

When it comes to how to reuse, the simplest approach seems to be to use top-level functions.

Once Trading Script handles swaps for Pair and Tuple TS types, create a single swap function that can handle both and write tests for it.

As noted here, we can use the out-given-in and in-given-out formulas for pair-swap verification instead of computing the value function, which seems to save gas.

Currently, Constant Weighted Prod Trading Strategy uses binomial approximation to calculate fractional exponentiation. We should implement it using Taylor approx (LogExpMath library) and compare gas costs and precision.

Solidity doesn't allow for arrays to be immutable, which means we can't have the array of token weights outside of storage.

The following workaround should work:

uint256 immutable weights; // 8 32-byte weights packed together. index 0 is LSB and index 7 is MSB

function getWeight(uint256 index) public view returns (uint32) {
  uint8 shift = index * 32;
  return (uint32) ((weights & (0xFF << shift)) >> shift);
}

I'd expect for the shifts and masks to be cheap, though it'd be good to measure gas regardless.

Currently the vault has a swap function that is optimized to trade 2 tokens per pool at a time. It is a ground in the middle solution between single and multi pool trades.
The gas cost to validate the swap will depend exclusively on the Trading Strategy. For example, some TS use only both traded balances to validate while others uses all balances of the pool.

struct Diff {
  address token;
  int256 vaultDelta; // Positive delta means the vault receives tokens
}
struct Swap {
  bytes32 poolId;
  TokenData tokenA;
  TokenData tokenB;
}
function batchSwap(
  Diff[] memory diffs, 
  Swap[] memory swaps, 
  FundsIn calldata fundsIn, 
  FundsOut calldata fundsOut 
);

Gas costs (as of today)
Using 1 pools: 122k (122k per pool)
Using 2 pools: 165k (82k per pool)
...
Using 8 pools: 424k (53k per pool)

(gas cost reduces drastically when swapping two or more times)

Alternative swap functions

Two tokens traded on a single pool:

It is possible to optimise for a scenario where one token is swap for another one on a single pool. Because the user trades directly with the Vault, there is no need to do more than one transferFrom and swap validation is done while calculating amounts.

function swapExactAmountInSingle(
  bytes32 poolId, 
  address tokenIn, 
  address tokenOut, 
  uint256 amountIn, 
  uint256 minAmountOut 
);

There is a pull request #33 where some tests have been done and the gas cost for the single trade is around 89k gas.

Pro:

• very cheap for a single trade

Cons:

• many entry points to do the same trade
• user needs now to give allowance to Vault

Multiple tokens trade per pool:

It is also possible to optimise for multiple swaps on a single pool. This is a bit more expensive in terms of gas compared to current solution when trading different pools, but very cheap when swapping many times on the same one.

struct Swaps {
        bytes32 poolId;
        uint256[] newBalances; //Pool token balances after swap
    }
function swap(address[] memory tokens, Swaps[] memory swaps) ;

Pro:

• very cheap for many swaps on a single pool

Cons:

• trading strategy cannot optimize with two tokens math.
• many entry points to do the same trade

Some features will require other components to trust the Vault to make external calls in specific ways. For example, in #36 the trade strategists will only update their state if the caller is the Vault, but this requires that the Vault doesn't ever perform this call outside of a swap. Flash loans, on the other hand, involve executing arbitrary untrusted code.

Alternatives to make this work include maintaining a blocklist of strategists and never do callbacks into them, or making all callbacks via a forwarder contract so that msg.sender is not the Vault.

contract Forwader {
  function forward(address destinatary, bytes calldata payload) external {
    (bool success, ) = destinatary.call(payload);
    require(success);
  }
}

contract Vault {
  function flashLoan(..., addess destinatary, bytes calldata payload) external nonReentrant {
    ...
    forwarder.forward(destinatary, payload);
    ...
  }
}

https://github.com/balancer-labs/balancer-core-v2/blob/2f1b278a541bcd6740dbe89ee611373a2d656f1f/contracts/Vault.sol#L55

I think the main reason for bind/rebind/unbind in V1 was to handle controlling weights and total Weight. We don't have that notion inside the vault anymore, so why do we need them?

IMO the pool owner/controller should just be able to deposit and withdraw liquidity freely from/to the pool. Ofc we need a check for when there's lending and the withdrawal might be denied to avoid making the position below the CR (collateralization ratio)

Create a test to check that moving liquidity from one pool to another works fine. It involves calling exit and join functions on two different pool entities.

As discussed here, the current interface is redundant in that maxPrice is indirectly specified twice. It can be argued however that this redundancy adds some safety in the form of protecting from errors in the diffs, swaps or amountsIn arrays.

We should consider what sort of guarantees we want to provide in this contract, and whether we can come up with a better interface. A possibility would be to provide the total amount in, and have ratios instead of amountsIn.

The 0.6 line of the compiler adds many improvements to the language that allow for more probably correct code (most notably virtual and override). It has been out for a long time now (released on December 2019) and has been adopted by many large projects, so the risk of unknown bugs is low.

There are several reasons to support the notion of user (address) balance, including:

• save gas by not transferring in for a swap
• save gas by not transferring out after a swap
• save gas by moving accross pools without token transfers
• (in the future) participate in lending mechanisms without joining a pool

At the very least, we need ways for users to deposit, withdraw, and use their balance. The idea of an 'operator' (like ERC777 has might be useful here, since the Vault will need to verify the user's intent in every balance change.

The TradeScript (TradingEngine) currently fetches balances, weights, and swap fee. Since weights are not expected to change frequently, we could have these be passed as arguments to avoid a roundtrip to the Vault. Naive tests show this reduces swap cost by ~16k gas.

The swap fee is trickier due to #38. Should the trading strategies provide a getter with the fee, for the off-chain SOR to feed from? Should we abandon #38?

#52 introduced a heavily optimized pattern to store the weights, by relying on immutable variables and if-else chains. While fast, this is error-prone and hard to verify and maintain.

It'd be good if we compared how much more costly it'd be to store a mapping from address to weight, and read them that way - if anything to have an idea of how much we're gaining with this approach.

Enable flashloans on the vault. This means allowing users to borrow any asset that the Vault has in its reserves (alias cash) without using any collateral as long as the borrowed amount plus fee are returned before the end of the transaction.

We sometimes use .sub() to discard error conditions (e.g. insufficient balance). It'd be great if we displayed an error message with more context than just ERR_SUB_UNDERFLOW in those cases.

We could follow OpenZeppelin's approach here (see sample usage).

We should have a sample pool controller that tokenizes pool participation, without any form of adminship or configurability - the token makeup of the pool will never change, nor will the trading strategy.

This would be equivalent to Balancer v1 shared pools.

https://github.com/balancer-labs/balancer-core-v2/blob/2f1b278a541bcd6740dbe89ee611373a2d656f1f/contracts/Vault.sol#L132

If we want the vault to have a SpotPrice function it should make an external call to the trade strategy.

I also think we should get rid of SpotPrice and only return amount out for a given amount in and vice versa. People should then do the ratio out/in for a very small amount to calculate an approximate SpotPrice themselves. No strong opinion here though but I think this would simplify the contracts and I guess most other AMMs also don't have the idea of SpotPrice

Because the vault will store token balances for all tokens, we'll need some internal accounting to know how many tokens each pool has.

If we receive tokens by pulling (transferFrom), then we'll need to somehow account for tokens being sent directly to the vault (like v1's gulp). transferFrom might also be undesirable because it adds interactions with external contracts.

An alternative is to follow Uniswap V2's example and instead assume the tokens have been sent beforehand, comparing the last total measured balance of the entire vault (balanceOf(address(this))) to the current one. This makes gulp unnecessary because any tokens will be immediately used by the next interaction with the vault (that involves that token).

We have many address types that are really IERC20 in interfaces, mappings, etc. It'd be good to use the type system to our advantage here and try to avoid errors by using proper types.

The TradingStrategy interface needs to be augmented with the following fields:

• from: the address that is providing the funds (either directly or via user balance)
• to: the address funds will be sent to (either directly or via user balance)
• userData: arbitrary data provided by the caller of swapBatch

from and to are required to implement certain desired features, such as whitelists. We need for the Vault to pass them so that Trading Strategy can trust these values are correct.

userData opens the door for alternative TS designs, such as only allowing a trade if it is accompanied by a signature from a trusted third party.

Once #35 goes in, we'll also have pools using this alternative trading strategy. We'll need to add support for it on the TradeScript, likely in the form of a tagged union when listing each swap.

We should keep #47 in mind while working on this, and add any parameters required to compute a trade to this union.

A few expected scenarios involve tokens being sent (transfer()) directly to the Vault, outside of a join or swap flow. This includes both user error and protocols giving out yield farming rewards. We want for those tokens to be redeemable by their 'rightful' owners, whomever they may be.

A proposed solution is to handle consensus on who the owner is at a separate layer, either off-chain or via some form of on-chain DAO governance (this could evolve over time). The key thing is we need for some trusted entity to be able to withdraw these tokens from the Vault.

The current balanceOf() - reserves approach we're taking, following Uni v2's steps, simply allocates thse 'unaccounted for' tokens to the next user interacting with the platform. The way to improve this is by having the Vault itself trigger the token transfer when receiving tokens. Any tokens received outside of this mechanism (i.e the difference between token.balanceOf(vault) and the internal bookeeping) will be claimable by this trusted entity.

function receive(IERC20 token, address sender, uint256 amount) private returns (uint256) {
   uint256 beforeBalance = token.balanceOf(address(this));
   token.transferFrom(sender, address(this), amount);
   uint256 afterBalance = token.balanceOf(address(this));

   // return the tokens received - accounts for transfer fees
   return afterBalance.sub(beforeBalance);
}

An issue arises here because the Vault stores balances for multiple entities, such as pools and individual user balances. Therefore, Vault allowance is not enough to prove that the user intended for their tokens to be used in this manner (e.g. perhaps they wanted to add liquidity to an alternative pool, or perform a swap).

The solution is to perform two transfers: one from the user into a contract they called, and one from said contract into the Vault itself. Because of EIP 2200, these intermediate balance changes are not too expensive, but we will incur in costs from the allowance being decremented and the token called into twice and not once.

Continuation of #83

Pack the pool struct and access it in such a way that storage reads are minimized in high-frequency actions. For example, swap might only require reading the swap fee and token balances.

We should make the call to the trade strategists non-static (i.e. not make the validation functions view) to allow for them to update their internal state and change their strategy over time, if they so desire. This would enable patterns such as e.g. Mooniswap's virtual balances.

The gas costs of call and staticcall are identical, so this feature has no added cost in the base case.

v1 relied on LOG_CALL and family to log all external calls made to any contract in the system. We may want to move away from that and have more intent-focused events instead.

Based on the Trading Strategy changes from #38, we should charge a percentage of the TS-reported swap fees. A simple and cheap way to manage these fees would be to take advantage of #40 and not store them anywhere, making fees claimable by the fee collector account.

It remains unknown how these fees will be adjusted. We'll likely have some form of an admin account, to be replaced in the future by more formalized governance.

It currently only supports exactIn - we should at least have this alternative variant.

To reduce onboarding requirements, we're introducing the notion of trusted operator factories. These are factories that will report their created contracts to the Vault. These contracts will become operators for all users in the system. We foresee pool controllers (tokenizers) and trade scripts to be created this way.

This is good because it means users don't need to call authorizeOperator on these scripts and controllers in order to use them to trade/join and exit a pool.

Trusted operator factories should:

• only report created contracts to the Vault
• their created contracts should only ever pass msg.sender as the from address, or otherwise do authentication (e.g. with signatures)

Note that users cannot opt-out of these 'default operators' - both becuase we don't see a point in allowing for this, but also to avoid having to do two storage reads (is the address a default operator, and has the user not opted out).

Currently, the Vault deducts some balance in the form of swap fees before handling control to the Trading Strategy for verification. While sound and simple, this prevents Trading Strategies from fee innovation: only fixed percentages would be supported.

The proposed solution is to have the Trading Strategy claim whatever fee it wants and to inform that back to the Vault, who will in turn grab a fixed percentage of that (as a protocol fee). Of course, the Vault has no way of knowing if the TS is actually charging the fee it says it is - we expect to rely on external incentives (such as liquidity mining) to prefer 'honest' TS's.

We're currently using GPLv3, sice it is what v1 had. As @gtaschuk pointed out, we never gave this much thought. We could keep it, go with a less restrictive one such as MIT, or do something else entirely.

Our revert reasons are a bit all over the place - we never settled on a format. We should make them consistent.

I suggest we pick a format that a) includes the contract that triggered the revert, and b) yields unique revert reasons.

The current Vault contract is so large that it cannot be deployed (it goes over the 24kb contract size limit). We'll need to extract some functionality away from it, perhaps to the trading engine, tokenizer, dedicated view contracts, etc.

This entrypoint is not yet exposed, though we have planned for it.

https://github.com/balancer-labs/balancer-core-v2/blob/2f1b278a541bcd6740dbe89ee611373a2d656f1f/contracts/Vault.sol#L59

In addition to #43, we should have a separate controller with an admin address that is free to modify any parameters of the pool. This includes:

• what tokens are in the pool
• what the trading strategy is
• who the controller of the pool is

Parameters such as swap fees, token weights, etc., will all exist in the trading strategy itself, so it will either be mutable, or (more likely) replaced by other immutable instances of it, with the tokenizer acting as a factory.

Balancer v1 does not support tokens that don't return a boolean.

We could use a pattern like OpenZeppelin's SafeERC20 and assume transfers with no return value are successful. This is the same thing Uniswap v2 does.

The fact that our trading strategies have differences in what they need in order to validate a trade is causing issues in the TradeScript interface, which is being expanded to accomodate for this extra data.

We should rethink what data we supply and how, so that calling into the TradeScript is not a gargantuan task. This is related to #47.

Check gas usage for Stable TS. If possible rewrite approximation to save even more gas. Document solution.

Currently we are using most of Balancer v1's math library, with some uint128 variants.

This is a fixed point 18 decimal library, that rounds up for multiplication and division. Open questions and areas to explore:

• Do we continue to always round up? Do we have both round up/down variants and use based on the strategy so that the rounding is in favor of the vault?
• Do we scale all token balances to be 10e18? Currently, a token like USDC with 6 decimals causes all sorts of oddities in BMath since we are dividing by a base unit of 18.
• Do we plan to limit token decimals? i.e. in v1 there is a min of 10^6 balance to prevent rounding issues at the extremes. Will our new exponentiation logic work on a token like Chi with 0 decimals?

Our BPTs deviate from a standard ERC20 in a few small ways that allow for gas savings, we should be upfront about those.

From a cursory overview, the main differences are:

• addition of increase/decrease allowance functions
• decrease allowance silently sets allowance to 0 if decreasing by more than current allowance (do we want this?)
• transferFrom can be used by the token holder without setting allowance for itself
• an allowance of 0xff..ff is considered infinite and is never reduced
• we emit Approval events on transferFrom

We should have a base contract that helps create factories, since many of the factories will share quite a bit of code. See #94 for some comments on this, and how usage of a factory looks like.

Pools with a tuple-type trading strategy validation require that all token balances are sent to the strategy. This is expensive because what the pools store is a list of tokens, plus a mapping with token balances.

mapping(bytes32 => EnumerableSet.AddressSet) internal _poolTokens;
mapping(bytes32 => mapping(address => BalanceLib.Balance)) internal _poolTokenBalance;

This means that reading all balances requires reading all token addresses, and then reading the balance for each. The addresses are not used except to access the balance mapping, and to compare them with tokenIn and tokenOut, in search for their indexes.

We could store balances and tokens in a different format for pools that use a tuple validation strategy. This would increase the cost of balance queries to the Vault (which could impact TradeScripts if we're not careful!), but make full-balance reads much cheaper.

In essence, we'd store an array of balances, and a mapping from token address to index in the array. Reading all balances is therefore just a sequential read, while fetching the index of tokenIn and tokenOut are two reads in the mapping. With storage reads becoming much more expensive in the next hardfork, this should lead to massive savings, especially for large pools.

A trade-off here is that pools would lose the capacity to migrate to a strategy of a different type. We could do the migration, but this seems very error-prone, and I'd rather suggest a liquidaty migration instead.

Some of the terms we're using could do with some refining, though it'd be best to reach a consensus before making any changes. We should use this issue to discuss potential changes before making them official.

Things I'm currently not a fan of:

• 'user balance', what is a user? I'd just go with 'internal balance' (we could then say "a user's internal balance")
• trusted operators: all operators are trusted, this is strange. Perhaps system-wide operators? Default operators? (this last one is not great since they cannot be opted-out).
• trusted operator reporters, and reportOperators

As we add investment related notions, surely more of these will come up.

This includes whether state variables and function arguments have leading underscores, distinctions between public and internal functions, etc. It'd be good to use a tool such as solhint for this, which already supports these rules.

The TradingStrategy interface potentially needs a caller field. This is because caller (eg. Trading Script contract) does not always equals from (eg. trader). It is necessary in order to validate the userData field which comes directly from the caller.

Even if the AMM release lacks any form or investing (lending), the AMM release should already support it, given there's deep interactions with the Vault.

I'll list here what we've discussed so far, separated in a number of sensible stages. We likely want to have distinct PRs for each stage. I think it's better to have all high-level discussion in this single issue, instead of separate ones.

We don't know for sure that this will be enough to support all use cases, though it seems quite flexible.

• Pool, user and Vault token balances will no longer be a single uint256, but rather a combination of 'cash' (funds immediately available to be withdrawn or used) and 'investments' (funds not present in the Vault, might not be immediately withdrawable, but are earning yield). We could store these two in a single slot if we use uint128 values:

struct Balance {
   uint128 cash;
   uint128 investments;
}

Whenever liquidity is added or removed from a pool, a pool performs a swap, or a user deposits or withdraws balance, funds are added/removed from cash.

There's also a function that converts Balance to uint256. In the trivial case it will sum cash and investments.

• We introduce 'investment strategies'. Investment strategies report exchange rates between invested and cash (withdrawn) assets. These rates are always larger or equal to 1, and increase over time.

Each pool chooses, for each of their assets, what the investment strategy will be. An address of zero means that asset cannot be invested. Whenever the full funds are computed for a pool's asset, we fetch the pool's strategy for that asset, and then the rate for that strategy for that asset (two extra reads).

• We let the pools manage their investments. An entity (the trading strategy?) reports the desired cash level (likely a percentage of holdings) to the Vault. This 'rebalancing' can be performed by any actor by calling a function in the Vault, either investing more assets or withdrawing into cash, depending on wheter a pool's asset is over or under invested.

I don't see much reason to be overly restrictive in how low fees can go, and would remove this restriction in favor of flexibility.

I think there's value in having a maximum fee to place an upper bound on intermediate values when thinking about internal arithmetic.

A Solidity array is not much more than a length field plus a mapping for index to value. Accessing a value triggers a boundary check, comparing the total length with the accessed index.

This leads to gas inefficiencies when looping over said arrays, since the compiler doesn't remove the length check. See ethereum/solidity#9117 for reference.

For arrays that are looped over (e.g. arrays of tokens or balances), we should use mappings instead, with a single boundary check at the beginning. Note that this also applies to more elaborate data structures such as OpenZeppelin's EnumerableSet.