-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
feat(protocol): add overridable getEIP1559Config() to TaikoL2 #13815
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.
Overall, this looks like a well-structured pull request. Here are some of the comments that I have:
- The
EIP1559Config
struct should be documented in the contract documentation. - The
getEIP1559Config()
function should be documented in the contract documentation. - In the
eip1559Config()
function, it would be better to use theconfig
parameter instead of_param1559
. This will make it easier to understand what is being passed as an argument. - In the
_calcBasefee()
function, it would be better to use theconfig
parameter instead of individual parameters forxscale
,yscale
, andgasIssuedPerSecond
. This will make it easier to understand what is being passed as an argument. - In the
getBlockHash()
function, there should be a brief explanation of what this function does.
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.
Overall, the code looks good. Here are some suggestions:
- Consider adding a comment at the top of the
eip1559Config()
function to explain why_param1559
is passed in as an argument. - Consider adding a comment to explain why you are using
uint256
for thebasefee
variable. - Consider adding a comment to explain why you are using
uint64
for the_xscale
and_gasExcess
variables. - Consider adding a comment to explain why you are using
uint128
for the_yscale
variable. - Consider renaming the
EIP1559Params
struct to something more descriptive, likeEIP1559Parameters
. - Consider renaming the
getEIP1559Config()
function to something more descriptive, likegetEIP1559Parameters()
.
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.
Overall, the code changes look good. Here are some suggestions:
- In the
eip1559Config()
function, consider renaming the_xscale
and_yscale
variables toxscale
andyscale
, respectively, to make them consistent with the other variables in this function. - Consider adding a comment to the
_calcBasefee()
function to explain what it does. - In the same function, consider using more descriptive names for the variables
a
andb
. - Consider adding a comment in the
getEIP1559Config()
function to explain why it is a virtual function and why it returns a constant EIP1559Config object. - In the same function, consider using more descriptive names for the variables
config
,timeSinceParent
, andgasLimit
. - In the
generateContractConfigs()
file, consider using more descriptive names for the variablesparam1559
andcontractConfigs
.
…3817) Co-authored-by: Daniel Wang <99078276+dantaik@users.noreply.github.com>
With this new function, we can upgrade the deployment with a derived contract to avoid one SLOAD: