-
Notifications
You must be signed in to change notification settings - Fork 18
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
New http api to nomos-node integration #490
Conversation
async fn add_cert(State(store): State<OverwatchHandle>, Json(cert): Json<Certificate>) -> Response { | ||
async fn add_cert( | ||
State(handle): State<OverwatchHandle>, | ||
Json(cert): Json<Certificate>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This handle should expect Cert that is serialized with wire::serialize
. At least mempool-da/add
had it in such way.
Expecting bytes and then deserializing it with wire::deserialize::<Certificate>(&payload).unwrap()
works, but I think this would ultimately need it's own extractor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, thanks! I think this should be the reason why the CI continues failing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh, if all other API are json we should be consistent and use json here as well unless there's a specific reason not to do so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh, if all other API are json we should be consistent and use json here as well unless there's a specific reason not to do so
Yes, make sense, I will try to make the CI works by using JSON args.
tests/Cargo.toml
Outdated
@@ -23,7 +23,7 @@ mixnet-topology = { path = "../mixnet/topology" } | |||
# Using older versions, since `mixnet-*` crates depend on `rand` v0.7.3. | |||
rand = "0.7.3" | |||
once_cell = "1" | |||
secp256k1 = { version = "0.26", features = ["rand"] } | |||
secp256k1 = { version = "0.28", features = ["rand"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we update this here? Are we using something not available in other versions? Otherwise it would be better if it has its own pr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be updated by vscode plugin. I will change it back.
Ok(Self { settings }) | ||
Ok(Self { | ||
settings, | ||
handle: service_state.overwatch_handle, | ||
}) | ||
} | ||
|
||
/// Service main loop | ||
async fn run(mut self) -> Result<(), DynError> { | ||
let endpoint = B::new(self.settings.backend_settings).await?; | ||
endpoint.serve().await?; | ||
endpoint.serve(self.handle).await?; | ||
Ok(()) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably needs to be updated. There are things to do here. Usually you would want the service to hold the whole service state. Lifecycle is ready as well. Not to be done now, but lets open an issue to handle the service here. I can probably implement that myself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created at #500.
#[derive(Debug, Clone, serde::Deserialize, serde::Serialize)] | ||
#[cfg_attr(feature = "clap", derive(clap::Args))] | ||
pub struct AxumBackendSettings { | ||
pub addr: SocketAddr, | ||
pub handle: OverwatchHandle, | ||
/// Socket where the server will be listening on for incoming requests. | ||
#[cfg_attr(feature = "clap", arg( | ||
short, long = "http-addr", | ||
default_value_t = std::net::SocketAddr::new( | ||
std::net::IpAddr::V4(std::net::Ipv4Addr::new(127, 0, 0, 1)), | ||
8080, | ||
), | ||
env = "HTTP_BIND_ADDRESS" | ||
))] | ||
pub address: std::net::SocketAddr, | ||
/// Allowed origins for this server deployment requests. | ||
#[cfg_attr(feature = "clap", arg(long = "http-cors-origin"))] | ||
pub cors_origins: Vec<String>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have clap here? Are we using it as standalone somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the old HTTP service, we have clap, so I just keep them in the new HTTP service. I have not seen we use it standalone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should remove it if we do not use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! and CI is 🟢 !
Just left a tiny comment.
use nomos_mempool::network::adapters::libp2p::{Libp2pAdapter, Settings as AdapterSettings}; | ||
use nomos_network::backends::libp2p::Libp2p; | ||
|
||
use nomos_mempool::network::adapters::libp2p::Settings as AdapterSettings; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this change here? This is related to mempool right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we need this setting to construct a mempool service.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why the adapter and the backend is gone now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the previous, we had type aliases for creating bridges.
* Add raw api module to nomos-cli Add a separate module which contains all raw api calls and make them reuse the same reqwest client for increased efficiency * Use json instead of wire format in mempool add api Uniform all http api to use json as the encoding
Put a hard limit of 512 blocks in the response returned by GetBlocks to avoid slowing things down. This number was chosen rather arbitrarily. We might want to do some more fine tuning.
* Readd docker build context * Configurable da protocol voter * Do not use private key naming for da voter
* Add option to provide custom writer to the logger backend * fmt
* Do not exit the process during dissemination This is now library code and we should not exit the process if an error is encountered * clippy happy
add HTTP requests header
23c023d
to
c6f9590
Compare
888d233
to
eda0e4e
Compare
Integrate new http api crate to nomos-node.