Skip to content

Conversation

@ong-jonas
Copy link
Collaborator

@ong-jonas ong-jonas commented Oct 19, 2023

Changes

  1. Implement TryFrom<libp2p::gossipsub::Message> for pchain Message.
  2. Moved EngineError to mod engine and renamed to EngineStartError
  3. Removed PeerBuilder struct
  4. Peer created through Peer::start()
  5. Engine::start() no longer returns peer but the JoinHandle and Sender

Copy link
Contributor

@manngayin612 manngayin612 left a comment

Choose a reason for hiding this comment

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

Made suggestions on required changes

@manngayin612 manngayin612 requested a review from AlvinHon October 20, 2023 01:51
@manngayin612
Copy link
Contributor

@AlvinHon Please also advise on whether EngineCommand should be moved into engine.rs or in peer.rs

@AlvinHon
Copy link
Contributor

@AlvinHon Please also advise on whether EngineCommand should be moved into engine.rs or in peer.rs

It should be in engine.rs. EngineCommand is provided by engine. engine mainly provides function start. Imagine that collaborators will search how to use function start, and they will also need to know what are the commands. Then, engine.rs is reasonable place.

@AlvinHon
Copy link
Contributor

I have some comments:

  1. the module engine seems not necessary when we now have the Peer struct as a handle of the pchain-network, so you can
  • remove engine and replace its functionality with Peer
  • rename the joinhandle as handle, the mpsc sender as just sender, EngineCommand as PeerAction
  1. in conversion.rs,
  • rename ConversionError to PeerIdTryFromPublicAddressError.
  • directly use DecodingError in TryFrom<PeerId> for PublicAddress
  1. in the start function, it is better to document more about what are the handlers about.

src/peer.rs Outdated
pub async fn start(config: Config, handlers: Vec<Box<dyn Fn(PublicAddress, Message) + Send>>) -> Result<Peer, PeerStartError> {
let swarm = set_up_transport(&config).await?;
let swarm = establish_network_connections(swarm, &config)?;
let (handle, sender) = main_loop(swarm, &config, handlers)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

"main_loop" doesn't seem to be a good naming...?

Copy link
Collaborator Author

@ong-jonas ong-jonas Oct 23, 2023

Choose a reason for hiding this comment

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

i agree, maybe "spawn_event_loop" , "start_event_detection" or "spawn_events_handler" ? any suggestions?

@ong-jonas ong-jonas marked this pull request as ready for review October 24, 2023 02:35
src/messages.rs Outdated
impl TryFrom<(libp2p::gossipsub::Message, pchain_types::cryptography::PublicAddress)> for Message {
type Error = MessageConversionError;

fn try_from((message , local_public_address): (libp2p::gossipsub::Message, pchain_types::cryptography::PublicAddress))
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this and the MessageConversionError into conversions.rs as these are all related to conversions

Copy link
Contributor

@manngayin612 manngayin612 left a comment

Choose a reason for hiding this comment

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

Comments on minor changes before merging

@manngayin612 manngayin612 merged commit 702dfe2 into refactor Oct 27, 2023
@manngayin612 manngayin612 deleted the message/refactor branch October 27, 2023 06:42
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.

4 participants