CurrencySettler -> DeltaResolver #186
Conversation
.forge-snapshots/PositionManager_multicall_initialize_mint.snap
Outdated
Show resolved
Hide resolved
| /// @param token The token to settle. This is known not to be the native currency | ||
| /// @param payer The address who should pay tokens | ||
| /// @param amount The number of tokens to send | ||
| function _pay(Currency token, address payer, uint256 amount) internal virtual; |
There was a problem hiding this comment.
lol i wish we could call this send ...
There was a problem hiding this comment.
note that this contract must be approved/permitted to pay on behalf of the payer?
There was a problem hiding this comment.
note that this contract must be approved/permitted to pay on behalf of the payer?
not necessarily true though... it could be that this function chooses to pull tokens out of compound to pay the debt, or any other random way they want to pay.
|
|
||
| /// @notice Abstract contract used to sync, send, and settle funds to the pool manager | ||
| /// @dev Note that sync() is called before any erc-20 transfer in `settle`. | ||
| abstract contract DeltaResolver is ImmutableState { |
There was a problem hiding this comment.
hm the inheriting structure in the base PositionManager contract now inherits two contracts(SafeCallback and this one) that both also inherit ImmutableState? Any way we can avoid that?
There was a problem hiding this comment.
maybe we need to expose a virtual function poolManager() that is overridden in the main base contract ?
There was a problem hiding this comment.
I personally think its fine that they both inherit it. The way inheritance works in solidity means it only actually gets inherited once, and it allows us to keep all the contracts super modular and not worry about each other
| // implementation of abstract function DeltaResolver._pay | ||
| function _pay(Currency token, address payer, uint256 amount) internal override { | ||
| // TODO this should use Permit2 | ||
| ERC20(Currency.unwrap(token)).safeTransferFrom(payer, address(poolManager), amount); |
There was a problem hiding this comment.
wondering if we should also support transfer not just transferFrom?
There was a problem hiding this comment.
oh this was just a draft function to get posm to functional-parity with where it was before this PR. I'm assuming youll redo the _pay implementation to use Permit2.
As for whether posm needs transfer (universal router does). That depends if posm will ever custody funds that need to go into another position? I dont think so because you just leave them in pool manager and the deltas handle it all. So I would imagine posm only needs transferFrom unless you have a counter example?
There was a problem hiding this comment.
we may want to consider supporting transfer? but youre right we dont need it in this PR
There was a problem hiding this comment.
Nice! Looks great
My only final comment is whether we should update core at all with these patterns? IK we have CurrencySettler in core which is mostly just used for testing purposes but I think it was nice that core/periphery had the same "infra" - I believe its why merging core into periphery became easier
|
yeah thats fair. I could make an update PR to core's currency settler to look like this too |
falseortrueas the final parameter in meaning that really we should just be writing more efficient functions.