Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests for fee controller gas limit #728

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

tests for fee controller gas limit #728

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0de069d
tests for fee controller gas limit
dianakocsis May 29, 2024
b71de5a
merge main
dianakocsis May 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .forge-snapshots/set protocol fee.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
31702
31746
2 changes: 1 addition & 1 deletion .forge-snapshots/swap skips hook call if hook is caller.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
208496
208508
2 changes: 1 addition & 1 deletion .forge-snapshots/swap with dynamic fee.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
140527
140539
2 changes: 1 addition & 1 deletion .forge-snapshots/swap with lp fee and protocol fee.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
170330
170342
2 changes: 1 addition & 1 deletion .forge-snapshots/swap with return dynamic fee.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
146758
146770
2 changes: 1 addition & 1 deletion .forge-snapshots/update dynamic fee in before swap.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
149016
149028
8 changes: 8 additions & 0 deletions src/test/ProtocolFeeControllerTest.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,3 +59,11 @@ contract InvalidReturnSizeProtocolFeeControllerTest is IProtocolFeeController {
}
}
}

contract GasLimitProtocolFeeControllerTest is IProtocolFeeController {
function protocolFeeForPool(PoolKey memory /* key */ ) external pure returns (uint24) {
// consume gas
while (true) {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol beautiful

return 1000;
}
}
25 changes: 25 additions & 0 deletions src/test/ProtocolFeesImplementation.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,29 @@ contract ProtocolFeesImplementation is ProtocolFees {
function updateProtocolFees(Currency currency, uint256 amount) public {
ProtocolFees._updateProtocolFees(currency, amount);
}

function consumeGasLimitAndFetchFee(PoolKey memory key) public {
// consume gas before calling fetchProtocolFee / getting the protocolFeeForPool from the controller
while (true) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like these functions are the same -- could just write one and have that value as an input i think

// once gas left is less than the limit, stop consuming gas
if (gasleft() < 9079256829993496519) {
break;
}
}
// fetch the protocol fee after consuming gas
// will revert since the gas left is less than the limit
fetchProtocolFee(key);
}

function consumeGasAndFetchFee(PoolKey memory key) public {
while (true) {
// consume just under the gas limit
if (gasleft() < 9079256829993490000) {
break;
}
}
// try to fetch the protocol fee
// will revert while fetching since the gas limit has been reached
fetchProtocolFee(key);
}
}
29 changes: 28 additions & 1 deletion test/ProtocolFeesImplementation.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ import {
OutOfBoundsProtocolFeeControllerTest,
RevertingProtocolFeeControllerTest,
OverflowProtocolFeeControllerTest,
InvalidReturnSizeProtocolFeeControllerTest
InvalidReturnSizeProtocolFeeControllerTest,
GasLimitProtocolFeeControllerTest
} from "../src/test/ProtocolFeeControllerTest.sol";

contract ProtocolFeesTest is Test, GasSnapshot, Deployers {
Expand Down Expand Up @@ -218,4 +219,30 @@ contract ProtocolFeesTest is Test, GasSnapshot, Deployers {
assertFalse(success);
assertEq(protocolFee, 0);
}

function test_fetchProtocolFee_gasLimit() public {
gasLimitFeeController = new GasLimitProtocolFeeControllerTest();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would love a clearer name - maybe ConsumesGasLimit...?

protocolFees.setProtocolFeeController(gasLimitFeeController);
vm.prank(address(gasLimitFeeController));
(bool success, uint24 protocolFee) = protocolFees.fetchProtocolFee(key);
assertFalse(success);
assertEq(protocolFee, 0);
}

function test_fetchProtocolFee_revertsWithProtocolFeeCannotBeFetched() public {
protocolFees = new ProtocolFeesImplementation(9079256829993496519);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@havi-kim havi-kim Jun 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

protocolFees.setProtocolFeeController(feeController);
vm.prank(address(feeController));
vm.expectRevert(IProtocolFees.ProtocolFeeCannotBeFetched.selector);
// consumes too much gas before fetching the fee
protocolFees.consumeGasLimitAndFetchFee(key);
}

function test_fetchProtocolFee_revertsWithGasLimitExceeded() public {
protocolFees = new ProtocolFeesImplementation(9079256829993496519);
protocolFees.setProtocolFeeController(feeController);
vm.prank(address(feeController));
vm.expectRevert();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please can you fill in what the revert message should be for this case

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yeah i messed up the numbers

protocolFees.consumeGasAndFetchFee(key);
}
}
5 changes: 4 additions & 1 deletion test/utils/Deployers.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ import {
OutOfBoundsProtocolFeeControllerTest,
RevertingProtocolFeeControllerTest,
OverflowProtocolFeeControllerTest,
InvalidReturnSizeProtocolFeeControllerTest
InvalidReturnSizeProtocolFeeControllerTest,
GasLimitProtocolFeeControllerTest
} from "../../src/test/ProtocolFeeControllerTest.sol";

contract Deployers {
Expand Down Expand Up @@ -72,6 +73,7 @@ contract Deployers {
OutOfBoundsProtocolFeeControllerTest outOfBoundsFeeController;
OverflowProtocolFeeControllerTest overflowFeeController;
InvalidReturnSizeProtocolFeeControllerTest invalidReturnSizeFeeController;
GasLimitProtocolFeeControllerTest gasLimitFeeController;

PoolKey key;
PoolKey nativeKey;
Expand Down Expand Up @@ -111,6 +113,7 @@ contract Deployers {
outOfBoundsFeeController = new OutOfBoundsProtocolFeeControllerTest();
overflowFeeController = new OverflowProtocolFeeControllerTest();
invalidReturnSizeFeeController = new InvalidReturnSizeProtocolFeeControllerTest();
gasLimitFeeController = new GasLimitProtocolFeeControllerTest();

manager.setProtocolFeeController(feeController);
}
Expand Down
Loading