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.

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

gas limit as percentage #861

Merged
merged 5 commits into from
Sep 4, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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/initialize.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
59963
60018
2 changes: 1 addition & 1 deletion .forge-snapshots/poolManager bytecode size.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
23925
23914
2 changes: 0 additions & 2 deletions src/PoolManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,6 @@ contract PoolManager is IPoolManager, ProtocolFees, NoDelegateCall, ERC6909Claim

mapping(PoolId id => Pool.State) internal _pools;

constructor(uint256 controllerGasLimit) ProtocolFees(controllerGasLimit) {}

/// @notice This will revert if the contract is locked
modifier onlyWhenUnlocked() {
if (!Lock.isUnlocked()) ManagerLocked.selector.revertWith();
Expand Down
15 changes: 9 additions & 6 deletions src/ProtocolFees.sol
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {IProtocolFeeController} from "./interfaces/IProtocolFeeController.sol";
import {IProtocolFees} from "./interfaces/IProtocolFees.sol";
import {PoolKey} from "./types/PoolKey.sol";
import {ProtocolFeeLibrary} from "./libraries/ProtocolFeeLibrary.sol";
import {BipsLibrary} from "./libraries/BipsLibrary.sol";
import {Owned} from "solmate/src/auth/Owned.sol";
import {PoolId, PoolIdLibrary} from "./types/PoolId.sol";
import {Pool} from "./libraries/Pool.sol";
Expand All @@ -17,18 +18,19 @@ abstract contract ProtocolFees is IProtocolFees, Owned {
using PoolIdLibrary for PoolKey;
using Pool for Pool.State;
using CustomRevert for bytes4;
using BipsLibrary for uint256;

/// @inheritdoc IProtocolFees
mapping(Currency currency => uint256 amount) public protocolFeesAccrued;

/// @inheritdoc IProtocolFees
IProtocolFeeController public protocolFeeController;

uint256 private immutable controllerGasLimit;
// a percentage of the block.gaslimit denoted in basis points, used as the gas limit for fee controller calls
// 100 bps is 1%, at 30M gas, the limit is 300K
Copy link
Member

Choose a reason for hiding this comment

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

Are we fine with this number?

This is what we use for subscribers on periphery?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah it seemed reasonable here too. 300k would be a lotttttttt. arguably id be happy to go with smaller lol

uint256 private constant BLOCK_LIMIT_BPS = 100;

constructor(uint256 _controllerGasLimit) Owned(msg.sender) {
controllerGasLimit = _controllerGasLimit;
}
constructor() Owned(msg.sender) {}
Comment on lines -28 to +32
Copy link
Collaborator

Choose a reason for hiding this comment

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

*happy noises*


/// @inheritdoc IProtocolFees
function setProtocolFeeController(IProtocolFeeController controller) external onlyOwner {
Expand Down Expand Up @@ -67,12 +69,13 @@ abstract contract ProtocolFees is IProtocolFees, Owned {
/// the success of this function is NOT checked on initialize and if the call fails, the protocol fees are set to 0.
function _fetchProtocolFee(PoolKey memory key) internal returns (uint24 protocolFee) {
if (address(protocolFeeController) != address(0)) {
uint256 controllerGasLimit = block.gaslimit.calculatePortion(BLOCK_LIMIT_BPS);

// note that EIP-150 mandates that calls requesting more than 63/64ths of remaining gas
// will be allotted no more than this amount, so controllerGasLimit must be set with this
// in mind.
if (gasleft() < controllerGasLimit) ProtocolFeeCannotBeFetched.selector.revertWith();

uint256 gasLimit = controllerGasLimit;
address toAddress = address(protocolFeeController);

bytes memory data = abi.encodeCall(IProtocolFeeController.protocolFeeForPool, (key));
Expand All @@ -81,7 +84,7 @@ abstract contract ProtocolFees is IProtocolFees, Owned {
uint256 returnData;
assembly ("memory-safe") {
// only load the first 32 bytes of the return data to prevent gas griefing
success := call(gasLimit, toAddress, 0, add(data, 0x20), mload(data), 0, 32)
success := call(controllerGasLimit, toAddress, 0, add(data, 0x20), mload(data), 0, 32)
// if success is false this wont actually be returned, instead 0 will be returned
returnData := mload(0)

Expand Down
17 changes: 17 additions & 0 deletions src/libraries/BipsLibrary.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// SPDX-License-Identifier: GPL-2.0-or-later
pragma solidity ^0.8.0;

/// @title For calculating a percentage of an amount, using bips
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is copied across from periphery now that its been audited.

library BipsLibrary {
uint256 internal constant BPS_DENOMINATOR = 10_000;

/// @notice emitted when an invalid percentage is provided
error InvalidBips();

/// @param amount The total amount to calculate a percentage of
/// @param bips The percentage to calculate, in bips
function calculatePortion(uint256 amount, uint256 bips) internal pure returns (uint256) {
if (bips > BPS_DENOMINATOR) revert InvalidBips();
return (amount * bips) / BPS_DENOMINATOR;
}
}
2 changes: 0 additions & 2 deletions src/test/ProtocolFeesImplementation.sol
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@ contract ProtocolFeesImplementation is ProtocolFees {

mapping(PoolId id => Pool.State) internal _pools;

constructor(uint256 _controllerGasLimit) ProtocolFees(_controllerGasLimit) {}

// Used to set the price of a pool to pretend that the pool has been initialized in order to successfully set a protocol fee
function setPrice(PoolKey memory key, uint160 sqrtPriceX96) public {
Pool.State storage pool = _getPool(key.toId());
Expand Down
2 changes: 1 addition & 1 deletion src/test/ProxyPoolManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ contract ProxyPoolManager is IPoolManager, ProtocolFees, NoDelegateCall, ERC6909

address internal immutable _delegateManager;

constructor(address delegateManager, uint256 controllerGasLimit) ProtocolFees(controllerGasLimit) {
constructor(address delegateManager) {
_delegateManager = delegateManager;
}

Expand Down
4 changes: 2 additions & 2 deletions test/NoDelegateCall.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ import {Deployers} from "./utils/Deployers.sol";
contract TestDelegateCall is Test, Deployers, GasSnapshot {
// override to use ProxyPoolManager
function deployFreshManager() internal virtual override {
IPoolManager delegateManager = new PoolManager(500000);
manager = new ProxyPoolManager(address(delegateManager), 500000);
IPoolManager delegateManager = new PoolManager();
manager = new ProxyPoolManager(address(delegateManager));
}

NoDelegateCallTest noDelegateCallTest;
Expand Down
2 changes: 1 addition & 1 deletion test/ProtocolFeesImplementation.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ contract ProtocolFeesTest is Test, GasSnapshot, Deployers {
ProtocolFeesImplementation protocolFees;

function setUp() public {
protocolFees = new ProtocolFeesImplementation(5000);
protocolFees = new ProtocolFeesImplementation();
feeController = new ProtocolFeeControllerTest();
(currency0, currency1) = deployAndMint2Currencies();
MockERC20(Currency.unwrap(currency0)).transfer(address(protocolFees), 2 ** 255);
Expand Down
2 changes: 1 addition & 1 deletion test/utils/Deployers.sol
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ contract Deployers {
}

function deployFreshManager() internal virtual {
manager = new PoolManager(500000);
manager = new PoolManager();
}

function deployFreshManagerAndRouters() internal {
Expand Down
Loading