Rename original_origin and origin in XcmExecutor #7136
Conversation
tonyalaribe
added
A0-please_review
|
I like the name. Some in code comments explain them will be better. |
xcm/xcm-executor/src/lib.rs
Outdated
| pub fn new(origin: impl Into<MultiLocation>, message_hash: XcmHash) -> Self { | ||
| let origin = origin.into(); | ||
| pub fn new(computed_origin: impl Into<MultiLocation>, message_hash: XcmHash) -> Self { | ||
| let computed_origin = computed_origin.into(); |
There was a problem hiding this comment.
This is not the computed origin -- whatever that gets sent in is definitely the physical origin.
There was a problem hiding this comment.
makes sense. I've updated it
xcm/src/v3/mod.rs
Outdated
| @@ -340,7 +340,9 @@ impl TryFrom<OldWeightLimit> for WeightLimit { | |||
| #[derive(Clone, Eq, PartialEq, Encode, Decode, Debug)] | |||
| pub struct XcmContext { | |||
| /// The `MultiLocation` origin of the corresponding XCM. | |||
| pub origin: Option<MultiLocation>, | |||
| /// The origin is could point to physical locations (Chains, etc), | |||
| /// but also conceptual locations such as Accounts or Pallet. | |||
There was a problem hiding this comment.
This is not what a computed origin is: check the docs here for info:
polkadot/xcm/xcm-builder/src/barriers.rs
Lines 115 to 118 in 254c521
There was a problem hiding this comment.
I'm not sure how to put it in words as a definition. Do you have any suggestions?
xcm/xcm-executor/src/lib.rs
Outdated
| @@ -191,21 +192,21 @@ impl<Config: config::Config> ExecuteXcm<Config::RuntimeCall> for XcmExecutor<Con | |||
| } | |||
| } | |||
| fn execute( | |||
| origin: impl Into<MultiLocation>, | |||
| computed_origin: impl Into<MultiLocation>, | |||
There was a problem hiding this comment.
Likewise, this surely isn't a computed origin that gets sent in. You can only get a computed origin as a result of an XCM execution, not when it is set up.
There was a problem hiding this comment.
I changed it back to origin
There was a problem hiding this comment.
I don't think that's correct either -- this is the physical origin that gets sent in. Calling it just origin naturally makes people question whether it's computed or physical, and in this case, we know it's physical for sure.
There was a problem hiding this comment.
I've changed it to physical_origin, but I also renamed origin to physical_origin in execute_xcm and execute_xcm_in_credit. Is that ok?
Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>
|
The CI pipeline was cancelled due to failure one of the required jobs. |
|
I'm not sure the new names are actually accurate. The origin with which the executor is initialised is obviously the Original Origin. The value in the Origin register may get mutated but I don't see why this wouldn't still be called the Origin. I think unfortunately people are not realising that the XCVM operates at a different abstraction level to that of the *MP routing system. The XCVM does not know or care about transport systems or the difference between "physical" and "computed" origins. |
Following I would close this PR, since the renamings don't make thinks as clear as we imagined, and muddles XCM and XCMP concepts. |
These are semantic changes for the variables to have clearer meanings.
original_originwas renamed tophysical_originsince it should point to the actual chain.originwas renamed tocomputed_originin some cases since those origins could be more conceptual and might point to Accounts, or Pallets or other entities. And not necessary the chain executing the XCM instructions.