Skip to content

Conversation

@torkelrogstad
Copy link
Contributor

@torkelrogstad torkelrogstad commented Feb 19, 2025

The idea with the mailbox item handling refactor is to make it possible to add tracing IDs to better figure out what's happening.

There's currently a linter failure here, which I'm not sure how to handle. @Ash-L2L are you able to help?

}

#[derive(Debug, Serialize, Deserialize)]
#[derive(Debug, Serialize, Deserialize, strum::Display)]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if a Display impl is a good idea here, debug should be enough.
If you just want to show the discriminant, use https://docs.rs/strum/latest/strum/derive.EnumDiscriminants.html and https://docs.rs/strum/latest/strum/trait.IntoDiscriminant.html


#[derive(BorshSerialize, Clone, Debug, Deserialize, Serialize)]
#[derive(
BorshSerialize, Clone, Debug, Deserialize, Serialize, strum::Display,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if a Display impl is a good idea here, debug should be enough.
If you just want to show the discriminant, use https://docs.rs/strum/latest/strum/derive.EnumDiscriminants.html and https://docs.rs/strum/latest/strum/trait.IntoDiscriminant.html

HashMap<mainchain_task::Request, HashSet<(SocketAddr, PeerStateId)>>,

/// PhantomData to use the MainchainTransport type parameter
_phantom: std::marker::PhantomData<MainchainTransport>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this type parameter needed at all here? Looks like the parameter should be on MailboxState::handle_mailbox_item

}

#[derive(Debug, strum::Display)]
enum MailboxItem {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move this above MailboxState, since it is used in the impl for MailboxState

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants