[CX_HARDENING] Evaluate and Remove Issue Links in HotShot #3701
Conversation
| // TODO (https://github.com/EspressoSystems/HotShot/issues/2296): | ||
| // switch to info! when INFO logs become less cluttered | ||
| error!("Progress: entered view {:>6}", *new_view); | ||
| info!("Progress: entered view {:>6}", *new_view); |
There was a problem hiding this comment.
Willing to swap this one back if we think it's relevant. My logic is that we pretty much can search for it anyway in all of our log ingestion sources, so it feels silly to place it on the error log.
| @@ -170,9 +170,6 @@ impl<TYPES: NodeType> NetworkMessageTaskState<TYPES> { | |||
| } | |||
| }, | |||
| }; | |||
| // TODO (Keyao benchmarking) Update these event variants (similar to the | |||
| // `TransactionsRecv` event) so we can send one event for a vector of messages. | |||
| // <https://github.com/EspressoSystems/HotShot/issues/1428> | |||
| broadcast_event(Arc::new(event), &self.internal_event_stream).await; | |||
There was a problem hiding this comment.
This one appears to be a very old ticket pertaining to events that no longer exist, so I'm not sure what the context is, but my thought is that if it were really a huge issue, we'd sort it out. To this point, I have not encountered this.
| @@ -363,7 +360,7 @@ impl< | |||
| /// which will be used to create a message and transmit on the wire. | |||
| /// Returns `None` if the parsing result should not be sent on the wire. | |||
| /// Handles the `VidDisperseSend` event separately using a helper method. | |||
| #[allow(clippy::too_many_lines)] // TODO https://github.com/EspressoSystems/HotShot/issues/1704 | |||
| #[allow(clippy::too_many_lines)] | |||
There was a problem hiding this comment.
This issue is just marking that this is a too_many_lines function, but we have several of these in the codebase, and the refactor is painful. Having a bespoke issue for each instance seems like overkill. These are easily "not correct", so they can be fixed whenever it's necessary. Therefore, having an issue seems to be making no material difference in team awareness of the issue. Furthermore, since it's one of our trademark enormous match statements it's actually quite easy to follow despite it's size.
| Proposal<TYPES, VidDisperse<TYPES>>, | ||
| Vec<Proposal<TYPES, VidDisperseShare<TYPES>>>, | ||
| ) { | ||
| ) -> VidProposal<TYPES> { |
There was a problem hiding this comment.
This name could be different. Just based on the function it seemed like an obvious name.
|
I agree with your reasoning for everything on this, looks like a nice cleanup to me |
Closes #3169
This PR:
There's lots of dead issues in the codebase, this PR removes them or, if they're easy enough, fixes them. It also fixes two other small issues with type complexity and general format.
This PR does not:
Update any tests or docs, that is coming later.
Key places to review: