STR-809 Fix Bridge Client Bug and add functional Test #564
Conversation
| /// The RPC client to connect to the RPC full node. | ||
| pub l2_rpc_client: L2Client, | ||
| pub l2_rpc_client_pool: Pool<WsClientManager>, |
There was a problem hiding this comment.
Perhaps make a type alias like type WsClientPool = Pool<WsClientManager>; that can be re-exported from where WsClientManager is defined instead of having to make downstream consumers import the Pool type.
| info!(scope=?scope, "getting messages from the L2 Client"); | ||
| let received_payloads = self | ||
| .l2_rpc_client | ||
| info!(scope = ?scope, "getting messages from the L2 Client"); |
| Err(_) => WsClientState::NotWorking, | ||
| }; |
There was a problem hiding this comment.
If we try to create a new instance and it starts off as being NotWorking that should be an error right? When do we update the state from Working to NotWorking?
| WsClientState::NotWorking => Err(ClientError::Transport(BoxError::from( | ||
| "Client is Not Working".to_string(), | ||
| ))), | ||
| } | ||
| } | ||
|
|
||
| /// Sends a method call request. | ||
| async fn request<R, Params>(&self, method: &str, params: Params) -> Result<R, ClientError> | ||
| where | ||
| R: DeserializeOwned, | ||
| Params: ToRpcParams + Send, | ||
| { | ||
| match &self.0 { | ||
| WsClientState::Working(inner) => inner.request(method, params).await, | ||
| WsClientState::NotWorking => Err(ClientError::Transport(BoxError::from( | ||
| "Client is Not Working".to_string(), | ||
| ))), | ||
| } | ||
| } | ||
|
|
||
| /// Sends a batch request. | ||
| async fn batch_request<'a, R>( | ||
| &self, | ||
| batch: BatchRequestBuilder<'a>, | ||
| ) -> Result<BatchResponse<'a, R>, ClientError> | ||
| where | ||
| R: DeserializeOwned + fmt::Debug + 'a, | ||
| { | ||
| match &self.0 { | ||
| WsClientState::Working(inner) => inner.batch_request(batch).await, | ||
| WsClientState::NotWorking => Err(ClientError::Transport(BoxError::from( | ||
| "Client is Not Working".to_string(), | ||
| ))), |
There was a problem hiding this comment.
As a cleanliness thing you could have a function like make_notworking_err to dedup constructing this complex error instance.
| # Local constants | ||
| # D BTC | ||
| DEPOSIT_AMOUNT = DEFAULT_ROLLUP_PARAMS["deposit_amount"] | ||
| # Gas for the withdrawal transaction | ||
| WITHDRAWAL_GAS_FEE = 22_000 # technically is 21_000 | ||
| # Ethereum Private Key | ||
| # NOTE: don't use this private key in production | ||
| ETH_PRIVATE_KEY = "0x0000000000000000000000000000000000000000000000000000000000000001" | ||
| # BTC Operator's fee for withdrawal | ||
| OPERATOR_FEE = DEFAULT_ROLLUP_PARAMS["operator_fee"] |
There was a problem hiding this comment.
Spaces between each of these after each assignment before the next comment to make them easier to read.
| let l2_rpc_client = self | ||
| .l2_rpc_client_pool | ||
| .get() | ||
| .await | ||
| .map_err(|err| ExecError::WsPool(err))?; |
There was a problem hiding this comment.
Maybe make an accessor on the type that wraps this repeated pattern so you can just do something like self.get_ready_rpc_client().await?.
| if let Ok(RpcBridgeDuties { | ||
| duties, | ||
| start_index, | ||
| stop_index, | ||
| } = self.poll_duties().await?; | ||
|
|
||
| let mut handles = JoinSet::new(); | ||
| for duty in duties { | ||
| let exec_handler = self.exec_handler.clone(); | ||
| let bridge_duty_ops = self.bridge_duty_db_ops.clone(); | ||
| let broadcaster = self.broadcaster.clone(); | ||
| handles.spawn(async move { | ||
| process_duty(exec_handler, bridge_duty_ops, broadcaster, &duty).await | ||
| }); | ||
| } | ||
| }) = self.poll_duties().await | ||
| { |
There was a problem hiding this comment.
Changing this to an if let like this is a minor change but it makes the structure much different. This should reason about the error conditions more explicitly.
I don't even see the error case being handled here, we need to do that. From what I can tell, if this errors immediately (like because the connection is broken) then we just spin in a hot loop eating a CPU core until we don't encounter the error anymore.
Description
The nature of websocket client is that it requires restart once the connection drops. Due to this
bridge-clientstops if the connection tostrata-clientdrops which is not the desired and must be handled. This PR fixes the bug by using managed connection pool calleddeadpool.Note
The functional test deliberately contains duplicated code which can be fixed after STR-734 is merged. STR-734 contains
BridgeTestBaseclass that will be utilized by this functional test after rebase. I opened this PR to discuss the bug fix partType of Change
Notes to Reviewers
Checklist
Related Issues
STR-809