Fee accrual as 1155 balance

[kevin-fruitful]

Related Issue

Referencing issue #203

Description of changes

Removed the 2 mappings protocolFeesAccrued and hookFeesAccrued.
swap and modifyPosition now mint ERC1155 currencies to the protocolFeeController and a pool's hook contract.

collectProtocolFees and collectHookFees now burn ERC1155 currencies.

Respective tests have been updated. Implemented onERC1155Received and onERC1155BatchReceived for ProtocolFeeControllerTest and MockHooks test contracts.

Additional thoughts

Change _balances from private to internal such that the variable can be read directly instead of going through balanceOf.

[hiroshitashir]

I'm not an official reviewer so I could be wrong. I just want someone more knowledgeable to take a look.

I wonder if we want to call _burn when msg.sender == address(protocolFeeController) since it's calling currency.transfer below.

[kevin-fruitful]

Thanks for the review!

In this singleton design, all ERC20s are held by the Pool Manager contract. Individual pool balances of ERC20s (aka currencies) are being accounted for within this Pool Manager contract.

This internal call burn() is decreasing the ERC1155 balance (or "internal" balance) of the protocol fee controller's fee token (some ERC20). Then, currency.transfer is transferring (ERC20.transfer()) the fee token rom the pool manager to the (external) protocol fee controller.

Let me know if that makes sense or if I'm misunderstanding something! Thanks!

[hiroshitashir]

Same here.

[NoahZinsmeister]

thanks for the PR @kevin-fruitful (and @hiroshitash for the review)!

i'm inclined to leave this on ice for a while, not because it's bad, but because a) the state of 1155 tokens in the pool manager is a bit up in the air (see #294), and b) using 1155 balances would probably complicate designs where custom hooks are custodying funds as 1155 balances

keeping the PR open as a reference though, and it might make sense to merge depending on where we land with 1155!

[hensha256]

Hi! Going to close this PR now for a few reasons

1. we have removed hook fees accruing in the contract and instead they are achieved in other methods (Remove hook fees and protocol fee on withdrawal #432)
2. we now use 6909s instead of 1155s (6909 fixed #446)
3. we tried to accrue protocol fees as 6909s (Accumulate protocol fees as 6909s  #449) but closed this as it made swap costs increase dramatically due to the event emitted when minting a 6909

Thanks again for your submission