-
Notifications
You must be signed in to change notification settings - Fork 3
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
Integrate Blocklist Client with Signer #154
Integrate Blocklist Client with Signer #154
Conversation
d83bd38
to
b28787c
Compare
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.
Took a quick glance, will circle back later.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
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.
Great stuff! Just commenting on two minor things that I think are redundant in the current code.
#[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What purpose does the Mutex
serve here? It looks to me like we could omit it entirely.
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.
Had a big question about how we should interface with the blocklist client from the signer. I'm curious about your thoughts.
use blocklist_client::common::error::Error; | ||
use blocklist_client::common::BlocklistStatus; |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.