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

audit(fees): specification audit #685

Merged
merged 12 commits into from
Jun 15, 2022
Merged

Conversation

danburck
Copy link
Contributor

@danburck danburck commented Jun 14, 2022

Description

As part of an internal audit, this PR refactors the specs of the fees module to improve text flow and add missing parts. For reviewers, please use this as an opportunity to fully understand the fees module.

@linear
Copy link

linear bot commented Jun 14, 2022

ENG-448 Module Specification audit

Improvements found

  • rename key prefixes

    	KeyPrefixFee           => KeyPrefixDeployerAddress
    	KeyPrefixFeeWithdrawal => KeyPrefixWithdrawAddress
    	KeyPrefixInverse       => KeyPrefixContractsByDeployer
    
  • Consider changing DevFeeInfo to Fee

@github-actions github-actions bot added the docs label Jun 14, 2022
@danburck danburck marked this pull request as ready for review June 14, 2022 13:57
@danburck danburck changed the title ENG 448 fees specification audit audit(fees): specification audit Jun 15, 2022
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, the only thing to point out is that 50% of the total tx fee goes to the validator instead of 50% of the base fee (which applies only to DynamicFeeTx (EIP-1559)).

Ideally, it should be:

  • For Legacy and AccessList txs: 50% should go to the validator and 50% to the developer (i.e same logic as now)
  • For DynamicFeeTx: Fee = BaseFee + PriorityFee. 50% of the BaseFee should go to the validator and 50% to the developer. 100% of the PriorityFee should go to the validator. Right now it's 50/50 for the Fee.

EDIT: it's fine as we leave it as is for now and we create an issue to work on it in the future

@danburck
Copy link
Contributor Author

@danburck danburck enabled auto-merge (squash) June 15, 2022 16:12
@danburck danburck merged commit 7a71f96 into main Jun 15, 2022
@danburck danburck deleted the ENG-448-fees-specification-audit branch June 15, 2022 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants