-
Notifications
You must be signed in to change notification settings - Fork 6
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
Uniswap liquidity manager #12
Conversation
Can you import interfaces and contracts where it is possible to do so? I think the number of duplicated files are getting difficult to manage. For example, |
contracts/LiquidityManager.sol
Outdated
int24 private constant TICK_SPACING = 60; | ||
|
||
address internal immutable WETH; | ||
address public nonfungiblePositionManager; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
INonfungiblePositionManager
contracts/LiquidityManager.sol
Outdated
int24 private constant MAX_TICK = -MIN_TICK; | ||
int24 private constant TICK_SPACING = 60; | ||
|
||
address internal immutable WETH; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WETH
contracts/LiquidityManager.sol
Outdated
|
||
address internal immutable WETH; | ||
address public nonfungiblePositionManager; | ||
address public uniswapV3Factory; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IUniswapV3Factory
contracts/LiquidityManager.sol
Outdated
token.approve(nonfungiblePositionManager, tokenAmount); | ||
|
||
uint256 eth = address(this).balance; | ||
IWETH(WETH).deposit{value: eth}(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not deposit everything to WETH. Just check if approved amount is already greater than what's needed. If there is a shortage, approve and deposit that amount
contracts/LiquidityManager.sol
Outdated
return pool; | ||
} | ||
|
||
function sqrt(uint256 x) internal pure returns (uint256) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use sqrt function in UD60X18 from prb-math instead? It is already installed in foundry dependency. Same approximation method, but checks for overflow, uses better initialization value, and limit the number of approximation steps
contracts/LiquidityManager.sol
Outdated
IWETH(WETH).deposit{value: eth}(); | ||
IWETH(WETH).approve(nonfungiblePositionManager, eth); | ||
|
||
uint160 sqrtPriceX96 = uint160(sqrt((ethAmount * 2 ** 192) / tokenAmount)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will overflow easily, since 1e18 ~= 2^60, so any amount of native token greater than 2^(256-192-60) = 2^14 = 16384 would make ethAmount * 2 ** 192
overflow. I suggest scaling it by 96 bits only (which gives 2^96 more "space" to prevent overflow) before taking the division and sqrt, then scaling it by another 48 bits afterwards
contracts/LiquidityManager.sol
Outdated
IWETH(WETH).approve(nonfungiblePositionManager, eth); | ||
|
||
uint160 sqrtPriceX96 = uint160(sqrt((ethAmount * 2 ** 192) / tokenAmount)); | ||
_nonfungiblePositionManager.createAndInitializePoolIfNecessary(tokenAddress, WETH, UNISWAP_FEE, sqrtPriceX96); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Method does not exist. Just call initialize with sqrtPriceX96? Pool should be already created
uint160 sqrtPriceX96 = uint160(sqrt((ethAmount * 2 ** 192) / tokenAmount)); | ||
_nonfungiblePositionManager.createAndInitializePoolIfNecessary(tokenAddress, WETH, UNISWAP_FEE, sqrtPriceX96); | ||
|
||
INonfungiblePositionManager.MintParams memory params = INonfungiblePositionManager.MintParams({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The convention (used in Uniswap code) is to order [token0,token1] addresses based on their hexadecimal value, and to have the lower valued address as token0. The pool does it automatically upon creation, so the parameters here should follow the same convention as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved
tickUpper: (MAX_TICK / TICK_SPACING) * TICK_SPACING, | ||
amount0Desired: tokenAmount, | ||
amount1Desired: ethAmount, | ||
amount0Min: 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be close to tokenAmount and ethAmount, as sanity check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved
token0: tokenAddress, | ||
token1: WETH, | ||
fee: UNISWAP_FEE, | ||
tickLower: (MIN_TICK / TICK_SPACING) * TICK_SPACING, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The initial liquidity added shouldn't concentrated at around the tick for initial price. It should spread out (from MIN to MAX, or some reasonable price range) so everyone can trade the token regardless of its current price
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved
contracts/TokenFactory.sol
Outdated
block.timestamp | ||
); | ||
if(winnerToken == tokenAddress) { | ||
address pool = _createLiquilityPool(tokenAddress); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should check that a pool was not created before, and revert if it was
contracts/TokenFactory.sol
Outdated
uint256 creatorBalance = token.balanceOf(creator); | ||
|
||
// burned all creator tokens | ||
token.burn(creator, creatorBalance); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Token creator's tokens shouldn't be burned and seized. Otherwise there is no incentive for them to create tokens and market that
contracts/TokenFactory.sol
Outdated
mintedAmount, | ||
block.timestamp | ||
); | ||
if(winnerToken == tokenAddress) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use a different function for "graduating to Uniswap"? Since the logic is completely different from burning losing tokens
For a new pool, the initial native token deposit (during liquidity provisioning) should be the total collateral for the competition round, which includes collateral for all tokens in that round, not just that particular token. The initial price of the pool should be the current price, as calculated based on bonding curve (I will add a new function for that). To have that price, the pool has to be deposited with some winning token at the same time. Since the pool itself has no winning token, it needs to mint some new winning tokens to be able to make that deposit. The amount of winning token minted for that purpose should be |
Correction: I shouldn't need to make a new function on bonding curve. Just query for
|
we can't do this: Error HH404: File @openzeppelin/contracts/token/ERC721/IERC721Metadata.sol, imported from @uniswap/v3-periphery/contracts/interfaces/INonfungiblePositionManager.sol, not found uniswap/v3-periphery use lower version of openzeppelin/contracts 3.4.2 |
try add a remapping in foundry.toml (and remappings.txt)? Not sure if single file remapping works now
|
|
token0: token0, | ||
token1: token1, | ||
fee: UNISWAP_FEE, | ||
tickLower: (MIN_TICK / TICK_SPACING) * TICK_SPACING, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MIN_TICK and MAX_TICK are actually not divisible by TICK_SPACING. Rounding is needed
contracts/TokenFactory.sol
Outdated
if (contributionWithoutFee > remainingEthNeeded) { | ||
contributionWithoutFee = remainingEthNeeded; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are several issues with the code below in this function.
- Let
r%
be fee percentage, andp
be the amount the user paid. Currently fee is calculated asp / (1+r) * r
where, andcontributionWithoutFee
(the collateral increment, i.e. ∆b) is calculated asp / (1+r)
. The two sums top
, since∆b + fee = p / (1+r) + p / (1+r) * r = p
. However, the fee charged is not reallyr%
. In reality it is 1 - 1 / (1+r) = r / (1+r), which is less thanr%
. - Rather than pass in
valueToBuy
, the parameter passed in should beamountPaid
(which is consistent with how it is currently used). The calculation ofcontributionWithoutFee
should then beamountPaid - fee
, wherefee
should be simply to becalculateFee(amountPaid, feePercent)
- We should remove valueToReturn if it is not used
- The public function
buy(address tokenAddress)
should also return the amount minted (so the client can accurately tell the user ahead of time via simulation-write) - There is no need to check
availableSupply
since we do not cap supply
contracts/TokenFactory.sol
Outdated
address tokenAddress, | ||
uint256 valueToBuy | ||
) public view returns (uint256) { | ||
uint256 contributionWithoutFee = (valueToBuy * FEE_DENOMINATOR) / |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same issue here, and valueToBuy
contracts/TokenFactory.sol
Outdated
WETH.deposit{value: totalCollateral}(); | ||
|
||
// calulate winner token amount | ||
uint256 contributionWithoutFee = (totalCollateral * FEE_DENOMINATOR) / |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fee was already assessed before being added into collateral. It should not be deducted again here
contracts/TokenFactory.sol
Outdated
uint256 winnerTokenAmount = bondingCurve.computeMintingAmountFromPrice( | ||
totalCollateral, | ||
Token(tokenAddress).totalSupply(), | ||
contributionWithoutFee |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What we want to know here is the per-token (1e18) unit price of the token, so this parameter should be 1.0 (1e18). After getting the per-unit price, we can then calculate the amount of tokens to be minted by totalCollateral / unitPrice (in decimal space), which would give us the number of units, i.e. unwrap(div(ud(totalCollateral), ud(unitPrice)))
contracts/TokenFactory.sol
Outdated
uint256 amount0, | ||
uint256 amount1 | ||
) = tokenAddress < address(WETH) ? | ||
_addLiquidity(tokenAddress, winnerTokenAmount, address(WETH), totalCollateral, tokensCreators[tokenAddress]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This liquidity does not belong to tokensCreators[tokenAddress]
. It belongs to the factory itself. Let's also make sure it has fee withdraw / WETH unwrap / token burning functions for admin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and token withdrawal - since the pool will earn fees in both WETH and the new token
We should have some tests for creating uniswap pools and trading on it, making sure price and liquidity are as expected |
contracts/TokenFactory.sol
Outdated
uint256 amount1 | ||
) = tokenAddress < address(WETH) ? | ||
_addLiquidity(tokenAddress, winnerTokenAmount, address(WETH), totalCollateral, tokensCreators[tokenAddress]): | ||
_addLiquidity(address(WETH), totalCollateral, tokenAddress, winnerTokenAmount, tokensCreators[tokenAddress]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also got a stack too deep compilation error here. I will do some local refactoring
…okenState; Initialize currentCompetitionId at 1. Use competitionIds for existence check
…plify parameters for _getCollateralAmountAndFee
…_manager Fix issues with liquidity calculation for publishing to uniswap, fees issue with buy and sell, and simplify contracts
Let's merge this |
…_manager Fix an issue in liquidity calculation which results in unexpected revert when collateral is less than 1.0 native asset
Some basic test for publishing to Uniswap end-to-end. More verification logic needed
No description provided.