posm: add staking through subscribers #229
Conversation
src/PositionManager.sol
Outdated
| BaseActionsRouter(_poolManager) | ||
| Notifier(_subscriberGasLimit) |
There was a problem hiding this comment.
i said this on slack but i think it got lost in that giant thread lol
Do we need a gas limit at all? If it’s to protect users from malicious subscribers, a user could just unstake before removing liquidity if they end up in that scenario right? We just need to make sure that unstake is safe always, and then we don’t need a gas limit on the subscriber imo
thoughts?
There was a problem hiding this comment.
ohhh i see youve just moved it to unsubscribe - nice. Do you think we should do this as a % of block gas limit?
There was a problem hiding this comment.
yeah sure, happy to do that. But should it still be a constructor param (the percentage?)
There was a problem hiding this comment.
oh i think it would be to remove constructor params entirely - if we dont care about the constructor having params then keep it as is!
|
|
||
| /// @dev Must always allow a user to unsubscribe. In the case of a malicious subscriber, a user can always unsubscribe safely, ensuring liquidity is always modifiable. | ||
| function _unsubscribe(uint256 tokenId, PositionConfig memory config) internal { | ||
| ISubscriber _subscriber = subscriber[tokenId]; |
There was a problem hiding this comment.
should it revert if they arent already subscribed? I believe currently if you call _unsubscribe and youre not subscribed it would succeed? Because contract calls to EOAs (like address(0)) succeed. Maybe add a test to check
There was a problem hiding this comment.
it reverts in the test I wrote with no code changes
src/base/Notifier.sol
Outdated
| abstract contract Notifier is INotifier { | ||
| using GasLimitCalculator for uint256; | ||
|
|
||
| error SubscriberCannotBeNotified(); |
There was a problem hiding this comment.
actually we dont even throw this anymore, removed
| configId = positionConfigs[tokenId] & MASK_UPPER_BIT; | ||
| } | ||
|
|
||
| function setConfigId( |
There was a problem hiding this comment.
should this be called in mint? or removed? it currently isnt called anywhere
There was a problem hiding this comment.
oh ya.. I think merge conflicts got the best of me here
| @@ -402,4 +407,35 @@ contract PositionManagerTest is Test, PosmTestSetup, LiquidityFuzzers { | |||
| assertEq(currency0.balanceOfSelf() - balance0Before, uint128(delta.amount0())); | |||
| assertEq(currency1.balanceOfSelf() - balance1Before, uint128(delta.amount1())); | |||
| } | |||
|
|
|||
| // this test fails unless subscribe is payable | |||
| function test_mutlicall_mint_subscribe_native() public { | |||
There was a problem hiding this comment.
| function test_mutlicall_mint_subscribe_native() public { | |
| function test_multicall_mint_subscribe_native() public { |
| @@ -338,4 +337,12 @@ contract PositionManager is | |||
| super.transferFrom(from, to, id); | |||
| if (positionConfigs.hasSubscriber(id)) _notifyTransfer(id, from, to); | |||
| } | |||
|
|
|||
| function getPositionConfigId(uint256 tokenId) external view returns (bytes32) { | |||
There was a problem hiding this comment.
@inheritdoc and both these 2 functions in interface with natspec
There was a problem hiding this comment.
oh theyre in with natspec - just inheritdoc!
TODO: