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): remove ETH deposit fees #16634

Closed
wants to merge 6 commits into from
Closed

Conversation

dantaik
Copy link
Contributor

@dantaik dantaik commented Apr 3, 2024

This is based on a feedback from an auditor - he is not a fun of the ETH deposit fee design.

How do you guys like the idea of completely remove the fee and lift the min ETH deposit amount?

BTW, I think these changes are backward compatible so Hekla doesn't need to regenesis. @Brechtpd @davidtaikocha

@dantaik dantaik changed the title feat(protocol): remove ETH deposit fees feat(protocol)!: remove ETH deposit fees Apr 3, 2024
@dantaik dantaik marked this pull request as ready for review April 3, 2024 15:06
Copy link

openzeppelin-code bot commented Apr 3, 2024

feat(protocol)!: remove ETH deposit fees

Generated at commit: 385b30b6c9d28f7140fd3a9ebc3968327a16a2f7

🚨 Report Summary

Severity Level Results
Contracts Critical
High
Medium
Low
Note
Total
2
2
0
3
39
46
Dependencies Critical
High
Medium
Low
Note
Total
0
0
0
0
0
0

For more details view the full report in OpenZeppelin Code Inspector

@@ -35,17 +35,11 @@ library TaikoData {
// The size of the ETH deposit ring buffer.
uint256 ethDepositRingBufferSize;
// The minimum number of ETH deposits allowed per block.
uint64 ethDepositMinCountPerBlock;
uint64 ethDepositsPerBlock;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we lose backward compatiblity with Hekla with the removal of these:
ethDepositMaxAmount, ethDepositGas, ethDepositMaxFee

because the blockSyncThreshold position within the struct slots will be changed, and we use that in LibVerifying.sol:

if (_lastVerifiedBlockId > lastSyncedBlock + _config.blockSyncThreshold)

Copy link
Contributor

Choose a reason for hiding this comment

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

If all smart contracts are updated should be good I guess because will point to the correct location?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok, since memory object, right !

Copy link
Contributor Author

@dantaik dantaik Apr 4, 2024

Choose a reason for hiding this comment

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

@Brechtpd, @adaki2004 do you prefer the entire EthDeposit feature is removed instead? Now we have our Bridge and it seems nobody used the EthDeposit feature, and the current implementation of this feature also assume the existence of the Bridge. #16638

@dantaik dantaik changed the title feat(protocol)!: remove ETH deposit fees feat(protocol): remove ETH deposit fees Apr 4, 2024
@dantaik dantaik marked this pull request as draft April 4, 2024 03:19
@dantaik dantaik closed this Apr 4, 2024
@dantaik
Copy link
Contributor Author

dantaik commented Apr 4, 2024

Closed as I'm in favor of #16638

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants