Fix Clippy + comments from previous PR #22
Conversation
| if let Ok(response) = | ||
| serde_json::from_str::<bitcoincore_rpc::jsonrpc::Response>(&resp) | ||
| { | ||
| let resp: Round1Response = response.result()?; |
There was a problem hiding this comment.
same problem for result()? though :D
There was a problem hiding this comment.
I'm wondering if it would be cleaner to do something like this:
let res = resp.map_err(|e| format!("connection error: {e}").and_then(|r| serde_json::from_str::< bitcoincore_rpc::jsonrpc::Response>(&r).map_err(|e| format!("json error: {e}"))).and_then(|r| r.result().map_err(|e| format!("rpc error: {e}")));
let resp = match res {
Ok(x) => x,
Err(e) => {
warn!("Round 1 error with {addr}: {e}", addr=member.address);
self.member_status.write().unwrap().mark_as_disconnected(member_id);
need_retry = true // since at this point we have waiting for all the response might as well check all of them before retrying (so that we potentially discover multiple disconnects?)
}
}There was a problem hiding this comment.
Yep, saw that. We won't even need the response.result()? part once we have a custom serde struct for Node RPC responses. I can open up an issue for this, but would rather fix this after we have at least some e2e tests so I can just run the job and make sure I don't broken anything by accident, without having to recreate the whole environment each time.
| if let Ok(response) = | ||
| serde_json::from_str::<bitcoincore_rpc::jsonrpc::Response>(&resp) | ||
| { | ||
| let round2_response: Round2Response = response.result()?; |
| pubkey_package, | ||
| committee_cfg, | ||
| member_status: member_status_state, | ||
| }; |
There was a problem hiding this comment.
perhaps it would be cleaner to do a let ctx = Orchestrator::new(pubkey_package, committee_cfg) and take care of creating the member_status_state there
| @@ -100,4 +100,4 @@ jobs: | |||
| uses: actions-rs/cargo@v1 | |||
| with: | |||
| command: clippy | |||
| args: --all-features -- -D warnings | |||
| args: --all-features -- -D warnings -A clippy::absurd_extreme_comparisons | |||
There was a problem hiding this comment.
better to add #[allow(clippy::absurd_Extreme_comparisons)] next to where the the warning is being given :o
There was a problem hiding this comment.
Yup, tried that but there were some issues with the ensure! macro where most absurd checks are being done. For some reason clippy ignored the decoration.
Hey there! Reviewed and changed some stuff we talked about in #18 . Also fixed clippy and ran a workflow locally and it's looking good now!
Haven't tested these new changes, but I have not modified any functionality whatsoever. The e2e testing pipeline we were talking about is going to be really useful.
closes #21