Integrate Blocklist Client with Signer #154
Conversation
8marz8
force-pushed
the
79-dynamic-blocklist-integration-with-bootstrap-signer
branch
from
d83bd38
to
b28787c
Compare
| if self.server.host.is_empty() { | ||
| return Err(ConfigError::Message("Host cannot be empty".to_string())); | ||
| } | ||
| if !(1..=65535).contains(&self.server.port) { |
There was a problem hiding this comment.
I think this can be simplified to port != 0.
Alternatively, maybe we could try to parse the ServerConfig field(s) as std::net::SocketAddr? I think this would require writing a custom deserialize_with function or something like that since they do not implement Deserialize.
There was a problem hiding this comment.
Actually, I'm not entirely sure we should check for port zero, even though the IANA lists the port as reserved. I think I'd skip this check entirely and accept whatever is acceptable to std::net::SocketAddr.
| #[async_trait] | ||
| pub trait BlocklistChecker { | ||
| /// Checks if the given address is blocklisted. | ||
| /// Returns `true` if the address is blocklisted, otherwise `false`. | ||
| async fn is_blocklisted(&self, address: &str) -> Result<bool, Error>; | ||
| } |
There was a problem hiding this comment.
Nice trait. We don't need to use #[async_trait] anymore though since async traits has been stabilized in Rust 1.75. I'd suggest skipping it entirely and formulating the trait as
| #[async_trait] | |
| pub trait BlocklistChecker { | |
| /// Checks if the given address is blocklisted. | |
| /// Returns `true` if the address is blocklisted, otherwise `false`. | |
| async fn is_blocklisted(&self, address: &str) -> Result<bool, Error>; | |
| } | |
| pub trait BlocklistChecker { | |
| /// Checks if the given address is blocklisted. | |
| /// Returns `true` if the address is blocklisted, otherwise `false`. | |
| fn is_blocklisted( | |
| &self, | |
| address: &str, | |
| ) -> impl std::future::Future<Output = Result<bool, Error>> + Send; | |
| } |
(the impl std::future::Future<...> is just desugaring the async fn, otherwise the compiler warns due to some current limitations I don't fully understand yet - but you can still do async fn in the impl block).
There was a problem hiding this comment.
Cool, yea I added that after getting the warning. But I can remove the extra crate that does the sugaring.
| const ADDRESS: &str = "0x2337bBCD5766Bf2A9462D493E9A459b60b41B7f2"; | ||
|
|
||
| struct TestContext { | ||
| server_guard: Mutex<ServerGuard>, |
There was a problem hiding this comment.
What purpose does the Mutex serve here? It looks to me like we could omit it entirely.
| use blocklist_client::common::error::Error; | ||
| use blocklist_client::common::BlocklistStatus; |
There was a problem hiding this comment.
Hmmm, I don't think we should import the blocklist client. Instead, we should have the blocklist client have an API and we build the types from the API. One way is to have an OpenAPI spec that we build the types from. Another approach is to write protobufs and build the types that way.
This is not a simple change, so what are your thoughts on this?
There was a problem hiding this comment.
I am open to those more robust approaches...Since the client is small and only exposes one API, I thought this type of dependency/API invocation would be sufficient.
@netrome any preferences here?
There was a problem hiding this comment.
Generating an API and building types from it seems like an excessive amount of work compared to just importing the types directly, leveraging the fact that we share a common code base.
In this scenario I'm more in favor of keeping things lean from now, although I would agree with the approach for a more extensive API with multiple consumers.
| if self.server.host.is_empty() { | ||
| return Err(ConfigError::Message("Host cannot be empty".to_string())); | ||
| } | ||
| if !(1..=65535).contains(&self.server.port) { |
There was a problem hiding this comment.
Actually, I'm not entirely sure we should check for port zero, even though the IANA lists the port as reserved. I think I'd skip this check entirely and accept whatever is acceptable to std::net::SocketAddr.
|
From our discussion, we decided to introduce an OpenAPI spec to interface with the Blocklist client. |
Partially addresses #79
This PR adds a blocklist interface/client to interact with the local blocklist server.
Basic config loading is added to optionally provide host/port for the blocklist server.
The full integration will happen using this client once the signer runloop/structure is implemented.