Using wrappedToken in CompositeLiquidityRouter in ERC4626Pool operations #1201

elshan-eth · 2024-12-24T15:46:18Z

Description

The current CLR implementation does not support adding mixed assets. For example, if my pool consists of two wrapped tokens, and I already have one of them and want to use it, this is not possible with the current CLR. The reason is that it treats all input tokens as underlying assets. This PR resolves the issue by introducing a new flag in the interface, allowing us to specify which type of token to use.

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

Closes #1192

jubeira

First pass done. I didn't get to the tests yet, but I think there are a few things that need to be adjusted before proceeding.

Overall the idea is correct; great job. It's just that some of the edge cases should behave differently now that we're not automatically wrapping / unwrapping.

pkg/vault/contracts/CompositeLiquidityRouter.sol

jubeira

Another pass done.

Changes are looking good for the most part. Just a few minor comments around names / docs / style; otherwise seems to be correct. I'll finish covering the tests in one more pass. Great job!

pkg/vault/contracts/CompositeLiquidityRouter.sol

EndymionJkb

Looks like a pretty elegant solution (especially to the limits problem). I have a conceptual issue with the useWrappedTokens name, and lots of little details.

I also think these PRs need a lot more in the description. Why do we need it? What problem does it address? What options did we consider and reject (e.g., in this case, the limits hack, replaced by splitting the wrap/unwrap functions)?

Otherwise we're not going to remember all this context when we go back and look at this in 6 months or a year.

pkg/interfaces/contracts/vault/ICompositeLiquidityRouter.sol

pkg/vault/contracts/CompositeLiquidityRouter.sol

Co-authored-by: EndymionJkb <[email protected]>

…ttps://github.com/balancer/balancer-v3-monorepo into using-wrapped-amount-in-erc4626-operations-in-clr

joaobrunoah

I'd recommend changing isWrappedToken to useWrappedToken, it's probably clearer. Also, since the amounts out are mixing wrapped and underlying, I'd return a token array, for UX purposes. Not sure if gas is a constraint in this case.

pkg/interfaces/contracts/vault/ICompositeLiquidityRouter.sol

joaobrunoah · 2025-01-13T19:47:29Z

I'd recommend changing isWrappedToken to useWrappedToken, it's probably clearer. Also, since the amounts out are mixing wrapped and underlying, I'd return a token array, for UX purposes. Not sure if gas is a constraint in this case.

Ohh, I just noticed it was useWrappedToken before. In my opinion, useWrappedToken is more intuitive than isWrappedToken. Unless you use something like returnInWrappedTokens (remove liquidity) and chargeInWrappedTokens (add liquidity)

EndymionJkb · 2025-01-13T20:59:20Z

I'd recommend changing isWrappedToken to useWrappedToken, it's probably clearer. Also, since the amounts out are mixing wrapped and underlying, I'd return a token array, for UX purposes. Not sure if gas is a constraint in this case.

Ohh, I just noticed it was useWrappedToken before. In my opinion, useWrappedToken is more intuitive than isWrappedToken. Unless you use something like returnInWrappedTokens (remove liquidity) and chargeInWrappedTokens (add liquidity)

I didn't have an issue with the name (also prefer useWrappedToken) - it just seemed like the logic was backwards to me. If the flag is true, it's treated like a standard token, whereas to me, "using" a wrapped token means wrapping/unwrapping with it.

Maybe reverse the sense of the name to make it clearer, like isStandardToken. If true, it ignores that it's a wrapped token. Also, that way it would default to false, which should be the usual case. Most of these will have wrapped tokens that we want the Vault to wrap/unwrap; the special case we're handling is when we don't want to do that.

EndymionJkb

I think we need to address the boolean array name (see my suggestion to use isStandardToken or useAsStandardToken), and returning the token addresses, which would resolve all the "sorted in ... order" comment issues.

elshan-eth · 2025-01-14T17:36:29Z

My eyes are already lost in the description, I double checked several times, but if you can double check, I would be very grateful

EndymionJkb

Looking much better! Note the suggested language for NatSpec, which I believe is much clearer.

EndymionJkb · 2025-01-14T18:35:11Z

pkg/interfaces/contracts/vault/ICompositeLiquidityRouter.sol

+     * @param useAsStandardToken An array indicating whether to use the token as standard or wrap it,
+     * sorted in token registration order of wrapped tokens in the pool
+     * @param exactAmountsIn Exact amounts of underlying/wrapped tokens in, sorted in token registration order
     * wrapped tokens in the pool


Suggested change

* @param useAsStandardToken An array indicating whether to use the token as standard or wrap it,

* sorted in token registration order of wrapped tokens in the pool

* @param exactAmountsIn Exact amounts of underlying/wrapped tokens in, sorted in token registration order

* wrapped tokens in the pool

* @param useAsStandardToken Flags indicating whether the corresponding token should be treated as an ERC4626

* (transfer the underlying asset and wrap) or as a standard ERC20 (transfer the wrapped token directly)

* @param exactAmountsIn Exact amounts of underlying/wrapped tokens in, sorted in token registration order

I think this spells it out. The tokens are specified in registration order, the only question is whether they'll be used directly or interpreted as underlying instead.

You could also have non-ERC4626 tokens (where the flag would be true), which is why I don't think "order of wrapped tokens" is appropriate. It's just the order of tokens as they were registered.

It's not in the diff, but we also need to fix other references to underlying tokens, such as the notice of this function:
"Add arbitrary amounts of underlying tokens to an ERC4626 pool through the buffer."

Generally "underlying tokens" should just be "tokens" where it appears (e.g., notice NatSpec for add/remove functions)

EndymionJkb · 2025-01-14T18:40:04Z

pkg/interfaces/contracts/vault/ICompositeLiquidityRouter.sol

+     * @param useAsStandardToken An array indicating whether to use the token as standard or wrap it,
+     * sorted in token registration order of wrapped tokens in the pool
+     * @param maxAmountsIn Maximum amounts of underlying/wrapped tokens in, sorted in token registration order
     * wrapped tokens in the pool


Suggested change

* @param useAsStandardToken An array indicating whether to use the token as standard or wrap it,

* sorted in token registration order of wrapped tokens in the pool

* @param maxAmountsIn Maximum amounts of underlying/wrapped tokens in, sorted in token registration order

* wrapped tokens in the pool

* @param useAsStandardToken Flags indicating whether the corresponding token should be treated as an ERC4626

* (transfer the underlying asset and wrap) or as a standard ERC20 (transfer the wrapped token directly)

* @param exactAmountsIn Exact amounts of underlying/wrapped tokens in, sorted in token registration order

Same as above

EndymionJkb · 2025-01-14T18:52:18Z

pkg/interfaces/contracts/vault/ICompositeLiquidityRouter.sol

+     * @param useAsStandardToken An array indicating whether to use the token as standard or unwrap it,
+     * sorted in token registration order of wrapped tokens in the pool


Suggested change

* @param useAsStandardToken An array indicating whether to use the token as standard or unwrap it,

* sorted in token registration order of wrapped tokens in the pool

* @param useAsStandardToken Flags indicating whether the corresponding token should be treated as an ERC4626

* (unwrap and transfer the underlying asset) or as a standard ERC20 (transfer the wrapped token directly)

EndymionJkb · 2025-01-14T18:53:35Z

pkg/interfaces/contracts/vault/ICompositeLiquidityRouter.sol

+     * @param minAmountsOut Minimum amounts of each token, sorted according to tokensIn array
     * wrapped tokens in the pool


Suggested change

* @param minAmountsOut Minimum amounts of each token, sorted according to tokensIn array

* wrapped tokens in the pool

* @param minAmountsOut Minimum amounts of each token, corresponding to `tokensOut`

EndymionJkb · 2025-01-14T18:56:26Z

pkg/interfaces/contracts/vault/ICompositeLiquidityRouter.sol

+     * @param useAsStandardToken An array indicating whether to use the token as standard or wrap it,
+     * sorted in token registration order of wrapped tokens in the pool


Suggested change

* @param useAsStandardToken An array indicating whether to use the token as standard or wrap it,

* sorted in token registration order of wrapped tokens in the pool

* @param useAsStandardToken Flags indicating whether the corresponding token should be treated as an ERC4626

* (transfer the underlying asset and wrap) or as a standard ERC20 (transfer the wrapped token directly)

EndymionJkb · 2025-01-14T19:04:06Z

pkg/interfaces/contracts/vault/ICompositeLiquidityRouter.sol

@@ -196,7 +214,7 @@ interface ICompositeLiquidityRouter {
     * @param tokensOut Output token addresses, sorted by user preference. `tokensOut` array must have all tokens from
     * child pools and all tokens that are not BPTs from the nested pool (parent pool). If not all tokens are informed,
     * balances are not settled and the operation reverts. Tokens that repeat must be informed only once.
-     * @param minAmountsOut Minimum amounts of each outgoing underlying token, sorted according to tokensIn array
+     * @param minAmountsOut Minimum amounts of each token, sorted according to tokensIn array
     * @param wethIsEth If true, incoming ETH will be wrapped to WETH and outgoing WETH will be unwrapped to ETH
     * @param userData Additional (optional) data required for the operation
     * @return amountsOut Actual amounts of tokens received, parallel to `tokensOut`


Suggested change

* @return amountsOut Actual amounts of tokens received, parallel to `tokensOut`

* @return amountsOut Actual amounts of tokens received, corresponding to `tokensOut`

Parallel would work too, but trying to be consistent.

EndymionJkb · 2025-01-14T19:04:38Z

pkg/interfaces/contracts/vault/ICompositeLiquidityRouter.sol

@@ -152,7 +170,7 @@ interface ICompositeLiquidityRouter {
     * @param parentPool Address of the highest level pool (which contains BPTs of other pools)
     * @param tokensIn Input token addresses, sorted by user preference. `tokensIn` array must have all tokens from
     * child pools and all tokens that are not BPTs from the nested pool (parent pool).
-     * @param exactAmountsIn Amount of each underlying token in, sorted according to tokensIn array
+     * @param exactAmountsIn Amount of each token in, sorted according to tokensIn array


Suggested change

* @param exactAmountsIn Amount of each token in, sorted according to tokensIn array

* @param exactAmountsIn Amount of each token in, corresponding to `tokensIn`

This seems more generic, as we might be interpreting tokens differently, such that they're not necessarily sorted

EndymionJkb · 2025-01-14T19:04:57Z

pkg/interfaces/contracts/vault/ICompositeLiquidityRouter.sol

@@ -172,7 +190,7 @@ interface ICompositeLiquidityRouter {
     * @param parentPool Address of the highest level pool (which contains BPTs of other pools)
     * @param tokensIn Input token addresses, sorted by user preference. `tokensIn` array must have all tokens from
     * child pools and all tokens that are not BPTs from the nested pool (parent pool).
-     * @param exactAmountsIn Amount of each underlying token in, sorted according to tokensIn array
+     * @param exactAmountsIn Amount of each token in, sorted according to tokensIn array


Suggested change

* @param exactAmountsIn Amount of each token in, sorted according to tokensIn array

* @param exactAmountsIn Amount of each token in, corresponding to `tokensIn`

EndymionJkb · 2025-01-14T19:07:32Z

pkg/vault/contracts/CompositeLiquidityRouter.sol

        for (uint256 i = 0; i < poolTokensLength; ++i) {
            // Treat all ERC4626 pool tokens as wrapped. The next step will verify if we can use the wrappedToken as
-            // a valid ERC4626.
+            // a valid ERC4626. Note that if `useWrappedTokens[i]` is false, we will treat it as a standard token.


Suggested change

// a valid ERC4626. Note that if `useWrappedTokens[i]` is false, we will treat it as a standard token.

// a valid ERC4626.

It's self-documenting now; don't even need a comment.

EndymionJkb · 2025-01-14T19:08:46Z

pkg/vault/contracts/CompositeLiquidityRouter.sol

+
+        for (uint256 i = 0; i < poolTokensLength; ++i) {
+            // Treat all ERC4626 pool tokens as wrapped. The next step will verify if we can use the wrappedToken as
+            // a valid ERC4626. Note that if `useAsStandardToken[i]` is false, we will treat it as a standard token.


Suggested change

// a valid ERC4626. Note that if `useAsStandardToken[i]` is false, we will treat it as a standard token.

// a valid ERC4626.

Don't need the comment (and backwards as written anyway)

jubeira · 2025-01-15T13:53:07Z

Sorry for the late comment, but I feel we're making up new terms here 😅

We're already dealing with wrapped and underlying, and that should be enough. I can't know which of them is standard in that context without reading the docs, which is not ideal.

The argument is consumed by a function called _wrapTokensExactInIfRequired or _wrapTokensExactOutIfRequired. Then, a more appropriate name for the array of flags would be wrapUnderlying. It's pretty clear that if an element is true, you wrap the given underlying amount; if not, you don't.

EndymionJkb · 2025-01-15T15:23:12Z

The argument is consumed by a function called _wrapTokensExactInIfRequired or _wrapTokensExactOutIfRequired. Then, a > more appropriate name for the array of flags would be wrapUnderlying. It's pretty clear that if an element is true, you wrap > the given underlying amount; if not, you don't.

Makes sense - especially because there isn't an unwrap, so there's only one name for the set of flags. The only drawback is the "default" value would be to set everything to true (kind of annoying), but I agree that reusing standard terms is more important.

jubeira

This is looking great.

Just a few more comments; otherwise LGTM. I like the new structure much better; it reuses less code, but it's more clear and goes straight to the point.

jubeira · 2025-01-15T14:19:26Z

pkg/vault/contracts/CompositeLiquidityRouter.sol

+                if (amountsOut[i] < params.minAmountsOut[i]) {
+                    revert IVaultErrors.AmountOutBelowMin(underlyingToken, amountsOut[i], params.minAmountsOut[i]);
+                }


Just for the record, I think this is not needed because we're already applying the limit in the unwrap operation, but it's fine to leave it too for consistency and readability.

jubeira · 2025-01-15T14:28:39Z

pkg/vault/contracts/CompositeLiquidityRouter.sol

-        AddLiquidityHookParams calldata params,
+    function _wrapTokensExactInIfRequired(
+        address sender,
+        bool[] memory useAsStandardToken,
        IERC20[] memory erc4626PoolTokens,
        uint256[] memory amountsIn,


Note for self: amountsIn is params.maxAmountsIn, so we don't need to apply limits here.

jubeira · 2025-01-15T15:50:20Z

pkg/vault/contracts/CompositeLiquidityRouter.sol

+                if (amountsOut[i] < params.minAmountsOut[i]) {
+                    revert IVaultErrors.AmountOutBelowMin(underlyingToken, amountsOut[i], params.minAmountsOut[i]);
+                }

-            if (isStaticCall == false) {
-                _sendTokenOut(params.sender, underlyingToken, underlyingAmountsOut[i], params.wethIsEth);
+                if (isStaticCall == false) {
+                    _sendTokenOut(params.sender, underlyingToken, amountsOut[i], params.wethIsEth);
+                }


Actually these 2 inner ifs are the same in the "no unwrap" and in the "wrap" case, as long as you set the token out and amount out properly.

If you put these lines inside the for loop the code might be a bit simpler.

jubeira · 2025-01-15T15:51:40Z

pkg/vault/contracts/CompositeLiquidityRouter.sol

-        RemoveLiquidityHookParams calldata params
-    ) external nonReentrant onlyVault returns (uint256[] memory underlyingAmountsOut) {
+        RemoveLiquidityHookParams calldata params,
+        bool[] calldata useAsStandardToken


About naming: in this case it would be unwrapWrapper or unwrapWrapped I guess

The opposite of wrapUnderlying would be unwrapWrapped, so we do need it in both directions, and the default values would be true. (And unwrapWrapped is a bit of a tongue twister.)

Unless useAsStandardToken -> doNotWrapOrUnwrap or doNotUseAsWrappedToken or doNotUseAsERC4626 (defaults to false, single name), or doNotWrap, doNotUnwrap (defaults to false, two names), without making up new/potentially confusing terms.

Names are hard.

I don't think we have 'default values' anymore. The default in the current version is always wrap / unwrap, but that doesn't mean it has to be that way for this version. Sending false arrays is easier than true in solidity, but for the SDK I don't think it's a big deal either way.

I don't have a strong opinion about the tone, but I do prefer positive flags (i.e. true == do something as opposed to true == don't do something) from a readability / comprehension perspective.

fist iteration

1ca2458

elshan-eth requested review from jubeira, joaobrunoah and EndymionJkb December 24, 2024 15:46

jubeira reviewed Jan 6, 2025

View reviewed changes

fix code and tests

757545a

elshan-eth requested a review from jubeira January 7, 2025 18:07

change var name

9dbf7fc

jubeira reviewed Jan 8, 2025

View reviewed changes

refactoring prepareTokens

4b39b72

elshan-eth changed the title ~~Using wrappedToken in CompositeLiquidityRouter in ERC4626Pool operations~~ [IN PROGRESS] Using wrappedToken in CompositeLiquidityRouter in ERC4626Pool operations Jan 9, 2025

fixes

f6bcd4e

elshan-eth changed the title ~~[IN PROGRESS] Using wrappedToken in CompositeLiquidityRouter in ERC4626Pool operations~~ Using wrappedToken in CompositeLiquidityRouter in ERC4626Pool operations Jan 10, 2025

elshan-eth requested a review from jubeira January 10, 2025 13:34

EndymionJkb reviewed Jan 11, 2025

View reviewed changes

elshan-eth and others added 14 commits January 13, 2025 18:10

Update pkg/interfaces/contracts/vault/ICompositeLiquidityRouter.sol

413315d

Co-authored-by: EndymionJkb <[email protected]>

Update pkg/interfaces/contracts/vault/ICompositeLiquidityRouter.sol

c441933

Co-authored-by: EndymionJkb <[email protected]>

Update pkg/interfaces/contracts/vault/ICompositeLiquidityRouter.sol

164cdae

Co-authored-by: EndymionJkb <[email protected]>

Update pkg/interfaces/contracts/vault/ICompositeLiquidityRouter.sol

6d7f538

Co-authored-by: EndymionJkb <[email protected]>

Update pkg/interfaces/contracts/vault/ICompositeLiquidityRouter.sol

c4a2662

Co-authored-by: EndymionJkb <[email protected]>

Update pkg/interfaces/contracts/vault/ICompositeLiquidityRouter.sol

6b1c8db

Co-authored-by: EndymionJkb <[email protected]>

Update pkg/vault/contracts/CompositeLiquidityRouter.sol

b0eeb6e

Co-authored-by: EndymionJkb <[email protected]>

Update pkg/vault/contracts/CompositeLiquidityRouter.sol

6008eca

Co-authored-by: EndymionJkb <[email protected]>

Update pkg/interfaces/contracts/vault/ICompositeLiquidityRouter.sol

06ab630

Co-authored-by: EndymionJkb <[email protected]>

Update pkg/vault/contracts/CompositeLiquidityRouter.sol

9fe3242

Co-authored-by: EndymionJkb <[email protected]>

small fixes

acac9ec

Merge branch 'using-wrapped-amount-in-erc4626-operations-in-clr' of h…

8a876c8

…ttps://github.com/balancer/balancer-v3-monorepo into using-wrapped-amount-in-erc4626-operations-in-clr

fixes

cf43702

update snapshots

af63d70

elshan-eth requested a review from EndymionJkb January 13, 2025 18:26

joaobrunoah reviewed Jan 13, 2025

View reviewed changes

EndymionJkb reviewed Jan 13, 2025

View reviewed changes

small fixes

8ca5aa6

elshan-eth requested review from EndymionJkb and joaobrunoah January 14, 2025 17:36

small fixes

537f45f

EndymionJkb reviewed Jan 14, 2025

View reviewed changes

jubeira reviewed Jan 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using wrappedToken in CompositeLiquidityRouter in ERC4626Pool operations #1201

Using wrappedToken in CompositeLiquidityRouter in ERC4626Pool operations #1201

elshan-eth commented Dec 24, 2024 •

edited

Loading

jubeira left a comment •

edited

Loading

jubeira left a comment

EndymionJkb left a comment •

edited

Loading

joaobrunoah left a comment

joaobrunoah commented Jan 13, 2025

EndymionJkb commented Jan 13, 2025 •

edited

Loading

EndymionJkb left a comment

elshan-eth commented Jan 14, 2025

EndymionJkb left a comment

EndymionJkb Jan 14, 2025

EndymionJkb Jan 14, 2025

EndymionJkb Jan 14, 2025

EndymionJkb Jan 14, 2025

EndymionJkb Jan 14, 2025

EndymionJkb Jan 14, 2025

EndymionJkb Jan 14, 2025

EndymionJkb Jan 14, 2025

EndymionJkb Jan 14, 2025

EndymionJkb Jan 14, 2025

EndymionJkb Jan 14, 2025

jubeira commented Jan 15, 2025

EndymionJkb commented Jan 15, 2025

jubeira left a comment

jubeira Jan 15, 2025

jubeira Jan 15, 2025

jubeira Jan 15, 2025

jubeira Jan 15, 2025

EndymionJkb Jan 15, 2025 •

edited

Loading

jubeira Jan 15, 2025

		* @param useAsStandardToken An array indicating whether to use the token as standard or unwrap it,
		* sorted in token registration order of wrapped tokens in the pool

		* @param minAmountsOut Minimum amounts of each token, sorted according to tokensIn array
		* wrapped tokens in the pool

	* @return amountsOut Actual amounts of tokens received, parallel to `tokensOut`
	* @return amountsOut Actual amounts of tokens received, corresponding to `tokensOut`

	* @param exactAmountsIn Amount of each token in, sorted according to tokensIn array
	* @param exactAmountsIn Amount of each token in, corresponding to `tokensIn`

	// a valid ERC4626. Note that if `useWrappedTokens[i]` is false, we will treat it as a standard token.
	// a valid ERC4626.

	// a valid ERC4626. Note that if `useAsStandardToken[i]` is false, we will treat it as a standard token.
	// a valid ERC4626.

Using wrappedToken in CompositeLiquidityRouter in ERC4626Pool operations #1201

Are you sure you want to change the base?

Using wrappedToken in CompositeLiquidityRouter in ERC4626Pool operations #1201

Conversation

elshan-eth commented Dec 24, 2024 • edited Loading

Description

Type of change

Checklist:

Issue Resolution

jubeira left a comment • edited Loading

Choose a reason for hiding this comment

jubeira left a comment

Choose a reason for hiding this comment

EndymionJkb left a comment • edited Loading

Choose a reason for hiding this comment

joaobrunoah left a comment

Choose a reason for hiding this comment

joaobrunoah commented Jan 13, 2025

EndymionJkb commented Jan 13, 2025 • edited Loading

EndymionJkb left a comment

Choose a reason for hiding this comment

elshan-eth commented Jan 14, 2025

EndymionJkb left a 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

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

jubeira commented Jan 15, 2025

EndymionJkb commented Jan 15, 2025

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EndymionJkb Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elshan-eth commented Dec 24, 2024 •

edited

Loading

jubeira left a comment •

edited

Loading

EndymionJkb left a comment •

edited

Loading

EndymionJkb commented Jan 13, 2025 •

edited

Loading

EndymionJkb Jan 15, 2025 •

edited

Loading