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.

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

Add validate_xcm_nesting to the ParentAsUmp and ChildParachainRouter #4236

Merged
merged 10 commits into from
Apr 23, 2024

Conversation

bkontur
Copy link
Contributor

@bkontur bkontur commented Apr 22, 2024

This PR:

  • moves validate_xcm_nesting from XcmpQueue into the VersionedXcm
  • adds validate_xcm_nesting to the ParentAsUmp
  • adds validate_xcm_nesting to the ChildParachainRouter

Based on discussion here and/or here and/or here

Question/TODO

  • To the comment - Why was validate_xcm_nesting added just to the XcmpQueue router and nowhere else? What kind of problem MAX_XCM_DECODE_DEPTH is solving? (see comment)

@bkontur bkontur added R0-silent Changes should not be mentioned in any release notes T6-XCM This PR/Issue is related to XCM. labels Apr 22, 2024
@bkontur bkontur requested a review from a team as a code owner April 22, 2024 09:03
@bkontur bkontur self-assigned this Apr 22, 2024
@acatangiu
Copy link
Contributor

moves validate_xcm_nesting from XcmpQueue into the VersionedXcm

this is not accurate anymore, it is also done inside XcmpQueue validations

Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

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

This is good!

@bkontur bkontur enabled auto-merge April 22, 2024 15:17
@bkontur bkontur disabled auto-merge April 22, 2024 15:18
@bkontur bkontur enabled auto-merge April 22, 2024 15:43
@bkontur bkontur added this pull request to the merge queue Apr 23, 2024
Merged via the queue into master with commit 7f1646e Apr 23, 2024
133 of 138 checks passed
@bkontur bkontur deleted the bko-validate-xcm-nesting branch April 23, 2024 09:07
github-merge-queue bot pushed a commit that referenced this pull request Apr 23, 2024
…Xcm` router (for testing purposes) (#4186)

This PR:
- adds `EnsureDecodableXcm` (testing) router that attempts to *encode*
and *decode* passed XCM `message` to ensure that the receiving side will
be able to decode, at least with the same XCM version.
- fixes `pallet_xcm` / `pallet_xcm_benchmarks` assets data generation

Relates to investigation of
https://substrate.stackexchange.com/questions/11288 and missing fix
#2129 which did not get
into the fellows 1.1.X release.

## Questions/TODOs

- [x] fix XCM benchmarks, which produces undecodable data - new router
catched at least two cases
  - `BoundedVec exceeds its limit`
  - `Fungible asset of zero amount is not allowed`  
- [x] do we need to add `sort` to the `prepend_with` as we did for
reanchor [here](#2129)?
@serban300 (**created separate/follow-up PR**:
#4235)
- [x] We added decoding check to `XcmpQueue` -> `validate_xcm_nesting`,
why not to added to the `ParentAsUmp` or `ChildParachainRouter`?
@franciscoaguirre (**created separate/follow-up PR**:
#4236)
- [ ] `SendController::send_blob` replace `VersionedXcm::<()>::decode(`
with `VersionedXcm::<()>::decode_with_depth_limit(MAX_XCM_DECODE_DEPTH,
data)` ?

---------

Co-authored-by: Adrian Catangiu <adrian@parity.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes T6-XCM This PR/Issue is related to XCM.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants