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

Minimal Weighted Pool #87

Merged
merged 181 commits into from
Oct 20, 2023
Merged

Minimal Weighted Pool #87

merged 181 commits into from
Oct 20, 2023

Conversation

ylv-io
Copy link
Contributor

@ylv-io ylv-io commented Sep 6, 2023

Description

This PR introduces the following enhancements and features:

  • Adds a "Minimal Weighted Pool" implementation
  • Introduces a "Pool Utils" package
  • Includes a "Pool Weighted" package
  • Segregates multi-token functions into a separate interface, IERC20MultiToken
  • Establishes the IBasePool interface, applicable to all V3 pools
  • Offers a BasePool contract as a base class for all V3 pools to inherit from
  • Enables factories to register pools as a distinct, standalone step
  • Introduces a new IVault.initialize method for explicit pool initialization, bypassing the need for joins
  • Adds a new math lib, BasePoolMath
  • Adds a new lib, ScalingHelpers.
  • Provides comprehensive tests for the WeightedPool class, covering methods such as addLiquidity, removeLiquidity, initialize, and swap.

Type of change

  • Bug fix
  • New feature
  • Breaking change
  • Dependency changes
  • Code refactor / cleanup
  • Documentation or wording changes
  • Other

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
Copy link
Contributor

jubeira commented Oct 18, 2023

Comments addressed! @danielmkm @EndymionJkb would you please take another look?

Copy link
Collaborator

@EndymionJkb EndymionJkb left a comment

Choose a reason for hiding this comment

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

This is changing out from under me, so will put in these comments for now.

pkg/interfaces/contracts/vault/IBasePool.sol Outdated Show resolved Hide resolved
pkg/interfaces/contracts/vault/IBasePool.sol Outdated Show resolved Hide resolved
pkg/interfaces/contracts/vault/IRouter.sol Outdated Show resolved Hide resolved
pkg/interfaces/contracts/vault/IVault.sol Outdated Show resolved Hide resolved
pkg/interfaces/contracts/vault/IVaultErrors.sol Outdated Show resolved Hide resolved
pkg/pool-utils/README.md Outdated Show resolved Hide resolved
uint256 private constant _MIN_TOKENS = 2;

uint256 private constant _DEFAULT_MINIMUM_BPT = 1e6;
uint256 private constant _SWAP_FEE_PERCENTAGE = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

From our earlier discussions, it seems like we don't need any limits (other than 0-1) on the swap fee percentage.

pkg/pool-utils/contracts/BasePool.sol Show resolved Hide resolved
pkg/pool-utils/contracts/BasePool.sol Outdated Show resolved Hide resolved
* @notice Return the current value of the swap fee percentage.
*/
function getSwapFeePercentage() public pure virtual returns (uint256) {
return _SWAP_FEE_PERCENTAGE;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Out of scope to actually implement static and dynamic swap fees (in the Vault as part of pool config + pool callback for dynamic), but for now we might want to hard-code this to something non-zero for testing.

Copy link
Contributor

@danielmkm danielmkm left a comment

Choose a reason for hiding this comment

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

The strategy for pool validation in swap is not the same as add/remove. I pushed a small change that removes the isInitialized modifier and does an inline check immediately after the registration check.

There appears to be an issue with bptAmountOut in initialize now that we are minting BPT to the zero address.

pkg/vault/contracts/Vault.sol Outdated Show resolved Hide resolved
pkg/pool-utils/contracts/test/PoolMock.sol Outdated Show resolved Hide resolved
pkg/interfaces/contracts/vault/IBasePool.sol Show resolved Hide resolved
pkg/pool-weighted/contracts/WeightedPool.sol Show resolved Hide resolved
@jubeira
Copy link
Contributor

jubeira commented Oct 19, 2023

The strategy for pool validation in swap is not the same as add/remove. I pushed a small change that removes the isInitialized modifier and does an inline check immediately after the registration check.

As discussed offline, I brought back the modifier.
The workflow should be:

pool created (can only register) --> registered (can only init) --> initialized (can operate)

Therefore, if we ask the pool to be initialized, it means it's also registered. I've modified the code that handles revert reasons for wrong token indexes inside swaps accordingly.

There appears to be an issue with bptAmountOut in initialize now that we are minting BPT to the zero address.

Corrected so that bptAmountOut corresponds to the amount minted to the target recipient.

Besides that, I'd like to wrap up this one. Hopefully this should be the last huge PR where we were in full draft mode, in favor of smaller, more focused PRs. I expect there will still be some exploration ahead as there are some base features to be implemented where we'll take a different path wrt. V2, but it should be more manageable. Let's please open tickets if you find something we're missing but can be addressed later.

Copy link
Collaborator

@EndymionJkb EndymionJkb left a comment

Choose a reason for hiding this comment

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

Have a couple questions, but overall - and given that we know major things will change, such as moving scaling factors, so no point reviewing details of that - I think it's good enough to merge this monster so that we can address the remaining items with smaller PRs. (Renaming suggestions are so minor they could be done now; otherwise a followup PR is fine, as was done with multitoken.)

pkg/pool-utils/contracts/BasePool.sol Outdated Show resolved Hide resolved
pkg/pool-weighted/contracts/WeightedPool.sol Outdated Show resolved Hide resolved
pkg/pool-utils/contracts/BasePool.sol Outdated Show resolved Hide resolved
pkg/pool-utils/contracts/test/PoolMock.sol Outdated Show resolved Hide resolved
pkg/pool-utils/contracts/test/PoolMock.sol Outdated Show resolved Hide resolved
pkg/pool-utils/contracts/test/PoolMock.sol Outdated Show resolved Hide resolved
pkg/pool-utils/contracts/test/PoolMock.sol Outdated Show resolved Hide resolved
pkg/pool-utils/contracts/test/PoolMock.sol Outdated Show resolved Hide resolved
pkg/pool-utils/contracts/test/PoolMock.sol Show resolved Hide resolved
pkg/pool-weighted/contracts/WeightedPool.sol Show resolved Hide resolved
jubeira and others added 3 commits October 19, 2023 16:23
Co-authored-by: EndymionJkb <EndymionJkb@gmail.com>
Co-authored-by: EndymionJkb <EndymionJkb@gmail.com>
Copy link
Contributor

@danielmkm danielmkm left a comment

Choose a reason for hiding this comment

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

Agree that we should get this merged! The bulk of my concerns have already been addressed, anything else is smaller details that we can address later on.

@jubeira jubeira merged commit 3e6ef02 into main Oct 20, 2023
6 of 7 checks passed
@jubeira jubeira deleted the join-exit branch October 20, 2023 12:11
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

Successfully merging this pull request may close these issues.

4 participants