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

Added tests for XCM barriers: AllowSubscriptions, WithUniqueTopic and TrailingSetTopicAsId #3955

Merged
merged 2 commits into from
Apr 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions polkadot/xcm/xcm-builder/src/tests/barriers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -309,3 +309,62 @@ fn suspension_should_work() {
);
assert_eq!(r, Ok(()));
}

#[test]
fn allow_subscriptions_from_should_work() {
// allow only parent
AllowSubsFrom::set(vec![Location::parent()]);

let valid_xcm_1 = Xcm::<TestCall>(vec![SubscribeVersion {
query_id: 42,
max_response_weight: Weight::from_parts(5000, 5000),
}]);
let valid_xcm_2 = Xcm::<TestCall>(vec![UnsubscribeVersion]);
let invalid_xcm_1 = Xcm::<TestCall>(vec![
SetAppendix(Xcm(vec![])),
SubscribeVersion { query_id: 42, max_response_weight: Weight::from_parts(5000, 5000) },
]);
let invalid_xcm_2 = Xcm::<TestCall>(vec![
SubscribeVersion { query_id: 42, max_response_weight: Weight::from_parts(5000, 5000) },
SetTopic([0; 32]),
]);

let test_data = vec![
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Not very important, but personally I would find it easier to follow and I think it would also be more extensible if we just asserted each case separately instead of iterating through a vec. I don't think it would create a lot of duplication.

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 saw this pattern on several other places and also used it elsewhere, where I had much more test data (corner) cases than just 4 here :)

As a part of #3692 I will add a new barrier, so I will come back to this and simplify.

(
valid_xcm_1.clone(),
Parachain(1).into_location(),
// not allowed origin
Err(ProcessMessageError::Unsupported),
),
(valid_xcm_1, Location::parent(), Ok(())),
(
valid_xcm_2.clone(),
Parachain(1).into_location(),
// not allowed origin
Err(ProcessMessageError::Unsupported),
),
(valid_xcm_2, Location::parent(), Ok(())),
(
invalid_xcm_1,
Location::parent(),
// invalid XCM
Err(ProcessMessageError::BadFormat),
),
(
invalid_xcm_2,
Location::parent(),
// invalid XCM
Err(ProcessMessageError::BadFormat),
),
];

for (mut message, origin, expected_result) in test_data {
let r = AllowSubscriptionsFrom::<IsInVec<AllowSubsFrom>>::should_execute(
&origin,
message.inner_mut(),
Weight::from_parts(10, 10),
&mut props(Weight::zero()),
);
assert_eq!(r, expected_result, "Failed for origin: {origin:?} and message: {message:?}");
}
}
1 change: 1 addition & 0 deletions polkadot/xcm/xcm-builder/src/tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ mod locking;
mod origins;
mod pay;
mod querying;
mod routing;
mod transacting;
mod version_subscriptions;
mod weight;
95 changes: 95 additions & 0 deletions polkadot/xcm/xcm-builder/src/tests/routing.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
// Copyright (C) Parity Technologies (UK) Ltd.
// This file is part of Polkadot.

// Polkadot is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.

// Polkadot is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.

// You should have received a copy of the GNU General Public License
// along with Polkadot. If not, see <http://www.gnu.org/licenses/>.

use super::*;
use frame_support::{assert_ok, traits::Everything};
use xcm_executor::traits::Properties;

fn props() -> Properties {
Properties { weight_credit: Weight::zero(), message_id: None }
}

#[test]
fn trailing_set_topic_as_id_with_unique_topic_should_work() {
type AllowSubscriptions = AllowSubscriptionsFrom<Everything>;

// check the validity of XCM for the `AllowSubscriptions` barrier
let valid_xcm = Xcm::<()>(vec![SubscribeVersion {
query_id: 42,
max_response_weight: Weight::from_parts(5000, 5000),
}]);
assert_eq!(
AllowSubscriptions::should_execute(
&Location::parent(),
valid_xcm.clone().inner_mut(),
Weight::from_parts(10, 10),
&mut props(),
),
Ok(())
);

// simulate sending `valid_xcm` with the `WithUniqueTopic` router
let mut sent_xcm = sp_io::TestExternalities::default().execute_with(|| {
assert_ok!(send_xcm::<WithUniqueTopic<TestMessageSender>>(Location::parent(), valid_xcm,));
sent_xcm()
});
assert_eq!(1, sent_xcm.len());

// `sent_xcm` should contain `SubscribeVersion` and have `SetTopic` added
let mut sent_xcm = sent_xcm.remove(0).1;
let _ = sent_xcm
.0
.matcher()
.assert_remaining_insts(2)
.expect("two instructions")
.match_next_inst(|instr| match instr {
SubscribeVersion { .. } => Ok(()),
_ => Err(ProcessMessageError::BadFormat),
})
.expect("expected instruction `SubscribeVersion`")
.match_next_inst(|instr| match instr {
SetTopic(..) => Ok(()),
_ => Err(ProcessMessageError::BadFormat),
})
.expect("expected instruction `SetTopic`");

// `sent_xcm` contains `SetTopic` and is now invalid for `AllowSubscriptions`
assert_eq!(
AllowSubscriptions::should_execute(
&Location::parent(),
sent_xcm.clone().inner_mut(),
Weight::from_parts(10, 10),
&mut props(),
),
Err(ProcessMessageError::BadFormat)
);

// let's apply `TrailingSetTopicAsId` before `AllowSubscriptions`
let mut props = props();
assert!(props.message_id.is_none());

// should pass, and the `message_id` is set
assert_eq!(
TrailingSetTopicAsId::<AllowSubscriptions>::should_execute(
&Location::parent(),
sent_xcm.clone().inner_mut(),
Weight::from_parts(10, 10),
&mut props,
),
Ok(())
);
assert!(props.message_id.is_some());
}
32 changes: 24 additions & 8 deletions polkadot/xcm/xcm-builder/src/tests/version_subscriptions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,32 @@ fn simple_version_subscriptions_should_work() {
]);
let mut hash = fake_message_hash(&message);
let weight_limit = Weight::from_parts(20, 20);
let r = XcmExecutor::<TestConfig>::prepare_and_execute(
origin,
message,
&mut hash,
weight_limit,
Weight::zero(),

// this case fails because the origin is not allowed
assert_eq!(
XcmExecutor::<TestConfig>::prepare_and_execute(
origin,
message.clone(),
&mut hash,
weight_limit,
Weight::zero(),
),
Outcome::Error { error: XcmError::Barrier }
);

// this case fails because the additional `SetAppendix` instruction is not allowed in the
// `AllowSubscriptionsFrom`
assert_eq!(
XcmExecutor::<TestConfig>::prepare_and_execute(
Parent,
message,
&mut hash,
weight_limit,
Weight::zero(),
),
Outcome::Error { error: XcmError::Barrier }
);
assert_eq!(r, Outcome::Error { error: XcmError::Barrier });

let origin = Parachain(1000);
let message = Xcm::<TestCall>(vec![SubscribeVersion {
query_id: 42,
max_response_weight: Weight::from_parts(5000, 5000),
Expand Down
4 changes: 2 additions & 2 deletions polkadot/xcm/xcm-executor/src/traits/should_execute.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,9 @@ pub struct Properties {
/// Trait to determine whether the execution engine should actually execute a given XCM.
///
/// Can be amalgamated into a tuple to have multiple trials. If any of the tuple elements returns
/// `Ok()`, the execution stops. Else, `Err(_)` is returned if all elements reject the message.
/// `Ok(())`, the execution stops. Else, `Err(_)` is returned if all elements reject the message.
pub trait ShouldExecute {
/// Returns `true` if the given `message` may be executed.
/// Returns `Ok(())` if the given `message` may be executed.
///
/// - `origin`: The origin (sender) of the message.
/// - `instructions`: The message itself.
Expand Down
Loading