posm : use actions router, remove return vals #214
Conversation
snreynolds
changed the title
| if (action == Actions.INCREASE_LIQUIDITY) { | ||
| _increase(params); | ||
| } else if (action == Actions.DECREASE_LIQUIDITY) { | ||
| _decrease(params); | ||
| } else if (action == Actions.MINT_POSITION) { | ||
| _mint(params); | ||
| } else if (action == Actions.CLOSE_CURRENCY) { | ||
| _close(params); | ||
| } else if (action == Actions.BURN_POSITION) { | ||
| // Will automatically decrease liquidity to 0 if the position is not already empty. | ||
| _burn(params); |
There was a problem hiding this comment.
dont need to change it now, and its more a discussion bit --
I think decoding the parameters before making the internal calls makes the internal function signatures much more readable (and easier to reference)
There was a problem hiding this comment.
I think this makes gas go down quite a bit though bc we're just passing calldata
src/PositionManager.sol
Outdated
| } | ||
|
|
||
| function _modifyLiquidity(PositionConfig memory config, int256 liquidityChange, bytes32 salt, bytes memory hookData) | ||
| internal | ||
| returns (BalanceDelta liquidityDelta) |
There was a problem hiding this comment.
we still need to return liquidityDelta because of slippage checks inside the internal functions
unless slippage should be on the CLOSE command 🤔
test/shared/LiquidityOperations.sol
Outdated
| function snapLastDelta() internal view returns (BalanceDelta delta) { | ||
| delta = hook.deltas(hook.getDeltasLength() - 1); // just want the most recetly written to delta | ||
| } |
There was a problem hiding this comment.
nit: imo a better name is getLastDelta
There was a problem hiding this comment.
also I think this (and HookSavesDelta hook;) belongs in PosmTestSetup.sol
There was a problem hiding this comment.
yeah agreed snap we use for gas snapshots - definitely prefer getLastDelta
There was a problem hiding this comment.
ah yes good catch, originally had it in LiquidityOperations cause I was referencing it but will change
test/shared/LiquidityOperations.sol
Outdated
| function snapLastDelta() internal view returns (BalanceDelta delta) { | ||
| delta = hook.deltas(hook.getDeltasLength() - 1); // just want the most recetly written to delta | ||
| } |
There was a problem hiding this comment.
yeah agreed snap we use for gas snapshots - definitely prefer getLastDelta
test/shared/Planner.sol
Outdated
| plan.actions = actions; | ||
| plan.params = params; | ||
|
|
||
| return plan; | ||
| } | ||
|
|
||
| function finalize(Plan memory plan, PoolKey memory poolKey) internal pure returns (bytes memory) { |
There was a problem hiding this comment.
imo this should now be called finalizeModifyLiquidity or something? To differentiate between what finalizeSwap and finalize are
| if (positionConfigs[tokenId] != config.toId()) revert IncorrectPositionConfigForTokenId(tokenId); | ||
|
|
||
| // Note: the tokenId is used as the salt. | ||
| BalanceDelta delta = _modifyLiquidity(config, -(liquidity.toInt256()), bytes32(tokenId), hookData); | ||
| return abi.encode(delta); | ||
| BalanceDelta liquidityDelta = _modifyLiquidity(config, -(liquidity.toInt256()), bytes32(tokenId), hookData); |
There was a problem hiding this comment.
this liquidityDelta is unused in all these functions? whats it for?
There was a problem hiding this comment.
its for slippage checks - sauce is working on that
src/PositionManager.sol
Outdated
| @@ -29,7 +31,8 @@ contract PositionManager is | |||
| Multicall, | |||
| SafeCallback, | |||
There was a problem hiding this comment.
can remove SafeCallback from this list now - thats inherited by BaseActionsRouter
Related Issue
Which issue does this pull request resolve?
Description of changes