Verification + refactoring with preparations to generalization #202

taco-paco · 2024-09-26T07:11:10Z

The main idea behind the refactoring is to prepare for the future extraction of main components into a separate library. The following components may roughly be noticed already.

pub trait MessageProcessor {
    type Message;
    async fn process_message(&self, message: Self::Message) -> impl Into<TaskResult>;
}

pub trait InputPreparator {
     type Request;
     type Output;
     async fn prepare_input(&self, request: &Self::Request) -> Result<Self::Output>;
}

pub trait MessageValidator {
     type Message;
     async fn validate(&self, message: &Self::Message) -> Result<()>
}

maksimryndin

LGTM!

maksimryndin · 2024-10-02T11:38:44Z

crates/lambdas/Cargo.toml

+
+[[bin]]
+name = "verify"
+version = "0.0.1"


Nitpick: conventionally, the first version is 0.1.0 https://semver.org/#how-should-i-deal-with-revisions-in-the-0yz-initial-development-phase. But it is only a convention :)

And the version can be set at a workspace level, e.g.
https://github.com/NethermindEth/dydx-v4-rust/blob/trunk/Cargo.toml and
https://github.com/NethermindEth/dydx-v4-rust/blob/trunk/client/Cargo.toml

0.1.0 makes sense.

And the version can be set at a workspace level, e.g.

I'm afraid release cycle for components won't be the same

maksimryndin · 2024-10-02T11:52:15Z

crates/lambdas/src/verify.rs

+            PutItemError::ConditionalCheckFailedException(_) => {
+                error!("Reverification attempt, id: {}", request.id);
+                let response = lambda_http::Response::builder()
+                    .status(400)


Nitpick: perhaps, it is worth to use a designated const for readability

All of them were replaced here. Child branch tho

maksimryndin · 2024-10-02T11:55:25Z

crates/lambdas/src/verify.rs

+            }
+            _ => return Err(Box::new(SdkError::ServiceError(val)).into()),
+        },
+        Err(err) => return Err(Box::new(err).into()),


Nitpick: probably, error handling would be more ergonomic with anyhow crate?

maksimryndin · 2024-10-02T11:58:21Z

crates/lambdas/src/verify.rs

+        .await
+        .map_err(Box::new)?;
+
+    if let None = &objects.contents {


Nitpick: more idiomatic

objects.contents.as_ref().ok_or_else(|| {...})?;

maksimryndin · 2024-10-02T12:11:35Z

crates/types/src/item/errors.rs

+#[derive(thiserror::Error, Debug)]
+pub enum ItemError {
+    #[error("Invalid Item format: {0}")]
+    FormatError(String),


Nitpick: FormatError can wrap anyhow::Error (https://github.com/dtolnay/thiserror). In this case it would be more ergonomic to use with anyhow macros

maksimryndin · 2024-10-02T12:25:03Z

crates/worker/src/commands/compile.rs

@@ -66,8 +78,19 @@ pub async fn do_compile(
    let hardhat_config_content = hardhat_config_builder.build().to_string_config();

    // create parent directories
-    tokio::fs::create_dir_all(hardhat_config_path.parent().unwrap()).await?;
-    tokio::fs::write(hardhat_config_path, hardhat_config_content).await?;
+    tokio::fs::create_dir_all(hardhat_config_path.parent().unwrap())


Can we raise an Error (with ?) here instead of panicking?

maksimryndin · 2024-10-02T12:30:36Z

crates/worker/src/commands/verify.rs

+        .to_string_config();
+
+    // create parent directories
+    tokio::fs::create_dir_all(hardhat_config_path.parent().unwrap())


Could we return early an Error (with ?) here instead of unwrap?

maksimryndin · 2024-10-02T12:53:12Z

crates/worker/src/processor/compile_processor.rs

+        };
+
+        let mut file_keys = Vec::with_capacity(compilation_output.artifacts_data.len());
+        for el in compilation_output.artifacts_data {


Could we upload files concurrently?

maksimryndin · 2024-10-02T12:54:15Z

crates/worker/src/processor/compile_processor.rs

+
+        let mut presigned_urls = Vec::with_capacity(file_keys.len());
+        for el in file_keys {
+            let expires_in = PresigningConfig::expires_in(DOWNLOAD_URL_EXPIRATION).unwrap();


early error return with ??

It's just never fails with current implementation. If it does in case dependancies change, then we would fail all the time here anyway.

pub fn build(self) -> Result<PresigningConfig, PresigningConfigError> { let expires_in = self.expires_in.ok_or(ErrorKind::ExpiresInRequired)?; if expires_in > ONE_WEEK { return Err(ErrorKind::ExpiresInDurationTooLong.into()); } Ok(PresigningConfig { start_time: self.start_time.unwrap_or_else( // This usage is OK—customers can easily override this. #[allow(clippy::disallowed_methods)] SystemTime::now, ), expires_in, }) }

maksimryndin · 2024-10-02T12:59:33Z

crates/worker/src/processor/compile_processor.rs

+        let mut presigned_urls = Vec::with_capacity(file_keys.len());
+        for el in file_keys {
+            let expires_in = PresigningConfig::expires_in(DOWNLOAD_URL_EXPIRATION).unwrap();
+            let presigned_request = self


Could we make requests concurrently?

varex83 · 2024-10-13T15:28:57Z

crates/worker/src/commands/compile.rs

+        })?;
+    tokio::fs::create_dir_all(&artifacts_path)
+        .await
+        .map_err(anyhow::Error::from)


Why do we use the anyhow::Error::from, can't we just implement From<...> for our CompilationError?

Noticed that can be with_context, it wraps to anyhow automatically.
with regards to:

Why do we use the anyhow::Error::from, can't we just implement From<...> for our CompilationError?

Not done this way since while every anyhow::Error is CompilationError::UnknownError, not every io::Error is CompilationError::UnknownError

taco-paco requested a review from varex83 September 26, 2024 07:11

maksimryndin approved these changes Oct 2, 2024

View reviewed changes

varex83 reviewed Oct 13, 2024

View reviewed changes

taco-paco force-pushed the feat/horizontal/verify branch from 209a387 to 5550912 Compare October 18, 2024 12:41

taco-paco added 13 commits October 18, 2024 15:46

feat: introduce processor to handle messages

910a32a

refactoring: split into domains, preparation for VerificationProcessor

b6a0186

feat: compiles

51392d5

refactor: some cleaning

d5c853e

refactor: fmt applied

0e1d64a

refactor: file structure - processors in separate module

0f9388c

feat: modify Item's format. added tests for conversions

eee6ea2

feat: Item::TaskFailure format changed

cd38fd1

feat: apply changes

d83a5df

feat: add verify processor

bd2e369

refactor: remove unused method

5409af7

refactor: renaming

6c0142f

random change

3bf7319

taco-paco force-pushed the feat/horizontal/verify branch from 5550912 to 3bf7319 Compare October 18, 2024 12:47

taco-paco changed the base branch from feat/horizontal/retriable-client to feat/horizontal/main October 18, 2024 12:48

taco-paco merged commit b87a0dd into feat/horizontal/main Oct 18, 2024
2 of 6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verification + refactoring with preparations to generalization #202

Verification + refactoring with preparations to generalization #202

taco-paco commented Sep 26, 2024 •

edited

Loading

maksimryndin left a comment

maksimryndin Oct 2, 2024

taco-paco Oct 17, 2024 •

edited

Loading

taco-paco Oct 17, 2024

maksimryndin Oct 2, 2024

taco-paco Oct 17, 2024

maksimryndin Oct 2, 2024

taco-paco Oct 17, 2024

maksimryndin Oct 2, 2024

taco-paco Oct 17, 2024

maksimryndin Oct 2, 2024

maksimryndin Oct 2, 2024

taco-paco Oct 17, 2024

maksimryndin Oct 2, 2024

taco-paco Oct 17, 2024

maksimryndin Oct 2, 2024

taco-paco Oct 17, 2024

maksimryndin Oct 2, 2024

taco-paco Oct 17, 2024

maksimryndin Oct 2, 2024

taco-paco Oct 17, 2024

varex83 Oct 13, 2024

taco-paco Oct 18, 2024

Verification + refactoring with preparations to generalization #202

Verification + refactoring with preparations to generalization #202

Conversation

taco-paco commented Sep 26, 2024 • edited Loading

maksimryndin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taco-paco Oct 17, 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

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

taco-paco commented Sep 26, 2024 •

edited

Loading

taco-paco Oct 17, 2024 •

edited

Loading