Skip to content

fix: Resolve compile errors with new nested error structure #8561

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

Merged
raymondkfcheung merged 12 commits into from
May 20, 2025
Merged

fix: Resolve compile errors with new nested error structure #8561

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
a51c241
Revert "Add tracing"
raymondkfcheung May 19, 2025
e919ab8
Add index
raymondkfcheung May 19, 2025
f047394
Update PRDoc
raymondkfcheung May 19, 2025
956635b
Fix bugs
raymondkfcheung May 19, 2025
057c1fd
Update PRDoc
raymondkfcheung May 19, 2025
7afad5e
Fix bugs
raymondkfcheung May 19, 2025
90fe1d6
Fix bugs
raymondkfcheung May 19, 2025
b862752
Update PRDoc
raymondkfcheung May 19, 2025
0661d3a
Fix tests
raymondkfcheung May 19, 2025
e2a1688
Fix tests
raymondkfcheung May 19, 2025
d91a856
Update PRDoc
raymondkfcheung May 19, 2025
51ab46e
Fix tests
raymondkfcheung May 19, 2025
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
4 changes: 2 additions & 2 deletions bridges/snowbridge/runtime/test-common/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ pub fn send_unpaid_transfer_token_message<Runtime, XcmConfig>(
Weight::zero(),
);
// check error is barrier
assert_err!(outcome.ensure_complete(), XcmError::Barrier);
assert_err!(outcome.ensure_complete(), (0, XcmError::Barrier));
});
}

Expand Down Expand Up @@ -423,7 +423,7 @@ pub fn send_transfer_token_message_failure<Runtime, XcmConfig>(
destination_address,
fee_amount,
);
assert_err!(outcome.ensure_complete(), expected_error);
assert_err!(outcome.ensure_complete(), (0, expected_error));
});
}

Expand Down
6 changes: 3 additions & 3 deletions cumulus/parachains/integration-tests/emulated/common/src/impls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ macro_rules! impl_assert_events_helpers_for_relay_chain {
vec![
// Dispatchable is properly executed and XCM message sent
[<$chain RuntimeEvent>]::<N>::XcmPallet(
$crate::impls::pallet_xcm::Event::Attempted { outcome: $crate::impls::Outcome::Incomplete { used: weight, error } }
$crate::impls::pallet_xcm::Event::Attempted { outcome: $crate::impls::Outcome::Incomplete { used: weight, error, .. } }
) => {
weight: $crate::impls::weight_within_threshold(
($crate::impls::REF_TIME_THRESHOLD, $crate::impls::PROOF_SIZE_THRESHOLD),
Expand Down Expand Up @@ -471,7 +471,7 @@ macro_rules! impl_assert_events_helpers_for_parachain {
vec![
// Dispatchable is properly executed and XCM message sent
[<$chain RuntimeEvent>]::<N>::PolkadotXcm(
$crate::impls::pallet_xcm::Event::Attempted { outcome: $crate::impls::Outcome::Incomplete { used: weight, error } }
$crate::impls::pallet_xcm::Event::Attempted { outcome: $crate::impls::Outcome::Incomplete { used: weight, error, .. } }
) => {
weight: $crate::impls::weight_within_threshold(
($crate::impls::REF_TIME_THRESHOLD, $crate::impls::PROOF_SIZE_THRESHOLD),
Expand All @@ -491,7 +491,7 @@ macro_rules! impl_assert_events_helpers_for_parachain {
vec![
// Execution fails in the origin with `Barrier`
[<$chain RuntimeEvent>]::<N>::PolkadotXcm(
$crate::impls::pallet_xcm::Event::Attempted { outcome: $crate::impls::Outcome::Error { error } }
$crate::impls::pallet_xcm::Event::Attempted { outcome: $crate::impls::Outcome::Error { error, .. } }
) => {
error: *error == expected_error.unwrap_or((*error).into()).into(),
},
Expand Down
5 changes: 4 additions & 1 deletion ...ation-tests/emulated/tests/bridges/bridge-hub-westend/src/tests/snowbridge_v2_outbound.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -883,7 +883,10 @@ fn export_message_from_asset_hub_to_ethereum_is_banned_when_set_operating_mode()
bx!(xcm),
Weight::from(EXECUTION_WEIGHT),
),
pallet_xcm::Error::<Runtime>::LocalExecutionIncompleteWithError
pallet_xcm::Error::<Runtime>::LocalExecutionIncompleteWithError {
error: XcmError::NoDeal.into(),
index: 0
}
);
});
}
2 changes: 1 addition & 1 deletion cumulus/parachains/runtimes/test-utils/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,7 +475,7 @@ impl<
pub fn execute_as_governance_call<Call: Dispatchable + Encode>(
call: Call,
governance_origin: GovernanceOrigin<Call::RuntimeOrigin>,
) -> Result<(), Either<DispatchError, XcmError>> {
) -> Result<(), Either<DispatchError, (u8, XcmError)>> {
// execute xcm as governance would send
let execute_xcm = |call: Call, governance_location, descend_origin| {
// prepare xcm
Expand Down
2 changes: 1 addition & 1 deletion cumulus/parachains/runtimes/test-utils/src/test_cases.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ where
/// `frame_system::Call::authorize_upgrade` from governance system.
pub fn can_governance_authorize_upgrade<Runtime, RuntimeOrigin>(
governance_origin: GovernanceOrigin<RuntimeOrigin>,
) -> Result<(), Either<DispatchError, XcmError>>
) -> Result<(), Either<DispatchError, (u8, XcmError)>>
where
Runtime: BasicParachainRuntime
+ frame_system::Config<RuntimeOrigin = RuntimeOrigin, AccountId = AccountId>,
Expand Down
22 changes: 9 additions & 13 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 @@ -365,13 +365,12 @@ pub mod pallet {

Self::deposit_event(Event::Attempted { outcome: outcome.clone() });
let weight_used = outcome.weight_used();
outcome.ensure_complete().map_err(|error| {
outcome.ensure_complete().map_err(|(index, error)| {
tracing::error!(target: "xcm::pallet_xcm::execute", ?error, "XCM execution failed with error");
Error::<T>::LocalExecutionIncompleteWithError(error.into()).with_weight(
weight_used.saturating_add(
Error::<T>::LocalExecutionIncompleteWithError { error: error.into(), index }
.with_weight(weight_used.saturating_add(
<Self::WeightInfo as ExecuteControllerWeightInfo>::execute(),
),
)
))
})?;
Ok(weight_used)
}
Expand Down Expand Up @@ -686,10 +685,7 @@ pub mod pallet {
AliasNotFound,
/// Local XCM execution incomplete with error.
#[codec(index = 28)]
LocalExecutionIncompleteWithError {
error: ExecutionError,
index: u8,
},
LocalExecutionIncompleteWithError { error: ExecutionError, index: u8 },
}

impl<T: Config> From<SendError> for Error<T> {
Expand Down Expand Up @@ -1455,9 +1451,9 @@ pub mod pallet {
weight,
weight,
);
outcome.ensure_complete().map_err(|error| {
outcome.ensure_complete().map_err(|(index, error)| {
tracing::error!(target: "xcm::pallet_xcm::claim_assets", ?error, "XCM execution failed with error");
Error::<T>::LocalExecutionIncompleteWithError(error.into())
Error::<T>::LocalExecutionIncompleteWithError {index, error: error.into()}
})?;
Ok(())
}
Expand Down Expand Up @@ -2087,12 +2083,12 @@ impl<T: Config> Pallet<T> {
weight,
);
Self::deposit_event(Event::Attempted { outcome: outcome.clone() });
outcome.clone().ensure_complete().map_err(|error| {
outcome.clone().ensure_complete().map_err(|(index, error)| {
tracing::error!(
target: "xcm::pallet_xcm::execute_xcm_transfer",
?error, "XCM execution failed with error with outcome: {:?}", outcome
);
Error::<T>::LocalExecutionIncompleteWithError(error.into())
Error::<T>::LocalExecutionIncompleteWithError { error: error.into(), index }
})?;

if let Some(remote_xcm) = remote_xcm {
Expand Down
12 changes: 8 additions & 4 deletions polkadot/xcm/pallet-xcm/src/tests/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -656,7 +656,10 @@ fn trapped_assets_can_be_claimed() {
]))),
weight
),
Error::<Test>::LocalExecutionIncompleteWithError(XcmError::UnknownClaim.into())
Error::<Test>::LocalExecutionIncompleteWithError {
index: 0,
error: XcmError::UnknownClaim.into()
}
);
});
}
Expand Down Expand Up @@ -753,9 +756,10 @@ fn incomplete_execute_reverts_side_effects() {
pays_fee: frame_support::dispatch::Pays::Yes,
},
error: sp_runtime::DispatchError::from(
Error::<Test>::LocalExecutionIncompleteWithError(
XcmError::FailedToTransactAsset("").into()
)
Error::<Test>::LocalExecutionIncompleteWithError {
index: 3,
error: XcmError::FailedToTransactAsset("").into()
}
)
})
);
Expand Down
12 changes: 6 additions & 6 deletions polkadot/xcm/src/v5/traits.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,11 +252,11 @@ pub enum Outcome {
}

impl Outcome {
pub fn ensure_complete(self) -> Result<(), (u8, Error)> {
pub fn ensure_complete(self) -> result::Result<(), (u8, Error)> {
Copy link
Copy Markdown
Contributor Author

@raymondkfcheung raymondkfcheung May 19, 2025

Choose a reason for hiding this comment

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

@franciscoaguirre Please let me know if you'd prefer to replace (u8, Error) with a named struct like ExecutionFailure (as shown below) in this PR, or if you'd rather have it done in a follow-up PR:

pub struct ExecutionFailure {
    pub error: Error,
    pub index: u8,
}

Copy link
Copy Markdown
Contributor

@franciscoaguirre franciscoaguirre May 19, 2025

Choose a reason for hiding this comment

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

I think a named struct is clearer. The only problem is we have so many structs named very similar and we'd have to import it everywhere. We could reuse the ExecutorError but the weight field wouldn't be useful for us here.

I would still use a named struct. It provides a much better API and the Outcome of an XCM is a somewhat public API

Copy link
Copy Markdown
Contributor Author

@raymondkfcheung raymondkfcheung May 20, 2025

Choose a reason for hiding this comment

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

Yes, I agree. There are quite a few similarly named structs, which can be a bit confusing, but a named struct does improve readability and long-term maintainability. I’ve merged the supporting change into your feature branch. Since the current PR still uses (u8, Error), I’ll raise a follow-up PR to introduce ExecutionFailure instead. We can always revisit the name later if we think of something more fitting.

Copy link
Copy Markdown
Contributor Author

@raymondkfcheung raymondkfcheung May 20, 2025

Choose a reason for hiding this comment

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

Maybe InstructionError would be a better name. It feels more focused since the struct is really about capturing an error at a specific instruction index.

match self {
Outcome::Complete { .. } => Ok(()),
Outcome::Incomplete { error, index, .. } => Err(error),
Outcome::Error { error, index, .. } => Err(error),
Outcome::Incomplete { error, index, .. } => Err((index, error)),
Outcome::Error { error, index, .. } => Err((index, error)),
}
}
pub fn ensure_execution(self) -> result::Result<Weight, Error> {
Expand All @@ -278,7 +278,7 @@ impl Outcome {

impl From<Error> for Outcome {
fn from(error: Error) -> Self {
Self::Error { error }
Self::Error { error, index: 0 }
}
}

Expand All @@ -305,11 +305,11 @@ pub trait ExecuteXcm<Call> {
) -> Outcome {
let pre = match Self::prepare(message) {
Ok(x) => x,
Err(_) => return Outcome::Error { error: Error::WeightNotComputable },
Err(_) => return Outcome::Error { error: Error::WeightNotComputable, index: 0 },
};
let xcm_weight = pre.weight_of();
if xcm_weight.any_gt(weight_limit) {
return Outcome::Error { error: Error::WeightLimitReached(xcm_weight) }
return Outcome::Error { error: Error::WeightLimitReached(xcm_weight), index: 0 }
}
Self::execute(origin, pre, id, weight_credit)
}
Expand Down
13 changes: 9 additions & 4 deletions polkadot/xcm/xcm-builder/src/process_xcm_message.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,18 +97,23 @@ impl<
);
(used, Ok(true))
},
Outcome::Incomplete { used, error } => {
Outcome::Incomplete { used, error, index } => {
tracing::trace!(
target: LOG_TARGET,
"XCM message execution incomplete, used weight: {used}, error: {error:?}",
?error,
?index,
?used,
"XCM message execution incomplete, used weight",
);
(used, Ok(false))
},
// In the error-case we assume the worst case and consume all possible weight.
Outcome::Error { error } => {
Outcome::Error { error, index } => {
tracing::trace!(
target: LOG_TARGET,
"XCM message execution error: {error:?}",
?error,
?index,
"XCM message execution error",
);
let error = match error {
xcm::latest::Error::ExceedsStackLimit => ProcessMessageError::StackLimitReached,
Expand Down
6 changes: 5 additions & 1 deletion polkadot/xcm/xcm-builder/src/tests/aliases.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,11 @@ fn alias_origin_should_work() {
);
assert_eq!(
r,
Outcome::Incomplete { used: Weight::from_parts(10, 10), error: XcmError::NoPermission }
Outcome::Incomplete {
used: Weight::from_parts(10, 10),
error: XcmError::NoPermission,
index: 0
}
);

let r = XcmExecutor::<TestConfig>::prepare_and_execute(
Expand Down
35 changes: 27 additions & 8 deletions polkadot/xcm/xcm-builder/src/tests/assets.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ fn exchange_asset_should_fail_when_no_deal_possible() {
);
assert_eq!(
r,
Outcome::Incomplete { used: Weight::from_parts(40, 40), error: XcmError::NoDeal }
Outcome::Incomplete { used: Weight::from_parts(40, 40), error: XcmError::NoDeal, index: 0 }
);
assert_eq!(asset_list(Parent), vec![(Parent, 1000u128).into()]);
assert_eq!(exchange_assets(), vec![(Here, 100u128).into()].into());
Expand Down Expand Up @@ -286,7 +286,11 @@ fn basic_asset_trap_should_work() {
);
assert_eq!(
r,
Outcome::Incomplete { used: Weight::from_parts(10, 10), error: XcmError::UnknownClaim }
Outcome::Incomplete {
used: Weight::from_parts(10, 10),
error: XcmError::UnknownClaim,
index: 0
}
);
assert_eq!(asset_list(Parachain(1)), vec![(Here, 900u128).into()]);
assert_eq!(asset_list(AccountIndex64 { index: 3, network: None }), vec![]);
Expand All @@ -311,7 +315,11 @@ fn basic_asset_trap_should_work() {
);
assert_eq!(
r,
Outcome::Incomplete { used: Weight::from_parts(10, 10), error: XcmError::UnknownClaim }
Outcome::Incomplete {
used: Weight::from_parts(10, 10),
error: XcmError::UnknownClaim,
index: 0
}
);
assert_eq!(asset_list(Parachain(1)), vec![(Here, 900u128).into()]);
assert_eq!(asset_list(AccountIndex64 { index: 3, network: None }), vec![]);
Expand All @@ -336,7 +344,11 @@ fn basic_asset_trap_should_work() {
);
assert_eq!(
r,
Outcome::Incomplete { used: Weight::from_parts(10, 10), error: XcmError::UnknownClaim }
Outcome::Incomplete {
used: Weight::from_parts(10, 10),
error: XcmError::UnknownClaim,
index: 0
}
);
assert_eq!(asset_list(Parachain(1)), vec![(Here, 900u128).into()]);
assert_eq!(asset_list(AccountIndex64 { index: 3, network: None }), vec![]);
Expand Down Expand Up @@ -382,7 +394,11 @@ fn basic_asset_trap_should_work() {
);
assert_eq!(
r,
Outcome::Incomplete { used: Weight::from_parts(10, 10), error: XcmError::UnknownClaim }
Outcome::Incomplete {
used: Weight::from_parts(10, 10),
error: XcmError::UnknownClaim,
index: 0
}
);
}

Expand Down Expand Up @@ -446,7 +462,8 @@ fn max_assets_limit_should_work() {
r,
Outcome::Incomplete {
used: Weight::from_parts(95, 95),
error: XcmError::HoldingWouldOverflow
error: XcmError::HoldingWouldOverflow,
index: 0
}
);

Expand Down Expand Up @@ -503,7 +520,8 @@ fn max_assets_limit_should_work() {
r,
Outcome::Incomplete {
used: Weight::from_parts(95, 95),
error: XcmError::HoldingWouldOverflow
error: XcmError::HoldingWouldOverflow,
index: 0
}
);

Expand Down Expand Up @@ -533,7 +551,8 @@ fn max_assets_limit_should_work() {
r,
Outcome::Incomplete {
used: Weight::from_parts(25, 25),
error: XcmError::HoldingWouldOverflow
error: XcmError::HoldingWouldOverflow,
index: 0
}
);
}
6 changes: 5 additions & 1 deletion polkadot/xcm/xcm-builder/src/tests/bridging/universal_exports.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,11 @@ fn sovereign_paid_remote_exporter_produces_xcm_which_does_not_trap_assets() {
);
assert_eq!(
r,
Outcome::Incomplete { used: Weight::from_parts(50, 50), error: XcmError::Unroutable }
Outcome::Incomplete {
used: Weight::from_parts(50, 50),
error: XcmError::Unroutable,
index: 0
}
);
// check empty trapped assets
assert!(TrappedAssets::get().is_empty());
Expand Down
Loading
Loading