feat: PositionManager mint #80
Conversation
tinaszheng
changed the title
| function lockAcquired(bytes calldata rawData) external returns (bytes memory) { | ||
| // TODO: implement this | ||
| require(msg.sender == address(poolManager)); |
There was a problem hiding this comment.
use custom err here too?
| @@ -133,7 +174,47 @@ contract NonfungiblePositionManagerV4 is | |||
| checkDeadline(params.deadline) | |||
| returns (uint256 tokenId, uint128 liquidity, uint256 amount0, uint256 amount1) | |||
| { | |||
| // TODO: implement this | |||
| (tokenId, liquidity, amount0, amount1) = abi.decode( | |||
There was a problem hiding this comment.
is it better if i split this out into two lines?
bytes result = poolManager.lock(abi.encode(CallbackData(CallbackAction.Mint, msg.sender, abi.encode(params))));
(tokenId, liquidity, amount0, amount1) = abi.decode(result, (uint256, uint128, uint256, uint256));
f8b7875
to
0156721
Compare
| function cachePoolKey(PoolKey memory poolKey) private returns (PoolId poolId) { | ||
| poolId = poolKey.toId(); | ||
| // uninitialized check | ||
| if (_poolIdToPoolKey[PoolId.unwrap(poolId)].tickSpacing == 0) { |
There was a problem hiding this comment.
is checking for tickSpacing the right thing to do here? just want to check if this key exists in the mapping yet
There was a problem hiding this comment.
also big difference between this mapping vs the one that exists in v3:
- in v3, there's a poolId indicator PER nft so during burn, we deleted the poolId from the mapping
- but in v4, this is a poolId PER poolkey and there could be multiple NFTs per poolKey. we don't want to burn during mint, so this is essentially an ever-growing list. but it also means not every
mintneeds to store an extra poolId mapping
There was a problem hiding this comment.
yea i think its a fair chec as tickSpacing can't be 0 in an initialized pool
tinaszheng
force-pushed
the
tina/position-manager-mint
branch
from
0156721
to
82c27a8
Compare
| @@ -120,7 +130,37 @@ contract NonfungiblePositionManagerV4 is | |||
| checkDeadline(params.deadline) | |||
| returns (uint256 tokenId, uint128 liquidity, uint256 amount0, uint256 amount1) | |||
| { | |||
| // TODO: implement this | |||
| (liquidity, amount0, amount1) = addLiquidity( | |||
| AddLiquidityParams({ | |||
There was a problem hiding this comment.
this re-defining of AddLiquidityParams is kinda ugly... i think i'd prefer either:
- We could have AddLiqParams as a sub-struct of MintParams like
struct MintParams {
AddLiquidityParams addLiqParams
address recipient
uint256 deadline
}
orrrr
2. We could not even bother with the MintParams entirely and just have
function mint(AddLiquidityParams calldata params, address recipient, uint256 deadline)
There was a problem hiding this comment.
then these 12 lines could just be like (liquidity, amount0, amount1) = addLiquidity(params); 😏
| }) | ||
| ); | ||
|
|
||
| tokenId = _nextId++; |
| function cachePoolKey(PoolKey memory poolKey) private returns (PoolId poolId) { | ||
| poolId = poolKey.toId(); | ||
| // uninitialized check | ||
| if (_poolIdToPoolKey[PoolId.unwrap(poolId)].tickSpacing == 0) { |
There was a problem hiding this comment.
yea i think its a fair chec as tickSpacing can't be 0 in an initialized pool
| @@ -120,7 +130,37 @@ contract NonfungiblePositionManagerV4 is | |||
| checkDeadline(params.deadline) | |||
| returns (uint256 tokenId, uint128 liquidity, uint256 amount0, uint256 amount1) | |||
| { | |||
| // TODO: implement this | |||
| (liquidity, amount0, amount1) = addLiquidity( | |||
| AddLiquidityParams({ | |||
|
Closing in favor of #141 |
No description provided.