Skip to content

Conversation

@manngayin612
Copy link
Contributor

The version in refactor branch is a refactor based on the comments and suggested structure that can be found it in here.

syrettaman and others added 26 commits October 10, 2023 09:18
WIP: Refactor pchain network engine
@manngayin612 manngayin612 requested a review from lyulka October 16, 2023 09:56
@lyulka
Copy link
Collaborator

lyulka commented Oct 18, 2023

Summary of review meeting (Oct 18, 2023)

  1. engine::start should not return a peer. It should instead return a JoinHandle to peer::start, which incorporates it into the Peer struct.
  2. We agreed to remove PeerBuilder and just create Peer through Peer::start.
  3. Instead of deserialize_message, consider impl TryFrom<(Topic, Vec<u8>)> for Message.
  4. EngineError should be in mod engine, and should be renamed EngineStartError.
  5. Consider moving some definitions into conversions.

Multiaddr,
};
use pchain_types::cryptography::PublicAddress;
use pchain_types::cryptography::{PublicAddress, self};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just import Keypair here instead of doing self here and qualify it as crytography::Keypair

src/lib.rs Outdated
//! use crate::Config;
//! use crate::message_gate::MessageGateChain;
//!
//! use crate::peer::Peer;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

use crate::Peer;

pub(crate) mod constants;

pub(crate) mod conversions;
pub mod peer;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

add pub use peer::Peer

//! keypair: pchain_types::cryptography::Keypair,
//! topics_to_subscribe: vec![Topic::HotStuffRsBroadcast],
//! listening_port: 25519,
//! boot_nodes: vec![],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

better to show format of boot_nodes here too

@AlvinHon AlvinHon merged commit 1b077da into v0.5 Nov 30, 2023
@AlvinHon AlvinHon deleted the refactor branch December 4, 2023 02:44
@AlvinHon AlvinHon mentioned this pull request Dec 5, 2023
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.

5 participants