tests for fee controller gas limit #728
Conversation
|
Forge code coverage:
|
| contract GasLimitProtocolFeeControllerTest is IProtocolFeeController { | ||
| function protocolFeeForPool(PoolKey memory /* key */ ) external pure returns (uint24) { | ||
| // consume gas | ||
| while (true) {} |
| } | ||
|
|
||
| function test_fetchProtocolFee_revertsWithProtocolFeeCannotBeFetched() public { | ||
| protocolFees = new ProtocolFeesImplementation(9079256829993496519); |
There was a problem hiding this comment.
Would be great to have this as a constant or something
this file can have a GAS_LIMIT and then set it as an immutable in the implementation contract. And then that contract can just access it too?
There was a problem hiding this comment.
Hello, I'm not part of your team, but I have a question. Doesn't this method increase the testing time?
function test_fetchProtocolFee_revertsWithProtocolFeeCannotBeFetched() public {
uint256 gasLimit = 5000;
protocolFees = new ProtocolFeesImplementation(gasLimit);
protocolFees.setProtocolFeeController(feeController);
vm.prank(address(feeController));
vm.expectRevert(IProtocolFees.ProtocolFeeCannotBeFetched.selector);
protocolFees.fetchProtocolFee{gas: gasLimit - 1}(key);
}
I'm suggesting this for testing time.
Thank you for considering my suggestion.
| @@ -218,4 +219,30 @@ contract ProtocolFeesTest is Test, GasSnapshot, Deployers { | |||
| assertFalse(success); | |||
| assertEq(protocolFee, 0); | |||
| } | |||
|
|
|||
| function test_fetchProtocolFee_gasLimit() public { | |||
| gasLimitFeeController = new GasLimitProtocolFeeControllerTest(); | |||
There was a problem hiding this comment.
would love a clearer name - maybe ConsumesGasLimit...?
| protocolFees = new ProtocolFeesImplementation(9079256829993496519); | ||
| protocolFees.setProtocolFeeController(feeController); | ||
| vm.prank(address(feeController)); | ||
| vm.expectRevert(); |
There was a problem hiding this comment.
please can you fill in what the revert message should be for this case
There was a problem hiding this comment.
I cant see why this one will be any different to the previous one? surely it also reverts with ProtocolFeeCannotBeFetched because its consuming even more gas and the amount left is even lower?
There was a problem hiding this comment.
would agree here - not sure what the difference is if there is one besides just consuming a different amount of gas before calling fetchProtocolFee
You could fuzz that amount even
There was a problem hiding this comment.
oh yeah i messed up the numbers
|
|
||
| function consumeGasLimitAndFetchFee(PoolKey memory key) public { | ||
| // consume gas before calling fetchProtocolFee / getting the protocolFeeForPool from the controller | ||
| while (true) { |
There was a problem hiding this comment.
Seems like these functions are the same -- could just write one and have that value as an input i think
| protocolFees = new ProtocolFeesImplementation(9079256829993496519); | ||
| protocolFees.setProtocolFeeController(feeController); | ||
| vm.prank(address(feeController)); | ||
| vm.expectRevert(); |
There was a problem hiding this comment.
would agree here - not sure what the difference is if there is one besides just consuming a different amount of gas before calling fetchProtocolFee
You could fuzz that amount even
Related Issue
Which issue does this pull request resolve?
Description of changes