feat(sidecar): firewall delegation #552
Conversation
…, return recv upon run
thedevbirb
force-pushed
the
lore/feat/firewall-delegation
branch
from
e17add1 to
ab05565
Compare
| long, | ||
| env = "BOLT_SIDECAR_FIREWALL_RPC", | ||
| value_delimiter = ',', | ||
| conflicts_with("commitments_port") |
There was a problem hiding this comment.
| conflicts_with("commitments_port") | |
| conflicts_with("port") |
| pub struct CommitmentOpts { | ||
| /// Port to listen on for incoming JSON-RPC requests of the Commitments API. | ||
| /// This port should be open on your firewall in order to receive external requests! | ||
| #[clap(long, env = "BOLT_SIDECAR_PORT", default_value = stringify!(DEFAULT_RPC_PORT))] |
There was a problem hiding this comment.
with the current logic, won't this default value always default to using the commitments server even if you don't specify it?
| group = ArgGroup::new("commitments-opts").required(true) | ||
| .args(&["commitments_port", "firewall_rpc"]) | ||
| )] | ||
| pub struct CommitmentOpts { |
There was a problem hiding this comment.
What do you think of this pattern to match on which method to use?
BOLT_SIDECAR_COMMITMENT_SERVER_PORT=8017
BOLT_SIDECAR_USE_GATEWAYS=true
BOLT_SIDECAR_GATEWAY_URLS="1, 2, 3, ..."if use_gateways {
// start the firewall thing
} else {
// start the server at port
}|
|
||
| impl Debug for CommitmentsFirewallRecv { | ||
| fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { | ||
| f.debug_struct("CommitmentsFirewallStream") |
There was a problem hiding this comment.
| f.debug_struct("CommitmentsFirewallStream") | |
| f.debug_struct("CommitmentsFirewallRecv") |
| } | ||
|
|
||
| // Reconnect on failure | ||
| // TODO: add backoff |
There was a problem hiding this comment.
we could use the retry_with_backoff util here :)
| /// NOTE: Is there a better way to avoid this monster type? | ||
| /// SAFETY: the `poll` implementation of this struct promptly handles these responses and | ||
| /// ensures this vector doesn't grow indefinitely. | ||
| pending_commitment_responses: FuturesUnordered< | ||
| Pin< | ||
| Box< | ||
| dyn Future< | ||
| Output = Result< | ||
| Result<SignedCommitment, CommitmentError>, | ||
| oneshot::error::RecvError, | ||
| >, | ||
| > + Send, | ||
| >, | ||
| >, | ||
| >, |
There was a problem hiding this comment.
A good start could be to extract the type alias like
type PendingCommitmentResult = impl Future<Output = Result<SignedCommitment, CommitmentError>>;
pending_commitment_responses: FuturesUnordered<PendingCommitmentResult>or similar (if you need pin<box as well)
Also I'm pretty sure we can get rid of the oneshot channel error and handle it in the poll implementation, as it can only happen when the sender is dropped aka if everything is shutting down
| while let Poll::Ready(maybe_message) = this.read_stream.poll_next_unpin(cx) { | ||
| progress = true; | ||
|
|
||
| match maybe_message { |
There was a problem hiding this comment.
overall the matching going on here feels a bit heavy!
can you make the control flow more linear by using something like:
let Some(Ok(message)) = maybe_message else {
warn!("websocket connection with {rpc_url} dropped");
return Poll::Ready(());
};
match message {
Message::Text(...
Message::Close(_) ...| Err(e) => { | ||
| error!(?e, "failed to receive commitment response"); | ||
| } |
There was a problem hiding this comment.
if this is a oneshot::error then we don't need to match on this, as it can only happen if the sender is dropped
No description provided.