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: added query to calc fees #1366

Merged
merged 6 commits into from
Dec 21, 2023
Merged

fix: added query to calc fees #1366

merged 6 commits into from
Dec 21, 2023

Conversation

bee344
Copy link
Contributor

@bee344 bee344 commented Dec 21, 2023

Partially #1365

Co-authored-by: Tarik Gul <47201679+TarikGul@users.noreply.github.com>
@bee344 bee344 changed the title added bool to calc fees fix: added bool to calc fees Dec 21, 2023
@bee344 bee344 changed the title fix: added bool to calc fees fix: added query to calc fees Dec 21, 2023
@bee344 bee344 marked this pull request as ready for review December 21, 2023 16:15
@bee344 bee344 requested a review from a team as a code owner December 21, 2023 16:15
Copy link
Member

@TarikGul TarikGul left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@marshacb marshacb left a comment

Choose a reason for hiding this comment

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

LGTM

@bee344 bee344 merged commit 203a257 into master Dec 21, 2023
15 checks passed
@bee344 bee344 deleted the anp-ordinals-fix branch December 21, 2023 16:24
@GCrispino
Copy link

Sorry to comment in a closed pull request, but if this was done to reduce performance issues, any specific reason for not making the loop that processes extrinsics concurrent?

@TarikGul
Copy link
Member

@GCrispino There is a few things to tackle here:

  1. This PR is aimed at providing an option to opt out of the fees. It's one of many ways we are going to increase the performance here.

  2. perf: transactionPaidFee event optimization #1367 We have a follow up PR here to increase the performance as well that does not require using a query parameter.

  3. You are correct as we should find fees for each extrinsic concurrently moving forward, and we are working on getting a good solution going. But we also need to optimize the way we structure the calls to get the information necessary for fee calculation. We already have a PromiseQueue implemented for this: https://github.com/paritytech/substrate-api-sidecar/blob/master/src/util/PromiseQueue.ts#L5

Overall, we are taking steps to do this correctly. It's a balance between getting some fixes out now, as well as sorting out the larger issue correctly.

@GCrispino
Copy link

@TarikGul

Got it! This makes sense, thanks for taking the time to explain 👍🏼

This pull request was closed.
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.

4 participants