CLI: Encode Call & Multiple Bridge Instances. #859

tomusdrw · 2021-04-02T14:32:12Z

Related #855
Related #854
Closes #853

This PR moves encode-call sub-command to it's own module and adds the select_bridge macro logic as well.
Additionally it removes CliChain::encode_call (which was a temporary measure anyway) and adds a separate trait for encoding calls. Instead of selecting a network, this command is now unified and requires a bridge as well to have full bridge context. (Temporarily) preprocess_call method is introduced to fill extra parameters that are related to specific instance of the bridge, currently we simply pass bridge_pallet_index from the context where we know which bridge we operate in.

Following: rewrite of send-message & encod-message-payload sub-commands and possibly fully solving #854 - I think after this preprocess_call will also not be required.

tomusdrw · 2021-04-02T14:34:01Z

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

+		/// An index of the pallet to encode.
+		#[structopt(skip = 255)]
+		pallet_index: u8,


That's arguably a bit hacky, but the only alternative I see is to have two separate enums (one for CLI and another one for encode_call) potentially generated by a macro to avoid duplication.

Just to make sure I understand what this is doing: based off the structopt docs this is skipping pallet_index as a CLI option and instead assigning it a default value of 255, right?

Second of all, if that is the case I'm confused as to what problem you're addressing here by doing this

Also, it looks like this is always overridden to 0

So the thing is that this type serves two purposes:

It's a generic (non-bridge-specific, non-chain-specific) definition of the Call that is used by structopt to generate the CLI. The user is not able to fill in pallet_index from the CLI, hence we set it to 255 by default.

It's used as chain-specific (and bridge-specific) encode_call parameter. For instance we encode call that can be submitted to Rialto chain. However Rialto might be bridged with multiple other chains (currently only Millau), so while encoding the BridgeSendMessage variant we need to know which one the user meant. On the CLI level we have bridge argument that clearly defines the bridge.

So what happens is:

structopt parses bridge and call parameters separately.

Based on bridge parameter the select_bridge macr sets the correct generic types (Source and Target).

Before calling into Source::encode_call we also preprocess_call to add bridge-specific parameters (like the pallet_index) which is then used internally by encode_call.

So for an example, if we had RialtoToMillau and RialtoToWestend the Source would be the same, but pallet_index would be different, so that Rialto's implementation of encode_call would be able to construct different target-specific call.

While writing this I've realised that pallet_index is actually quite misleading, so renamed it to bridge_instance_index.

Thanks for the write up. The relationship between bridge and bridge_instance_index makes more sense now.

Changing pallet_index to bridge_instance_index is definitely a good call. Some of the confusion was also the use of 0 for bridge_instance_index, but it's usage should become more clear as we add more bridges with different indices.

HCastano

I left a couple of questions

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

HCastano · 2021-04-02T19:52:01Z

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

+		/// An index of the pallet to encode.
+		#[structopt(skip = 255)]
+		pallet_index: u8,


Just to make sure I understand what this is doing: based off the structopt docs this is skipping pallet_index as a CLI option and instead assigning it a default value of 255, right?

Second of all, if that is the case I'm confused as to what problem you're addressing here by doing this

HCastano · 2021-04-02T19:54:38Z

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

+pub const RIALTO_TO_MILLAU_INDEX: u8 = 0;
+pub const MILLAU_TO_RIALTO_INDEX: u8 = 0;


Don't these need to be configurable/dynamic?

These depend on the runtime configuration of specfiic source chains. So if Millau is the first bridge pallet in Rialto thn it's 0 index, if Rialto was also bridged with, say, Westend then Westend would have index 1.

HCastano · 2021-04-02T19:56:20Z

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

+		/// An index of the pallet to encode.
+		#[structopt(skip = 255)]
+		pallet_index: u8,


Also, it looks like this is always overridden to 0

HCastano · 2021-04-02T20:08:43Z

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

+	use super::*;
+
+	#[test]
+	fn should_encode_transfer_call() {


Is it worth having tests for the other supported calls?

Yes, although note that there were no tests earlier and it's not strictly the goal of this PR. Will log it down and someone can pick it up (#863)

HCastano · 2021-04-02T20:15:01Z

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

+	pub async fn run(mut self) -> anyhow::Result<()> {
+		println!("{:?}", self.encode()?);
+		Ok(())
+	}


Does this really need to be async?

No, but it makes the code in src/cli/mod.rs nicer, cause all sub-commands are expected to be async. If I could I would make it a trait, but async fn traits are not supported yet.

There is the async_trait crate.

But I guess this is fine as it is, we can always change it in a different PR

* Encode Call & Multiple Bridge Instances. * Remove redundant clone. * Fix comment. * Rename pallet index bridge instance index. * Update error messages related to target instances Co-authored-by: Hernando Castano <hernando@hcastano.com>

Encode Call & Multiple Bridge Instances.

7bce2ca

tomusdrw commented Apr 2, 2021

View reviewed changes

tomusdrw changed the title ~~Encode Call & Multiple Bridge Instances.~~ CLI: Encode Call & Multiple Bridge Instances. Apr 2, 2021

tomusdrw mentioned this pull request Apr 2, 2021

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

Closed

6 tasks

Remove redundant clone.

44ba61e

HCastano added the P-Relay label Apr 2, 2021

HCastano reviewed Apr 2, 2021

View reviewed changes

tomusdrw added 3 commits April 3, 2021 21:08

Merge branch 'master' into td-relay-encode

9590fec

Fix comment.

28f731a

Rename pallet index bridge instance index.

c46bc6f

tomusdrw mentioned this pull request Apr 3, 2021

Add CLI tests for other Call variants #863

Closed

HCastano added 3 commits April 5, 2021 12:28

Merge branch 'master' into td-relay-encode

1133f03

Update error messages related to target instances

f8def20

Merge branch 'master' into td-relay-encode

76dab63

HCastano approved these changes Apr 5, 2021

View reviewed changes

HCastano enabled auto-merge (squash) April 5, 2021 17:08

HCastano merged commit 4d30e66 into master Apr 5, 2021

HCastano deleted the td-relay-encode branch April 5, 2021 17:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLI: Encode Call & Multiple Bridge Instances. #859

CLI: Encode Call & Multiple Bridge Instances. #859

tomusdrw commented Apr 2, 2021

tomusdrw Apr 2, 2021

HCastano Apr 2, 2021

HCastano Apr 2, 2021

tomusdrw Apr 3, 2021 •

edited

Loading

HCastano Apr 5, 2021

HCastano left a comment

HCastano Apr 2, 2021

HCastano Apr 2, 2021

tomusdrw Apr 3, 2021

HCastano Apr 2, 2021

HCastano Apr 2, 2021

tomusdrw Apr 3, 2021

HCastano Apr 2, 2021

tomusdrw Apr 3, 2021

HCastano Apr 5, 2021

		pub const RIALTO_TO_MILLAU_INDEX: u8 = 0;
		pub const MILLAU_TO_RIALTO_INDEX: u8 = 0;

CLI: Encode Call & Multiple Bridge Instances. #859

CLI: Encode Call & Multiple Bridge Instances. #859

Conversation

tomusdrw commented Apr 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomusdrw Apr 3, 2021 • edited Loading

Choose a reason for hiding this 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

tomusdrw Apr 3, 2021 •

edited

Loading