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

Introduce XCM matcher for writing barriers #6756

Merged
merged 12 commits into from
Mar 4, 2023

Conversation

KiChjang
Copy link
Contributor

Resolves #4276.

Introduces a new API fn matchers() for &mut [Instructions<Call>] that is similar to iterators: it can advance the current instruction to the next while ensuring the current instruction matches/fulfills a certain condition; it can assert the remaining number of instructions left to process; and it can also skip instructions while a condition is being evaluated as true.

The main purpose of creating a Matcher API instead of using iterators is that of purpose and clarity -- it is much easier to maintain, audit and write matchers over iterators, as they are intended to be declarative in nature.

@KiChjang KiChjang added 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. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Feb 21, 2023
@kianenigma
Copy link
Contributor

This could potentially be b1-note-worthy, if we expect teams to rewrite their barriers with it.

@KiChjang KiChjang added B1-note_worthy Changes should be noted in the release notes and removed B0-silent Changes should not be mentioned in any release notes labels Feb 22, 2023
@KiChjang KiChjang added the T6-XCM This PR/Issue is related to XCM. label Feb 22, 2023
@KiChjang KiChjang added T1-runtime This PR/Issue is related to the topic “runtime”. and removed T6-XCM This PR/Issue is related to XCM. labels Feb 22, 2023
@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/new-matcher-api-facilitating-the-building-of-xcm-barriers/2126/1

Comment on lines +255 to +256
})?;
Ok(())
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
})?;
Ok(())
})

Copy link
Contributor Author

@KiChjang KiChjang Feb 24, 2023

Choose a reason for hiding this comment

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

Suggested change
})?;
Ok(())
})
.map(|_| ())

But this is why I opted for the Ok(()) at the end instead -- because the Ok type is not () but rather Self.

fn match_next_inst<F>(self, f: F) -> Result<Self, Self::Error>
where
Self: Sized,
F: FnMut(&mut Self::Inst) -> Result<(), Self::Error>;
Copy link
Member

Choose a reason for hiding this comment

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

Why is this whole API exclusively tailored to mutable Instruction slices?
That seems to limit the environments that it can be used in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because you may want to mutate the instruction whilst iterating through it -- this is how the AllowUnpaidExecutionFrom barrier works.

Copy link
Member

Choose a reason for hiding this comment

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

But can we then not have two flavours? One for mut and one normal? Or do you think its not worth it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not entirely sure how it is limited tbh, we mutably borrow instructions one at a time, so unless there is another (mutable) borrow of the same instruction somewhere else, it shouldn't restrict the places in which you can use this API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In addition, the Matcher struct already mutably borrows the entire slice, so the mutable borrow of the instruction here isn't really doing anything worse than that; instead, it's narrowing the scope of the mutable borrow to a single instruction.

xcm/src/lib.rs Show resolved Hide resolved
xcm/src/v3/matcher.rs Outdated Show resolved Hide resolved
xcm/src/v3/matcher.rs Show resolved Hide resolved
@KiChjang
Copy link
Contributor Author

KiChjang commented Mar 4, 2023

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 962bc21 into master Mar 4, 2023
@paritytech-processbot paritytech-processbot bot deleted the kckyeung/xcm-pattern-matcher branch March 4, 2023 05:37
ordian added a commit that referenced this pull request Mar 7, 2023
* master: (27 commits)
  bump `zombienet` version to v1.3.37 (#6773)
  Bump `blake2b_simd` to 1.0.1 (#6829)
  changelog: update template for new label behavior (E3/E4) (#6804)
  Companion for paritytech/substrate#12828 (#6380)
  Don't send `ActiveLeaves` from leaves in db on startup in Overseer (#6727)
  Polkadot XCM Body constants (#6788)
  Decrease expected peer count in zombinenet tests (#6826)
  Additional tracing in `provisioner`, `vote_selection` and `dispute-coordinator` (#6775)
  Change node-key for bootnodes (#6772)
  Change handle_import_statements to FatalResult (#6820)
  Introduce XCM matcher for writing barriers (#6756)
  Freeze note on `SessionInfo`. (#6818)
  Bump parity-db (#6816)
  Removing Outdated References to Misbehavior Arbitration Subsystem (#6814)
  Forgotten re-export for `MatchedConvertedConcreteId` (#6815)
  Companion for substrate#13509: bump API versions of {Beefy,Mmr}Api (#6809)
  Migrate to `Weight::from_parts` (#6794)
  [XCM] Multiple `FungiblesAdapter`s support + `WeightTrader::buy_weight` more accurate error (#6739)
  Get rid of unnecessary cloning and work. (#6808)
  changelog: fix migration listing (#6806)
  ...
ordian added a commit that referenced this pull request Mar 7, 2023
* master: (27 commits)
  bump `zombienet` version to v1.3.37 (#6773)
  Bump `blake2b_simd` to 1.0.1 (#6829)
  changelog: update template for new label behavior (E3/E4) (#6804)
  Companion for paritytech/substrate#12828 (#6380)
  Don't send `ActiveLeaves` from leaves in db on startup in Overseer (#6727)
  Polkadot XCM Body constants (#6788)
  Decrease expected peer count in zombinenet tests (#6826)
  Additional tracing in `provisioner`, `vote_selection` and `dispute-coordinator` (#6775)
  Change node-key for bootnodes (#6772)
  Change handle_import_statements to FatalResult (#6820)
  Introduce XCM matcher for writing barriers (#6756)
  Freeze note on `SessionInfo`. (#6818)
  Bump parity-db (#6816)
  Removing Outdated References to Misbehavior Arbitration Subsystem (#6814)
  Forgotten re-export for `MatchedConvertedConcreteId` (#6815)
  Companion for substrate#13509: bump API versions of {Beefy,Mmr}Api (#6809)
  Migrate to `Weight::from_parts` (#6794)
  [XCM] Multiple `FungiblesAdapter`s support + `WeightTrader::buy_weight` more accurate error (#6739)
  Get rid of unnecessary cloning and work. (#6808)
  changelog: fix migration listing (#6806)
  ...
@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-release-analysis-v0-9-40/2468/1

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider a declarative API to describe XCM barrier conditions
5 participants