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

Send priority fees to collators #3120

Merged
merged 48 commits into from
Jan 14, 2025
Merged

Send priority fees to collators #3120

merged 48 commits into from
Jan 14, 2025

Conversation

TarekkMA
Copy link
Contributor

@TarekkMA TarekkMA commented Dec 27, 2024

What does it do?

Expand on the work done in #2923 and #3043, and send the priority fees to the block author (collator).

What important points reviewers should know?

  • This is how we handle fees in this change:
    • FeesTreasuryProportion is used to split the following
      • Substrate txs fees
      • Eth base fees
    • The eth priority fees / tips and substrate tips are sent fully to the block author.
  • I've removed DealWithFees struct and replaced it with
    • DealWithSubstrateFeesAndTip
    • DealWithEthereumBaseFees
    • DealWithEthereumPriorityFees

Tests

Some tests that were checking fees have been updated. Previously, they used the block author (ALITH), who will now receive tips due to this change, causing inaccuracies in the calculations.

Reviewer Checklist:

  • Base Eth Fees are split using FeesTreasuryProportion
  • Substrate Base fees are split using FeesTreasuryProportion
  • Priority fees and substrate tips are sent to the collator (Block author)
  • Make sure all runtimes have the same implementation
  • Changed test work the same way as before (I just changed the sender/revicver account)
  • All the new functionality are tested correctly for all scenarios.

Is there something left for follow-up PRs?

What alternative implementations were considered?

Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?

What value does it bring to the blockchain users?

@TarekkMA TarekkMA added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited not-breaking Does not need to be mentioned in breaking changes labels Dec 27, 2024
Copy link
Contributor

github-actions bot commented Dec 27, 2024

WASM runtime size check:

Compared to target branch

Moonbase runtime: 2268 KB (no changes) ✅

Moonbeam runtime: 2244 KB (no changes) ✅

Moonriver runtime: 2248 KB (no changes) ✅

Compared to latest release (runtime-3400)

Moonbase runtime: 2268 KB (+240 KB compared to latest release) ⚠️

Moonbeam runtime: 2244 KB (+232 KB compared to latest release) ⚠️

Moonriver runtime: 2248 KB (+236 KB compared to latest release) ⚠️

@TarekkMA TarekkMA marked this pull request as draft December 30, 2024 12:40
Copy link
Contributor

github-actions bot commented Dec 31, 2024

Coverage Report

@@                    Coverage Diff                     @@
##           master   tarekkma/priority-fees      +/-   ##
==========================================================
+ Coverage   74.40%                   74.51%   +0.11%     
  Files         376                      376              
- Lines       95432                    95401      -31     
==========================================================
+ Hits        70998                    71088      +90     
- Misses      24434                    24313     -121     
Files Changed Coverage
/runtime/moonbase/src/lib.rs 50.34% (-2.51%) 🔽
/runtime/moonbase/tests/common/mod.rs 97.10% (+0.03%) 🔼
/runtime/moonbase/tests/integration_test.rs 99.39% (+0.01%) 🔼
/runtime/moonbeam/src/lib.rs 44.41% (-2.70%) 🔽
/runtime/moonbeam/tests/common/mod.rs 94.80% (+0.06%) 🔼
/runtime/moonbeam/tests/integration_test.rs 99.39% (+0.01%) 🔼
/runtime/moonriver/src/lib.rs 44.63% (-2.71%) 🔽
/runtime/moonriver/tests/common/mod.rs 94.91% (+0.06%) 🔼
/runtime/moonriver/tests/integration_test.rs 99.37% (+0.01%) 🔼

Coverage generated Tue Jan 14 11:11:24 UTC 2025

@TarekkMA TarekkMA marked this pull request as ready for review January 9, 2025 14:31
@TarekkMA TarekkMA requested review from librelois and RomarQ January 9, 2025 14:44
runtime/moonbase/src/lib.rs Outdated Show resolved Hide resolved
RomarQ
RomarQ previously approved these changes Jan 10, 2025
Copy link
Contributor

@RomarQ RomarQ left a comment

Choose a reason for hiding this comment

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

The changes look good.

I think we should have common implementations of DealWithSubstrateFeesAndTip, DealWithEthereumBaseFees, DealWithEthereumPriorityFees in all runtimes.

stiiifff
stiiifff previously approved these changes Jan 13, 2025
@TarekkMA TarekkMA dismissed stale reviews from RomarQ and stiiifff via 3fdb11d January 14, 2025 08:58
RomarQ
RomarQ previously approved these changes Jan 14, 2025
@TarekkMA TarekkMA marked this pull request as draft January 14, 2025 10:18
@RomarQ RomarQ marked this pull request as ready for review January 14, 2025 11:42
Copy link
Contributor

@stiiifff stiiifff left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@RomarQ RomarQ merged commit 08ce140 into master Jan 14, 2025
37 of 38 checks passed
@RomarQ RomarQ deleted the tarekkma/priority-fees branch January 14, 2025 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited not-breaking Does not need to be mentioned in breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants