Base Hook Middleware #144
Base Hook Middleware #144
Conversation
There was a problem hiding this comment.
I think it's worth writing a more thorough PR description in this case, given we are working with a public repo here, and many (most) people stumble upon this won't have the slightest of context of what hook middleware is and can do. It's even more worth it if the goal is to have people adopt it to write 'certified safe' hooks
test/middleware/Counter.sol
Outdated
| mapping(PoolId => uint256) public beforeDonateCount; | ||
| mapping(PoolId => uint256) public afterDonateCount; | ||
|
|
||
| bytes public lastHookData; |
There was a problem hiding this comment.
What's the use of this? And does it make sense to make it a mapping from selectors to lastHookData instead
There was a problem hiding this comment.
it's just for testing to make sure hookData is passed through correctly
| _delegate(_implementation()); | ||
| } | ||
|
|
||
| function _ensureValidFlags(address _impl) internal view virtual { |
There was a problem hiding this comment.
I wonder if the underlying implementation actually even needs to support the flags in this case given the middleware already does. What are your thoughts for requiring both addresses to be mined?
There was a problem hiding this comment.
initially, i did not require the implementation to be mined. however, when working on middleware-protect, i realized doing so would save a bit of gas on flag checks. as the dev would already have to mine a middleware, i don't think it's a big deal to enforce them to mine an implementation as well. also, it's likely that some implementations are already hooks and are already mined.
There was a problem hiding this comment.
there's a caveat in middleware-protect that requires it to read the implementation permissions
|
|
||
| contract BaseMiddleware is Proxy { | ||
| /// @notice The address of the pool manager | ||
| IPoolManager public immutable manager; |
There was a problem hiding this comment.
you're never using the manager immutable as far as I can tell - is this just to be referenced by the underlying impl? if so please elaborate in the comment
|
|
||
| error FlagsMismatch(); | ||
|
|
||
| constructor(IPoolManager _manager, address _impl) { |
There was a problem hiding this comment.
what is the purpose of this BaseMiddleware? It seems to be mostly just a proxy wrapper. Are there any useful things that we can add for downstream implementers? Some ideas -
- Standard modifiers i.e. onlyPoolManager at least?
- Standard implementations of some hook functions, with virtual functions that downstream implementers can override with their custom checks?
There was a problem hiding this comment.
it's pretty much just oz proxy yeah.
- i think it's up to the implementation to add the onlyPoolManager modifier. most hooks already use it, so adding onlyPoolManager to the middleware itself would result in a redundant check.
- like a beforeBeforeSwap, afterBeforeSwap pattern? this sounds a bit complicated and also creates deployment size bloat for middlewares that don't implement the hook.
There was a problem hiding this comment.
Yeah onlyPoolManager is already defined in contracts we've written upstream like SafeCallback
| abstract contract BaseMiddleware is Proxy { | ||
| /// @notice The address of the pool manager | ||
| /// @dev Use in middleware implementations to access the pool manager | ||
| IPoolManager public immutable manager; |
There was a problem hiding this comment.
If you do need the manager var, there will soon be merged an ImmutableState contract you can use to initialize maanger
|
|
||
| error FlagsMismatch(); | ||
|
|
||
| constructor(IPoolManager _manager, address _impl) { |
There was a problem hiding this comment.
Yeah onlyPoolManager is already defined in contracts we've written upstream like SafeCallback
| _delegate(_implementation()); | ||
| } | ||
|
|
||
| function _ensureValidFlags(address _impl) internal view virtual { |
Related Issue
Hooks can be made to do bad things. Because anyone can create their own arbitrary logic for a hook contract, it's difficult for third parties to decide which hooks are "safe" and which are "dangerous". We propose a hook middleware that performs sanity checks on the result of a hooks to block malicious actions.
Description of changes
Middleware factory creates middlewares.
hook.factory.mp4
Each middleware is the hook and points to another hook as the implementation. This allows for some convenient configurations where a pool can use a hook and another pool can use the middlewared hook.
all are valid configurations
Best of all, attaching a middleware to a hook is easy and usually requires no extra coding. The main caveat1 is (because of the proxy pattern) constructors will never be called, so it may be necessary to revise the implementation contract to use an initialize function if the constructor needs to set non-immutable variables.
For now, this branch implements and tests only base contracts, which do nothing. middleware-remove and middleware-protect extend these base contracts to include specific protections. Of course, these are only proposed solutions, and anyone can create their own middleware factory design as they see fit.
Footnotes
there are some more small caveats. let's say hook A calls a permissioned function on external contract E. a middleware pointing to hook A would then not be able to call contract E. ↩