Refactoring addLiquidityUnbalancedNestedPool in CLR #1203

elshan-eth · 2024-12-27T19:21:06Z

Description

This refactoring fixes implicit recursion by turning it into explicit recursion.

Type of change

Checklist:

The diff is legible and has no extraneous changes
Complex code has been commented, including external interfaces
Tests have 100% code coverage
The base branch is either main, or there's a description of how to merge

Issue Resolution

jubeira

First pass done. Just a few comments, otherwise makes sense.

Other than code specific comments, just a few generic comments:

I don't think we want to support nesting beyond 1 level. That makes me feel a bit reluctant to use recursion. The implementation here is rather simple, but we've had instances in the past where being too flexible ended up being counterproductive. I'd give it a second thought, maybe just using a function that goes down only one level instead of full blown recursion.
I get that the case described in the PR title is being covered. What I'd like to see is an explicit test where a pool has mixed tokens (e.g. child pool has sDAI and waUSDT) and the user is able to add liquidity using sDAI (wrapped) and USDT (underlying), which is the case we don't support today (in the current main we must add dai and usdt).

jubeira · 2024-12-30T12:50:12Z

pkg/vault/contracts/CompositeLiquidityRouter.sol

+            } else if (_settledTokenAmounts().tGet(childToken) == 0) {
+                amountsIn[i] = _currentSwapTokenInAmounts().tGet(childToken);
+                _settledTokenAmounts().tSet(childToken, amountsIn[i]);


I don't fully get this last if clause. Could you please add some comments here? Why are we checking settled token amounts?

We are covering:

tokens that are pools (we keep adding recursively)

wrapper case: user wants to add using underlying (buffer is initialized, there is no wrapped amount specified)

I understand the missing case is user wants to add with wrapped tokens, but I think that's not necessarily the case when you get here. User could specify underlying, but if the buffer is not initialized you'll still end up here.

Sounds like the case where bufferInitialized == false and user specifies underlying should revert.

There are three cases being covered:

tokens that are pools (we keep adding recursively)

wrapper case: user wants to add using underlying (buffer is initialized, there is no wrapped amount specified)

Normal ERC20 tokens

If the buffer is not initialized, that token may or may not be ERC4626, we treat it as a normal ERC20 token.

The if is in there to make sure the token amount was not spent in a previous child pool. In the previous solution I modified the token in amounts directly, so the if was not needed, but then I needed to restore the token in amounts array at the end. This solution is better.

pkg/vault/contracts/CompositeLiquidityRouter.sol

joaobrunoah

It mostly LGTM, but I'm worried too with the recursion. I mean, the Vault is safe, but the router can behave unpredictably if there's a cyclic path where the same pool appears multiple times. Maybe it reverts, maybe it adds zero tokens, but what happens with the transient accounting? I believe we'd need to check that case if we want to proceed with the unlimited recursion.

pkg/vault/contracts/CompositeLiquidityRouter.sol

pkg/vault/test/foundry/CompositeLiquidityRouterNestedPools.t.sol

elshan-eth · 2025-01-06T12:41:15Z

@jubeira @joaobrunoah

We use almost the same code inside these two functions (_getPoolAmountsIn and addLiquidityUnbalancedNestedPoolHook), so I think the recursion here will look more obvious. I will add recursion constraints to avoid such flexibility. What do you think about it?

EndymionJkb

I'm not sure I have full context on all this, but tried to clarify some things in the comments.

EndymionJkb · 2025-01-11T16:27:58Z

pkg/vault/contracts/CompositeLiquidityRouter.sol

        }

-        IERC20[] memory parentPoolTokens = _vault.getPoolTokens(params.pool);
+        (uint256[] memory amountsIn, ) = _addLiquidityRecursive(params.pool, params);


At first glance it looks like you could just have _addLiquidityRecursive take a single parameter, since the pool is already in the params - but you can't, because sometimes it's the child pool, different from the one in params. Would be good to document that somewhere (maybe where the function's defined).

We might also note that the Vault explicitly prohibits "Linear pool-style" recursion, where a pool contains its own BPT, so that case is impossible and we don't need to handle it.

EndymionJkb · 2025-01-11T16:29:13Z

pkg/vault/contracts/CompositeLiquidityRouter.sol

+        }
+    }
+
+    function _addLiquidityRecursive(


Suggested change

function _addLiquidityRecursive(

function _addLiquidity(

Rather than overloading _addLiquidityRecursive, consider naming the "outer" one _addLiquidity, leaving the "inner" one (that is actually recursive, with the level argument) as _addLiquidityRecursive.

EndymionJkb · 2025-01-11T16:33:16Z

pkg/vault/contracts/CompositeLiquidityRouter.sol

@@ -32,6 +32,8 @@ contract CompositeLiquidityRouter is ICompositeLiquidityRouter, BatchRouterCommo
    using TransientEnumerableSet for TransientEnumerableSet.AddressSet;
    using TransientStorageHelpers for *;

+    uint256 constant _MAX_LEVEL_IN_NESTED_OPERATIONS = 2;


Suggested change

uint256 constant _MAX_LEVEL_IN_NESTED_OPERATIONS = 2;

// The level is 1-based, so the outermost pool (level 1) can contain a nested BPT, but any pool tokens in the child

// pool (level 2) will not be expanded further, but simply treated as standard tokens.

uint256 constant _MAX_LEVEL_IN_NESTED_OPERATIONS = 2;

Clarify what 2 levels means (there was some confusion off-line).

EndymionJkb · 2025-01-11T16:39:04Z

pkg/vault/contracts/CompositeLiquidityRouter.sol

-                    params.sender,
-                    params.wethIsEth
+                if (level > _MAX_LEVEL_IN_NESTED_OPERATIONS) {
+                    // if we have reached the maximum level of nested operations, the token will be calculated as ERC20


Suggested change

// if we have reached the maximum level of nested operations, the token will be calculated as ERC20

// If we have exceeded the maximum level of nested operations, the token will be considered a standard ERC20.

EndymionJkb · 2025-01-11T16:45:25Z

pkg/vault/contracts/CompositeLiquidityRouter.sol

-                _wrapAndUpdateTokenInAmounts(IERC4626(childToken), params.sender, params.wethIsEth);
+                amountsIn[i] = _wrapAndUpdateTokenInAmounts(IERC4626(childToken), params.sender, params.wethIsEth);
+            } else if (_settledTokenAmounts().tGet(childToken) == 0) {
+                // if this token is ERC20 and the amount was not settled in a previous operation, it should be added


Suggested change

// if this token is ERC20 and the amount was not settled in a previous operation, it should be added

// Set this token's amountIn if it's a standard token that was not previously settled.

EndymionJkb · 2025-01-11T16:53:47Z

pkg/vault/contracts/CompositeLiquidityRouter.sol

+        // Remove the underlying token from the tokensIn and set the amount as zero because we took it here.
+        // We will take other tokens at the end of the calculation.


Suggested change

// Remove the underlying token from the tokensIn and set the amount as zero because we took it here.

// We will take other tokens at the end of the calculation.

// Remove the underlying token from `_currentSwapTokensIn` and zero out the amount, as these tokens were paid

// in advance and wrapped. Remaining tokens will be transferred in at the end of the calculation.

Is this correct? I wasn't sure what "taking the token" meant in the original.

elshan-eth added 2 commits December 27, 2024 20:19

addLiquidityUnbalancedNestedPool refactoring

9fda3dd

add size snapshot

33c3308

elshan-eth requested review from joaobrunoah, EndymionJkb and jubeira and removed request for joaobrunoah and EndymionJkb December 27, 2024 19:21

jubeira reviewed Dec 30, 2024

View reviewed changes

joaobrunoah approved these changes Dec 30, 2024

View reviewed changes

elshan-eth added 3 commits January 6, 2025 13:48

add recursion level

e5d4745

add recursion level

6927331

fix codestyle

f88a69c

elshan-eth requested a review from jubeira January 6, 2025 15:03

add commants

465f0af

EndymionJkb reviewed Jan 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring addLiquidityUnbalancedNestedPool in CLR #1203

Refactoring addLiquidityUnbalancedNestedPool in CLR #1203

elshan-eth commented Dec 27, 2024 •

edited

Loading

jubeira left a comment

jubeira Dec 30, 2024

joaobrunoah Dec 30, 2024

joaobrunoah left a comment

elshan-eth commented Jan 6, 2025

EndymionJkb left a comment •

edited

Loading

EndymionJkb Jan 11, 2025

EndymionJkb Jan 11, 2025

EndymionJkb Jan 11, 2025

EndymionJkb Jan 11, 2025

EndymionJkb Jan 11, 2025

EndymionJkb Jan 11, 2025

EndymionJkb Jan 11, 2025

	// if we have reached the maximum level of nested operations, the token will be calculated as ERC20
	// If we have exceeded the maximum level of nested operations, the token will be considered a standard ERC20.

	// if this token is ERC20 and the amount was not settled in a previous operation, it should be added
	// Set this token's amountIn if it's a standard token that was not previously settled.

		// Remove the underlying token from the tokensIn and set the amount as zero because we took it here.
		// We will take other tokens at the end of the calculation.

Refactoring addLiquidityUnbalancedNestedPool in CLR #1203

Are you sure you want to change the base?

Refactoring addLiquidityUnbalancedNestedPool in CLR #1203

Conversation

elshan-eth commented Dec 27, 2024 • edited Loading

Description

Type of change

Checklist:

Issue Resolution

jubeira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joaobrunoah left a comment

Choose a reason for hiding this comment

elshan-eth commented Jan 6, 2025

EndymionJkb left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elshan-eth commented Dec 27, 2024 •

edited

Loading

EndymionJkb left a comment •

edited

Loading