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

Added smoke tests for dynamic fee monitoring #2069

Merged
merged 8 commits into from
Feb 18, 2023

Conversation

timbrinded
Copy link
Contributor

@timbrinded timbrinded commented Jan 30, 2023

What does it do?

Adds new smoke test suite for dynamic-fees added in #1765

What important points reviewers should know?

  • Test currently skips execution for any chain < RT2100 or not Moonbase (since we are delaying roll out to other chain types)
  • Last test is skipped until we add BaseFee to emulated block header via substrate RPC

Run with:

npm run smoke-test:alphanet -- -- --grep S550

image

What value does it bring to the blockchain users?

On-going monitoring of baseFee calculation to make sure users are being fairly charged as expected.

@timbrinded timbrinded added B0-silent Changes should not be mentioned in any release notes D2-notlive PR doesn't change runtime code (so can't be audited) labels Jan 30, 2023
@timbrinded timbrinded added the A8-mergeoncegreen Pull request is reviewed well. label Feb 7, 2023
@timbrinded timbrinded marked this pull request as ready for review February 7, 2023 09:23
Comment on lines +223 to +225
const expectedBaseFeePerGasInWei =
(nextFeeMultiplier.toBigInt() * WEIGHT_FEE * WEIGHT_PER_GAS * supplyFactor) /
ethers.utils.parseEther("1").toBigInt();
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the lossy part of the equation, as expected. What could be useful is to show that the loss (the truncation) here is always less than some amount.

Copy link
Contributor

Choose a reason for hiding this comment

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

...or maybe not, that might just look very redundant...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have added a new test which checks that fee charged = gas used * weight_to_gas const.

I think this isn't what you were exactly asking for, but by inference we have enough coverage of the individual components that fee->gas has been derisked for it to be low concern.

Copy link
Contributor

@notlesh notlesh left a comment

Choose a reason for hiding this comment

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

LGTM. I'd still like to see a precision test where we take block's extrinsics -> exact fee and compare to on-chain, but we can rethink that later. I also think it's useful to intentionally not care about that and focus on the direction of change (as done here).

@timbrinded timbrinded merged commit 5809079 into master Feb 18, 2023
@timbrinded timbrinded deleted the timbo-dynamic-fees-test branch February 18, 2023 08:00
imstar15 pushed a commit to AvaProtocol/moonbeam that referenced this pull request May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A8-mergeoncegreen Pull request is reviewed well. B0-silent Changes should not be mentioned in any release notes D2-notlive PR doesn't change runtime code (so can't be audited)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants