ENG-314: copy monza crates to suzuka #47

mzabaluev · 2024-05-10T07:09:02Z

Create boilerplate for suzuka network and executor by copying the monza crates.

Shared crates renamed and moved under protocol-units/execution/aptos:

opt-executor
utils

Move and rename shared crates under protocol-units/execution/aptos: - opt-executor - utils

l-monninger · 2024-05-10T11:01:27Z

protocol-units/execution/aptos/opt-executor/Cargo.toml

@@ -1,5 +1,5 @@
 [package]
-name = "monza-opt-executor"
+name = "movement-opt-executor"


I would call this aptos-opt-executor

This would make it look like one of the aptos-core crates, which would be confusing.

How about suzuka-opt-executor?

This is now shared between Monza and Suzuka, though I don't know if it will always be so.

l-monninger · 2024-05-10T11:02:02Z

protocol-units/execution/aptos/util/Cargo.toml

@@ -1,5 +1,5 @@
 [package]
-name = "monza-execution-util"
+name = "movement-execution-util"


Again, if we're talking about extracting generalized aptos executors, I would title these as such.

l-monninger · 2024-05-10T11:02:59Z

protocol-units/execution/suzuka/executor/src/v1.rs

+use aptos_types::transaction::SignedTransaction;
+
+#[derive(Clone)]
+pub struct MonzaExecutorV1 {


Should be called SuzukaExecutorV1. Similar updates elsewhere.

0xmovses

Thanks! Just renaming comments and a few questions.

0xmovses · 2024-05-10T11:12:50Z

networks/monza/monza-config/src/lib.rs

@@ -1,19 +1,19 @@
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub struct Config {
-    pub execution_config : monza_execution_util::config::Config,
+    pub execution_config : movement_execution_util::config::Config,
 }


Should the directory for this file still be monza/monza-config/? I think we want it renamed to networks/suzuka/suzuka-config/src/lib, or are we keeping these monza dir names? Seems confusing if we kept them.

If I understood @l-monninger correctly, there is to be the common part of configuration, now in movement_execution_util, composed with the protocol-specific configuration like here in monza_config.

Yeah, that util will be shared across units using the movement-opt-executor -> maptos-opt-executor.

networks/suzuka/suzuka-client/src/tests/mod.rs

0xmovses · 2024-05-10T11:15:08Z

networks/monza/monza-config/src/lib.rs

 }

 impl Config {

-    pub fn new(execution_config : monza_execution_util::config::Config) -> Self {
+    pub fn new(execution_config : movement_execution_util::config::Config) -> Self {


Suggested change

pub fn new(execution_config : movement_execution_util::config::Config) -> Self {

pub fn new(execution_config : suzuka_execution_util::config::Config) -> Self {

Same as above.

0xmovses · 2024-05-10T11:15:23Z

networks/suzuka/suzuka-client/src/tests/mod.rs

+
+    Url::from_str(
+       format!("http://{}", MONZA_CONFIG.monza_config.aptos_rest_listen_url.as_str()).as_str()
+    ).unwrap()


Same as above

0xmovses · 2024-05-10T11:16:18Z

networks/suzuka/suzuka-client/src/tests/mod.rs

+#[tokio::test]
+async fn test_example_interaction() -> Result<()> {
+    // :!:>section_1a
+    let rest_client = Client::new(NODE_URL.clone());


Can we drop the comments //!:>section_1a they seem overally verbose

Perhaps, I just copied this from the monza crate.

It's because this is also a an example interaction copied from the Aptos upstream.

Fine to leave for now IMO.

networks/suzuka/suzuka-full-node/src/partial.rs

0xmovses · 2024-05-10T11:19:37Z

networks/suzuka/suzuka-full-node/src/partial.rs

+    light_node_client: Arc<RwLock<LightNodeServiceClient<tonic::transport::Channel>>>,
+}
+
+impl <T : MonzaExecutor + Send + Sync + Clone>MonzaPartialFullNode<T> {


Suggested change

impl <T : MonzaExecutor + Send + Sync + Clone>MonzaPartialFullNode<T> {

impl <T : SuzukaExecutor + Send + Sync + Clone>SuzukaPartialFullNode<T> {

Also it looks like proper formatting is not enforced in the workspace, which it should be. I'll look into adding a CI check.

0xmovses · 2024-05-10T11:20:07Z

protocol-units/execution/suzuka/executor/Cargo.toml

@@ -0,0 +1,68 @@
+[package]
+name = "suzuka-executor"


0xmovses · 2024-05-10T11:20:46Z

protocol-units/execution/suzuka/executor/src/lib.rs

+
+
+#[tonic::async_trait]
+pub trait MonzaExecutor {


If this is going to be a generalised Executor :

Suggested change

pub trait MonzaExecutor {

pub trait MovementExecutor {

No, this is a Suzuka-specific crate.

But the trait itself could be migrated into a common crate if it needs to be, I haven't looked into such things yet.

Yeah, this should be Suzuka specific.

0xmovses · 2024-05-10T11:21:30Z

protocol-units/execution/suzuka/fin-executor/src/executor.rs

+	/// The block is committed to the state, at this point
+	/// fork choices must be resolved otherwise the commitment and subsequent execution will fail.
+	Commit,
+}


Let's remove this enum, its dead code now. And all its doc comments

@l-monninger agree?

Yeah, I honestly thought I had already dropped this.

mzabaluev · 2024-05-10T13:53:27Z

These environment variable names are prefixed with MONZA, but the configuration is currently shared. Any ideas how to improve this?

These names were confusing, rename to MonzaPartialNode and SuzukaPartialNode

l-monninger · 2024-05-10T17:34:21Z

These environment variable names are prefixed with MONZA, but the configuration is currently shared. Any ideas how to improve this?

I like prefixes targeting the particular unit. So, let's go with whatever we call the opt-executor. I think maptos is best, so MAPTOS.

l-monninger · 2024-05-10T17:35:12Z

Also, @mzabaluev, why are you force pushing? Is something up?

l-monninger · 2024-05-10T17:54:25Z

@mzabaluev @0xmovses maptos is a feature.

l-monninger · 2024-05-10T17:59:57Z

@mzabaluev
For me, we can call this good for now. But...

We still need to rename the ENV vars. However, that will require updating the process-compose scripts.
I am going to make a separate ticket for adding suzuka process-compose scripts.
The finality mode bit is a little awkward because not necessarily all crates using maptos-opt-executor will have those finality modes, and they won't necessarily mean the exact same thing even if the name is the same. We may want to move that FinalityMode enum into monza.

I am going to approve. In particular, because Richard and I may have an easier time working on Suzuka over the weekend if this is merged. If you get a chance to merge, go for it.

mzabaluev · 2024-05-10T18:39:35Z

Also, @mzabaluev, why are you force pushing? Is something up?

No, just coalescing some fixup commits into larger topical changes, also rebased against latest main. If this is too disruptive for review, I will not do this. We should consider, as general pracitce, if we want to squash for the final merge to avoid many WIP and fixup commits that would be difficult to cherry-pick or revert later.

l-monninger · 2024-05-10T18:56:41Z

No, just coalescing some fixup commits into larger topical changes, also rebased against latest main. If this is too disruptive for review, I will not do this. We should consider, as general pracitce, if we want to squash for the final merge to avoid many WIP and fixup commits that would be difficult to cherry-pick or revert later.

Idc. With PR flow it doesn't matter too much. Was just wondering.

mzabaluev added 2 commits May 10, 2024 10:04

suzuka crates as clones of monza

0421d2e

Move and rename shared crates under protocol-units/execution/aptos: - opt-executor - utils

Specify workspace resolver version 2

c7190e5

mzabaluev requested a review from l-monninger May 10, 2024 07:09

l-monninger requested changes May 10, 2024

View reviewed changes

0xmovses reviewed May 10, 2024

View reviewed changes

mzabaluev requested a review from l-monninger May 10, 2024 13:58

mzabaluev added 2 commits May 10, 2024 18:55

Fix up names after Suzuka reorg

7cca168

Rename *PartialFullNode

664e786

These names were confusing, rename to MonzaPartialNode and SuzukaPartialNode

mzabaluev force-pushed the mikhail/suzuka-network-boilerplate branch from 1b0c7b3 to 664e786 Compare May 10, 2024 15:55

feat: maptos.

a0e2ae8

l-monninger approved these changes May 10, 2024

View reviewed changes

Merge branch 'main' into mikhail/suzuka-network-boilerplate

8f5db80

mzabaluev mentioned this pull request May 10, 2024

Rename MONZA_* environment variables used to configure Aptos #53

Closed

mzabaluev merged commit 4911871 into main May 10, 2024
0 of 4 checks passed

mzabaluev deleted the mikhail/suzuka-network-boilerplate branch May 10, 2024 21:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENG-314: copy monza crates to suzuka #47

ENG-314: copy monza crates to suzuka #47

mzabaluev commented May 10, 2024

l-monninger May 10, 2024

mzabaluev May 10, 2024

0xmovses May 10, 2024

mzabaluev May 10, 2024

l-monninger May 10, 2024

l-monninger May 10, 2024

0xmovses left a comment

0xmovses May 10, 2024

mzabaluev May 10, 2024

l-monninger May 10, 2024

0xmovses May 10, 2024

l-monninger May 10, 2024

0xmovses May 10, 2024

0xmovses May 10, 2024

mzabaluev May 10, 2024

l-monninger May 10, 2024

l-monninger May 10, 2024

0xmovses May 10, 2024

mzabaluev May 10, 2024

0xmovses May 10, 2024

0xmovses May 10, 2024

mzabaluev May 10, 2024

mzabaluev May 10, 2024

l-monninger May 10, 2024

0xmovses May 10, 2024

mzabaluev May 10, 2024

l-monninger May 10, 2024

mzabaluev commented May 10, 2024

l-monninger commented May 10, 2024

l-monninger commented May 10, 2024

l-monninger commented May 10, 2024

l-monninger commented May 10, 2024

mzabaluev commented May 10, 2024

l-monninger commented May 10, 2024 •

edited

Loading

	pub fn new(execution_config : movement_execution_util::config::Config) -> Self {
	pub fn new(execution_config : suzuka_execution_util::config::Config) -> Self {

	impl <T : MonzaExecutor + Send + Sync + Clone>MonzaPartialFullNode<T> {
	impl <T : SuzukaExecutor + Send + Sync + Clone>SuzukaPartialFullNode<T> {

ENG-314: copy monza crates to suzuka #47

ENG-314: copy monza crates to suzuka #47

Conversation

mzabaluev commented May 10, 2024

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

0xmovses 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

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

mzabaluev commented May 10, 2024

l-monninger commented May 10, 2024

l-monninger commented May 10, 2024

l-monninger commented May 10, 2024

l-monninger commented May 10, 2024

mzabaluev commented May 10, 2024

l-monninger commented May 10, 2024 • edited Loading

l-monninger commented May 10, 2024 •

edited

Loading