Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from 23 commits
f4ef24d
d2f40a0
7ddbce3
2e12898
1407fa9
50c22ec
78cb4cb
6a78e21
fb53c85
b56048a
7c90f2b
f4e27b3
1526415
7085d98
148230b
fbf8dfb
48a25af
98fdb84
ead7704
1ade65a
4673839
b945faf
2f73dc2
43c5ea9
2efe231
816cc6c
a8e094e
1314351
63d1760
76fa61a
3afa960
f45744f
2a64355
2f0187f
1625dab
bc293f2
ff1aa7f
317ac76
bbb9e05
4c3eaca
cfdc6fc
3db2793
e2af323
b88dcf8
dcf4bdd
6ad2938
3d9f967
3c99591
2bf6246
4e0f724
9ab37f9
c7efd31
6c57838
1aa27f4
219e72c
762dbf7
99c9f4d
18c8127
54ea6a4
6512664
c7cffb1
0accd5f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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, likeOperatorFee
operatorFeeScalar
operatorFeeConstant
or something similar that attaches more meaning to them?
fixedFee...
could also work.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
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.
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
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 theoperatorFeeConstant
andoperatorFeeScalar
.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.
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.
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.
on second thought maybe this isn't necessary -- if we read the scalars from the l1block before isthmus is set, we'll just read from uninitialized storage, which defaults to 0. So the derived operator fee will just be 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.
This is useful for the
GasOracle
predeploy that attempts to do L2 fee estimationThere 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.
You can see here how we have handled this in the past. I believe we will need to include a calculation of the operator fee in the
GasPriceOracle
now