Skip to content

Commit

Permalink
Revamp with ExportOrigin
Browse files Browse the repository at this point in the history
  • Loading branch information
@yrong
yrong committed Oct 19, 2023
1 parent 876425c commit a6317f0
Show file tree
Hide file tree
Showing 4 changed files with 44 additions and 61 deletions.
2 changes: 1 addition & 1 deletion cumulus
Submodule cumulus updated 1 files
+1 −2 parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/lib.rs
39 changes: 12 additions & 27 deletions parachain/pallets/outbound-queue/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,7 @@ use sp_std::prelude::*;

use snowbridge_core::outbound::{
AggregateMessageOrigin, Command, EnqueuedMessage, GasMeter, Message, MessageHash,
OutboundQueue as OutboundQueueTrait, OutboundQueueTicket, PreparedMessage, Priority,
SubmitError,
OutboundQueue as OutboundQueueTrait, OutboundQueueTicket, PreparedMessage, SubmitError,
};
use snowbridge_outbound_queue_merkle_tree::merkle_root;

Expand All @@ -63,7 +62,7 @@ pub mod pallet {
use frame_system::pallet_prelude::*;

use bp_runtime::{BasicOperatingMode, OwnedBridgeModule};
use frame_support::traits::Contains;
use snowbridge_core::outbound::ExportOrigin;

This comment has been minimized.

Sign in to view

Copy link
@vgeddes

vgeddes Oct 19, 2023

Collaborator

I kinda feel like all these imports should be at the top of the file and not within the pallet module. For the sake of consistency.

#[pallet::pallet]
pub struct Pallet<T>(_);
Expand Down Expand Up @@ -105,9 +104,6 @@ pub mod pallet {
#[pallet::constant]
type DeliveryReward: Get<u128>;

/// Determines whether a command is high priority
type HighPriorityCommands: Contains<Command>;

/// Weight information for extrinsics in this pallet
type WeightInfo: WeightInfo;
}
Expand Down Expand Up @@ -330,32 +326,21 @@ pub mod pallet {
let encoded =
enqueued_message.encode().try_into().map_err(|_| SubmitError::MessageTooLarge)?;

let priority = match T::HighPriorityCommands::contains(&message.command) {
true => Priority::High,
_ => Priority::Normal,
};

let ticket = OutboundQueueTicket {
id: message_id,
origin: message.origin,
message: encoded,
priority,
};
let ticket =
OutboundQueueTicket { id: message_id, origin: message.origin, message: encoded };

Ok((ticket, delivery_fee))
}

fn submit(ticket: Self::Ticket) -> Result<MessageHash, SubmitError> {
let origin = match (ticket.origin, ticket.priority) {
(origin, Priority::High) if origin == T::OwnParaId::get() =>
AggregateMessageOrigin::BridgeHub(Priority::High),
(origin, Priority::Normal) if origin == T::OwnParaId::get() =>
AggregateMessageOrigin::BridgeHub(Priority::Normal),
(origin, _) => AggregateMessageOrigin::Parachain(origin),
let origin = match ticket.origin {
origin if origin == T::OwnParaId::get() =>
AggregateMessageOrigin::Export(ExportOrigin::Here),
origin => AggregateMessageOrigin::Export(ExportOrigin::Sibling(origin)),

This comment has been minimized.

Sign in to view

Copy link
@vgeddes

vgeddes Oct 19, 2023

Collaborator

The let origin = if ... else ...; syntax makes more sense here. It is easier to read.
};

match origin {
AggregateMessageOrigin::BridgeHub(Priority::High) => {
AggregateMessageOrigin::Export(ExportOrigin::Here) => {
// Increase PendingHighPriorityMessageCount by one
PendingHighPriorityMessageCount::<T>::mutate(|count| {
*count = count.saturating_add(1)
Expand All @@ -381,15 +366,15 @@ pub mod pallet {
_: &mut [u8; 32],
) -> Result<bool, ProcessMessageError> {
// Yield for hard limit to ensure the weight of `on_finalize` is bounded.
let current_size = MessageLeaves::<T>::decode_len().unwrap_or(0);
ensure!(
MessageLeaves::<T>::decode_len().unwrap_or(0) <
T::MaxMessagesPerBlock::get() as usize,
current_size < T::MaxMessagesPerBlock::get() as usize,
ProcessMessageError::Yield
);

// Yield for halt check or if there is pending high priority message
match origin {
AggregateMessageOrigin::BridgeHub(Priority::High) => {
AggregateMessageOrigin::Export(ExportOrigin::Here) => {

This comment has been minimized.

Sign in to view

Copy link
@vgeddes

vgeddes Oct 19, 2023

Collaborator

Rather use the if let <pattern> ... else ... syntax if you are matching on a single pattern. Its simpler and requires less indenting.
// Decrease PendingHighPriorityMessageCount by one
PendingHighPriorityMessageCount::<T>::mutate(|count| {
*count = count.saturating_sub(1)
Expand Down
37 changes: 23 additions & 14 deletions parachain/pallets/outbound-queue/src/test.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use frame_support::{
weights::WeightMeter,
};

use snowbridge_core::outbound::{AgentExecuteCommand, Command, HighPriorityCommands, Initializer};
use snowbridge_core::outbound::{AgentExecuteCommand, Command, ExportOrigin, Initializer};
use sp_core::{ConstU128, H160, H256};
use sp_runtime::{
testing::Header,
Expand Down Expand Up @@ -99,7 +99,6 @@ impl crate::Config for Test {
type DeliveryFeePerGas = ConstU128<1>;
type DeliveryRefundPerGas = ConstU128<1>;
type DeliveryReward = ConstU128<1>;
type HighPriorityCommands = HighPriorityCommands;
type WeightInfo = ();
}

Expand Down Expand Up @@ -226,7 +225,7 @@ fn process_message_yields_on_max_messages_per_block() {
MessageLeaves::<Test>::append(H256::zero())
}

let origin = AggregateMessageOrigin::Parachain(1000.into());
let origin = AggregateMessageOrigin::Export(ExportOrigin::Sibling(1000.into()));
let message = EnqueuedMessage {
id: Default::default(),
origin: 1000.into(),
Expand All @@ -250,7 +249,7 @@ fn process_message_yields_on_max_messages_per_block() {
#[test]
fn process_message_fails_on_overweight_message() {
new_tester().execute_with(|| {
let origin = AggregateMessageOrigin::Parachain(1000.into());
let origin = AggregateMessageOrigin::Export(ExportOrigin::Sibling(1000.into()));

let message = EnqueuedMessage {
id: Default::default(),
Expand Down Expand Up @@ -306,12 +305,12 @@ fn submit_low_priority_messages_yield_when_there_is_high_priority_message() {
let ticket = result.unwrap();
assert_ok!(OutboundQueue::submit(ticket.0));
let mut footprint =
MessageQueue::footprint(AggregateMessageOrigin::BridgeHub(Priority::High));
MessageQueue::footprint(AggregateMessageOrigin::Export(ExportOrigin::Here));
println!("{:?}", footprint);
assert_eq!(footprint.count, 1);

// process a low priority message from asset_hub will yield
let origin = AggregateMessageOrigin::Parachain(1000.into());
let origin = AggregateMessageOrigin::Export(ExportOrigin::Sibling(1000.into()));
let message = EnqueuedMessage {
id: Default::default(),
origin: 1000.into(),
Expand Down Expand Up @@ -339,7 +338,7 @@ fn submit_low_priority_messages_yield_when_there_is_high_priority_message() {
let digest = System::digest();
let digest_items = digest.logs();
assert!(digest_items.len() == 1 && digest_items[0].as_other().is_some());
footprint = MessageQueue::footprint(AggregateMessageOrigin::Parachain(1013.into()));
footprint = MessageQueue::footprint(AggregateMessageOrigin::Export(ExportOrigin::Here));
assert_eq!(footprint.count, 0);
});
}
Expand Down Expand Up @@ -368,7 +367,9 @@ fn submit_high_priority_message_will_not_blocked_even_when_low_priority_queue_ge
assert_ok!(OutboundQueue::submit(ticket.0));
}

let footprint = MessageQueue::footprint(AggregateMessageOrigin::Parachain(1000.into()));
let footprint = MessageQueue::footprint(AggregateMessageOrigin::Export(
ExportOrigin::Sibling(1000.into()),
));
assert_eq!(footprint.count, (max_messages) as u64);

// submit high priority message from bridge_hub
Expand All @@ -384,7 +385,7 @@ fn submit_high_priority_message_will_not_blocked_even_when_low_priority_queue_ge
assert!(result.is_ok());
let ticket = result.unwrap();
assert_ok!(OutboundQueue::submit(ticket.0));
let footprint = MessageQueue::footprint(AggregateMessageOrigin::BridgeHub(Priority::High));
let footprint = MessageQueue::footprint(AggregateMessageOrigin::Export(ExportOrigin::Here));
println!("{:?}", footprint);
assert_eq!(footprint.count, 1);

Expand All @@ -395,27 +396,35 @@ fn submit_high_priority_message_will_not_blocked_even_when_low_priority_queue_ge
let digest = System::digest();
let digest_items = digest.logs();
assert!(digest_items.len() == 1 && digest_items[0].as_other().is_some());
let footprint = MessageQueue::footprint(AggregateMessageOrigin::Parachain(1000.into()));
let footprint = MessageQueue::footprint(AggregateMessageOrigin::Export(
ExportOrigin::Sibling(1000.into()),
));
assert_eq!(footprint.count, 41);
let footprint = MessageQueue::footprint(AggregateMessageOrigin::BridgeHub(Priority::High));
let footprint = MessageQueue::footprint(AggregateMessageOrigin::Export(ExportOrigin::Here));
assert_eq!(footprint.count, 0);

// move to the next block, some low priority messages get executed
ServiceWeight::set(Some(Weight::MAX));
run_to_end_of_next_block();
let footprint = MessageQueue::footprint(AggregateMessageOrigin::Parachain(1000.into()));
let footprint = MessageQueue::footprint(AggregateMessageOrigin::Export(
ExportOrigin::Sibling(1000.into()),
));
assert_eq!(footprint.count, 21);

// move to the next block, some low priority messages get executed
ServiceWeight::set(Some(Weight::MAX));
run_to_end_of_next_block();
let footprint = MessageQueue::footprint(AggregateMessageOrigin::Parachain(1000.into()));
let footprint = MessageQueue::footprint(AggregateMessageOrigin::Export(
ExportOrigin::Sibling(1000.into()),
));
assert_eq!(footprint.count, 1);

// move to the next block, the last low priority messages get executed
ServiceWeight::set(Some(Weight::MAX));
run_to_end_of_next_block();
let footprint = MessageQueue::footprint(AggregateMessageOrigin::Parachain(1000.into()));
let footprint = MessageQueue::footprint(AggregateMessageOrigin::Export(
ExportOrigin::Sibling(1000.into()),
));
assert_eq!(footprint.count, 0);
});
}
Expand Down
27 changes: 8 additions & 19 deletions parachain/primitives/core/src/outbound.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use codec::{Decode, Encode, MaxEncodedLen};
use derivative::Derivative;
use ethabi::Token;
use frame_support::{
traits::{tokens::Balance, Contains, Get},
traits::{tokens::Balance, Get},
BoundedVec, CloneNoBound, DebugNoBound, EqNoBound, PartialEqNoBound, RuntimeDebugNoBound,
};
pub use polkadot_parachain::primitives::Id as ParaId;
Expand Down Expand Up @@ -286,7 +286,6 @@ pub struct OutboundQueueTicket<MaxMessageSize: Get<u32>> {
pub id: H256,
pub origin: ParaId,
pub message: BoundedVec<u8, MaxMessageSize>,
pub priority: Priority,
}

/// Message which is awaiting processing in the MessageQueue pallet
Expand Down Expand Up @@ -336,7 +335,7 @@ impl From<PreparedMessage> for Token {

impl From<u32> for AggregateMessageOrigin {
fn from(value: u32) -> Self {
AggregateMessageOrigin::Parachain(value.into())
AggregateMessageOrigin::Export(ExportOrigin::Sibling(value.into()))
}
}

Expand All @@ -350,25 +349,15 @@ pub struct OriginInfo {
pub agent_id: H256,
}

/// Priority for an outbound message
#[derive(Copy, Encode, Decode, Clone, MaxEncodedLen, Eq, PartialEq, RuntimeDebug, TypeInfo)]
pub enum Priority {
Normal,
High,
#[derive(Encode, Decode, Clone, MaxEncodedLen, Eq, PartialEq, RuntimeDebug, TypeInfo)]
pub enum ExportOrigin {
Here,
Sibling(ParaId),
}

/// Aggregate message origin for the `MessageQueue` pallet.
#[derive(Encode, Decode, Clone, MaxEncodedLen, Eq, PartialEq, RuntimeDebug, TypeInfo)]
pub enum AggregateMessageOrigin {
#[codec(index = 0)]
BridgeHub(Priority),
#[codec(index = 1)]
Parachain(ParaId),
}

pub struct HighPriorityCommands;
impl Contains<Command> for HighPriorityCommands {
fn contains(command: &Command) -> bool {
matches!(command, Command::Upgrade { .. } | Command::SetOperatingMode { .. })
}
/// Message is to be exported via a bridge
Export(ExportOrigin),
}

0 comments on commit a6317f0

Please sign in to comment.