Skip to content

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

Merged
raymondkfcheung merged 12 commits intoparitytech:nest-inner-xcm-errorsfrom
raymondkfcheung:ray-fix-nested-errors-bugs
May 20, 2025
Merged

fix: Resolve compile errors with new nested error structure#8561
raymondkfcheung merged 12 commits intoparitytech:nest-inner-xcm-errorsfrom
raymondkfcheung:ray-fix-nested-errors-bugs

Conversation

@raymondkfcheung
Copy link
Copy Markdown
Contributor

This PR fixes compile errors introduced in #7730, which adds nested error reporting and instruction index tracking to pallet-xcm, xcm-executor, and related components.

Specifically:

  • Updates all usage sites to match the new nested structure.
  • Ensures proper propagation of the index field in Outcome::Error and Outcome::Incomplete.
  • Adjusts method signatures and error handling to reflect the new error format.

This change helps improve XCM observability and debugging by explicitly identifying the failing instruction in error contexts.

@raymondkfcheung raymondkfcheung requested a review from a team as a code owner May 19, 2025 12:11
@raymondkfcheung raymondkfcheung self-assigned this May 19, 2025
@raymondkfcheung raymondkfcheung added the T6-XCM This PR/Issue is related to XCM. label May 19, 2025
@raymondkfcheung raymondkfcheung moved this to In-Review in Bridges + XCM May 19, 2025
Err(message)
},
Err(_) => Err(message),
}
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.

Reverted it, in favour to #8549


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

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

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

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

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.


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

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

@franciscoaguirre
Copy link
Copy Markdown
Contributor

Thanks for doing this!

@raymondkfcheung raymondkfcheung merged commit c2609f1 into paritytech:nest-inner-xcm-errors May 20, 2025
225 of 250 checks passed
@github-project-automation github-project-automation bot moved this from In-Review to Done in Bridges + XCM May 20, 2025
@raymondkfcheung raymondkfcheung deleted the ray-fix-nested-errors-bugs branch May 20, 2025 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T6-XCM This PR/Issue is related to XCM.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants