Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

AssetTxFeePaid event should return fee and tip amounts in the asset ID #12987

Closed
2 tasks done
IkerAlus opened this issue Dec 21, 2022 · 5 comments · Fixed by #13083
Closed
2 tasks done

AssetTxFeePaid event should return fee and tip amounts in the asset ID #12987

IkerAlus opened this issue Dec 21, 2022 · 5 comments · Fixed by #13083
Labels
J2-unconfirmed Issue might be valid, but it’s not yet known.

Comments

@IkerAlus
Copy link

Is there an existing issue?

  • I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

  • This is not a support question.

Description of bug

When an AssetTxFeePaid event of the asset-tx-payment pallet is emitted, the fees and tip are subtracted from the asset ID balance of the sending account. However this event currently returns the tip and actualFee fields in balances of the chain native token. This is confusing for users and external tools, since they assume the values displayed in those fields are given in units of the asset ID.

Steps to reproduce

Example screenshot of a transaction on Statemint paying the fees in the asset ID 1984 but emitting the AssetTxFeePaid event in DOT balances:
Screenshot 2022-12-21 at 09 23 38

@github-actions github-actions bot added the J2-unconfirmed Issue might be valid, but it’s not yet known. label Dec 21, 2022
@Szegoo
Copy link
Contributor

Szegoo commented Dec 29, 2022

I think this is a reasonable change(I am not the one to make the decision here though). If this gets confirmed I will open a simple PR for this.

@bkchr
Copy link
Member

bkchr commented Jan 3, 2023

Sounds reasonable as well to me! Please open a pr @Szegoo

@Szegoo
Copy link
Contributor

Szegoo commented Jan 4, 2023

I realised that this is not as easy and simple to implement as I thought :P
This would require us to make the BalanceOf<T> generic inside the transaction-payment pallet.
We could do something like this:

pub fn compute_actual_fee<Balance: FixedPointOperand + AtLeast32BitUnsigned>(

So when we make a call to this from the asset-tx-payment pallet we would call this like the following:

let actual_fee = pallet_transaction_payment::Pallet::<T>::compute_actual_fee::<AssetBalanceOf<T>>(
	len as u32, info, post_info, tip,
);

This way we could make the Balance type generic but this does complicate the code a bit and also results in less readable code.

I think the person who originally wrote the code was aware of this compromise and decided that it was not worth it.

I am curious what you think about this @bkchr.

@bkchr
Copy link
Member

bkchr commented Jan 5, 2023

Not sure I can follow you, don't we just need to add a BalanceToAsset: BalanceConversion<BalanceOf<Self>, AssetIdOf<Self>, AssetBalanceOf<Self>> to the Config trait of asset-tx-payment and then call this code:

let converted_fee = CON::to_asset_balance(corrected_fee, paid.asset())
.map_err(|_| -> TransactionValidityError { InvalidTransaction::Payment.into() })?
.max(min_converted_fee);
to convert the balance fee to the asset balance fee?

@Szegoo
Copy link
Contributor

Szegoo commented Jan 5, 2023

Correct, I overlooked that and started complicating 😅

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
J2-unconfirmed Issue might be valid, but it’s not yet known.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants