Create tests to BaseHooks contract

[joaobrunoah]

Description

Create tests to cover 100% the BaseHooks.sol contract.

Type of change

• [ ] Bug fix
• [ ] New feature
• [ ] Breaking change
• [ ] Dependency changes
• [x] Code refactor / cleanup
• [ ] Optimization: [ ] gas / [ ] bytecode
• [ ] Documentation or wording changes
• [ ] Other

Checklist:

• [x] The diff is legible and has no extraneous changes
• [x] Complex code has been commented, including external interfaces
• [x] Tests have 100% code coverage
• [x] The base branch is either main, or there's a description of how to merge

Issue Resolution

Closes #712

[joaobrunoah]

Started reviewing... and unless I'm missing something, I don't think we need this at all. Concrete hooks contracts are never going to call the base functions (that return false); we need to make sure the derived contracts enforce the hook constraints themselves (see #772).

Since they're not called, putting modifiers on the abstract contract functions doesn't really do anything. Making this concrete and testing that it has the right modifiers is really just testing a kind of mock. Presumably it's just for coverage, but we don't need coverage on abstract contracts whose functions are never called.

@EndymionJkb , I've put some thought on that before implementing the tests. BaseHooks will be used by most, if not all, hooks. It'll be part of hooks codes, so it's not just a mock. I imagine that an external developer may configure the hook flags wrongly and trigger a function that he didn't implement. In this case, we must ensure the function reverts by returning success = false.

About onlyVault modifiers, I agree that BaseHooks don't need that. But why would any hook require that? I mean, what's the risk of calling a hook outside of the vault? We won't be able to ensure that all developed hooks implement onlyVault modifier in their functions, so that's why I'm asking.

[EndymionJkb]

BaseHooks will be used by most, if not all, hooks. It'll be part of hooks codes, so it's not just a mock. I imagine that an >external developer may configure the hook flags wrongly and trigger a function that he didn't implement. In this case, we >must ensure the function reverts by returning success = false.

I didn't explain what I meant well enough there. BaseHooks will be inherited, but its functions will never be called by derived contracts (in fact can't be in the current version, since they're external).

You can make it concrete and test it like you're doing here, but (to me at least) it seems more natural to leave it abstract, explicitly make a mock, and have the mock actually call the BaseHooks functions. That way you're really directly testing what would happen if a misconfigured hook contract didn't override a hook, while keeping BaseHooks abstract, as it should be. (The only downside is you have to change the visibility to be able to do this, though it would enable derived hooks contracts to "fall through" in error conditions and invoke the parent, as a way to revert.)

See #774 for what I mean.

About onlyVault modifiers, I agree that BaseHooks don't need that. But why would any hook require that? I mean, what's >the risk of calling a hook outside of the vault? We won't be able to ensure that all developed hooks implement onlyVault >modifier...

Well, the reason hooks are in the Vault is to guarantee they will be called in the correct order with correct arguments. Hooks are supposed to be invoked during transactions at particular points, so any functions that are part of transactions should be onlyVault - or reentrancy could be used to call things out of order or otherwise change the hook contract state. Hooks can define other functions that don't need to be called by the Vault (e.g., getHooksFlags from IHooks, or their own custom functions).

Since real derived contracts would never call the BaseHooks functions (at least without reverting), there is no way to test onlyVault modifiers except in the most derived contracts (e.g., the hook examples).

[joaobrunoah]

Superseded by #774