-
Notifications
You must be signed in to change notification settings - Fork 99
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
base: main
Are you sure you want to change the base?
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.
Hi! Sorry for the delay, but I've updated the spec setting the operator fee manager in the Roles struct. See |
|
||
| Input arg | Type | Calldata bytes | Segment | | ||
| ----------------- | ------- | -------------- | ------- | | ||
| {0x098999be} | | 0-3 | n/a | |
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.
what is this?
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.
That's the call signature for the function. Those 4 bytes are the first four bytes of keccak("setL1BlockValuesIsthmus()")
. I'll clarify this in the spec.
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.
Bump on adding this to the spec to make it clear what this is
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 |
|
||
#### Configuring Parameters | ||
|
||
`operatorFeeScalar` and `operatorFeeConstant` are loaded in a similar way to the `baseFeeScalar` and |
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 saw comment somewhere, maybe gone by now, that there would be restrictions on how large the operator fee scalar and fee constant should be set. If this feature becomes a standard chain features I think that this restriction is very important to protect user funds.
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 think that the operator fee will probably not become part of the superchain config since it adds complexity. Its purpose is more intended for non-standard chains, like op-succinct chains, for example.
My previous proposal wa the at the constant was between 0 and 600 Gwei and that the scalar was between 0 and 0.5x (basefee + priorityfee).
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 looks good, just add a clarifier for the l1 block attributes function signature so it's clear what that value is at the top of the table in l1-attributes.md
.
Thanks! Just fixed. |
@sebastianst If you have a moment could you please review this spec? |
specs/protocol/isthmus/predeploys.md
Outdated
function getOperatorFee(uint256 gasUsed)(uint256) | ||
``` | ||
|
||
##### `getL1Fee` |
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.
If a user wanted to calculate the L1 data fee and the Operator fee separately as this is specified, then...
l1DataFee=getL1Fee-getOperatorFee
and operatorFee=getOperatorFee
which is strange. I can totally imagine a wallet accidentally thinking that the way to calculate the total non-L2 fee would be to do getOperatorFee + getL1Fee
While I also would like to keep as much backwards compatibility as possible, upon thinking about this more, I could imagine this being confusing, since the operatorFee doesn't actually have to have anything to do with L1 costs - for op-succinct it accounts for off-chain proving costs, which are completely separate from L1.
I think it makes more sense to keep getL1Fee as is (just L1 data fees) and have the separate getOperatorFee.
Then total fee is just getL1Fee + getOperatorFee
|
||
### `Roles` | ||
|
||
The `Roles` struct is updated to include the new `OPERATOR_FEE_MANAGER` role. |
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.
@tynes About the possible conflicts in SystemConfig
, my concern is mainly about that part with the Roles
struct that don't exist yet, but added in ethereum-optimism/optimism#12932
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.
While the simplicity here is appealing, this method of using gas to meter proving costs won't really work. The underlying assumption here is that gas correlates with proving cost, which doesn't really hold at all, and would lead often to a higher fee than what should be paid if the constant & scalar billing parameters are set to assume the worst-case ratio.
For example, SSTORE costs differ based on the delta from zero to non-zero values, and have gas refunds for clearing values. Cycle counts for a trie update in the stateless zk clients don't really follow these dynamics.
Do you think we can generalize this further into a more customizeable or precise metering approach?
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.