Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

XCM canonicalize + prepend_with fix #3269

Merged
12 commits merged into from
Jul 2, 2021
Merged

XCM canonicalize + prepend_with fix #3269

12 commits merged into from
Jul 2, 2021

Conversation

shawntabrizi
Copy link
Member

@shawntabrizi shawntabrizi commented Jun 16, 2021 •
edited
Loading

This introduces a new function canonicalize which will convert an XCM into its canonical/normalized form.

I would have preferred the name normalize but it seems this is the standard naming in Rust: https://doc.rust-lang.org/std/fs/fn.canonicalize.html

Using this function, we repair a bug with prepend_with, where an assumption about the number of elements we skip when forming the new object was incorrect.

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Jun 16, 2021
@shawntabrizi shawntabrizi added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited. labels Jun 16, 2021
@stze stze added D1-audited 👍 PR contains changes to critical logic that has been properly reviewed and externally audited. and removed D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited. labels Jun 18, 2021
@shawntabrizi shawntabrizi added this to the v0.9.8 milestone Jul 1, 2021
rphmeier
rphmeier approved these changes Jul 2, 2021
View reviewed changes
Copy link
Contributor

@rphmeier rphmeier left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Not sure I completely understand this, but it seems reasonable to minimize/canonicalize paths to ensure that all parent junctions are leading. Maybe XCM v1 should prevent this by differentiation the path up the tree with the path down the tree.

How would this work for non-hierarchical (sibling) paths?

A <-- B
      ^
      |
      v
C <-- D

From B, would something like (Sibling(D), Parent) be 'normalized' to point to A instead of C?

@shawntabrizi
Copy link
Member Author

shawntabrizi commented Jul 2, 2021 •
edited
Loading

@rphmeier Gav proposed that we actually remove the parent junction from XCM all together: #3312

Not sure if we want to put this in XCM v1 and have a breaking change in the format for everyone, or actually increment to XCM v2.

As for your scenario, I don't think i quite understand the diagram you made. For canonicalization, a parachain should only ever have one parent, and a sibling parachains are only those that share a parent, so Sibling(X), Parent should always resolve back to where we started.

@shawntabrizi @rphmeier
Update xcm/src/v0/multi_location.rs
1e5d9e1 
Co-authored-by: Robert Habermeier <rphmeier@gmail.com>
@shawntabrizi
Copy link
Member Author

bot merge

@ghost
Copy link

ghost commented Jul 2, 2021

Waiting for commit status.

@ghost ghost merged commit e714cc6 into master Jul 2, 2021
@ghost ghost deleted the shawntabrizi-canonicalize-xcm branch July 2, 2021 16:31
@emostov emostov requested review from emostov and removed request for emostov July 3, 2021 18:24
ordian added a commit that referenced this pull request Jul 6, 2021
@ordian
Merge branch 'master' into rh-chain-selection-db-and-stagnant
8feb436 
* master: (33 commits)
  Update all weights, add run_all_benches.sh script (#3400)
  Enable over-bridge-messaging in Rococo/Wococo runtime (#3377)
  paras.rs to FRAME V2 (#3403)
  Add XCM Tracing (#3353)
  Use MaxEncodedLen trait from new parity-scale-codec v2.2 (#3412)
  bump a bunch of deps in parity-common (#3402)
  Warn on low connectivity. (#3408)
  origin to frame v2 (#3405)
  Enable logging in the puppet worker (#3411)
  make it easier to dbg stalls (#3351)
  XCM `canonicalize` + `prepend_with` fix (#3269)
  cleanup stream polls (#3397)
  Staking Miner (#3141)
  Companion for Substrate#8953 (#3140)
  Bump version, specs & substrate in prep for v0.9.8 (#3387)
  Fix busy loops. (#3392)
  Minor refactor (#3386)
  add simnet tests (#3381)
  BEEFY: adjust gossip (#3372)
  Companion for #9193 (#3376)
  ...
s3krit pushed a commit that referenced this pull request Jul 6, 2021
@shawntabrizi @rphmeier @s3krit
XCM canonicalize + prepend_with fix (#3269)
c4cff28 
* canonicalize + prepend_with fix

* fix doc

* not needed

* better docs, and more deterministic logic

* one more test

* Update xcm/src/v0/multi_location.rs

* Update multi_location.rs

Co-Authored-By: parity-processbot <>

* follow style guide

Co-Authored-By: parity-processbot <>

* oops

* Update xcm/src/v0/multi_location.rs

Co-authored-by: Robert Habermeier <rphmeier@gmail.com>

* Update xcm/src/v0/multi_location.rs

Co-authored-by: Robert Habermeier <rphmeier@gmail.com>
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rphmeier rphmeier rphmeier approved these changes

@gavofyork gavofyork Awaiting requested review from gavofyork

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to critical logic that has been properly reviewed and externally audited.
Projects
None yet
Milestone
v0.9.8
Development

Successfully merging this pull request may close these issues.
4 participants
@shawntabrizi @gavofyork @rphmeier @stze