Conversation
|
I've assigned @benthecarman as a reviewer! |
Anyitechs
left a comment
Anyitechs
left a comment
There was a problem hiding this comment.
Thank you for looking into this. Please fix CI if this is ready for review (The proto files are now out of date and needs to be updated, and the changes introduced needs to be formatted too)
|
🔔 1st Reminder Hey @benthecarman! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @benthecarman! This PR has been waiting for your review. |
| matches!( | ||
| reason, | ||
| Some(ClosureReason::FundingTimedOut) | ||
| | Some(ClosureReason::DisconnectedPeer) | ||
| | Some(ClosureReason::CounterpartyCoopClosedUnfundedChannel) | ||
| | Some(ClosureReason::LocallyCoopClosedUnfundedChannel) | ||
| | Some(ClosureReason::FundingBatchClosure) | ||
| ) |
There was a problem hiding this comment.
instead of like this, can we do a match reason { ... then we can explicitly set true or false for each and we'll get a compile error when new reasons are added in the future
| | Some(ClosureReason::CounterpartyCoopClosedUnfundedChannel) => { | ||
| events::ChannelClosureInitiator::Remote | ||
| }, | ||
| Some(_) => events::ChannelClosureInitiator::Unknown, |
There was a problem hiding this comment.
can we enumerate all the other ones still instead of just Some(_) for same reason
| peer_feerate_sat_per_kw: *peer_feerate_sat_per_kw, | ||
| required_feerate_sat_per_kw: *required_feerate_sat_per_kw, | ||
| })), | ||
| _ => None, |
There was a problem hiding this comment.
again, can we enumerate them all
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_subscribe_events_channel_state_lifecycle_pending_ready_closed() { |
There was a problem hiding this comment.
for both of these tests, can we check the events on both sides?
There was a problem hiding this comment.
also we should be able to assert more things on these events
| mod service; | ||
| mod util; | ||
|
|
||
| use std::collections::HashSet; |
There was a problem hiding this comment.
Do we want to use the HashSet from HashBrown here instead, since it's already in the tree?
4 participants
fixes #134