v4-core icon indicating copy to clipboard operation
v4-core copied to clipboard

Separate Fee logic

Open hensha256 opened this issue 1 year ago • 3 comments

Description of changes

The PoolManager was quite bloated with logic around handling the different types of fee. This PR tries to move some of that logic into a Fees contract. Given the different types of fee, we also have a FeeLibrary to handle the fee flag bits.

Not sure this is the best separation yet

hensha256 avatar Jun 15 '23 15:06 hensha256

Hi! I'm attempting to figure out this one out, I can't for the life of me get the testextsloadforpoolprice/testextsloadmultipleslots to show a different output than the tests on this draft, i.e 0/79228162514264337593543950336 etc etc. -- do you have any tips on where I could look to figure this out?

Porting everything over one by one, I was able to get _checkprotocolfee over to "FeesManager.sol" (testing 1 by 1 to see what's breaking those 2 tests in Fees.sol) and all tests passed but, once I tried _fetchprotocolfee it started showing the testextsloadforpoolprice / multipleslots error

mugrebot avatar Jun 20 '23 01:06 mugrebot

I think I might have found something, working on the second test but for now, just changed the order that manager.initialize was called to reflect patterns used before with vm.expectemit and that test ended up passing. testExtsloadMultipleSlots() is still giving issues

edit: JK if I call the values before the manager is initialized it'll be 0 so the test passes :/

mugrebot avatar Jun 21 '23 17:06 mugrebot

Thanks for your comments @mugrebot ! The issue was just that the pools mapping had moved to a new slot. You can see the storage layout of a contract using forge inspect PoolManager.sol:PoolManager storageLayout - and you'll see that pools is at slot 10, whereas it was at slot 8 before the PR :)

Commit 19a8c72 shows the fix!

hensha256 avatar Jun 23 '23 14:06 hensha256