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

fix(calc): rework calculating fees #937

Merged
merged 11 commits into from
Jun 3, 2022
Merged

fix(calc): rework calculating fees #937

merged 11 commits into from
Jun 3, 2022

Conversation

TarikGul
Copy link
Member

@TarikGul TarikGul commented Jun 2, 2022

This PR focuses on removing CalcFee from sidecar completely, and now relies on rpc.payment.queryInfo in order to retrieve the partialFee of an extrinsic.

Notes

  1. Some e2e-tests needed to be adjusted, our implementation of CalcFee was incredibly accurate, but in some cases there was a marginal difference between results.

  2. Excluding the few marginal differences in partialFee all e2e passed, meaning we are maintaining historical integrity and no longer need CalcFee.

  3. This PR removes ALOT of core code, tests, and types which is awesome. It really cleans up the /blocks endpoint and makes it more digestible.

  4. It should now be relatively straightforward to implement this feature: Fetch tx fee from onchain events #756

  5. A follow up PR should be made to remove calc from sidecar, place it in its own repository and archive it.

closes: #936
closes: #935

@TarikGul TarikGul requested a review from a team as a code owner June 2, 2022 13:18
@TarikGul TarikGul self-assigned this Jun 2, 2022
@TarikGul TarikGul added A0 - Please Review PR is ready for review P2 - ASAP Get fixed ASAP labels Jun 2, 2022
@@ -2027,7 +2027,7 @@
"info": {
"weight": "1177134000",
"class": "Normal",
"partialFee": "51999544"
"partialFee": "52002823"
Copy link
Member

Choose a reason for hiding this comment

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

@TarikGul what's up with all these changes of partialFee?

Copy link
Member Author

Choose a reason for hiding this comment

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

So here's my thought on this: @substrate/calc is part of older substrate legacy code when it comes to calculating fees. For a majority of our e2e-tests the partialFee we had before matched perfectly to the new partialFee we retrieve from the rpc method payment::queryInfo, but for the few cases that failed the accuracy was off by at most 4 decimal places (which is super tiny). I'd be lying if I said I knew exactly what the issue was when it came to our intrinsic calculations. But, moving forward IMO the substrate rpc method is more reliable/accurate and if there was an discrepancy it was with the way we were either:

a) pulling the necessary, encodedLength, multiplier, perByteFee, or weights per runtime.

b) Handling calculations and what runtime was using what fee calc version.

Either way, moving off of @substrate/calc, and relying on the intrinsic calculations provided by the node seems like the right move given these results.

Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

LGTM, but would good to know if these partialFree changes breaks something 🤔

@TarikGul TarikGul merged commit 3306466 into master Jun 3, 2022
@TarikGul TarikGul deleted the tarik-calc-reform branch June 3, 2022 17:52
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A0 - Please Review PR is ready for review P2 - ASAP Get fixed ASAP
Projects
None yet
2 participants