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

Add LBPool #1001

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

Add LBPool #1001

wants to merge 63 commits into from

Conversation

gerrrg
Copy link
Collaborator

@gerrrg gerrrg commented Sep 22, 2024

Description

tl;dr: Create a weight shifting LBPool for V3.

Note

This PR replaces and obsoletes #971 by turning it into a PR from the add-lbpool branch from w/in this repo rather than my fork.

What

  • Port over the main functionality of LBPool from V2 to V3
  • Port over the gradual value change library
  • Create a WeightValidation library for improved portability (could use this in standard WP if desired)
  • Adhere to events for V2 LBP to improve the ease of integration/compatibility
  • Make the pool contract its own hook contract and do beforeJoin hook for onlyOwner LP
  • Store info for only a single weight for higher precision/lower gas since weights must sum to FP.ONE
  • Add relevant tests
  • Modify some inherited tests (BasePoolTest.sol) to be more generalizable/override-able

Why

Modernize a highly popular feature from v2

Type of change

  • Bug fix
  • New feature
  • Breaking change
  • Dependency changes
  • Code refactor / cleanup
  • Optimization: [ ] gas / [ ] bytecode
  • Documentation or wording changes
  • Other

Checklist:

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

Issue Resolution

n/a

gerrrg and others added 30 commits July 23, 2024 13:47
…x time to avoid time overflow attack; add misc events and errors; change paused vars to be swapEnabled; add setters and getters for swap enabled/swap fee/gradual update params; add 3 hooks for dynamic swap fee percentage, paused blocking swaps, and only owner can join; build out GradualValueChange library
…BP's add liquidity hook; change terminology back to what it originally was in the interface
…enable swap fee changes by the assigned admin rather than assuming that power has been delegated to governance
…R rather than the placeholder trusted router provider; leave references in comments for future addition
@gerrrg
Copy link
Collaborator Author

gerrrg commented Sep 24, 2024

@EndymionJkb I'm not sure if this is a bug in lcov, or if I'm actually doing something wrong here.

For LBPoolFactory.sol, lcov is claiming that the two SLOC in the constructor (setting _poolVersion and _TRUSTED_ROUTER) are not covered, which appears to be incorrect. The constructor has to be invoked in order for the rest of the tests to run, and other tests (like getPoolVersion and the LBP's trusted router test) would undoubtedly fail if these lines were not run.

Other than that, I think I'm at 100% coverage now -- just a bit puzzled by that result

@EndymionJkb
Copy link
Collaborator

@EndymionJkb I'm not sure if this is a bug in lcov, or if I'm actually doing something wrong here.
Other than that, I think I'm at 100% coverage now -- just a bit puzzled by that result

We're aware of issues with coverage. It behaves differently / has holes on both hardhat and forge. Modifiers aren't handled very well, for instance (though that's not what's going on here), which is why they can be run either independently or merged.

It's good to run coverage to make sure you aren't missing anything, but some anomalies are expected, so don't worry about it if it flags something you know actually does run.

# Conflicts:
#	pkg/pool-weighted/test/gas/.hardhat-snapshots/[WeightedPool - ERC4626 - BatchRouter] swapExactIn - with buffer liquidity - warm slots
#	pkg/pool-weighted/test/gas/.hardhat-snapshots/[WeightedPool - ERC4626 - BatchRouter] swapExactOut - no buffer liquidity - warm slots
# Conflicts:
#	pkg/pool-weighted/test/gas/.hardhat-snapshots/[WeightedPool - With rate] swap single token exact in with fees - cold slots
#	pkg/pool-weighted/test/gas/.hardhat-snapshots/[WeightedPool - With rate] swap single token exact in with fees - warm slots
# Conflicts:
#	pkg/pool-weighted/test/gas/.hardhat-snapshots/[WeightedPool - ERC4626 - BatchRouter] swapExactIn - with buffer liquidity - warm slots
# Conflicts:
#	pkg/pool-stable/test/gas/.hardhat-snapshots/[StablePool - WithNestedPool - BatchRouter] swap exact in - tokenA-tokenD
# Conflicts:
#	pkg/vault/test/foundry/utils/BasePoolTest.sol
Copy link
Contributor

@elshan-eth elshan-eth left a comment

Choose a reason for hiding this comment

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

Looks good! I have a couple of minor comments

pkg/pool-weighted/contracts/lbp/LBPool.sol Outdated Show resolved Hide resolved
LiquidityManagement calldata
) public view override onlyVault returns (bool) {
// Since in this case the pool is the hook, we don't need to check anything else.
// We *could* check that it's two tokens, but better to let that be caught later, as it will fail with a more
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that the code does not have this check

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What specifically are you asking for here? The check here verifies that this contract is both the pool and the hook

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean checking that the pool contains only 2 tokens

pkg/pool-weighted/contracts/lbp/LBPool.sol Outdated Show resolved Hide resolved
pkg/pool-weighted/test/LBPool.test.ts Outdated Show resolved Hide resolved
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