Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement API Gateway skeleton and unit tests #3

Merged
merged 1 commit into from
Mar 27, 2024

Conversation

ayeletstarkware
Copy link
Contributor

@ayeletstarkware ayeletstarkware commented Mar 14, 2024

This change is Reviewable

@codecov-commenter
Copy link

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

Copy link
Contributor

@ArniStarkware ArniStarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 5 files at r1.
Reviewable status: 2 of 5 files reviewed, 4 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @giladchase)


crates/gateway/src/gateway.rs line 31 at r1 (raw file):

        let make_svc = make_service_fn(|_conn| async {
            Ok::<_, Infallible>(service_fn(Self::handle_request))
        });

is svc an abbriviation?
I suggest we avoid those unless it is super clear. Otherwise document?

Code quote:

        let make_svc = make_service_fn(|_conn| async {
            Ok::<_, Infallible>(service_fn(Self::handle_request))
        });

crates/gateway/src/gateway.rs line 36 at r1 (raw file):

            .serve(make_svc)
            .await
            .map_err(|_| GatewayError::ServerError);

Does this work?

Is the return value interesting in any way?

Suggestion:

        Server::bind(&addr)
            .serve(make_svc)
            .await
            .map_err(|_| GatewayError::ServerError);

crates/gateway/src/gateway.rs line 51 at r1 (raw file):

        };
        response
    }

What am I missing? How come this method has no self?

Code quote:

    async fn handle_request(request: Request<Body>) -> Result<Response<Body>, GatewayError> {
        let (parts, _body) = request.into_parts();
        let response = match (parts.method, parts.uri.path()) {
            (Method::GET, "/is_alive") => is_alive(),
            _ => Ok(Response::builder()
                .status(404)
                .body(Body::from(NOT_FOUND_RESPONSE))
                .map_err(|_| GatewayError::InternalServerError)?),
        };
        response
    }

crates/gateway/src/gateway.rs line 59 at r1 (raw file):

        .body(Body::from("Server is alive"))
        .map_err(|_| GatewayError::InternalServerError)
}

What am I missing? How come this method has no self?

Code quote:

fn is_alive() -> Result<Response<Body>, GatewayError> {
    Response::builder()
        .status(200)
        .body(Body::from("Server is alive"))
        .map_err(|_| GatewayError::InternalServerError)
}

@ArniStarkware
Copy link
Contributor

crates/gateway/src/gateway.rs line 59 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

What am I missing? How come this method has no self?

This method will always succeed, as it does not actually query an instance of the gateway to see if it is alive.
Either explain that and add a Todo or make it into a method.

@ArniStarkware
Copy link
Contributor

crates/gateway/src/errors.rs line 9 at r1 (raw file):

    #[error("Error while starting the server")]
    ServerError,
}

Is there a way to add a test that will cause these Errors to be raised?

Code quote:

    #[error("Internal server error")]
    InternalServerError,
    #[error("Error while starting the server")]
    ServerError,
}

@ayeletstarkware ayeletstarkware force-pushed the ayelet/mempool/gateway-skeleton branch from 39ac4d7 to 8941ef1 Compare March 19, 2024 08:55
Copy link
Contributor Author

@ayeletstarkware ayeletstarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 of 5 files reviewed, 4 unresolved discussions (waiting on @ArniStarkware, @elintul, and @giladchase)


crates/gateway/src/gateway.rs line 36 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Does this work?

Is the return value interesting in any way?

When trying your solution I get "unused std::result::Result that must be used"
Tried another solution. Is it better now?


crates/gateway/src/gateway.rs line 51 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

What am I missing? How come this method has no self?

I think we'll need to use self when using the gateway's configurations to set up the server. The decision to place handle_request under impl gateway is worth reconsidering. What do you think?

Copy link
Contributor

@ArniStarkware ArniStarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 of 5 files reviewed, 4 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @giladchase)


crates/gateway/src/gateway.rs line 36 at r1 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

When trying your solution I get "unused std::result::Result that must be used"
Tried another solution. Is it better now?

Ok - another connected issue. Is the error being raised here? I think it is not.

Maybe this one (with the question mark - it also alters the behavior hopefully for the better.)?
In any case, your original solution is better, other than the fact that the error is not raised.

        Server::bind(&addr)
            .serve(make_svc)
            .await
            .map_err(|_| GatewayError::ServerError)?;

Copy link
Collaborator

@giladchase giladchase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 of 5 files reviewed, 11 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, @elintul, and @staticmethod)


crates/gateway/Cargo.toml line 5 at r1 (raw file):

version = "0.1.0"
edition = "2021"

if we have a main cargo.toml now move these there


crates/gateway/src/gateway.rs line 3 at r1 (raw file):

use crate::errors::GatewayError;
use hyper::service::{make_service_fn, service_fn};
use hyper::{Body, Method, Request, Response, Server};

Have you considered using axum instead of hyper for this?
They seem to be working on top of hyper and incur less boilerplate (see their usage in papyrus for example).

Code quote:

use hyper::service::{make_service_fn, service_fn};
use hyper::{Body, Method, Request, Response, Server};

crates/gateway/src/gateway.rs line 14 at r1 (raw file):

pub struct GatewayConfig {
    pub config: String,

You meant for this to be some dummy value?

For dummy values you use ZSTs (zero sized struct, that is struct GatewayConfig;.
Moreover, for non-test code we probably shouldn't add components that don't have immediate use (google YAGNI).

This isn't the case here however, because we can already add some things here that are relevant in your code, like server address (see comments below).

Code quote:

    pub config: String,

crates/gateway/src/gateway.rs line 15 at r1 (raw file):

pub struct GatewayConfig {
    pub config: String,
}

Could you move this section further down in the file plz?

Order of items in a file should be by importance, so configX should be below X stuff.

Code quote:

pub struct GatewayConfig {
    pub config: String,
}

crates/gateway/src/gateway.rs line 24 at r1 (raw file):

    pub fn new(gateway_config: GatewayConfig) -> Self {
        Self { gateway_config }
    }

new constructor is typically intended for situations where the struct's public constructor isn't available (when there are private fields).
This is true for constructors in general, they should only be present if there is some custom construction logic.


crates/gateway/src/gateway.rs line 26 at r1 (raw file):

    }

    pub async fn build_server(&self) -> Result<(), GatewayError> {

Add GatewayResult

Code quote:

Result<(), GatewayError>

crates/gateway/src/gateway.rs line 27 at r1 (raw file):

    pub async fn build_server(&self) -> Result<(), GatewayError> {
        let addr = SocketAddr::from(([127, 0, 0, 1], 8080));

This should take the address from the config.

This might be a good value for a mock though, when we end up writing one, but even then we might opt for 0.0.0.0 instead of 127.0.0.1 (lookup the difference between the two)

Code quote:

     let addr = SocketAddr::from(([127, 0, 0, 1], 8080));

crates/gateway/src/gateway.rs line 51 at r1 (raw file):

What am I missing? How come this method has no self?

Associated method (like a @staticmethod).

This is less common in rust, free functions are preferred over associated functions unless the function references Self in some way (usually constructors or using associated consts).


crates/gateway/src/gateway.rs line 59 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

This method will always succeed, as it does not actually query an instance of the gateway to see if it is alive.
Either explain that and add a Todo or make it into a method.

Yep.
Better put unimplemented!() if you can't implement it fully just yet.
Rationale: this looks like a bug for readers, also users might waste time going over this code without realizing it's a placeholder.

@ArniStarkware
Copy link
Contributor

crates/gateway/src/gateway.rs line 51 at r1 (raw file):

Previously, giladchase wrote…

What am I missing? How come this method has no self?

Associated method (like a @staticmethod).

This is less common in rust, free functions are preferred over associated functions unless the function references Self in some way (usually constructors or using associated consts).

My question is about the logic. The method handle_request should know what request it is handling and who is handling it (I assumed self).

@ayeletstarkware ayeletstarkware force-pushed the ayelet/mempool/gateway-skeleton branch from 8941ef1 to 4ce9181 Compare March 19, 2024 13:11
@ayeletstarkware ayeletstarkware changed the base branch from ayelet/mempool/setup to main March 19, 2024 13:11
Copy link
Contributor Author

@ayeletstarkware ayeletstarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 of 6 files reviewed, 11 unresolved discussions (waiting on @ArniStarkware, @elintul, @giladchase, and @staticmethod)


crates/gateway/Cargo.toml line 5 at r1 (raw file):

Previously, giladchase wrote…

if we have a main cargo.toml now move these there

Done.


crates/gateway/src/errors.rs line 9 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Is there a way to add a test that will cause these Errors to be raised?

InternalServerError is checked in test_invalid_request. I haven't tested ServerError because, so far, is_alive has never failed.


crates/gateway/src/gateway.rs line 3 at r1 (raw file):

Previously, giladchase wrote…

Have you considered using axum instead of hyper for this?
They seem to be working on top of hyper and incur less boilerplate (see their usage in papyrus for example).

Will consider it in a different PR.


crates/gateway/src/gateway.rs line 14 at r1 (raw file):

Previously, giladchase wrote…

You meant for this to be some dummy value?

For dummy values you use ZSTs (zero sized struct, that is struct GatewayConfig;.
Moreover, for non-test code we probably shouldn't add components that don't have immediate use (google YAGNI).

This isn't the case here however, because we can already add some things here that are relevant in your code, like server address (see comments below).

Thanks!


crates/gateway/src/gateway.rs line 15 at r1 (raw file):

Previously, giladchase wrote…

Could you move this section further down in the file plz?

Order of items in a file should be by importance, so configX should be below X stuff.

Done.


crates/gateway/src/gateway.rs line 24 at r1 (raw file):

Previously, giladchase wrote…

new constructor is typically intended for situations where the struct's public constructor isn't available (when there are private fields).
This is true for constructors in general, they should only be present if there is some custom construction logic.

Done.


crates/gateway/src/gateway.rs line 26 at r1 (raw file):

Previously, giladchase wrote…

Add GatewayResult

Done.


crates/gateway/src/gateway.rs line 27 at r1 (raw file):

Previously, giladchase wrote…

This should take the address from the config.

This might be a good value for a mock though, when we end up writing one, but even then we might opt for 0.0.0.0 instead of 127.0.0.1 (lookup the difference between the two)

Done.


crates/gateway/src/gateway.rs line 36 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Ok - another connected issue. Is the error being raised here? I think it is not.

Maybe this one (with the question mark - it also alters the behavior hopefully for the better.)?
In any case, your original solution is better, other than the fact that the error is not raised.

        Server::bind(&addr)
            .serve(make_svc)
            .await
            .map_err(|_| GatewayError::ServerError)?;

Done.


crates/gateway/src/gateway.rs line 51 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

My question is about the logic. The method handle_request should know what request it is handling and who is handling it (I assumed self).

Done.


crates/gateway/src/gateway.rs line 59 at r1 (raw file):

Previously, giladchase wrote…

Yep.
Better put unimplemented!() if you can't implement it fully just yet.
Rationale: this looks like a bug for readers, also users might waste time going over this code without realizing it's a placeholder.

Done.

Copy link
Contributor

@ArniStarkware ArniStarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 of 6 files reviewed, 9 unresolved discussions (waiting on @elintul, @giladchase, and @staticmethod)


crates/gateway/src/gateway.rs line 51 at r1 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

Done.

Please give this function the same treatment is_alive() is getting. (With the same rational @giladchase mentioned).

@ayeletstarkware ayeletstarkware force-pushed the ayelet/mempool/gateway-skeleton branch from 4ce9181 to 236945f Compare March 19, 2024 15:36
Copy link
Contributor Author

@ayeletstarkware ayeletstarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 of 6 files reviewed, 9 unresolved discussions (waiting on @ArniStarkware, @elintul, @giladchase, and @staticmethod)


crates/gateway/src/gateway.rs line 51 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Please give this function the same treatment is_alive() is getting. (With the same rational @giladchase mentioned).

Done. Reference:
https://github.com/microsoft/vscode/blob/9a2716e2fbb7e00f983d4e2fe35af9cd2bfc37f3/cli/src/commands/serve_web.rs#L16

Copy link
Contributor

@ArniStarkware ArniStarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 of 6 files reviewed, 8 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @giladchase)


crates/gateway/src/gateway.rs line 43 at r4 (raw file):

#[derive(Clone)]
struct HandleContext {}

Please document.
Holds the routing info to...

Code quote:

#[derive(Clone)]
struct HandleContext {}

Copy link
Collaborator

@giladchase giladchase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 5 files at r1, 1 of 2 files at r4, all commit messages.
Reviewable status: 2 of 6 files reviewed, 5 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, and @elintul)


Cargo.toml line 14 at r4 (raw file):

[workspace.dependencies]
hyper = "0.13.9"
tokio = { version = "0.2", features = ["full"] }

What does this give us?

Code quote:

 features = ["full"] 

crates/gateway/Cargo.toml line 12 at r4 (raw file):

[lib]
name = "gateway_lib"

What does this do?

Code quote:

[lib]
name = "gateway_lib"

crates/gateway/src/gateway.rs line 59 at r1 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

Done.

I meant unimplemented! as the entire function, adding it as dead code is somewhat confusing 😅
Either that or what Arni said, add a todo (without the if true thing)


crates/gateway/src/gateway.rs line 22 at r4 (raw file):

    pub async fn build_server(&self) -> GatewayResult {
        let addr = SocketAddr::from_str(&self.gateway_config.bind_address)
            .map_err(|_| GatewayError::ServerError)?;
  1. Try making this one fail :)
    The trace will say:
called `Result::unwrap()` on an `Err` value: ServerError

Which is somewhat hard to debug.

There's a missing abstraction.

Add GatewayConfigError thiserror enum with

    #[error("Server address is not an IP address: {0}")]
    InvalidServerAddress(String),

then add to GatewayError:

#[transparent]
ConfigError(#[from] GatewayConfigError)

finally make the line into

        let addr = SocketAddr::from_str(&self.gateway_config.bind_address).or_else(|_| {
            GatewayConfigError::InvalidServerAddress(self.gateway_config.bind_address.clone())
        })?;

where the ? will implicitly cast the GatewayConfigError into GatewayError.

  1. Revisit the other .map_err(|_| GatewayError::ServerError)?; and do the same process: make them fail (for sanity), then improve the error message into something that'll give the person debugging this in production the relevant information to fix the bug.

Code quote:

        let addr = SocketAddr::from_str(&self.gateway_config.bind_address)
            .map_err(|_| GatewayError::ServerError)?;

crates/gateway/src/gateway.rs line 67 at r4 (raw file):

            .status(200)
            .body(Body::from("Server is alive"))
            .map_err(|_| GatewayError::InternalServerError);

Lets encapsulate this in a some function, will make it easier to move to switch from hyper if we decide to do so.

Code quote:

        return Response::builder()
            .status(200)
            .body(Body::from("Server is alive"))
            .map_err(|_| GatewayError::InternalServerError);

@ayeletstarkware ayeletstarkware force-pushed the ayelet/mempool/gateway-skeleton branch from 236945f to 465fd77 Compare March 20, 2024 14:04
Copy link
Contributor Author

@ayeletstarkware ayeletstarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 of 6 files reviewed, 5 unresolved discussions (waiting on @ArniStarkware, @elintul, and @giladchase)


Cargo.toml line 14 at r4 (raw file):

Previously, giladchase wrote…

What does this give us?

It enables all Tokio features, following the usage pattern found in the Microsoft repository.


crates/gateway/Cargo.toml line 12 at r4 (raw file):

Previously, giladchase wrote…

What does this do?

Apparently nothing


crates/gateway/src/gateway.rs line 22 at r4 (raw file):

Previously, giladchase wrote…
  1. Try making this one fail :)
    The trace will say:
called `Result::unwrap()` on an `Err` value: ServerError

Which is somewhat hard to debug.

There's a missing abstraction.

Add GatewayConfigError thiserror enum with

    #[error("Server address is not an IP address: {0}")]
    InvalidServerAddress(String),

then add to GatewayError:

#[transparent]
ConfigError(#[from] GatewayConfigError)

finally make the line into

        let addr = SocketAddr::from_str(&self.gateway_config.bind_address).or_else(|_| {
            GatewayConfigError::InvalidServerAddress(self.gateway_config.bind_address.clone())
        })?;

where the ? will implicitly cast the GatewayConfigError into GatewayError.

  1. Revisit the other .map_err(|_| GatewayError::ServerError)?; and do the same process: make them fail (for sanity), then improve the error message into something that'll give the person debugging this in production the relevant information to fix the bug.

1 - I get the following error when using or_else, so I used map_err

Code snippet:

using `Result.or_else(|x| Err(y))`, which is more succinctly expressed as `map_err(|x| y)`
for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#bind_instead_of_map
`#[warn(clippy::bind_instead_of_map)]` on by defaultclippyClick for full compiler diagnostic
gateway.rs(28, 20): try: `SocketAddr::from_str(&self.gateway_config.bind_address).map_err(|_| GatewayError::ConfigError(GatewayConfigError::InvalidServerBindAddress(self.gateway_config.bind_address.clone())))`

crates/gateway/src/gateway.rs line 43 at r4 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Please document.
Holds the routing info to...

Done.


crates/gateway/src/gateway.rs line 67 at r4 (raw file):

Previously, giladchase wrote…

Lets encapsulate this in a some function, will make it easier to move to switch from hyper if we decide to do so.

Done.

Copy link
Collaborator

@giladchase giladchase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 of 6 files reviewed, 5 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, and @elintul)


Cargo.toml line 14 at r4 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

It enables all Tokio features, following the usage pattern found in the Microsoft repository.

I'd rather be incremental if possible.
VSCode probably uses many features which we (for now) don't need, let's remove all if possible and reintroduce on demand.


crates/gateway/Cargo.toml line 12 at r4 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

Apparently nothing

:)


crates/gateway/src/gateway.rs line 22 at r4 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

1 - I get the following error when using or_else, so I used map_err

Ya yr right, but you can remove the GatewayError::ConfigError() part, only GatewayConfigerror::InvalidServerBindAddress is needed.
Ping me if you want me to explain why that works.


crates/gateway/src/gateway.rs line 73 at r5 (raw file):

}

fn build_generic_response(

Succinctness

Suggestion:

fn response(

crates/gateway/src/gateway.rs line 81 at r5 (raw file):

        .status(status)
        .body(Body::from(body_content))
        .map_err(|_| error)

Add to GatewayError

#[error(transparent)]
HTTPError(#[from] hyper::http::Error)

GatewayError needs to know about all errors coming from this interface

Suggestion:

    Ok(Response::builder()
        .status(status)
        .body(Body::from(body_content))?)

@ayeletstarkware ayeletstarkware force-pushed the ayelet/mempool/gateway-skeleton branch from 465fd77 to b66023d Compare March 21, 2024 11:59
Copy link
Contributor Author

@ayeletstarkware ayeletstarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 of 6 files reviewed, 5 unresolved discussions (waiting on @ArniStarkware, @elintul, and @giladchase)


Cargo.toml line 14 at r4 (raw file):

Previously, giladchase wrote…

I'd rather be incremental if possible.
VSCode probably uses many features which we (for now) don't need, let's remove all if possible and reintroduce on demand.

Done.


crates/gateway/src/gateway.rs line 22 at r4 (raw file):

Previously, giladchase wrote…

Ya yr right, but you can remove the GatewayError::ConfigError() part, only GatewayConfigerror::InvalidServerBindAddress is needed.
Ping me if you want me to explain why that works.

Done.


crates/gateway/src/gateway.rs line 73 at r5 (raw file):

Previously, giladchase wrote…

Succinctness

Done.


crates/gateway/src/gateway.rs line 81 at r5 (raw file):

Previously, giladchase wrote…

Add to GatewayError

#[error(transparent)]
HTTPError(#[from] hyper::http::Error)

GatewayError needs to know about all errors coming from this interface

Done.

Copy link
Collaborator

@giladchase giladchase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 of 6 files reviewed, 3 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, and @elintul)


crates/gateway/src/errors.rs line 12 at r6 (raw file):

    InternalServerError,
    #[error("Invalid transaction format")]
    InvalidTransactionFormat,

unused?

Code quote:

    #[error("Invalid transaction format")]
    InvalidTransactionFormat,

crates/gateway/src/gateway.rs line 35 at r6 (raw file):

            .serve(make_service)
            .await
            .map_err(|_| GatewayError::ServerStartError)?;

and add a from in the error enum

Suggestion:

        Server::bind(&addr)
            .serve(make_service)
            .await?;

@ayeletstarkware ayeletstarkware force-pushed the ayelet/mempool/gateway-skeleton branch from b66023d to eed3f44 Compare March 25, 2024 09:37
Copy link
Contributor Author

@ayeletstarkware ayeletstarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 of 6 files reviewed, 3 unresolved discussions (waiting on @ArniStarkware, @elintul, and @giladchase)


crates/gateway/src/errors.rs line 12 at r6 (raw file):

Previously, giladchase wrote…

unused?

unused in this PR. will be added again in the next one.


crates/gateway/src/gateway.rs line 35 at r6 (raw file):

Previously, giladchase wrote…

and add a from in the error enum

Done.

Copy link
Contributor

@ArniStarkware ArniStarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 5 files at r5, 2 of 3 files at r6, 2 of 2 files at r7.
Reviewable status: all files reviewed (commit messages unreviewed), 2 unresolved discussions (waiting on @elintul and @giladchase)

Copy link
Contributor

@ArniStarkware ArniStarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed (commit messages unreviewed), 4 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @giladchase)


crates/gateway/src/gateway.rs line 44 at r7 (raw file):

/// Stores routing information for request's handling.
#[derive(Clone)]
struct HandleContext {}

Non-blocking.

Sorry for being inconsistent.

Now that we simply set is_alive as unimplemented!, there is no need for this struct.
Let us delete it for now, and add it when the need arises.


crates/gateway/src/gateway.rs line 59 at r7 (raw file):

// This function hasn't been implemented yet. It might need a HandleContext parameter to verify if
// the server is alive.

Non-blocking.

Delete this comment if the HandleContext parameter is removed.

@ayeletstarkware ayeletstarkware force-pushed the ayelet/mempool/gateway-skeleton branch from eed3f44 to 7643196 Compare March 25, 2024 15:20
Copy link
Collaborator

@giladchase giladchase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 5 files at r5, 1 of 3 files at r6, 1 of 2 files at r7, 2 of 2 files at r8, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @elintul)


Cargo.toml line 28 at r8 (raw file):

serde = { version = "1.0.193", features = ["derive"] }
serde_json = "1.0"
assert_matches = "1.5.0"

alph., also in the crate's cargo.toml

Code quote:

hyper = "0.13.9"
tokio = { version = "0.2", features = ["macros"] }
thiserror = "1.0"
starknet_api = "0.8.0"
serde = { version = "1.0.193", features = ["derive"] }
serde_json = "1.0"
assert_matches = "1.5.0"

crates/gateway/src/gateway.rs line 44 at r8 (raw file):

    let response = match (parts.method, parts.uri.path()) {
        (Method::GET, "/is_alive") => is_alive(),
        _ => response(StatusCode::NOT_FOUND, NOT_FOUND_RESPONSE.to_string()),

minor could we have used StatusCode::NOT_FOUND instead of our own stringy version?
Not critical for now though, we might be moving to axum anyway...

Code quote:

        _ => response(StatusCode::NOT_FOUND, NOT_FOUND_RESPONSE.to_string()),

Copy link
Contributor Author

@ayeletstarkware ayeletstarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @elintul)


Cargo.toml line 28 at r8 (raw file):

Previously, giladchase wrote…

alph., also in the crate's cargo.toml

Done in the next PR.

@ayeletstarkware ayeletstarkware merged commit 4a0e789 into main Mar 27, 2024
6 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants