feat: fee estimation api #962
Conversation
- Introduces runtime API to simplify fee payment calculation from weight+account_id or extrinsic+account_id. - Integrates native and custom currency pricing. - Adds tests covering fallback scenarios, large extrinsic handling, minimal weight fees, and currency switching.
| use sp_weights::Weight; | ||
|
|
||
| sp_api::decl_runtime_apis! { | ||
| pub trait FeeEstimationApi<AccountId, AssetId, Balance> |
There was a problem hiding this comment.
Naming.
I would go with slightly different naming .
TransactionFeeAPI
and methods:
- estimate_fee
- estimate_uxt_fee
I let @jak-pan share his thoughts here on naming,
| AssetId: Codec, | ||
| Balance: Codec, | ||
| { | ||
| fn estimate_fee_payment(weight: Weight, account_id: AccountId) -> (AssetId, Balance); |
There was a problem hiding this comment.
The result of both methods should return an Option.
In the actual implementation it seems it does unwrap_or in multiple places , so it can potentially return incorrect result, not the one you expect.
I would change the return value to be a struct. This would help with potential future extension if we want to include additional details.
| fn estimate_fee_payment(weight: Weight, account_id: AccountId) -> (AssetId, Balance); | ||
|
|
||
| fn estimate_fee_payment_for_extrinsic( | ||
| uxt: BoundedVec<u8, MaxExtrinsicSize>, |
There was a problem hiding this comment.
No need to use boundedVec here and also no need for new constant.
Use the same as it is done in TransactionPaymentAPI.
`uxt: <Block as BlockT>::Extrinsic,`
| @@ -450,19 +457,7 @@ use sp_core::OpaqueMetadata; | |||
| use xcm_fee_payment_runtime_api::Error as XcmPaymentApiError; | |||
|
|
|||
| impl_runtime_apis! { | |||
| impl sp_api::Core<Block> for Runtime { | |||
There was a problem hiding this comment.
nitpick: not sure what happened here but if possible, would you revert these changes
this PR should not touch/change anything else.
| let native_fee = TransactionPayment::weight_to_fee(weight); | ||
|
|
||
| // Get the price of the account's currency if applicable | ||
| let price = MultiTransactionPayment::price(currency).unwrap_or(Price::one()); |
There was a problem hiding this comment.
this is not correct. if there is not price, it should not return anything, hence the comment about return an option.
| let price = MultiTransactionPayment::price(currency).unwrap_or(Price::one()); | ||
|
|
||
| // Calculate the final fee in the account's currency | ||
| let fee_in_currency = price.checked_mul_int(native_fee).unwrap_or(native_fee); |
There was a problem hiding this comment.
again, if it fails - it returns incorrect info.
after you change, this should look something like :
`price.checked_mul_int(fee).map(|f| f.max(One::one()))`
we dont want to see 0 fee. this would be consistent with other stuff.
|
|
||
| // Decode the extrinsic | ||
| let uxt = UncheckedExtrinsic::decode_all(&mut &uxt[..]) | ||
| .expect("Invalid extrinsic provided"); |
There was a problem hiding this comment.
this could cause panics easily.
there is not need to have boundedvev and and decode stuff. See my other comment to use differetn type for the paramter.
| let currency = MultiTransactionPayment::account_currency(&account_id); | ||
| let price = MultiTransactionPayment::price(currency).unwrap_or(Price::one()); | ||
| let fee_in_currency = price.checked_mul_int(native_fee).unwrap_or(native_fee); | ||
|
|
There was a problem hiding this comment.
no need to do this again - just call the other method.
Hi @jak-pan,
I’ve opened a draft PR that addresses #954 by introducing a runtime API for fee estimation. The API provides two key methods:
I’ve also included extensive test coverage covering fallback scenarios (no price available), invalid extrinsic inputs, minimal weight fees, large extrinsics near max size, and account currency switching.
I made it a draft PR since there are only a few days left in the Kudos Carnival, and I wanted to ensure you have a chance to review and provide feedback before finalizing. Please let me know if any changes are needed. Thank you!