Outbound queue v2 #4

yrong · 2024-10-18T01:35:21Z

Resolves: https://linear.app/snowfork/issue/SNO-1205

yrong · 2024-10-21T05:36:08Z

Workflow

on AH we add a custom exporter returns a lower fee(without the cost on Ethereum) charged from the user when exporting to Ethereum, essentially predict the route by content of XCM, more details in Predicate route table by Xcm paritytech/polkadot-sdk#6074, since there is no XCMV5 branch ready for tests yet, as a workaround we can execute raw xcm with some custom instruction on AH to differ V2 with V1.
on BH we route the new XCM(with V2 specific instructions) to the new pallet OutboundQueueV2, in which we convert the XCM to Message structure as following:

pub struct Message {
    /// Origin
    pub origin: H256,
    /// ID
    pub id: H256,
    /// Fee
    pub fee: u128,
    /// Commands
    pub commands: BoundedVec<Command, ConstU32<5>>,
}

which means we can execute multiple commands on Ethereum in one message.

Then we abi encode the message and store it into a merkle tree as before, meanwhile add the pending fee into LockedFee storage and increase the nonce. more detais here
add a new extrinsic submit_delivery_proof in which we verify the event log is truely happened on Ethereum side, and add the fee to RewardLeger.

bridges/snowbridge/primitives/core/src/merkle.rs

bridges/snowbridge/primitives/core/src/outbound_v2.rs

vgeddes · 2024-10-22T18:08:15Z

bridges/snowbridge/primitives/core/src/lib.rs

@@ -172,3 +174,13 @@ impl Default for AssetMetadata {

 /// Maximum length of a string field in ERC20 token metada
 const METADATA_FIELD_MAX_LEN: u32 = 32;
+
+// Origin for high-priority governance commands
+pub fn primary_governance_origin() -> H256 {


This is now a V1-only type, so should go in core/src/v1/mod.rs.

850fe96
Actually it's V2 specific.

The usage in

polkadot-sdk/bridges/snowbridge/pallets/outbound-queue-v2/src/send_message_impl.rs

Lines 45 to 47 in ba03ae6

if ticket.origin != primary_governance_origin() {

ensure!(!Self::operating_mode().is_halted(), SendError::Halted);

}

is to allow upgrading even when bridge is halted.

vgeddes · 2024-10-22T18:08:26Z

bridges/snowbridge/primitives/core/src/lib.rs

+}
+
+// Origin for lower-priority governance commands
+pub fn second_governance_origin() -> H256 {


#4 (comment)

...rachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/src/tests/snowbridge.rs

bridges/snowbridge/pallets/outbound-queue-v2/src/types.rs

bridges/snowbridge/primitives/core/src/outbound_v2.rs

yrong · 2024-10-29T12:38:51Z

Check details in emulated tests how to construct transfer message for both ENA&PNA with XCMV5 instructions.

cargo test -p bridge-hub-westend-integration-tests --lib tests::snowbridge_v2  -- --nocapture

bridges/snowbridge/pallets/outbound-queue-v2/src/send_message_impl.rs

bridges/snowbridge/pallets/outbound-queue-v2/runtime-api/src/lib.rs

bridges/snowbridge/pallets/outbound-queue-v2/src/envelope.rs

bridges/snowbridge/pallets/outbound-queue-v2/src/lib.rs

bridges/snowbridge/pallets/outbound-queue-v2/src/types.rs

bridges/snowbridge/pallets/outbound-queue-v2/src/lib.rs

bridges/snowbridge/pallets/outbound-queue-v2/src/api.rs

vgeddes · 2024-11-12T08:58:27Z

bridges/snowbridge/pallets/system/src/lib.rs

+		/// - `metadata`: Metadata to include in the instantiated ERC20 contract on Ethereum
+		#[pallet::call_index(11)]
+		#[pallet::weight(T::WeightInfo::register_token())]
+		pub fn register_token_v2(


Alternatively can we reimplement the original register_token extrinsic to optionally use V2 protocol, depending on a config field in storage.

When the ethereum contracts have been upgraded to V2, we can configure register_token to use V2.

The benefit is that user-level apps won't need to change any code to register tokens using V2.

Actually, ignore my comment, I realize we need separate register extrinsics for V1 and V2 as they have different fee-taking behavior.

I see now there is also the problem of how to provide WETH as the fees for these governance calls. On BH users can only provide DOT

Maybe we need to make the rewards pallet support both WETH and DOT.

how to provide WETH as the fees for these governance calls.

Actually there is no fee charged for governance calls. So it's possible pendingOrder bound with zero fee and no need to reward in this case.

polkadot-sdk/bridges/snowbridge/pallets/outbound-queue-v2/src/lib.rs

Lines 278 to 281 in fb3b30c

// No fee for governance order

if !order.fee.is_zero() {

T::RewardLedger::deposit(envelope.reward_address, order.fee.into())?;

}

The problem is that then no off-chain relayer will be incentivized to deliver the message. Letting users optionally pay using DOT is a solution we should consider.

Another issue is that when we make register_token_v2 permissionless, there is still no way for many parachains to call the extrinsic since they won't have HRMP channels registered.

So I think we have to force users to route their governance calls through AH and provide DOT. In which case, BH will see message like:

AliasOrigin /Parachain/Hydra ReceiveTeleportedAsset (DOT, 10) DepositAsset (DOT, /Parachain/Hydra) Transact(EthereumSystem.register_token_v2)

So I think we have to force users to route their governance calls through AH and provide DOT.

Agree. All calls should be through AH.

But I'd assume in this case the fee could also be swapped to WETH beforehand in AH so that in BH we only use WETH as fee for simplicity. Some todo logic here:

polkadot-sdk/bridges/snowbridge/primitives/router/src/outbound/v2/convert.rs

Lines 289 to 294 in cff1cba

// Todo: Validate fee asset is WETH

let fee_amount = match fee_asset {

Asset { id: _, fun: Fungible(amount) } => Some(*amount),

_ => None,

}

.ok_or(AssetResolutionFailed)?;

I would also prefer a solution where we only have 1 asset type in the rewards pallet.

Yeah, that would be best.

If we move governance extrinsics such as register-token, create-agent to AH, then that would neatly solve the problem

As we could burn the WETH on AH and then send message to BH with the weth reward for the outbound message

Example flow:

User calls EthereumSystem.register_token extrinsic on AH, supplying WETH for reward

WETH is burnt, and AH sends Transact(EthereumSystem.register_token) XCM to BH

On BH, the low-level EthereumSystem.register_token adds burnt WETH to rewards pallet and queues outbound command

vgeddes · 2024-11-12T09:03:29Z

bridges/snowbridge/primitives/core/src/outbound/v2.rs

+use alloy_sol_types::SolValue;
+
+sol! {
+	struct InboundMessageWrapper {


Is it possible to use modules as namespaces instead of appending Wrapper to the struct names? Thinking of this pattern:

mod abi { sol! { struct InboundMessageWrapper { ... } } }

Then we can reference the type using abi::InboundMessage

The point of adding the abi namespace is that we rename InboundMessageWrapper and CommandWrapper to InboundMessage and Command respectively

Yeah, it seems a bit cubersome with InboundMessage and InboundMessageWrapper.

The reason is that it's not allowed to add macro #[derive(Encode, Decode, RuntimeDebug, PartialEq,TypeInfo)] into InboundMessageWrapper which is required for storing the data on chain, or error as follows:

--> /Users/yangrong/Projects/polkadot-sdk/bridges/snowbridge/primitives/core/src/outbound/v2.rs:31:12 | 31 | #[derive(Encode, Decode, RuntimeDebug, PartialEq,TypeInfo)] | ^^^^^^ the trait `WrapperTypeEncode` is not implemented for `alloy_primitives::FixedBytes<32>`, which is required by `alloy_primitives::FixedBytes<32>: Encode` | = help: the following other types implement trait `WrapperTypeEncode`: &T &mut T Arc<T> Box<T> Cow<'a, T> Rc<T> alloy_primitives::bytes::Bytes parity_scale_codec::Ref<'a, T, U> and 3 others = note: required for `alloy_primitives::FixedBytes<32>` to implement `Encode` = note: this error originates in the derive macro `Encode` (in Nightly builds, run with -Z macro-backtrace for more info) ...

So InboundMessageWrapper is the intermediate data structure only for abi encode, will be converted to InboundMessage here when storing on chain.

bridges/snowbridge/primitives/core/src/outbound/v2.rs

vgeddes · 2024-11-12T09:16:32Z

bridges/snowbridge/primitives/router/src/outbound/v2.rs

+		})?;
+
+		// Inspect AliasOrigin as V2 message
+		let mut instructions = message.clone().0;


Seems like this logic should be inside XcmConverter

It's for compatible with V1, for xcm which does not contain AliasOrigin will fallback to EthereumBlobExporterV1.

polkadot-sdk/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/xcm_config.rs

Lines 220 to 224 in cff1cba

type MessageExporter = (

XcmOverBridgeHubRococo,

crate::bridge_to_ethereum_config::SnowbridgeExporterV2,

crate::bridge_to_ethereum_config::SnowbridgeExporter,

);

vgeddes · 2024-11-12T09:21:19Z

bridges/snowbridge/primitives/router/src/outbound/v2.rs

+	};
+}
+
+pub struct XcmConverter<'a, ConvertAssetId, Call> {


There is going to be a lot more conversion code for v2, so I suggest moving XcmConverter to a new file like outbound/v2/converter.rs.

vgeddes · 2024-11-12T09:22:30Z

bridges/snowbridge/primitives/router/src/outbound/v2.rs

+	}
+
+	pub fn convert(&mut self) -> Result<Message, XcmConverterError> {
+		let result = match self.jump_to() {


I don't think its safe to just "jump" over instructions, we should validate each instruction in turn to see whether it matches expectations.

vgeddes · 2024-11-12T09:27:37Z

bridges/snowbridge/primitives/router/src/outbound/v2.rs

+		}
+	}
+
+	pub fn convert(&mut self) -> Result<Message, XcmConverterError> {


For V2, an InboundMessage can contain many commands. This conversion code looks like it only matches enough instructions to produce a single command?

Also we need support for XCM::Transact, ie convert it to gateway CallContract command.

We should support any combination of UnlockNativeToken, MintForeignToken, CallContract commands.

Yeah, for simplicity we start from a single command, will improve it later.

af928ba

Btw: Transact support is not included yet. I'd prefer to limit the scope of this PR and implement transact in another one.

Co-authored-by: Vincent Geddes <117534+vgeddes@users.noreply.github.com>

bridges/snowbridge/pallets/outbound-queue-v2/src/lib.rs

claravanstaden · 2024-11-18T10:36:05Z

bridges/snowbridge/pallets/outbound-queue-v2/src/lib.rs

+			ensure!(<PendingOrders<T>>::contains_key(nonce), Error::<T>::PendingNonceNotExist);
+
+			let order = <PendingOrders<T>>::get(nonce).ok_or(Error::<T>::PendingNonceNotExist)?;


Suggested change

ensure!(<PendingOrders<T>>::contains_key(nonce), Error::<T>::PendingNonceNotExist);

let order = <PendingOrders<T>>::get(nonce).ok_or(Error::<T>::PendingNonceNotExist)?;

let order = <PendingOrders<T>>::get(nonce).ok_or(Error::<T>::PendingNonceNotExist)?;

Is the first check necessary? Would it not return an error if the nonce does not exist?

claravanstaden · 2024-11-18T11:09:33Z

bridges/snowbridge/primitives/core/src/outbound/v2.rs

+
+/// A message which can be accepted by implementations of `/[`SendMessage`\]`
+#[derive(Encode, Decode, TypeInfo, PartialEq, Clone, RuntimeDebug)]
+pub struct Message {


For both the inbound and outbound pallets, I was wondering if we want to do message versioning at all like we did for v1? Or did we see that if we want to make large changes we'll create a new pallet like we are doing for v2? @vgeddes

claravanstaden · 2024-11-18T11:10:43Z

bridges/snowbridge/primitives/core/src/outbound/v2.rs

+pub fn primary_governance_origin() -> H256 {
+	hex!("0000000000000000000000000000000000000000000000000000000000000001").into()
+}
+
+// Origin for lower-priority governance commands
+pub fn second_governance_origin() -> H256 {
+	hex!("0000000000000000000000000000000000000000000000000000000000000002").into()
+}


Where is this used?

#4 (comment)

claravanstaden · 2024-11-18T11:21:15Z

bridges/snowbridge/primitives/router/src/outbound/v2/mod.rs

+				}
+			},
+		);
+		ensure!(result.is_err(), SendError::NotApplicable);


I don't quite understand this. Why are we asserting that we get an error here?

Yeah, seems a bit weird.

Because we're using match_next_inst_while macro to match against AliasOrigin with error return, if not matched, then fallback to V1.

#4 (comment)

But doesn't this expect the result to always be an error? For example, if result is Ok(), this ensure will still error?

claravanstaden · 2024-11-18T11:26:37Z

cumulus/parachains/runtimes/bridge-hubs/common/src/message_queue.rs

@@ -55,7 +57,7 @@ impl From<AggregateMessageOrigin> for Location {
 			Sibling(id) => Location::new(1, Junction::Parachain(id.into())),
 			// NOTE: We don't need this conversion for Snowbridge. However, we have to
 			// implement it anyway as xcm_builder::ProcessXcmMessage requires it.
-			Snowbridge(_) => Location::default(),
+			_ => Location::default(),


Why not add SnowbridgeV2 here?

We don't need this conversion for Snowbridge. However, we have to implement it anyway as xcm_builder::ProcessXcmMessage requires it.

Just as the comment above.

yrong mentioned this pull request Oct 18, 2024

Initialize bridge V2 paritytech/polkadot-sdk#6113

Draft

yrong mentioned this pull request Oct 22, 2024

Parachain Relayer V2 Snowfork/snowbridge#1321

Open

vgeddes reviewed Oct 22, 2024

View reviewed changes

claravanstaden mentioned this pull request Oct 23, 2024

Snowbridge V2 - Unified Rewards claravanstaden/polkadot-sdk#1

Open

yrong changed the base branch from v2 to master October 24, 2024 16:55

yrong changed the base branch from master to xcm-v5 October 29, 2024 02:03

yrong marked this pull request as ready for review October 29, 2024 12:44

vgeddes reviewed Oct 30, 2024

View reviewed changes

bridges/snowbridge/pallets/outbound-queue-v2/src/send_message_impl.rs Show resolved Hide resolved

yrong force-pushed the outbound-queue-v2 branch 3 times, most recently from fffd895 to df137c0 Compare November 5, 2024 17:03

yrong changed the base branch from xcm-v5 to master November 11, 2024 02:33

Merge from master

d359108

yrong force-pushed the outbound-queue-v2 branch from 9492f1e to d359108 Compare November 11, 2024 02:35

yrong added 2 commits November 11, 2024 11:57

Fix v2 tests

4d6c678

Fix compile

a72aa5a

vgeddes reviewed Nov 12, 2024

View reviewed changes

yrong and others added 10 commits November 14, 2024 21:15

Merge branch 'master' into outbound-queue-v2

cf8a9c3

Rename to OutboundQueueV2Api

056fd7f

Return raw balance for dry run

49837cf

Decode account asap

030d95c

Revamp comments for V2

206b300

Custom digest for V2

8b3e178

Cleanup imports

a22f0ac

Update bridges/snowbridge/pallets/outbound-queue-v2/src/types.rs

e346bf6

Co-authored-by: Vincent Geddes <117534+vgeddes@users.noreply.github.com>

Clean up with the insert

9847bc9

Remove Fee for V2

fb3b30c

yrong added 3 commits November 15, 2024 01:08

Reorgnize InboundMessage to abi module

83b6ff8

Seperate XcmConverter

f08e36e

Merge branch 'master' into outbound-queue-v2

cff1cba

yrong changed the base branch from master to snowbridge-v2 November 15, 2024 06:45

Merge branch 'snowbridge-v2' into outbound-queue-v2

faf25e9

claravanstaden reviewed Nov 18, 2024

View reviewed changes

yrong added 5 commits November 19, 2024 21:15

Support multiple commands in one message

af928ba

Rename to InvalidPendingNonce & Cleanup

d5ab77b

Improve comment

f96a6fc

Fix breaking tests

d4910ea

Merge branch 'snowbridge-v2' into outbound-queue-v2

7586a28

claravanstaden mentioned this pull request Nov 20, 2024

Inbound queue v2 claravanstaden/polkadot-sdk#3

Open

yrong added 2 commits November 20, 2024 21:43

Remove Inbound-queue V2 completely

8e80315

Reorgnize code layout

e8951e4

	if ticket.origin != primary_governance_origin() {
	ensure!(!Self::operating_mode().is_halted(), SendError::Halted);
	}

	// No fee for governance order
	if !order.fee.is_zero() {
	T::RewardLedger::deposit(envelope.reward_address, order.fee.into())?;
	}

	// Todo: Validate fee asset is WETH
	let fee_amount = match fee_asset {
	Asset { id: _, fun: Fungible(amount) } => Some(*amount),
	_ => None,
	}
	.ok_or(AssetResolutionFailed)?;

	type MessageExporter = (
	XcmOverBridgeHubRococo,
	crate::bridge_to_ethereum_config::SnowbridgeExporterV2,
	crate::bridge_to_ethereum_config::SnowbridgeExporter,
	);

		ensure!(<PendingOrders<T>>::contains_key(nonce), Error::<T>::PendingNonceNotExist);

		let order = <PendingOrders<T>>::get(nonce).ok_or(Error::<T>::PendingNonceNotExist)?;

Outbound queue v2 #4

Are you sure you want to change the base?

Outbound queue v2 #4

Conversation

yrong commented Oct 18, 2024 • edited Loading

yrong commented Oct 21, 2024 • edited Loading

Workflow

Choose a reason for hiding this comment

yrong Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yrong commented Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

vgeddes Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yrong Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yrong Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgeddes Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yrong Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yrong Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yrong Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

yrong Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yrong commented Oct 18, 2024 •

edited

Loading

yrong commented Oct 21, 2024 •

edited

Loading

yrong Oct 23, 2024 •

edited

Loading

yrong commented Oct 29, 2024 •

edited

Loading

vgeddes Nov 14, 2024 •

edited

Loading

yrong Nov 14, 2024 •

edited

Loading

yrong Nov 15, 2024 •

edited

Loading

vgeddes Nov 20, 2024 •

edited

Loading

yrong Nov 15, 2024 •

edited

Loading

yrong Nov 14, 2024 •

edited

Loading

yrong Nov 14, 2024 •

edited

Loading

yrong Nov 19, 2024 •

edited

Loading