-
Notifications
You must be signed in to change notification settings - Fork 78
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
feat: fee estimation api #962
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
use crate::constants::transaction::MaxExtrinsicSize; | ||
use codec::Codec; | ||
use frame_support::BoundedVec; | ||
use sp_weights::Weight; | ||
|
||
sp_api::decl_runtime_apis! { | ||
pub trait FeeEstimationApi<AccountId, AssetId, Balance> | ||
where | ||
AccountId: Codec, | ||
AssetId: Codec, | ||
Balance: Codec, | ||
{ | ||
fn estimate_fee_payment(weight: Weight, account_id: AccountId) -> (AssetId, Balance); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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_for_extrinsic( | ||
uxt: BoundedVec<u8, MaxExtrinsicSize>, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to use boundedVec here and also no need for new constant. Use the same as it is done in TransactionPaymentAPI.
|
||
account_id: AccountId | ||
) -> (AssetId, Balance); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -66,6 +66,7 @@ use sp_std::{convert::From, prelude::*}; | |
use sp_version::NativeVersion; | ||
use sp_version::RuntimeVersion; | ||
// A few exports that help ease life for downstream crates. | ||
|
||
use frame_support::{construct_runtime, pallet_prelude::Hooks, weights::Weight}; | ||
pub use hex_literal::hex; | ||
use orml_traits::MultiCurrency; | ||
|
@@ -79,6 +80,12 @@ pub use primitives::{ | |
}; | ||
use sp_api::impl_runtime_apis; | ||
pub use sp_consensus_aura::sr25519::AuthorityId as AuraId; | ||
use sp_runtime::traits::One; | ||
|
||
|
||
use frame_support::dispatch::GetDispatchInfo; | ||
use frame_support::BoundedVec; | ||
use primitives::constants::transaction::MaxExtrinsicSize; | ||
|
||
/// Opaque types. These are used by the CLI to instantiate machinery that don't need to know | ||
/// the specifics of the runtime. They can then be made to be agnostic over specific formats | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: not sure what happened here but if possible, would you revert these changes this PR should not touch/change anything else. |
||
fn version() -> RuntimeVersion { | ||
VERSION | ||
} | ||
|
||
fn execute_block(block: Block) { | ||
Executive::execute_block(block) | ||
} | ||
|
||
fn initialize_block(header: &<Block as BlockT>::Header) -> ExtrinsicInclusionMode { | ||
Executive::initialize_block(header) | ||
} | ||
} | ||
|
||
impl sp_api::Metadata<Block> for Runtime { | ||
fn metadata() -> OpaqueMetadata { | ||
|
@@ -1186,4 +1181,66 @@ impl_runtime_apis! { | |
Default::default() | ||
} | ||
} | ||
impl sp_api::Core<Block> for Runtime { | ||
fn version() -> RuntimeVersion { | ||
VERSION | ||
} | ||
|
||
fn execute_block(block: Block) { | ||
Executive::execute_block(block) | ||
} | ||
|
||
fn initialize_block(header: &<Block as BlockT>::Header) -> ExtrinsicInclusionMode { | ||
Executive::initialize_block(header) | ||
} | ||
} | ||
impl primitives::runtime_api::FeeEstimationApi<Block, AccountId, AssetId, Balance> for Runtime { | ||
fn estimate_fee_payment(weight: Weight, account_id: AccountId) -> (AssetId, Balance) { | ||
// Get the currency configured for this account | ||
let currency = MultiTransactionPayment::account_currency(&account_id); | ||
|
||
// Convert weight to base fee using native asset calculations | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is not correct. if there is not price, it should not return anything, hence the comment about return an option. |
||
|
||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. again, if it fails - it returns incorrect info. after you change, this should look something like :
we dont want to see 0 fee. this would be consistent with other stuff. |
||
|
||
(currency, fee_in_currency) | ||
} | ||
|
||
fn estimate_fee_payment_for_extrinsic( | ||
uxt: BoundedVec<u8, MaxExtrinsicSize>, | ||
account_id: AccountId, | ||
) -> (AssetId, Balance) { | ||
use codec::DecodeAll; | ||
|
||
// Decode the extrinsic | ||
let uxt = UncheckedExtrinsic::decode_all(&mut &uxt[..]) | ||
.expect("Invalid extrinsic provided"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
// Get dispatch info for weight calculation | ||
let info = <UncheckedExtrinsic as GetDispatchInfo>::get_dispatch_info(&uxt); | ||
|
||
// Calculate length component | ||
let len = uxt.encoded_size() as u32; | ||
let len_fee = TransactionPayment::length_to_fee(len); | ||
|
||
// Calculate weight component | ||
let weight_fee = TransactionPayment::weight_to_fee(info.weight); | ||
|
||
// Total native fee | ||
let native_fee = len_fee.saturating_add(weight_fee); | ||
|
||
// Convert to account's preferred currency | ||
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); | ||
|
||
Comment on lines
+1238
to
+1241
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no need to do this again - just call the other method. |
||
(currency, fee_in_currency) | ||
} | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming.
I would go with slightly different naming .
TransactionFeeAPI
and methods:
I let @jak-pan share his thoughts here on naming,