Yield fees (Step 2) #210
Merged
Yield fees (Step 2) #210
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
# Conflicts: # pkg/vault/contracts/Vault.sol
# Conflicts: # pkg/interfaces/contracts/vault/IVault.sol # pkg/pool-weighted/contracts/WeightedPool8020Factory.sol # pkg/pool-weighted/contracts/WeightedPoolFactory.sol # pkg/pool-weighted/test/foundry/WeightedPool.t.sol # pkg/vault/contracts/test/PoolFactoryMock.sol # pkg/vault/contracts/test/VaultMock.sol
# Conflicts: # pkg/vault/contracts/Vault.sol
Yield fees (Step 2)
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
# Conflicts: # pkg/vault/.forge-snapshots/vaultProtocolSwapFeeGivenIn.snap # pkg/vault/contracts/VaultExtension.sol # pkg/vault/test/Vault.test.ts
jubeira
reviewed
Jan 29, 2024
# Conflicts: # pkg/vault/.forge-snapshots/routerRemoveLiquidityNative.snap # pkg/vault/.forge-snapshots/routerRemoveLiquidityWETH.snap # pkg/vault/.forge-snapshots/vaultRemoveLiquidityProportional.snap # pkg/vault/.forge-snapshots/vaultRemoveLiquiditySingleTokenExactIn.snap # pkg/vault/.forge-snapshots/vaultRemoveLiquiditySingleTokenExactOut.snap
# Conflicts: # pkg/vault/.forge-snapshots/routerAddLiquidityNative.snap # pkg/vault/.forge-snapshots/routerAddLiquidityWETH.snap # pkg/vault/.forge-snapshots/routerRemoveLiquidityNative.snap # pkg/vault/.forge-snapshots/routerRemoveLiquidityWETH.snap # pkg/vault/.forge-snapshots/routerSwapExactInNative.snap # pkg/vault/.forge-snapshots/routerSwapExactInWETH.snap # pkg/vault/.forge-snapshots/vaultAddLiquiditySingleTokenExactOut.snap # pkg/vault/.forge-snapshots/vaultAddLiquidityUnbalanced.snap # pkg/vault/.forge-snapshots/vaultProtocolSwapFeeGivenIn.snap # pkg/vault/.forge-snapshots/vaultRemoveLiquidityProportional.snap # pkg/vault/.forge-snapshots/vaultRemoveLiquiditySingleTokenExactIn.snap # pkg/vault/.forge-snapshots/vaultRemoveLiquiditySingleTokenExactOut.snap # pkg/vault/.forge-snapshots/vaultSwapExactIn.snap # pkg/vault/.forge-snapshots/vaultSwapFeeGivenIn.snap
EndymionJkb
commented
Feb 3, 2024
EndymionJkb
commented
Feb 3, 2024
danielmkm
reviewed
Feb 5, 2024
| TokenType tokenType = poolData.tokenConfig[i].tokenType; | ||
|
|
||
| // Do not charge yield fees until the pool is initialized. | ||
| // ERC4626 tokens always pay yield fees; WITH_RATE tokens pay unless exempt. |
There was a problem hiding this comment.
The flag yieldFeeExempt is essentially ignored for ERC4626 in this case. This is not really clear in how we define the TokenConfig currently and will cause some confusion.
There was a problem hiding this comment.
I've updated the TokenConfig documentation. I believe it's true that ERC4646 tokens charge yield fees by definition, and cannot be marked exempt. In the buffer PR #250, it explicitly checks for this (vs silently failing). This was something in the old "internal buffer" PR that didn't get copied over to the external pool one, so good catch.
# Conflicts: # pkg/vault/contracts/Vault.sol # pkg/vault/contracts/VaultExtension.sol
ylv-io
approved these changes
Feb 5, 2024
There was a problem hiding this comment.
Great job @EndymionJkb . Thank you for adding tests @jubeira 🙏
This was referenced Feb 6, 2024
Closed
This was referenced Feb 14, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Introduce yield fee computation in
computePoolData(formerlygetPoolData).There are some tricky reentrancy issues. I believe
computePoolDatahas to update the balances (otherwise, the "before" call on a swap would be inaccurate), which means it needs to be reentrant.This is fine except for
initialize(probably should be initializePool, come to think of it, so that people don't think the Vault is being initialized...), which is already reentrant. So I made a non-reentrantinitialize(with no external calls), and a reentrant_initializewith the callback, similar to swap/_swap, addLiquidity/_addLiquidity, etc.Generalized the protocol fee handling (since we now have both swap and yield fees, which are collected together), and added events to enable fine-grained tracking by pool (and also validation through a simple invariant).
Deferring "boosted" pool handling, buffers, etc. And deferring optimizing pool balance storage (see separate PR for the infrastructure for that), since those are huge things by themselves.
Type of change
Checklist:
main, or there's a description of how to mergeIssue Resolution
Closes #171