-
Notifications
You must be signed in to change notification settings - Fork 946
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
Conversation
// SPDX-License-Identifier: GPL-2.0-or-later | ||
pragma solidity ^0.8.0; | ||
|
||
/// @title For calculating a percentage of an amount, using bips |
There was a problem hiding this comment.
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.
Forge code coverage:
|
constructor(uint256 _controllerGasLimit) Owned(msg.sender) { | ||
controllerGasLimit = _controllerGasLimit; | ||
} | ||
constructor() Owned(msg.sender) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*happy noises*
|
||
/// @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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me; 300K also probably chill (running it by erin too)
is there a test that confirms the 1% is the indeed the gas limit. just to validate that the 1% math is working, and its actually being picked up correctly by ProtocolFees.sol
I wrote tests for this in periphery - maybe let's transfer them over @hensha256 ? |
add tests
No description provided.