-
Notifications
You must be signed in to change notification settings - Fork 645
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
Xcm SetAssetClaimer 1 #5585
base: xcm-pay-fees
Are you sure you want to change the base?
Xcm SetAssetClaimer 1 #5585
Conversation
Co-authored-by: Adrian Catangiu <adrian@parity.io>
Co-authored-by: Adrian Catangiu <adrian@parity.io>
Co-authored-by: Adrian Catangiu <adrian@parity.io>
…ghts/xcm/pallet_xcm_benchmarks_generic.rs
bot bench polkadot-pallet --subcommand=xcm --runtime=westend --pallet=pallet_xcm_benchmarks::generic |
@x3c41a https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7265736 was started for your command Comment |
@x3c41a Command |
@x3c41a Command |
bot bench polkadot-pallet --subcommand=xcm --runtime=westend --pallet=pallet_xcm_benchmarks::generic |
@x3c41a https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7266819 was started for your command Comment |
…stend --target_dir=polkadot --pallet=pallet_xcm_benchmarks::generic
@x3c41a Command |
@x3c41a Command |
The CI pipeline was cancelled due to failure one of the required jobs. |
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.
The implementation looks good, the unit tests look good. My ideal test for this would be:
- User in penpal a wants to reserve asset transfer some WND to a user on penpal b (through AssetHub)
- Sets amount for fees as too low and the assets are trapped on AssetHub
- Thankfully he set the claimer
- He sends a new message (using
polkadotXcm::send
because of theClearOrigin
problem with transfers) with theClaimAsset
instruction. - With the claimed assets he can finish the send to penpal b
@@ -98,6 +106,8 @@ mod imports { | |||
pub type ParaToParaThroughRelayTest = Test<PenpalA, PenpalB, Westend>; | |||
pub type ParaToParaThroughAHTest = Test<PenpalA, PenpalB, AssetHubWestend>; | |||
pub type RelayToParaThroughAHTest = Test<Westend, PenpalA, AssetHubWestend>; | |||
pub type BridgeToAssetHubTest = Test<BridgeHubWestend, AssetHubWestend>; |
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.
pub type BridgeToAssetHubTest = Test<BridgeHubWestend, AssetHubWestend>; | |
pub type BridgeHubToAssetHubTest = Test<BridgeHubWestend, AssetHubWestend>; |
let xcm_here = Xcm::<RuntimeCall>::builder_unsafe() | ||
.withdraw_asset((Parent, 200_000_000_000u128)) | ||
.pay_fees((Parent, 40_000_000_000u128)) | ||
.export_message( |
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.
This instruction is only used to send a message across a bridge. There's no need to use it here
@@ -343,4 +343,12 @@ impl<T: frame_system::Config> pallet_xcm::WeightInfo for WeightInfo<T> { | |||
.saturating_add(T::DbWeight::get().reads(1)) | |||
.saturating_add(T::DbWeight::get().writes(1)) | |||
} | |||
|
|||
fn set_asset_claimer() -> Weight { |
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.
This shouldn't be here in pallet_xcm
, since it's not a call you can make on the pallet. It's correct to have it in pallet-xcm-benchmarks
because we use that for xcm instructions.
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.
Remove
fn set_asset_claimer() -> Weight { | ||
Weight::from_parts(2_176_000, 0) | ||
} |
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.
This should be removed. There's no call in pallet_xcm
like this. This trait has a function for each pallet call but returns a weight.
let result = vm.bench_process(xcm); | ||
assert!(result.is_err()); | ||
assert_eq!(vm.asset_claimer(), Some(bob)); | ||
} |
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.
New line at end of file 😁
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.
I don't know why this is marked as changed. Maybe a space?
Added SetAssetClaimer instruction