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.

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

Not able to join managed pools #2233

Closed
enzeve opened this issue Feb 10, 2023 · 11 comments
Closed

Not able to join managed pools #2233

enzeve opened this issue Feb 10, 2023 · 11 comments

Comments

@enzeve
Copy link

enzeve commented Feb 10, 2023

Note: this issue is based on commit e67646a26c1312d5d6bc3808ea310641a355e3b8


In the _joinOrExit() function in the Vault is is checked that the tokens and limits array have the same length:

https://github.com/balancer-labs/balancer-v2-monorepo/blob/e67646a26c1312d5d6bc3808ea310641a355e3b8/pkg/vault/contracts/PoolBalances.sol#L119

In the _onInitializePool() function in ManagePool this is rechecked. However, now it is checked on the returned value of _getPoolTokens() (which removes the bpt token), while still using the same amounts from the userData (which still has the bpt amount too).

https://github.com/balancer-labs/balancer-v2-monorepo/blob/e67646a26c1312d5d6bc3808ea310641a355e3b8/pkg/pool-weighted/contracts/managed/ManagedPool.sol#L482

https://github.com/balancer-labs/balancer-v2-monorepo/blob/e67646a26c1312d5d6bc3808ea310641a355e3b8/pkg/pool-weighted/contracts/managed/ManagedPool.sol#L748

This results in me not being able to join a managed pool (in my unit tests). I can join weighted and composable pools in my tests.

Is there something that I am missing that would allow me to join the pool?

@jubeira
Copy link
Contributor

jubeira commented Feb 10, 2023

Hi @enzeve,

If the Vault token length check passes, the problem could be in the way you are encoding userData, and that depends on what you want to do (note that you can do regular joins or join-swaps in managed pools, since they contain BPT as you say).

I'd recommend you to take a look at the managed pool test for reference.

If you are forking the repository and doing your own tests, I'd suggest you to use the helpers that are extensively used in our tests for convenience. Otherwise, you can dig in and check what the helpers do and how they encode the arguments..

Hope it helps!

@EndymionJkb
Copy link
Collaborator

EndymionJkb commented Feb 10, 2023

Generally for composable pools, user data only contains the BPT in the special case of the initialization join. (This is how the pre-minted BPT get to the Vault.) Other joins (e.g., from external users) do NOT include the BPT token. It gets inserted by the code when interacting with the Vault (which as you observe, does require that the token lists match), and then removed again when returning results, as necessary.

In our tests (as in Juani's example above), you may notice "poolTokens" (without BPT, as passed in by a user), and "allTokens" (complete set with BPT)

@enzeve
Copy link
Author

enzeve commented Feb 11, 2023

Thank you for the quick replies. However, I'm still a bit confused.

I'm using foundry. So my tests are in Solidity and thus can only look at what your tests are doing.

I have created a ManagedPool using the ManagedPoolFactory. The constructor of the ManagedPool creates a ComposableStablePool (meaning that the BPT token is also registered) based on NewBasePool with the MINIMAL_SWAP_INFO specialization.

function createPool() internal {
    string memory name = "LP-TOKEN";
    string memory symbol = "lpToken";
    address[] memory assetManagers = new address[](3);
    ManagedPool.ManagedPoolParams memory params = ManagedPool.ManagedPoolParams(name, symbol, assetManagers);

    IERC20[] memory tokens = _sortTokens(IERC20(address(tokenA)), IERC20(address(tokenB)));
    tokens = _insertSorted(tokens, IERC20(address(tokenC)));

    uint256[] memory weights = new uint256[](tokens.length);
    weights[0] = uint256(1e18) / 3;
    weights[1] = uint256(1e18) / 3;
    weights[2] = uint256(1e18) - (2 * (uint256(1e18) / 3)); // To account for the rounding error

    ManagedPoolSettings.ManagedPoolSettingsParams memory settingsParams = ManagedPoolSettings
        .ManagedPoolSettingsParams(tokens, weights, poolSwapFeePercentage, false, false, 0, 0);

    pool = ManagedPool(managedPoolFactory.create(params, settingsParams, poolOwner));
}

Now I have this pool and I want to to add liquidity to it. This means that I have to call IVault.joinPool() right? Just like any other pool.

function joinPool(
        uint256 tokenAAmount,
        uint256 tokenBAmount,
        uint256 tokenCAmount
    ) internal {
        vm.assume(pool.getJoinExitEnabled());

        address sender = initialFundsOwner;
        address recipient = initialFundsOwner;

        IERC20[] memory _tokens = _sortTokens(IERC20(address(tokenA)), IERC20(address(tokenB)));
        _tokens = _insertSorted(_tokens, IERC20(address(tokenC)));

        IERC20[] memory tokens = new IERC20[](_tokens.length + 1);
        tokens[0] = IERC20(address(pool));
        for (uint256 i = 0; i < _tokens.length; i++) {
            tokens[i + 1] = _tokens[i];
        }

        IAsset[] memory assets = new IAsset[](tokens.length);
        assets[0] = IAsset(address(tokens[0]));
        assets[1] = IAsset(address(tokens[1]));
        assets[2] = IAsset(address(tokens[2]));
        assets[3] = IAsset(address(tokens[3]));

        uint256[] memory maxAmountsIn = new uint256[](assets.length);
        for (uint256 i = 0; i < maxAmountsIn.length; i++) {
            bytes32 encodedTokenName = keccak256(abi.encodePacked(ERC20(address(tokens[i])).name()));

            if (encodedTokenName == keccak256(abi.encodePacked("TOKENA"))) {
                maxAmountsIn[i] = tokenAAmount;
                vm.prank(sender);
                tokenA.approve(address(vault), maxAmountsIn[i]);
            } else if (encodedTokenName == keccak256(abi.encodePacked("TOKENB"))) {
                maxAmountsIn[i] = tokenBAmount;
                vm.prank(sender);
                tokenB.approve(address(vault), maxAmountsIn[i]);
            } else if (encodedTokenName == keccak256(abi.encodePacked("TOKENC"))) {
                maxAmountsIn[i] = tokenCAmount;
                vm.prank(sender);
                tokenC.approve(address(vault), maxAmountsIn[i]);
            } else if (encodedTokenName == keccak256(abi.encodePacked("LP-TOKEN"))) {
                maxAmountsIn[i] = 2**(111); // _PREMINTED_TOKEN_BALANCE. On the first liquidity provide all the LP tokens are minted for the first sender.
            }
        }

        bytes memory userData = abi.encode(StablePoolUserData.JoinKind.INIT, maxAmountsIn);
        bool fromInternalBalance = false;
        IVault.JoinPoolRequest memory request = IVault.JoinPoolRequest(
            assets,
            maxAmountsIn,
            userData,
            fromInternalBalance
        );

        bytes32 poolId = pool.getPoolId();

        vm.prank(sender);
        vault.joinPool(poolId, sender, recipient, request);
}

Now first PoolBalances.joinPool() is called, which calls PoolBalances._joinOrExit(). The check for PoolBalances._validateTokensAndGetBalances() passes since _getPoolTokens(poolId) includes the BPT token (due to the specialization being MINIMAL_SWAP_INFO). The call fails in _callPoolBalanceChange(). First pool.onJoinPool() is called inside _callPoolBalanceChange(). Due to it being a ManagedPool, NewPasePool.onJoinPool() is called. Since totalSupply is zero _onInitializePool() is called. This is called on the ManagedPool. Now the problem is, like I said in initial post in the following snippet. The _getPoolTokens() function inside the ManagedPool contract removed the BPT token while the amountsIn array is untouched.

// Extract the initial token balances `sender` is sending to the Pool.
(IERC20[] memory tokens, ) = _getPoolTokens();
amountsIn = userData.initialAmountsIn();
InputHelpers.ensureInputLengthMatch(amountsIn.length, tokens.length);

Please correct me if I'm wrong. But can it be that the await pool.init({ from: other, initialBalances }); init is on the weightedPool instead due to this?

Also, looking at this snipped in ManagedPool._onInitializePool(), it seems that also during initialization no BPT token is expected in the input:

// The Vault expects an array of amounts which includes BPT (which always sits in the first position).
// We then add an extra element to the beginning of the array and set it to `initialBpt`.
amountsIn = ComposablePoolLib.prependZeroElement(amountsIn);
amountsIn[0] = initialBpt;

So, looking at this, it seems that the main 'problem' is the use of the MINIMAL_SWAP_INFO specialization which makes it so that PoolTokens._getPoolTokens(poolId) returns an array including the BPT token. I would assume that if an extra specialization would be created for managed pools (which seems like a mix between stable, due to registerComposablePool() and weighted pool) it could be enforced that ManagedPool._getPoolTokens() (or at least something that drops the BPT token) could be called.

@enzeve
Copy link
Author

enzeve commented Feb 11, 2023

For now I made my initial join work with the following edit:

// ManagedPool.sol

 function _onInitializePool(
        address sender,
        address,
        bytes memory userData
) internal override returns (uint256 bptAmountOut, uint256[] memory amountsIn) {
        
    // ...

    // Extract the initial token balances `sender` is sending to the Pool.
    (IERC20[] memory tokens, ) = _getPoolTokens();
    amountsIn = userData.initialAmountsIn();

    // #EDITED
    assembly {
        // Ignore first amount, which is the BPT amount
        mstore(add(amountsIn, 32), sub(mload(amountsIn), 1))
        amountsIn := add(amountsIn, 32)
     }

    InputHelpers.ensureInputLengthMatch(amountsIn.length, tokens.length);
     // ...
}

@EndymionJkb
Copy link
Collaborator

It looks like you're trying to do the initialization "manually", but the pool handles all the BPT-token-munging for you.

Another part of what you might be missing is that ManagedPool overrides _getPoolTokens. So when _getPoolTokens is called in the joins (including initialization), it calls the overridden one, which gets the tokens from the Vault (including BPT), then drops the BPT token, so that the result is just the regular pool tokens.

You never pass in the BPT token from the outside on a join (well, other than a single token joinSwap, where you're swapping the BPT for another pool token). For your three-token pool, you just pass in the three tokens, and the userData should also encode three tokens, so the input length checks will pass.

Inside _onInitializePool, it will mint the initial BPT, and use ComposablePoolLib functions to add the BPT token to the amountsIn (setting its balance to the pre-minted amount), before returning to the Vault. The Vault will receive your three tokens and their initial balances, plus the BPT with its computed initial balance: even though you only passed in the three pool tokens. If you look at the initialize closely, what it's doing is sending you all the pre-minted BPT tokens, so that when the Vault pulls all the initial balances from you - including the BPT - you will have it.

Similarly, on a regular join, you're again just passing in three tokens, and the pool will use ComposablePoolLib again to insert the BPT token (with 0 balance) before returning to the Vault. So all external I/O uses three tokens, yet the Vault always sees 4 tokens: because ComposablePoolLib is adding and dropping tokens as necessary within the ManagedPool code.

Same thing on exits. Internally, the pool prepends the BPT token (with 0 value) when returning the amountsOut.

@enzeve
Copy link
Author

enzeve commented Feb 11, 2023

I fully agree with the fact that _getPoolTokens() is overridden by the managed pool and that it drops the BPT token. However, _getPoolTokens(bytes32 poolId) is not overridden and only has an implementation in PoolTokens. The thing, however, is that _getMinimalSwapInfoPoolTokens() doesn't drop the BPT token. When doing this "manual" join using a call to PoolBalances._validateTokensAndGetBalances() the result of _getMinimalSwapInfoPoolTokens() is used to compare with the input token list.

What would be the "non-manual" method then?

@EndymionJkb
Copy link
Collaborator

EndymionJkb commented Feb 11, 2023

We're talking past each other somehow.

My point is you don't need to worry about these internal Vault details. The composability is hidden by design. You interact with a ManagedPool initialization just as if it were a regular WeightedPool. By "manual," I mean you're trying to calculate and pass in the pre-minted BPT amounts to initialize it. You don't have to do that, because _onIntializePool does it for you. (You couldn't in any case, since you don't have the tokens to join with, as they don't yet exist.)

Everything inside the Vault (such as _getMinimalSwapInfoTokens, _getPoolTokens(bytes32 poolId), etc.) uses the full set of tokens, including the BPT. All external interfaces to the pool (e.g., weights) use the regular token list, without BPT. The composable pool code handles all the BPT adding and dropping internally, and transparently to end users.

So when you join from the Vault, including initialization, you just send the regular token amounts (3 in your case): no BPT. The pool will add the BPT token amount in when interacting with the Vault, then subtract it when returning results from the Vault to you.

@enzeve
Copy link
Author

enzeve commented Feb 12, 2023

Sorry to keep coming back to the same thing.

So you mean to just remove the BPT token from the list?

I am indeed able to successfully join a weighted pool by not adding the BPT token to the list if that is what you mean. I can also successfully join a composable stable pool by adding the BPT token to the list (needed due to it being registered in the token list here).

However, when I remove the BPT token from the list when joining the managed pool (see the updated join function below), the tx fails in ensureInputLengthMatch() inside PoolBalances._validateTokensAndGetBalances(). I think this is due to the call to registerComposablePool in the constructor of the ManagedPool which inserts the BPT token in the 0 index of the token array. This whole array then gets registered to the pool.

https://github.com/balancer-labs/balancer-v2-monorepo/blob/e67646a26c1312d5d6bc3808ea310641a355e3b8/pkg/pool-weighted/contracts/managed/ManagedPool.sol#L87

function joinPool(
        uint256 tokenAAmount,
        uint256 tokenBAmount,
        uint256 tokenCAmount
) internal {
    vm.assume(pool.getJoinExitEnabled());

    address sender = initialFundsOwner;
    address recipient = initialFundsOwner;

    IERC20[] memory tokens = _sortTokens(IERC20(address(tokenA)), IERC20(address(tokenB)));
    tokens = _insertSorted(tokens, IERC20(address(tokenC)));

    // IERC20[] memory _tokens = _sortTokens(IERC20(address(tokenA)), IERC20(address(tokenB)));
    // _tokens = _insertSorted(_tokens, IERC20(address(tokenC)));

    // IERC20[] memory tokens = new IERC20[](_tokens.length + 1);
    // tokens[0] = IERC20(address(pool));
    // for (uint256 i = 0; i < _tokens.length; i++) {
    //     tokens[i + 1] = _tokens[i];
    // }

    IAsset[] memory assets = new IAsset[](tokens.length);
    assets[0] = IAsset(address(tokens[0]));
    assets[1] = IAsset(address(tokens[1]));
    assets[2] = IAsset(address(tokens[2]));
    // assets[3] = IAsset(address(tokens[3]));

    uint256[] memory maxAmountsIn = new uint256[](assets.length);
    for (uint256 i = 0; i < maxAmountsIn.length; i++) {
        bytes32 encodedTokenName = keccak256(abi.encodePacked(ERC20(address(tokens[i])).name()));

        if (encodedTokenName == keccak256(abi.encodePacked("TOKENA"))) {
            maxAmountsIn[i] = tokenAAmount;
            vm.prank(sender);
            tokenA.approve(address(vault), maxAmountsIn[i]);
        } else if (encodedTokenName == keccak256(abi.encodePacked("TOKENB"))) {
            maxAmountsIn[i] = tokenBAmount;
            vm.prank(sender);
            tokenB.approve(address(vault), maxAmountsIn[i]);
        } else if (encodedTokenName == keccak256(abi.encodePacked("TOKENC"))) {
            maxAmountsIn[i] = tokenCAmount;
            vm.prank(sender);
            tokenC.approve(address(vault), maxAmountsIn[i]);
        }
        // else if (encodedTokenName == keccak256(abi.encodePacked("LP-TOKEN"))) {
        //     maxAmountsIn[i] = 2**(111); // _PREMINTED_TOKEN_BALANCE. On the first liquidity provide all the LP tokens are minted for the first sender.
        // }
    }

    bytes memory userData = abi.encode(StablePoolUserData.JoinKind.INIT, maxAmountsIn);
    bool fromInternalBalance = false;
    IVault.JoinPoolRequest memory request = IVault.JoinPoolRequest(
        assets,
        maxAmountsIn,
        userData,
        fromInternalBalance
    );

    bytes32 poolId = pool.getPoolId();

    vm.prank(sender);
    vault.joinPool(poolId, sender, recipient, request);
}

If I see correctly in your tests, you are using getPoolTokens(poolId) to get the token list. When I call that in my tests, I receive an array of 4 tokens where the first token is the BPT token (as expected). So I'm not really sure what is going on then.

@EndymionJkb
Copy link
Collaborator

EndymionJkb commented Feb 12, 2023

Ok; I think I finally understand how we're talking past each other! I was mainly talking about composing the userData, and the interface between the pool and Vault: how the pool handles dropping and adding BPT tokens at the interfaces, so that internal pool code didn't have to worry about it. But calling it externally: e.g., from a test, is a little different. (I think the solidity was throwing me off here, and I wasn't thinking of it calling in externally; we also use Foundry, but mainly for fuzzing.)

When you deploy, you only pass in the 3 pool tokens and 3 amountsIn, and the pool handles adding the BPT: token and amount. Then you have a 4-token pool in the Vault.

It is a little counter-intuitive from that point, since the userData and assets diverge. You never include BPT in userData, but you do need to include it in the assets of requests. So a join has to have 4 assets, but 3 amounts in the userData (maxAmountsIn).

So in the code above, it should work if you say:

IAsset[] memory assets = new IAsset[](tokens.length + 1);
assets[0] = IAsset(address(pool));
assets[1] = IAsset(address(tokens[0]));
assets[2] = IAsset(address(tokens[1]));
assets[3] = IAsset(address(tokens[2]));

@enzeve
Copy link
Author

enzeve commented Feb 12, 2023

Thank you very much!

The current function looks like this now:

function joinPool(
    uint256 tokenAAmount,
    uint256 tokenBAmount,
    uint256 tokenCAmount
) internal {
    vm.assume(pool.getJoinExitEnabled());

    address sender = initialFundsOwner;
    address recipient = initialFundsOwner;

    IERC20[] memory tokens = _sortTokens(IERC20(address(tokenA)), IERC20(address(tokenB)));
    tokens = _insertSorted(tokens, IERC20(address(tokenC)));

    IAsset[] memory assets = new IAsset[](tokens.length + 1);
    assets[0] = IAsset(address(pool));
    for (uint256 i = 0; i < tokens.length; i++) {
        assets[i + 1] = IAsset(address(tokens[i]));
    }

    uint256[] memory maxAmountsIn = new uint256[](tokens.length);
    for (uint256 i = 0; i < maxAmountsIn.length; i++) {
        bytes32 encodedTokenName = keccak256(abi.encodePacked(ERC20(address(tokens[i])).name()));

        if (encodedTokenName == keccak256(abi.encodePacked("TOKENA"))) {
            maxAmountsIn[i] = tokenAAmount;
            vm.prank(sender);
            tokenA.approve(address(vault), maxAmountsIn[i]);
        } else if (encodedTokenName == keccak256(abi.encodePacked("TOKENB"))) {
            maxAmountsIn[i] = tokenBAmount;
            vm.prank(sender);
            tokenB.approve(address(vault), maxAmountsIn[i]);
        } else if (encodedTokenName == keccak256(abi.encodePacked("TOKENC"))) {
            maxAmountsIn[i] = tokenCAmount;
            vm.prank(sender);
            tokenC.approve(address(vault), maxAmountsIn[i]);
        }
    }

    uint256[] memory limits = new uint256[](assets.length);
    limits[0] = uint256(2**(111)); 
    for (uint256 i = 0; i < maxAmountsIn.length; i++) {
        limits[i + 1] = maxAmountsIn[i];
    }

    bytes memory userData = abi.encode(StablePoolUserData.JoinKind.INIT, maxAmountsIn);
    bool fromInternalBalance = false;
    IVault.JoinPoolRequest memory request = IVault.JoinPoolRequest(assets, limits, userData, fromInternalBalance);

    bytes32 poolId = pool.getPoolId();

    vm.prank(sender);
    vault.joinPool(poolId, sender, recipient, request);
}

I wasn't fully aware yet that the JoinPoolRequest.maxAmountsIn (limits) and userData.initialAmountsIn() were that distinct in this view.

Now I am indeed able to join the managed pool (for initialization).

Thanks again for your time!

@EndymionJkb
Copy link
Collaborator

Sorry it's a bit confusing! We've deployed a lot of versions of a lot of pools. I managed to get a bit confused myself at times, despite being one of the authors :)

The documentation is being revamped; thanks for the feedback on parts that still need some clarification. Our tests are "concise" (i.e., there's a lot hidden in the helpers), so it can be hard to see what's going on at that level of detail.

I do have a PR in to separate a lot of that deployment helper logic - so that it doesn't lump everything into "Weighted" pool. (Just waiting for the dust to settle on the flurry of patches and deployments.)

Closing this now, then. Thanks for your patience!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants