Skip to content
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

Merged
merged 5 commits into from
May 25, 2023

Conversation

dantaik
Copy link
Contributor

@dantaik dantaik commented May 25, 2023

With this new function, we can upgrade the deployment with a derived contract to avoid one SLOAD:

contract CustomTaikoL2 is TaikoL2 {
    function getEIP1559Config() public pure override returns (EIP1559Config memory config) {
        config.xscale = 123;
        config.yscale = 456;
        config.gasIssuedPerSecond = 789;
    }
}

contract ProxiedCustomTaikoL2 is Proxied, CustomTaikoL2 {}
  • @davidtaikocha After this PR is merged, please cherry-pick it into alpha-3 branch.

@dantaik dantaik marked this pull request as ready for review May 25, 2023 05:46
Copy link
Contributor

@github-actions github-actions bot left a 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 the config 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 the config parameter instead of individual parameters for xscale, yscale, and gasIssuedPerSecond. 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.

Copy link
Contributor

@github-actions github-actions bot left a 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 the basefee 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, like EIP1559Parameters.
  • Consider renaming the getEIP1559Config() function to something more descriptive, like getEIP1559Parameters().

Copy link
Contributor

@github-actions github-actions bot left a 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 to xscale and yscale, 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 and b.
  • 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, and gasLimit.
  • In the generateContractConfigs() file, consider using more descriptive names for the variables param1559 and contractConfigs.

@dantaik dantaik requested a review from adaki2004 May 25, 2023 09:38
@dantaik dantaik added this pull request to the merge queue May 25, 2023
Merged via the queue into main with commit e15a9c1 May 25, 2023
@dantaik dantaik deleted the improve_taiko_l2 branch May 25, 2023 10:32
@github-actions github-actions bot mentioned this pull request May 25, 2023
davidtaikocha added a commit that referenced this pull request May 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants