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

Fetch tx fee from onchain events #756

Closed
xlc opened this issue Nov 10, 2021 · 10 comments · Fixed by #1040
Closed

Fetch tx fee from onchain events #756

xlc opened this issue Nov 10, 2021 · 10 comments · Fixed by #1040
Labels
A3 - In Progress PR in progress, not ready for review I8 - Enhancement Additional feature request

Comments

@xlc
Copy link
Contributor

xlc commented Nov 10, 2021

It will be useful if there is a mode to query tx fee info from onchain events data instead of calculating it.

But need to consider about paritytech/polkadot#4230

@joepetrowski
Copy link
Collaborator

We explicitly decided against this because there is no guarantee that the balances.Deposit and treasury.Deposit events are related to transaction fees. For example, if you just send a transfer to the Treasury, there will be a treasury.Deposit event.

We also discussed adding an event to the runtime for the fee, but decided against that as well because events are not free and the information can be deterministically reproduced by...something like Sidecar.

@joepetrowski
Copy link
Collaborator

joepetrowski commented Nov 10, 2021

Although I suppose if a parachain wanted to add a runtime event for this purpose, then sure, we could send Sidecar down that logic road. So we could make certain chain configs look for an event instead of providing a calc_fee package.

But to be clear: using the existing events in the Relay Chain runtime is not a good way to calculate fees.

@joepetrowski joepetrowski reopened this Nov 10, 2021
@xlc
Copy link
Contributor Author

xlc commented Nov 10, 2021

I see. That make sense. I know Subscan is using events to fetch fees and they had some issues with new runtime.

Also the last treasury deposit should always be the fee deposit right?

@joepetrowski
Copy link
Collaborator

Also the last treasury deposit should always be the fee deposit right?

I thought of that too when we first designed Sidecar, but decided it was not an assumption we could rely on.

@xlc
Copy link
Contributor Author

xlc commented Nov 11, 2021

I see. While that may not be true universally, it could be true for a particular parachain so could be a useful opt-in option.

@TarikGul
Copy link
Member

I wanted to bring this back to light.

I think it could be useful to have a query parameter such like ?calcFeeByEvent=treasury_deposit, this way parachains can opt in to the events they feel might be useful for calculating fees, but by default we would stick to the calcFee package. This could be a good opportunity to abstract the fee calculation logic for /blocks, and seperate it into a cleaner more "plug-in style" container.

  1. Could give more extensibility to Parachains, and users for how they would like their fees to be represented or fetched (Makes sense in regards to what Joe said about a chain potentially having their own runtime logic with an event like this attached).

  2. BlocksService is starting to get large, and having the ability to pull fee calculation logic out, and create a clean way for parachains to build on top of, or contribute to will help a lot.

  3. Potential for performance increases for Parachains if sidecar doesn't have to calculate the fees, since some unique runtime event will have it stored.

Just some thoughts I had.

@xlc
Copy link
Contributor Author

xlc commented Jun 24, 2022

Now transaction payment pallet is emitting an event to include the fee amount, sidecar should support it paritytech/substrate#11618

Although we have implemented similar event in our pallet, the format is not exactly the same, so need someway to handle the difference. I guess the easy way is simply expose the whole event details to the relevant field
AcalaNetwork/Acala#2218

@TarikGul
Copy link
Member

TarikGul commented Jun 24, 2022

Now transaction payment pallet is emitting an event to include the fee amount, sidecar should support it

Agreed, I will attach a separate issue so we can track that within the following releases that those changes are included in via polkadot/substrate. As to how we will parse that info and display it i'll have to do some playing and give it some thought. I do like the idea of attaching an extra field in info for the extrinsic that way we dont interfere with the base of the api, but still display all the info for users. It will be some refactoring, but easily doable now that we have way less bloat around fee calculation.

That all being said, Im currently working through the events and writing an implementation for extracting the fee. I'm seeing the Event::Withdraw is really the source of truth, and will use some comparisons to make sure it is the fee. But overall looks pretty solid

@joepetrowski
Copy link
Collaborator

That all being said, Im currently working through the events and writing an implementation for extracting the fee. I'm seeing the Event::Withdraw is really the source of truth, and will use some comparisons to make sure it is the fee.

Not sure of edge cases, but Withdraw also happens in balance changes on XCM instructions. So It's not strictly used for the fee. Plus you have the refund cases where some is withdrawn pre-dispatch and then returned (Deposit) post-dispatch.

@TarikGul
Copy link
Member

That all being said, Im currently working through the events and writing an implementation for extracting the fee. I'm seeing the Event::Withdraw is really the source of truth, and will use some comparisons to make sure it is the fee.

Not sure of edge cases, but Withdraw also happens in balance changes on XCM instructions. So It's not strictly used for the fee. Plus you have the refund cases where some is withdrawn pre-dispatch and then returned (Deposit) post-dispatch.

Good to know! Thanks.

Yea my approach for now is to do some shallow analysis on the events, and compare it to the fee returned by rpc::payment::queryInfo, depending on the results to then move on to the balances::deposit, and so forth.

I think the Event::TransactionFeePaid will help sort this out, but it wont cover historic blocks so we'll still need a proper solution relevant to the approach above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A3 - In Progress PR in progress, not ready for review I8 - Enhancement Additional feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants