Make relay CLI generic #849

tomusdrw · 2021-03-30T15:22:29Z

The purpose of this PR is to make it easy to add new chains. It's a follow up on #734.

Initially I thought that we can orient the CLI around various bridges/chains and simply duplicate the code responsible for handling the CLI commands (see src/rialto_millau module). Later on I figure that it's quite a lot of code to duplicate, but was still hoping that we can get around this by using some macros.
However since the code is split into multiple files it's not that trivial to macro-ify the code + it heavily reduces readability and ability to debug errors.

This PR introduces a different approach, where we try to keep the code generic enough so that a lot of code can be shared without introducing a macro, but all the things that are difficult to make generic, or depend on particular sourceNetwork-targetNetwork bridge pair are resolved via macro-shared code.

The most important part of the PR is the select_bridge! macro, which allows us to write some $generic code, without actually naming all of the types (which would be hard).
I've refactored RelayHeaders and InitBridge for now, but since the PR is getting quite big, I've decided to make it a draft and let you decide if I should continue or if we rather review & merge like this and change the rest in a follow up PR.

There is a bunch of unaddressed and unassigned TODOs for now, and most likely some bash scripts require updating as well.

Follow up tasks:

Remove target-specific stuff from CliChain.
Support multiple bridges in BridgeSendMessage Call.
Remove source_* target_* CLI methods (e.g. add Into<ConnectionParams>)
Consider using structopt renames for Singing and Connection parameters.
Create arg-enum-like macro with kebab-case support or use existing proc-macro crates.

HCastano

I didn't have the brainpower to look at this as in depth as I wanted to today. I do have a couple of thoughts though:

My gut says no to the macros, but it might just be PTSD from the Substrate ones
Please don't refactor the whole CLI in this PR, the diff is already large enough 😄

I'll also try and do some thinking about how we can architect this so that multi-network support isn't such a pain

HCastano

In general it looks all right.

Before we merge this I would like to see the TODOs have issues attached to them so any of us can pick them up and help with this effort.

I'll take another look after this leaves the draft state.

HCastano · 2021-03-31T17:24:20Z

primitives/chain-westend/src/lib.rs

+	spec_name: sp_version::create_runtime_str!("westend"),
+	impl_name: sp_version::create_runtime_str!("parity-westend"),
+	authoring_version: 2,
+	spec_version: 50,


Do we need to make sure we keep up with the spec_version changes in the Westend runtime?

Yup, but I think it will be covered by a test in Polkadot repo. Alternatively the relayer could query a running node and make sure this matches.

relays/bin-substrate/src/cli/init_bridge.rs

HCastano · 2021-03-31T17:30:45Z

relays/bin-substrate/src/cli/mod.rs

+
+	/// Bridge Message Payload type.
+	///
+	/// TODO [ToDr] This should be removed in favour of target-specifc types.


Can you elaborate on what you mean here?

In case we have multiple bridges the MessagePayload type will be different.
For instance for RialtoToWestend and RialtoToMillau the MessagePayload and encode_call will look quite differently.
The TODO is there cause it's not the right way to abstract the code, however since right now we don't have a chain with two bridges that include MessageLanes it's good enough, but I already have an idea how to improve it, hence the TODO.

HCastano · 2021-03-31T17:32:35Z

relays/bin-substrate/src/cli/mod.rs


 	/// Numeric value of SS58 format.
 	fn ss58_format() -> u16;
+
+	/// Parse CLI call and encode it to be dispatched on this specific chain.
+	fn encode_call(call: crate::rialto_millau::cli::Call) -> Result<Self::Call, String>;


Shouldn't this call type be generic?

If you mean the fact that it has rialto_millau in the module path, then yes. The Call is in fact already generic, but I wanted to avoid moving all the types to top-level cli/mod.rs file just yet. Will be fixed later when the rialto_millau module is dissolved.

HCastano · 2021-03-31T17:38:44Z

relays/bin-substrate/src/rialto_millau/cli.rs

@@ -19,68 +19,24 @@
 use frame_support::weights::Weight;


With your follow up PRs this file will end up being replaced by an enum + macro combo, right?

Each sub-command will have it's own module in src/cli/ and there is going to be less duplication. I expect the entire file to be removed after the refactor is finished.

relays/bin-substrate/src/rialto_millau/cli.rs

HCastano · 2021-03-31T17:42:54Z

relays/bin-substrate/src/rialto_millau/mod.rs

-		}
-	}
-}
-
 async fn run_relay_messages(command: cli::RelayMessages) -> Result<(), String> {
 	match command {
 		cli::RelayMessages::MillauToRialto {


This will be macro-ified too, right?

Yup. I initially started off with trying to extract the common parts, so that both branches look the same. After that I started moving the code to make it generic, but these commands are still left to do.

HCastano · 2021-03-31T17:50:01Z

relays/bin-substrate/src/cli/mod.rs

 	///
 	/// This will change the `ss58format` of the account to match the requested one.
 	/// Note that a warning will be produced in case the current format does not match
 	/// the requested one, but the conversion always succeeds.
-	pub fn enforce_chain<T: AccountChain>(&mut self) {
+	pub fn enforce_chain<T: CliChain>(&mut self) {


Looking back at it I'm not sure if enforce is the right word to use here, considering that even if the expected format doesn't match this call still "succeeds" (by converting to the right address format)

What do you suggest then? force_convert::<Rialto>()?

HCastano · 2021-03-31T17:56:41Z

relays/bin-substrate/src/rialto_millau/mod.rs

 	}
 }

-impl AccountChain for Rialto {
-	const NAME: &'static str = "Rialto";
+impl CliChain for Westend {


This impl should probably have an issue attached to it

How so? We don't really support Westend, the fact that we can't implement all methods means the abstraction is not fine-grained enough (i.e. there are sub-commands that are not even available for westend, but the trait is shared between sub-commands), but I would rather go with coarse abstraction and later on split it if it turns out to be correct.

Co-authored-by: Hernando Castano <[email protected]>

svyatonik · 2021-04-01T06:02:39Z

relays/bin-substrate/src/rialto_millau/cli.rs

-	/// A call to the Rialto Bridge Messages pallet to send a message over the bridge.
-	RialtoSendMessage {
+	// TODO [#853] Support multiple bridges.
+	/// A call to the specific Message Lane pallet to send a message over the bridge.


Quote from #853:

Currently the Call CLI enum, supports BridgeSendMessage variant. However in case there are multiple bridges on one source chain, we need to specify what Target chain we want to send the message to.

I'm a bit confused now - I thought that this variant is to call over the bridge the other bridge (deployed on the target chain) to send other message. So it may be Source --- (M1) ---> Target --- (M2) ---> Source or Source1 --- (M1) ---> Target --- (M2) ---> Source2 iiuc. So the quote should be the "multiple bridges on the target chain", no?

I guess it was introduced in the PR about printing encoded messages, so it isn't actually used in messages sending. In practice it is only used by the encode-call subcommand. But since it is available to the send-message command as well, maybe it worth at least to expand docs a bit - e.g instead of "specific Message Lane pallet" say "Message Lane pallet on the target chain", for payload change it to the "encoded Call of the source chain", etc.

P.S.: 'Message Lane pallet' is also changed to 'Messages pallet' everywhere ;)

Yes, that's a good point. Indeed I was thinking about it from the encode-call perspective, I'll update the docs.

adoerr

For me, this goes into the right direction. If anything, I want to see more of this approach. My kind of ideal in the end would be having a bridge procedural macro, similar to the pallet macro. Basically generating a specific bridge implementation, batteries (CLI) included.

So I 'approve' but keep this as comment only.

adoerr · 2021-04-01T06:17:21Z

relays/bin-substrate/src/cli/init_bridge.rs

+}
+
+macro_rules! select_bridge {
+	($bridge: expr, $generic: tt) => {


generic is generic here in the sense that it can be an arbitrary closure?

adoerr · 2021-04-01T06:18:15Z

relays/bin-substrate/src/cli/relay_headers.rs

+}
+
+macro_rules! select_bridge {
+	($bridge: expr, $generic: tt) => {


Same here w.r.t. to generic.

It's actually not a closure, but a piece of arbitrary code that will use generic parameters type Source =/ type Target = that is set in the particular branch of match statement.

adoerr · 2021-04-01T06:24:36Z

relays/client-substrate/src/error.rs

@@ -42,6 +42,19 @@ pub enum Error {
 	Custom(String),
 }

+impl std::error::Error for Error {


Perhaps we can agree on using the thiserror / anyhow combo for all our errors and start migrate towards this.

Yes, logged here #857

* ERCSC * update * Update Substrate & Polkadot Co-authored-by: doordashcon <[email protected]> Co-authored-by: Bastian Köcher <[email protected]>

* Start generalizing rialto-millau commands. * cargo fmt --all * Introduce generic balance. * Unify message payloads. * cargo fmt --all * init - generic * Attempt to unify send message. * Start moving things around. * cargo fmt --all * Move init-bridge. * cargo fmt --all * Improve UX of bridge argument. * Fix clippy. * Fix docs and scripts. * Add docs. * Apply suggestions from code review Co-authored-by: Hernando Castano <[email protected]> * Fix copyright. * Add issue numbers. * More todos. * Update comments. Co-authored-by: Hernando Castano <[email protected]>

tomusdrw added 11 commits March 30, 2021 13:01

Start generalizing rialto-millau commands.

178834d

cargo fmt --all

fdffb4c

Introduce generic balance.

fe57ad8

Unify message payloads.

2828922

cargo fmt --all

681f551

init - generic

fcb2c60

Attempt to unify send message.

7dbc947

Start moving things around.

7579354

cargo fmt --all

7b677be

Move init-bridge.

1df21f2

cargo fmt --all

1663f33

tomusdrw requested review from svyatonik and HCastano March 30, 2021 15:22

HCastano reviewed Mar 30, 2021

View reviewed changes

tomusdrw added 5 commits March 31, 2021 13:05

Improve UX of bridge argument.

e957b38

Fix clippy.

558be61

Fix docs and scripts.

7b0b4ee

Merge branch 'master' into td-relay-macro

76a7773

Add docs.

2f4fd60

HCastano reviewed Mar 31, 2021

View reviewed changes

tomusdrw and others added 2 commits March 31, 2021 21:00

Apply suggestions from code review

5b0432a

Co-authored-by: Hernando Castano <[email protected]>

Fix copyright.

54b0ad4

This was referenced Mar 31, 2021

Use kebab-case for bridge arguments #851

Closed

De-duplicate Source/Target Signing/Connection Params #852

Closed

Support sending messages to multiple bridges. #853

Closed

Fix CliChain abstraction #854

Closed

Add issue numbers.

db16c93

tomusdrw mentioned this pull request Mar 31, 2021

Move CLI commands to own modules and DRY them. #855

Closed

6 tasks

More todos.

6ce1383

tomusdrw marked this pull request as ready for review March 31, 2021 19:24

adoerr self-requested a review April 1, 2021 05:37

svyatonik approved these changes Apr 1, 2021

View reviewed changes

adoerr reviewed Apr 1, 2021

View reviewed changes

tomusdrw mentioned this pull request Apr 1, 2021

Unify error enums & generation #857

Closed

Update comments.

28bc36a

tomusdrw merged commit 2a07bd5 into master Apr 1, 2021

tomusdrw deleted the td-relay-macro branch April 1, 2021 10:49

tomusdrw mentioned this pull request Apr 3, 2021

CLI: Derive Account #860

Merged

tomusdrw mentioned this pull request Apr 13, 2021

Make relay (easy) configurable for other chains #61

Closed

svyatonik pushed a commit that referenced this pull request Jul 17, 2023

Update Substrate & Polkadot (#849)

1b13673

* ERCSC * update * Update Substrate & Polkadot Co-authored-by: doordashcon <[email protected]> Co-authored-by: Bastian Köcher <[email protected]>

tomusdrw mentioned this pull request Apr 13, 2021

Make relayer loops generic #2491

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make relay CLI generic #849

Make relay CLI generic #849

tomusdrw commented Mar 30, 2021 •

edited

Loading

HCastano left a comment

HCastano left a comment

HCastano Mar 31, 2021

tomusdrw Mar 31, 2021

HCastano Mar 31, 2021

tomusdrw Mar 31, 2021

HCastano Mar 31, 2021

tomusdrw Mar 31, 2021

HCastano Mar 31, 2021

tomusdrw Mar 31, 2021

HCastano Mar 31, 2021

tomusdrw Mar 31, 2021

HCastano Mar 31, 2021

tomusdrw Mar 31, 2021

HCastano Mar 31, 2021

tomusdrw Mar 31, 2021

svyatonik Apr 1, 2021

tomusdrw Apr 1, 2021

adoerr left a comment

adoerr Apr 1, 2021

adoerr Apr 1, 2021

tomusdrw Apr 1, 2021

adoerr Apr 1, 2021

tomusdrw Apr 1, 2021

Make relay CLI generic #849

Make relay CLI generic #849

Conversation

tomusdrw commented Mar 30, 2021 • edited Loading

HCastano left a comment

Choose a reason for hiding this comment

HCastano left a 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

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

adoerr left a 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

tomusdrw commented Mar 30, 2021 •

edited

Loading