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

Refactoring addLiquidityUnbalancedNestedPool in CLR #1203

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Refactoring addLiquidityUnbalancedNestedPool in CLR #1203

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9fda3dd
addLiquidityUnbalancedNestedPool refactoring
elshan-eth Dec 27, 2024
33c3308
add size snapshot
elshan-eth Dec 27, 2024
e5d4745
add recursion level
elshan-eth Jan 6, 2025
6927331
add recursion level
elshan-eth Jan 6, 2025
f88a69c
fix codestyle
elshan-eth Jan 6, 2025
465f0af
add commants
elshan-eth Jan 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
127 changes: 44 additions & 83 deletions pkg/vault/contracts/CompositeLiquidityRouter.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,10 +482,42 @@ contract CompositeLiquidityRouter is ICompositeLiquidityRouter, BatchRouterCommo
// Loads a Set with all amounts to be inserted in the nested pools, so we don't need to iterate in the tokens
// array to find the child pool amounts to insert.
for (uint256 i = 0; i < tokensIn.length; ++i) {
if (params.maxAmountsIn[i] == 0) {
continue;
}
elshan-eth marked this conversation as resolved.
Show resolved Hide resolved

_currentSwapTokenInAmounts().tSet(tokensIn[i], params.maxAmountsIn[i]);
_currentSwapTokensIn().add(tokensIn[i]);
}

IERC20[] memory parentPoolTokens = _vault.getPoolTokens(params.pool);
(uint256[] memory amountsIn, ) = _addLiquidityRecursive(params.pool, params);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// Adds liquidity to the parent pool, mints parentPool's BPT to the sender and checks the minimum BPT out.
(, exactBptAmountOut, ) = _vault.addLiquidity(
AddLiquidityParams({
pool: params.pool,
to: isStaticCall ? address(this) : params.sender,
maxAmountsIn: amountsIn,
minBptAmountOut: params.minBptAmountOut,
kind: params.kind,
userData: params.userData
})
);

// Settle the amounts in.
if (isStaticCall == false) {
_settlePaths(params.sender, params.wethIsEth);
}
}

function _addLiquidityRecursive(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

address pool,
AddLiquidityHookParams calldata params
) internal returns (uint256[] memory amountsIn, bool allAmountsEmpty) {
allAmountsEmpty = true;

IERC20[] memory parentPoolTokens = _vault.getPoolTokens(pool);
amountsIn = new uint256[](parentPoolTokens.length);

// Iterate over each token of the parent pool. If it's a BPT, add liquidity unbalanced to it.
for (uint256 i = 0; i < parentPoolTokens.length; i++) {
Expand All @@ -494,11 +526,9 @@ contract CompositeLiquidityRouter is ICompositeLiquidityRouter, BatchRouterCommo
if (_vault.isPoolRegistered(childToken)) {
// Token is a BPT, so add liquidity to the child pool.

IERC20[] memory childPoolTokens = _vault.getPoolTokens(childToken);
(uint256[] memory childPoolAmountsIn, bool childPoolAmountsEmpty) = _getPoolAmountsIn(
childPoolTokens,
params.sender,
params.wethIsEth
(uint256[] memory childPoolAmountsIn, bool childPoolAmountsEmpty) = _addLiquidityRecursive(
elshan-eth marked this conversation as resolved.
Show resolved Hide resolved
childToken,
params
);

if (childPoolAmountsEmpty == false) {
Expand All @@ -515,9 +545,7 @@ contract CompositeLiquidityRouter is ICompositeLiquidityRouter, BatchRouterCommo
})
);

// Sets the amount in of child BPT to the exactBptAmountOut of the child pool, so all the minted BPT
// will be added to the parent pool.
_currentSwapTokenInAmounts().tSet(childToken, exactChildBptAmountOut);
amountsIn[i] = exactChildBptAmountOut;

// Since the BPT will be inserted into the parent pool, gets the credit from the inserted BPTs in
// advance.
Expand All @@ -530,78 +558,14 @@ contract CompositeLiquidityRouter is ICompositeLiquidityRouter, BatchRouterCommo
// The ERC4626 token has a buffer initialized within the Vault. Additionally, since the sender did not
// specify an input amount for the wrapped token, the function will wrap the underlying asset and use
// the resulting wrapped tokens to add liquidity to the pool.
_wrapAndUpdateTokenInAmounts(IERC4626(childToken), params.sender, params.wethIsEth);
}
}

(uint256[] memory parentPoolAmountsIn, ) = _getPoolAmountsIn(parentPoolTokens, params.sender, params.wethIsEth);

// Adds liquidity to the parent pool, mints parentPool's BPT to the sender and checks the minimum BPT out.
(, exactBptAmountOut, ) = _vault.addLiquidity(
AddLiquidityParams({
pool: params.pool,
to: isStaticCall ? address(this) : params.sender,
maxAmountsIn: parentPoolAmountsIn,
minBptAmountOut: params.minBptAmountOut,
kind: params.kind,
userData: params.userData
})
);

// Since all values from _currentSwapTokenInAmounts are erased, recreates the set of amounts in so
// `_settlePaths()` can charge the sender.
for (uint256 i = 0; i < tokensIn.length; ++i) {
address tokenIn = tokensIn[i];
// Wrap operations take underlying token in advance, so we discount them.
uint256 amountIn = params.maxAmountsIn[i] - _settledTokenAmounts().tGet(tokenIn);
// Reset _settledTokensAmount, in case the router is called again in the same transaction.
_settledTokenAmounts().tSet(tokenIn, 0);
if (amountIn > 0) {
_currentSwapTokensIn().add(tokensIn[i]);
_currentSwapTokenInAmounts().tSet(tokenIn, amountIn);
amountsIn[i] = _wrapAndUpdateTokenInAmounts(IERC4626(childToken), params.sender, params.wethIsEth);
} else if (_settledTokenAmounts().tGet(childToken) == 0) {
elshan-eth marked this conversation as resolved.
Show resolved Hide resolved
amountsIn[i] = _currentSwapTokenInAmounts().tGet(childToken);
_settledTokenAmounts().tSet(childToken, amountsIn[i]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

// Settle the amounts in.
if (isStaticCall == false) {
_settlePaths(params.sender, params.wethIsEth);
}
}

/**
* @notice Creates an array of amounts in to insert in a pool, given an array of tokens.
* @dev This function requires the transient set `_currentSwapTokenInAmounts` to be initialized first with all the
* amount in values that the sender informed in the addLiquidity call.
*/
function _getPoolAmountsIn(
IERC20[] memory poolTokens,
address sender,
bool wethIsEth
) private returns (uint256[] memory poolAmountsIn, bool amountsEmpty) {
poolAmountsIn = new uint256[](poolTokens.length);
amountsEmpty = true;

for (uint256 j = 0; j < poolTokens.length; j++) {
address poolToken = address(poolTokens[j]);
if (
_vault.isERC4626BufferInitialized(IERC4626(poolToken)) &&
_currentSwapTokenInAmounts().tGet(poolToken) == 0 // wrapped amount in was not specified
) {
// The token is an ERC4626 and has a buffer initialized within the Vault. Additionally, since the
// sender did not specify an input amount for the wrapped token, the function will wrap the underlying
// asset and use the resulting wrapped tokens to add liquidity to the pool.
uint256 wrappedAmount = _wrapAndUpdateTokenInAmounts(IERC4626(poolToken), sender, wethIsEth);
poolAmountsIn[j] = wrappedAmount;
} else {
poolAmountsIn[j] = _currentSwapTokenInAmounts().tGet(poolToken);
// This operation does not support adding liquidity multiple times to the same token. So, we set
// the amount in of the child pool token to 0. If the same token appears more times, the amount in
// will be 0 for any other pool.
_currentSwapTokenInAmounts().tSet(poolToken, 0);
}

if (poolAmountsIn[j] > 0) {
amountsEmpty = false;
if (amountsIn[i] > 0) {
allAmountsEmpty = false;
}
}
}
Expand All @@ -628,8 +592,6 @@ contract CompositeLiquidityRouter is ICompositeLiquidityRouter, BatchRouterCommo
if (isStaticCall == false) {
// Take the underlying token amount, required to wrap, in advance.
_takeTokenIn(sender, IERC20(underlyingToken), underlyingAmountIn, wethIsEth);
// Since the wrap operation was paid in advance, set the underlying as settled.
_settledTokenAmounts().tSet(underlyingToken, underlyingAmountIn);
}

(, , wrappedAmountOut) = _vault.erc4626BufferWrapOrUnwrap(
Expand All @@ -642,9 +604,8 @@ contract CompositeLiquidityRouter is ICompositeLiquidityRouter, BatchRouterCommo
})
);

// Remove the underlying amount from `_currentSwapTokenInAmounts` and add the wrapped amount.
_currentSwapTokensIn().remove(underlyingToken);
_currentSwapTokenInAmounts().tSet(underlyingToken, 0);
_currentSwapTokenInAmounts().tSet(address(wrappedToken), wrappedAmountOut);
elshan-eth marked this conversation as resolved.
Show resolved Hide resolved

// Updates the reserves of the vault with the wrappedToken amount.
_vault.settle(IERC20(address(wrappedToken)), wrappedAmountOut);
Expand Down
4 changes: 2 additions & 2 deletions pkg/vault/test/.contract-sizes/CompositeLiquidityRouter
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
Bytecode 21.554
InitCode 23.372
Bytecode 20.973
InitCode 22.723
26 changes: 14 additions & 12 deletions pkg/vault/test/foundry/CompositeLiquidityRouterNestedPools.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,21 +210,20 @@ contract CompositeLiquidityRouterNestedPoolsTest is BaseERC4626BufferTest {
);

// Check ChildPoolB balances.
assertApproxEqAbs(
vars.childPoolBAfter.dai,
vars.childPoolBBefore.dai + amountsIn[vars.daiIdx],
MAX_ROUND_ERROR,
"ChildPoolB Dai Balance is wrong"
);
assertEq(vars.childPoolBAfter.dai, vars.childPoolBBefore.dai, "ChildPoolB Dai Balance is wrong");
elshan-eth marked this conversation as resolved.
Show resolved Hide resolved
assertEq(
vars.childPoolBAfter.wsteth,
vars.childPoolBBefore.wsteth + amountsIn[vars.wstethIdx],
"ChildPoolB Wsteth Balance is wrong"
);

// Check ParentPool balances.
// The ParentPool's DAI balance does not change since all DAI amount is inserted in the child pool A.
assertEq(vars.parentPoolAfter.dai, vars.parentPoolBefore.dai, "ParentPool Dai Balance is wrong");
assertApproxEqAbs(
vars.parentPoolAfter.dai,
vars.parentPoolBefore.dai + amountsIn[vars.daiIdx],
MAX_ROUND_ERROR,
"ParentPool Dai Balance is wrong"
);
assertEq(
vars.parentPoolAfter.childPoolABpt,
vars.parentPoolBefore.childPoolABpt + mintedChildPoolABpt,
Expand Down Expand Up @@ -262,7 +261,6 @@ contract CompositeLiquidityRouterNestedPoolsTest is BaseERC4626BufferTest {

_fillNestedPoolTestLocalsAfter(vars);
uint256 mintedChildPoolABpt = vars.childPoolAAfter.totalSupply - vars.childPoolABefore.totalSupply;
uint256 mintedChildPoolBBpt = vars.childPoolBAfter.totalSupply - vars.childPoolBBefore.totalSupply;

// Check exact BPT out.
// Since all pools are linear and there's no rate, the expected BPT amount out is the sum of all amounts in.
Expand Down Expand Up @@ -932,7 +930,7 @@ contract CompositeLiquidityRouterNestedPoolsTest is BaseERC4626BufferTest {
parentPoolWithWrapper,
tokensIn,
amountsIn,
address(this),
lp,
bytes("")
);

Expand Down Expand Up @@ -1050,15 +1048,19 @@ contract CompositeLiquidityRouterNestedPoolsTest is BaseERC4626BufferTest {
// Check ChildPoolB balances.
assertApproxEqAbs(
vars.childPoolBAfter.dai,
vars.childPoolBBefore.dai + amountsIn[vars.daiIdx],
vars.childPoolBBefore.dai,
MAX_ROUND_ERROR,
"ChildPoolB Dai Balance is wrong"
);
assertEq(vars.childPoolBAfter.wsteth, vars.childPoolBBefore.wsteth, "ChildPoolB Wsteth Balance is wrong");

// Check ParentPool balances.
// The ParentPool's DAI balance does not change since all DAI amount is inserted in the child pool A.
assertEq(vars.parentPoolAfter.dai, vars.parentPoolBefore.dai, "ParentPool Dai Balance is wrong");
assertEq(
vars.parentPoolAfter.dai,
vars.parentPoolBefore.dai + amountsIn[vars.daiIdx],
"ParentPool Dai Balance is wrong"
);
assertEq(
vars.parentPoolAfter.childPoolABpt,
vars.parentPoolBefore.childPoolABpt + mintedChildPoolABpt,
Expand Down
Loading