-
Notifications
You must be signed in to change notification settings - Fork 120
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
Isthmus: operator fee #382
Isthmus: operator fee #382
Conversation
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.
Also curious to hear from @tynes and @roberto-bayardo, who's implemented changes to the fee function in Fjord.
I propose to use a prefix for this feature that conveys more meaning, like OperatorFee
or FixedFee
.
calculation: the `ConfigurableFee`, which is parameterized by two scalars: the `configurableFeeScalar` | ||
and the `configurableFeeConstant`. |
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.
I find the use of the prefix "configurable" a bit meaningless for this feature. Other fee parameters, like the (blob)BaseFeeScalar
s are also "configurable". Maybe we use a prefix that better describes the reason for their introduction, like
OperatorFee
operatorFeeScalar
operatorFeeConstant
or something similar that attaches more meaning to them?fixedFee...
could also work.
Blocks after the Isthmus activation block contain all pre-Isthmus values 1:1, | ||
and also set the following new attributes: | ||
|
||
- The `configurableFeeScalar` is set to `0`. |
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.
Don't we want to set it to 1
? Otherwise there's no fees any more.
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.
The configurableFeeScalar is only scaled by the gas used -- it doesn't scale any of the existing fees. The goal is to add a separate component to the fee calculation, like base fee and priority fee.
|
||
The configurable fee is set as follows: | ||
|
||
`configurableFee = gas_used * configurableFeeScalar + configurableFeeConstant` |
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.
So we don't need any fractional scaling, like we introduced with Fjord for the model parameters? I mean something like
`configurableFee = gas_used * configurableFeeScalar + configurableFeeConstant` | |
`configurableFee = (gas_used * configurableFeeScalar + configurableFeeConstant) / 1e6` |
to allow for a decimal precision of 6.
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.
Good point -- it makes sense for users to be able to have fractional scalars. However, I don't know why a user would want to have a fractional constant. The only reason I can think would be to save bits -- see my other comment.
| configurableFeeScalar | uint64 | 180-187 | | | ||
| configurableFeeConstant | uint64 | 188-195 | | |
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.
Do we really want or need 64 bits instead of 32 bits size for the new parameters? E.g. the (blob)baseFeeScalar
s also worked with 32 bits (and also a decimal scaling factor, see other comment).
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.
Thanks for the feedback! I agree with your point about renaming to operatorFee
and allowing for 6 decimal points of precision, but I was a little unsure about reducing the bit width of the operatorFeeConstant
and operatorFeeScalar
.
I think it should be fine to decrease the Scalar
to 32 bits, but I'm concerned that 32 bits won't be enough to represent the constant factor. For example, in this transaction https://optimistic.etherscan.io/tx/0xa6dfc18c35bf39fa60823e9280bde18496e27e9016040f7ad9ded6797c374f05, the total transaction fee in wei requires 43 bits to represent.
If we scale the constant term by a fixed factor we could fit it in 32 bits. But I don't know how much control a user might want over this constant.
```function | ||
function getOperatorFee(uint256 gasUsed)(uint256) | ||
``` | ||
|
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.
We will also need an isHolocene
bool as well as branching logic in the fee estimation functions
- [System config contents (version 0)](#system-config-contents-version-0) | ||
- [Scalars](#scalars) | ||
- [Holocene `scalar`, `overhead` (`uint256,uint256`) change](#holocene-scalar-overhead-uint256uint256-change) | ||
|
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.
We will also want to include the specific ABI changes to the SystemConfig
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.
Nice work.
- [`operatorFeeScalar`](#operatorfeescalar) | ||
- [`operatorFeeConstant`](#operatorfeeconstant) | ||
- [`setOperatorFeeScalars`](#setoperatorfeescalars) | ||
- [`setOperatorFeeManager`](#setoperatorfeemanager) |
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.
Can we create a role in the SuperchainConfig
called "operator fee manager" and source the value from there? https://github.com/ethereum-optimism/specs/blob/d6b979ab67bd98bc3a15bf4988df08a066788047/specs/protocol/isthmus/superchain-config.md
This will make standard definition more simple, use the same superchain config that op gov recognizes, chain can also use their own superchain config if they are their own L2
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.
Follow up: trying to align ongoing work that is changing with this so that things are coherent, this is the design that we are landing on.
The SystemConfig.initialize
function would accept a Roles
struct, it would be good to add the operatorFeeManager
to that Roles
struct and have no setter function meaning the operator can only change with another call to initialize
. This helps keep the ABI slim and allows for each chain operator to have its own individual account that is used.
It is still possible to have each chain have its own individual accounts in the world where its sourced from the SuperchainConfig
, it just means that chains that aren't governed by Optimism would deploy their own SuperchainConfig
. Apologies for the back and forth, was trying to be consistent with designs in the Standard L2 Genesis project and we were flip flopping internally on how we were going to do it
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 makes more sense to me than the superchain solution, happy to proceed in this direction. Once the spec lands for the Roles
struct then I'll update this spec accordingly -- I've been yet unable to find reference to the design choices you linked.
@@ -67,19 +82,73 @@ The following actions should happen during the initialization of the `SystemConf | |||
- `emit ConfigUpdate.GAS_LIMIT` | |||
- `emit ConfigUpdate.UNSAFE_BLOCK_SIGNER` | |||
- `emit ConfigUpdate.EIP_1559_PARAMS` | |||
- `emit ConfigUpdate.OPERATOR_FEE_PARAMS` | |||
- `emit ConfigUpdate.OPERATOR_FEE_MANAGER` |
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.
We don't need to have the manager be a ConfigUpdate
, you can remove this
function setOperatorFeeScalar(uint32 _operatorFeeScalar, uint64 _operatorFeeConstant)() | ||
``` | ||
|
||
##### `setOperatorFeeManager` |
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.
I would like to delete this setter and instead source the fee manager from the superchain config as mentioned in a previous comment
This spec is looking great so far! Implementation will want to build on top of ethereum-optimism/optimism#12057 which is targeting audit Nov 4th, so it should be merged in over the next 2 weeks |
Any updates here? |
Hi! Sorry for the delay, but I've updated the spec setting the operator fee manager in the Roles struct. See |
This function MUST only be callable by the [`OperatorFeeManager`](#operator-fee-manager). | ||
|
||
```solidity | ||
function setOperatorFeeScalar(uint32 _operatorFeeScalar, uint64 _operatorFeeConstant)() |
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.
small nit: I think its fine to omit the trailing ()
when there is no return arg
|
||
**Description:** Operator fee scalar -- used to calculate the operator fee<br/> | ||
**Administrator:** [Operator Fee Manager](#operator-fee-manager)<br/> | ||
**Requirement:** Between 0 and 0.5 * (baseFee + priorityFee) <br/> |
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 isn't noted anywhere, apologies about that, but its probably best to put these configurability requirements in this file so they are all in a single place
Also we would like to say that the requirement for these values is 0 for now for the superchain, with op-succinct you can use them however you would like, then when we do add zk as part of the superchain's proof system we can define the ranges of values. We don't have bandwidth to really think deeply about what the standard values should be from a product perspective and while what you have now could make sense, I don't want to ratify something thru gov that ultimately isnt right
This generally looks good to me. Some work started in ethereum-optimism/optimism#12847 to define the config for isthmus that you can build on. cc @vdamle for visibility |
cc @trianglesphere come leave a review please |
specs/protocol/configurability.md
Outdated
|
||
**Description:** Account authorized to modify the operator fee scalar. <br/> | ||
**Administrator:** [System Config Owner](#admin-roles)<br/> | ||
**Requirement:** `address(0)`<br/> |
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.
Looking at this again, the requirement of address(0)
makes no sense. We cannot require the system config owner to be null. There currently is no such thing as the operator fee manager in the code
function setOperatorFeeScalars(uint32 _operatorFeeScalar, uint64 _operatorFeeConstant) external onlyOwner {
_setOperatorFeeScalars(_operatorFeeScalar, _operatorFeeConstant);
}
We should delete this section and just make the above 2 be managed by the System Config Owner
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.
The idea here is that "standard" chains cannot have an operator fee manager. Maybe the "Requirement" description is a bit misleading here.
But I agree that the operator fee manager isn't super necessary as a role. I can't imagine many situations where you'd need it to be distinct from the system config owner
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.
There's a few TODO
items here that need to be filled out ASAP
@leruaa Will merge after the lint error is fixed |
Overview
We propose adding additional fee scalars to the fee formula, which allow for more flexibility for chains that leverage alt-DA, ZK proving, or custom gas tokens.
This spec goes with this design doc.