[XCMv5] Better fee mechanism #5420
Conversation
franciscoaguirre
force-pushed
the
xcm-pay-fees
branch
from
1f82808
to
50b06d6
Compare
|
bot bench polkadot-pallet --subcommand=xcm --runtime=westend --pallet=pallet_xcm_benchmarks::generic bot bench cumulus-assets --subcommand=xcm --runtime=asset-hub-westend --pallet=pallet_xcm_benchmarks::generic bot bench cumulus-coretime --subcommand=xcm --runtime=coretime-westend --pallet=pallet_xcm_benchmarks::generic bot bench cumulus-bridge-hubs --subcommand=xcm --runtime=bridge-hub-rococo --pallet=pallet_xcm_benchmarks::generic bot bench cumulus-contracts --subcommand=xcm --pallet=pallet_xcm_benchmarks::generic bot bench cumulus-people --subcommand=xcm --runtime=people-westend --pallet=pallet_xcm_benchmarks::generic |
…coco --target_dir=polkadot --pallet=pallet_xcm_benchmarks::generic
…stend --target_dir=polkadot --pallet=pallet_xcm_benchmarks::generic
…retime-westend --runtime_dir=coretime --target_dir=cumulus --pallet=pallet_xcm_benchmarks::generic
…set-hub-rococo --runtime_dir=assets --target_dir=cumulus --pallet=pallet_xcm_benchmarks::generic
…set-hub-westend --runtime_dir=assets --target_dir=cumulus --pallet=pallet_xcm_benchmarks::generic
…retime-rococo --runtime_dir=coretime --target_dir=cumulus --pallet=pallet_xcm_benchmarks::generic
…idge-hub-rococo --runtime_dir=bridge-hubs --target_dir=cumulus --pallet=pallet_xcm_benchmarks::generic
…idge-hub-westend --runtime_dir=bridge-hubs --target_dir=cumulus --pallet=pallet_xcm_benchmarks::generic
|
bot fmt |
|
@franciscoaguirre https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7522122 was started for your command Comment |
|
@franciscoaguirre Command |
|
bot fmt |
|
@franciscoaguirre https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7522964 was started for your command Comment |
|
@franciscoaguirre Command |
When trying to pay delivery fees, check for both PayFees and BuyExecution assets and try to auto-swap either of them.
| PayFees { .. } => { | ||
| return Err(()); | ||
| }, |
There was a problem hiding this comment.
I think there are more new instructions coming, do we want to make this generic or case-by-case e.g.: https://github.com/paritytech/polkadot-sdk/blob/master/polkadot/xcm/src/v2/mod.rs#L1182
| PayFees { .. } => { | |
| return Err(()); | |
| }, | |
| i => { | |
| log::warn!(target: "xcm::v5tov4", "`{i:?}` not supported by v4"); | |
| return Err(()); | |
| }, |
| let maybe_builder_attr = variant.attrs.iter().find(|attr| match attr.meta { | ||
| Meta::List(ref list) => list.path.is_ident("builder"), | ||
| _ => false, | ||
| }); | ||
| let builder_attr = match maybe_builder_attr { | ||
| Some(builder) => builder.clone(), | ||
| None => return Ok(None), /* It's not going to be an instruction that pays fees */ | ||
| }; | ||
| let Meta::List(ref list) = builder_attr.meta else { unreachable!("We checked before") }; | ||
| let inner_ident: Ident = syn::parse2(list.tokens.clone()).map_err(|_| { | ||
| Error::new_spanned( | ||
| &builder_attr, | ||
| "Expected `builder(loads_holding)` or `builder(pays_fees)`", | ||
| ) | ||
| })?; | ||
| let ident_to_match: Ident = syn::parse_quote!(pays_fees); | ||
| if inner_ident == ident_to_match { | ||
| Ok(Some(variant)) | ||
| } else { | ||
| Ok(None) // Must have been `loads_holding` instead. | ||
| } | ||
| }) | ||
| .collect::<Result<Vec<_>>>()?; |
There was a problem hiding this comment.
This could be deduplicated. Parsing pay_fees_variants and load_holding_variants uses exactly the same code, differing only in: syn::parse_quote!(pays_fees); vs syn::parse_quote!(loads_holding).
So it should be possible to extract it to some helper function:
let load_holding_variants = parse_variants(data_enum, syn::parse_quote!(loads_holding));
let pay_fees_variants = parse_variants(data_enum, syn::parse_quote!(pays_fees));
| _ => | ||
| return Err(Error::new_spanned( | ||
| variant, | ||
| "Both BuyExecution and PayFees have named fields", |
There was a problem hiding this comment.
I'm not fully convinced that this builder(pay_fees) assumes specific instructions like BuyExecution or PayFees and their structure. Why do we assume that these instructions can't have unnamed fields?
I suggest extracting the entire block of code that parses let load_holding_methods = load_holding_variants, and doing the following:
- Turn it into a helper function
parse_variant_methods(or better name). - It can nicely parse both named and unnamed fields
- Add a parameter that specifies the result type (that is the only difference here), such as
LoadedHoldingorAnythingGoes.
This way, we could have a single helper function, like parse_variant_methods, which will make generate_builder_impl simpler and more deduplicated:
fn generate_builder_impl(..) {
// We first require an instruction that load the holding register
let load_holding_methods = parse_variant_methods(data_enum, syn::parse_quote!(loads_holding), LoadedHolding);
...
// Some operations are allowed after the holding register is loaded
let allowed_after_load_holding_methods: ...
...
// Then we require fees to be paid
let pay_fees_methods = parse_variant_methods(data_enum, syn::parse_quote!(pays_fees), AnythingGoes);
}
There was a problem hiding this comment.
@franciscoaguirre please do this in a followup PR, let's merge this one so we can work on top of the new fees mechanism
cumulus/parachains/integration-tests/emulated/common/src/macros.rs
Outdated
Show resolved
Hide resolved
Implements RFC#105 which, at the time of this PR, has not been approved yet. Some aspects might be changed as a result of discussion.
TODO