Skip to content
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.

Sign up for GitHub

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

Jump to bottom

pallet-xcm: added useful error logs (#2408) #4982

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
+88 −25
Open

pallet-xcm: added useful error logs (#2408) #4982

Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
991c9da
pallet-xcm: added useful error logs (#2408)
Jul 9, 2024
84e540a
Merge branch 'master' into ayevbeosa-log-xcm-errors
ayevbeosa Jul 9, 2024
3cb6ad3
Merge branch 'master' into ayevbeosa-log-xcm-errors
ayevbeosa Jul 9, 2024
eae27b6
xcm-executor: added logs
Jul 11, 2024
e9ecfba
Merge branch 'master' into ayevbeosa-log-xcm-errors
ayevbeosa Jul 11, 2024
7a4eef7
Apply suggestions from code review
acatangiu Oct 2, 2024
6bbfffa
Apply suggestions from code review
acatangiu Oct 2, 2024
6aa4816
Merge branch 'master' into ayevbeosa-log-xcm-errors
Oct 3, 2024
6a3e140
added logs
Oct 3, 2024
03e0b88
Update polkadot/xcm/pallet-xcm/src/lib.rs
bkontur Oct 3, 2024
92e5cbd
Update polkadot/xcm/pallet-xcm/src/lib.rs
bkontur Oct 4, 2024
9104421
Update polkadot/xcm/pallet-xcm/src/lib.rs
bkontur Oct 4, 2024
5011a47
Update polkadot/xcm/pallet-xcm/src/lib.rs
ayevbeosa Oct 4, 2024
208f62d
Merge branch 'master' into ayevbeosa-log-xcm-errors
ayevbeosa Oct 4, 2024
601e110
Update polkadot/xcm/pallet-xcm/src/lib.rs
bkontur Oct 4, 2024
081c929
Update polkadot/xcm/pallet-xcm/src/lib.rs
bkontur Oct 4, 2024
cbc97b5
Update polkadot/xcm/xcm-executor/src/lib.rs
bkontur Oct 4, 2024
9d3ec46
Update polkadot/xcm/pallet-xcm/src/lib.rs
bkontur Oct 4, 2024
40ec293
Update polkadot/xcm/xcm-executor/src/lib.rs
bkontur Oct 4, 2024
76ecb82
Apply suggestions from code review
bkontur Oct 4, 2024
bf76568
Update polkadot/xcm/xcm-executor/src/lib.rs
bkontur Oct 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 64 additions & 16 deletions polkadot/xcm/pallet-xcm/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1836,7 +1836,10 @@ impl<T: Config> Pallet<T> {
// no custom fees instructions, they are batched together with `assets` transfer;
// BuyExecution happens after receiving all `assets`
let reanchored_fees =
fees.reanchored(&dest, &context).map_err(|_| Error::<T>::CannotReanchor)?;
fees.reanchored(&dest, &context).map_err(|e| {
log::error!(target: "xcm::pallet_xcm::add_fees_to_xcm", "Failed to re-anchor fees: {e:?} for dest: {dest:?} and context {context:?}");
Error::<T>::CannotReanchor
})?;
// buy execution using `fees` batched together with above `reanchored_assets`
remote.inner_mut().push(BuyExecution { fees: reanchored_fees, weight_limit });
},
Expand Down Expand Up @@ -1901,7 +1904,10 @@ impl<T: Config> Pallet<T> {
let mut reanchored_assets = assets.clone();
reanchored_assets
.reanchor(&dest, &context)
.map_err(|_| Error::<T>::CannotReanchor)?;
.map_err(|e| {
log::error!(target: "xcm::pallet_xcm::local_reserve_transfer_programs", "Failed to re-anchor assets: {e:?} for dest: {dest:?} and context {context:?}");
Error::<T>::CannotReanchor
})?;

// XCM instructions to be executed on local chain
let mut local_execute_xcm = Xcm(vec![
Expand Down Expand Up @@ -1948,7 +1954,10 @@ impl<T: Config> Pallet<T> {
let reanchored_fees = fees
.clone()
.reanchored(&dest, &context)
.map_err(|_| Error::<T>::CannotReanchor)?;
.map_err(|e| {
log::error!(target: "xcm::pallet_xcm::destination_reserve_fees_instructions", "Failed to re-anchor fees: {e:?} for dest: {dest:?} and context {context:?}");
Error::<T>::CannotReanchor
})?;
let fees: Assets = fees.into();

let local_execute_xcm = Xcm(vec![
Expand Down Expand Up @@ -1992,7 +2001,10 @@ impl<T: Config> Pallet<T> {
let mut reanchored_assets = assets.clone();
reanchored_assets
.reanchor(&dest, &context)
.map_err(|_| Error::<T>::CannotReanchor)?;
.map_err(|e| {
log::error!(target: "xcm::pallet_xcm::destination_reserve_transfer_programs", "Failed to re-anchor assets: {e:?} for dest: {dest:?} and context {context:?}");
Error::<T>::CannotReanchor
})?;

// XCM instructions to be executed on local chain
let mut local_execute_xcm = Xcm(vec![
Expand Down Expand Up @@ -2046,13 +2058,22 @@ impl<T: Config> Pallet<T> {
// identifies fee item as seen by `reserve` - to be used at reserve chain
let reserve_fees = fees_half_1
.reanchored(&reserve, &context)
.map_err(|_| Error::<T>::CannotReanchor)?;
.map_err(|e| {
log::error!(target: "xcm::pallet_xcm::remote_reserve_transfer_program", "Failed to re-anchor reserve_fees: {e:?} for reserve: {reserve:?} and context {context:?}");
Error::<T>::CannotReanchor
})?;
// identifies fee item as seen by `dest` - to be used at destination chain
let dest_fees = fees_half_2
.reanchored(&dest, &context)
.map_err(|_| Error::<T>::CannotReanchor)?;
.map_err(|e| {
log::error!(target: "xcm::pallet_xcm::remote_reserve_transfer_program", "Failed to re-anchor dest_fees: {e:?} for dest: {dest:?} and context {context:?}");
Error::<T>::CannotReanchor
})?;
// identifies `dest` as seen by `reserve`
let dest = dest.reanchored(&reserve, &context).map_err(|_| Error::<T>::CannotReanchor)?;
let dest = dest.reanchored(&reserve, &context).map_err(|e| {
log::error!(target: "xcm::pallet_xcm::remote_reserve_transfer_program", "Failed to re-anchor dest: {e:?} for reserve: {reserve:?} and context {context:?}");
Error::<T>::CannotReanchor
})?;
// xcm to be executed at dest
let mut xcm_on_dest =
Xcm(vec![BuyExecution { fees: dest_fees, weight_limit: weight_limit.clone() }]);
Expand Down Expand Up @@ -2098,7 +2119,10 @@ impl<T: Config> Pallet<T> {
let reanchored_fees = fees
.clone()
.reanchored(&dest, &context)
.map_err(|_| Error::<T>::CannotReanchor)?;
.map_err(|e| {
log::error!(target: "xcm::pallet_xcm::teleport_fees_instructions", "Failed to re-anchor fees: {e:?} for dest: {dest:?} and context {context:?}");
Error::<T>::CannotReanchor
})?;

// XcmContext irrelevant in teleports checks
let dummy_context =
Expand All @@ -2112,7 +2136,10 @@ impl<T: Config> Pallet<T> {
&fees,
&dummy_context,
)
.map_err(|_| Error::<T>::CannotCheckOutTeleport)?;
.map_err(|e| {
log::error!(target: "xcm::pallet_xcm::teleport_fees_instructions", "Failed can_check_out fees: {fees:?} for dest: {dest:?} with error: {e:?}");
Error::<T>::CannotCheckOutTeleport
})?;
// safe to do this here, we're in a transactional call that will be reverted on any
// errors down the line
<T::XcmExecutor as XcmAssetTransfers>::AssetTransactor::check_out(
Expand Down Expand Up @@ -2163,7 +2190,10 @@ impl<T: Config> Pallet<T> {
let mut reanchored_assets = assets.clone();
reanchored_assets
.reanchor(&dest, &context)
.map_err(|_| Error::<T>::CannotReanchor)?;
.map_err(|e| {
log::error!(target: "xcm::pallet_xcm::teleport_assets_program", "Failed to re-anchor asset: {e:?} for dest: {dest:?} and context {context:?}");
Error::<T>::CannotReanchor
})?;

// XcmContext irrelevant in teleports checks
let dummy_context =
Expand All @@ -2178,7 +2208,10 @@ impl<T: Config> Pallet<T> {
asset,
&dummy_context,
)
.map_err(|_| Error::<T>::CannotCheckOutTeleport)?;
.map_err(|e| {
log::error!(target: "xcm::pallet_xcm::teleport_assets_program", "Failed can_check_out asset: {asset:?} for dest: {dest:?} with error: {e:?}");
Error::<T>::CannotCheckOutTeleport
})?;
}
for asset in assets.inner() {
// safe to do this here, we're in a transactional call that will be reverted on any
Expand Down Expand Up @@ -2451,7 +2484,10 @@ impl<T: Config> Pallet<T> {
log::debug!(target: "xcm::send_xcm", "dest: {:?}, message: {:?}", &dest, &message);
let (ticket, price) = validate_send::<T::XcmRouter>(dest, message)?;
if let Some(fee_payer) = maybe_fee_payer {
Self::charge_fees(fee_payer, price).map_err(|_| SendError::Fees)?;
Self::charge_fees(fee_payer, price).map_err(|e| {
log::error!(target: "xcm::pallet_xcm::send_xcm", "Charging fees failed: {:?}", e);
SendError::Fees
})?;
}
T::XcmRouter::deliver(ticket)
}
Expand Down Expand Up @@ -2561,7 +2597,10 @@ impl<T: Config> Pallet<T> {

pub fn query_xcm_weight(message: VersionedXcm<()>) -> Result<Weight, XcmPaymentApiError> {
let message = Xcm::<()>::try_from(message)
.map_err(|_| XcmPaymentApiError::VersionedConversionFailed)?;
.map_err(|e| {
log::error!(target: "xcm::pallet_xcm::query_xcm_weight", "Failed to convert versioned `message`: {:?}", e);
XcmPaymentApiError::VersionedConversionFailed
})?;

T::Weigher::weight(&mut message.into()).map_err(|()| {
log::error!(target: "xcm::pallet_xcm::query_xcm_weight", "Error when querying XCM weight");
Expand All @@ -2577,10 +2616,16 @@ impl<T: Config> Pallet<T> {

let destination = destination
.try_into()
.map_err(|_| XcmPaymentApiError::VersionedConversionFailed)?;
.map_err(|e| {
log::error!(target: "xcm::pallet_xcm::query_delivery_fees", "Failed to convert versioned `destination`: {:?}", e);
XcmPaymentApiError::VersionedConversionFailed
})?;

let message =
message.try_into().map_err(|_| XcmPaymentApiError::VersionedConversionFailed)?;
message.try_into().map_err(|e| {
log::error!(target: "xcm::pallet_xcm::query_delivery_fees", "Failed to convert versioned `message`: {:?}", e);
XcmPaymentApiError::VersionedConversionFailed
})?;

let (_, fees) = validate_send::<T::XcmRouter>(destination, message).map_err(|error| {
log::error!(target: "xcm::pallet_xcm::query_delivery_fees", "Error when querying delivery fees: {:?}", error);
Expand All @@ -2589,7 +2634,10 @@ impl<T: Config> Pallet<T> {

VersionedAssets::from(fees)
.into_version(result_version)
.map_err(|_| XcmPaymentApiError::VersionedConversionFailed)
.map_err(|e| {
log::error!(target: "xcm::pallet_xcm::query_delivery_fees", "Failed to convert fees into version: {result_version:?} with error: {e:?}");
XcmPaymentApiError::VersionedConversionFailed
})
}

/// Create a new expectation of a query response with the querier being here.
Expand Down
33 changes: 24 additions & 9 deletions polkadot/xcm/xcm-executor/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -508,7 +508,10 @@ impl<Config: config::Config> XcmExecutor<Config> {
let assets_taken_from_holding_to_pay_delivery_fees = self
.holding
.try_take(asset_to_pay_for_fees.clone().into())
.map_err(|_| XcmError::NotHoldingFees)?;
.map_err(|e| {
log::error!(target: "xcm::fees", "Failed to take asset_to_pay_for_fees: {asset_to_pay_for_fees:?} (based on fee: {fee:?}) from holding with error: {e:?}");
XcmError::NotHoldingFees
})?;
tracing::trace!(target: "xcm::fees", ?assets_taken_from_holding_to_pay_delivery_fees);
let mut iter = assets_taken_from_holding_to_pay_delivery_fees.fungible_assets_iter();
let asset = iter.next().ok_or(XcmError::NotHoldingFees)?;
Expand Down Expand Up @@ -587,8 +590,10 @@ impl<Config: config::Config> XcmExecutor<Config> {
Ok(match local_querier {
None => None,
Some(q) => Some(
q.reanchored(&destination, &Config::UniversalLocation::get())
.map_err(|_| XcmError::ReanchorFailed)?,
q.reanchored(&destination, &Config::UniversalLocation::get()).map_err(|e| {
log::error!(target: "xcm::xcm_executor::to_querier", "Failed to re-anchor local_querier: {e:?} for destination: {destination:?} and context: {:?}", Config::UniversalLocation::get());
XcmError::ReanchorFailed
})?,
),
})
}
Expand Down Expand Up @@ -923,7 +928,10 @@ impl<Config: config::Config> XcmExecutor<Config> {
.as_mut()
.ok_or(XcmError::BadOrigin)?
.append_with(who)
.map_err(|_| XcmError::LocationFull),
.map_err(|e| {
log::error!(target: "xcm::process_instruction::descend_origin", "Failed to append junctions: {e:?}");
XcmError::LocationFull
}),
ClearOrigin => {
self.context.origin = None;
Ok(())
Expand Down Expand Up @@ -1087,7 +1095,10 @@ impl<Config: config::Config> XcmExecutor<Config> {
tracing::trace!(target: "xcm::executor::BuyExecution", asset_used_for_fees = ?self.asset_used_for_fees);
// pay for `weight` using up to `fees` of the holding register.
let max_fee =
self.holding.try_take(fees.into()).map_err(|_| XcmError::NotHoldingFees)?;
self.holding.try_take(fees.clone().into()).map_err(|e| {
log::error!(target: "xcm::process_instruction::buy_execution", "Failed to take fees {fees:?} from holding with error: {e:?}");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ayevbeosa can you please:

  • change all log::error to tracing::error format for xcm-executor
  • check existing logs, e.g. this one can be extended with more info tracing::error!(target: "xcm::reanchor", ?error, "Failed reanchoring with error");

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will get on it

XcmError::NotHoldingFees
})?;
let result = || -> Result<(), XcmError> {
let unspent = self.trader.buy_weight(weight, max_fee, &self.context)?;
self.holding.subsume_assets(unspent);
Expand Down Expand Up @@ -1150,7 +1161,10 @@ impl<Config: config::Config> XcmExecutor<Config> {
Ok(())
},
ExpectAsset(assets) =>
self.holding.ensure_contains(&assets).map_err(|_| XcmError::ExpectationFalse),
self.holding.ensure_contains(&assets).map_err(|e| {
log::error!(target: "xcm::process_instruction::expect_asset", "Failed ensure_contains for assets: {assets:?} with error: {e:?}");
XcmError::ExpectationFalse
}),
ExpectOrigin(origin) => {
ensure!(self.context.origin == origin, XcmError::ExpectationFalse);
Ok(())
Expand Down Expand Up @@ -1272,9 +1286,10 @@ impl<Config: config::Config> XcmExecutor<Config> {
let (remote_asset, context) = Self::try_reanchor(asset.clone(), &unlocker)?;
let lock_ticket =
Config::AssetLocker::prepare_lock(unlocker.clone(), asset, origin.clone())?;
let owner = origin
.reanchored(&unlocker, &context)
.map_err(|_| XcmError::ReanchorFailed)?;
let owner = origin.reanchored(&unlocker, &context).map_err(|e| {
log::error!(target: "xcm::xcm_executor::process_instruction", "Failed to re-anchor for unlocker: {unlocker:?} and context: {context:?}");
bkontur marked this conversation as resolved.
Show resolved Hide resolved
XcmError::ReanchorFailed
})?;
let msg = Xcm::<()>(vec![NoteUnlockable { asset: remote_asset, owner }]);
let (ticket, price) = validate_send::<Config::XcmSender>(unlocker, msg)?;
self.take_fee(price, FeeReason::LockAsset)?;
Expand Down
Loading