Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add function to create all channels #261

Closed
wants to merge 1 commit into from

Conversation

lev-starkware
Copy link
Contributor

@lev-starkware lev-starkware commented Jun 20, 2024

@lev-starkware lev-starkware changed the title feat: Add function to create all channels feat: add function to create all channels Jun 20, 2024
@lev-starkware lev-starkware changed the title feat: add function to create all channels feat: Add function to create all channels Jun 20, 2024
@lev-starkware lev-starkware force-pushed the pr/lev-starkware/lev_dev/4cc36c62 branch from 5bc1d25 to da5398c Compare June 20, 2024 10:35
Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions (waiting on @lev-starkware)


crates/mempool_node/src/com_channels.rs line 4 at r1 (raw file):

use tokio::sync::mpsc::{channel, Receiver, Sender};

pub struct SingleChannel<T: Send + Sync> {

Please change this to ComponentCommunication.
Either in this pr, or in a refactoring pr in the end, and if the latter, please add a "todo" to address this issue here.

Code quote:

SingleChannel

crates/mempool_node/src/com_channels.rs line 5 at r1 (raw file):

pub struct SingleChannel<T: Send + Sync> {
    pub tx: Sender<T>,

Please change these to be private members, and have a get_tx and get_rx functions.

Also, not for this pr, but I suspect eventually we'll need to move ownership of the rx part to the server. It might make sense to change the definition here to Optional and use the take() function.

Code quote:

pub

crates/mempool_node/src/com_channels.rs line 9 at r1 (raw file):

}

pub struct CommChannels {

Please change this name to MempoolNodeCommunication.
Same comment regarding in which pr to do that (here / later -- add a todo).

Code quote:

CommChannels

crates/mempool_node/src/com_channels.rs line 10 at r1 (raw file):

pub struct CommChannels {
    pub mempool_channel: SingleChannel<MempoolRequestAndResponseSender>,

Ditto regarding visibility (pub) and getters.

Code quote:

pub

crates/mempool_node/src/com_channels.rs line 13 at r1 (raw file):

}

pub fn create_channels() -> CommChannels {

create_mempool_node_channels

Code quote:

create_channels

Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a comment

Choose a reason for hiding this comment

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

Please rename the file to communication.rs

Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions (waiting on @lev-starkware)

Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @lev-starkware)


-- commits line 1 at r1:
Please rename the file to communication.rs

Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @lev-starkware)


-- commits line 1 at r1:

Previously, Itay-Tsabary-Starkware wrote…

Please rename the file to communication.rs

Can be in a refactoring pr, if so, add a todo.

@lev-starkware lev-starkware changed the title feat: Add function to create all channels feat: add function to create all channels Jun 20, 2024
@lev-starkware lev-starkware force-pushed the pr/lev-starkware/lev_dev/4cc36c62 branch from da5398c to ca086af Compare June 20, 2024 10:47
Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @lev-starkware)


crates/mempool_node/src/com_channels.rs line 13 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

create_mempool_node_channels

Ignore the previous message, please rename to create_node_channels.

Copy link
Contributor Author

@lev-starkware lev-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @Itay-Tsabary-Starkware)


-- commits line 1 at r1:

Previously, Itay-Tsabary-Starkware wrote…

Can be in a refactoring pr, if so, add a todo.

I don't like that name but Done.


crates/mempool_node/src/com_channels.rs line 4 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Please change this to ComponentCommunication.
Either in this pr, or in a refactoring pr in the end, and if the latter, please add a "todo" to address this issue here.

Done.


crates/mempool_node/src/com_channels.rs line 5 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Please change these to be private members, and have a get_tx and get_rx functions.

Also, not for this pr, but I suspect eventually we'll need to move ownership of the rx part to the server. It might make sense to change the definition here to Optional and use the take() function.

Done.


crates/mempool_node/src/com_channels.rs line 9 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Please change this name to MempoolNodeCommunication.
Same comment regarding in which pr to do that (here / later -- add a todo).

Done.


crates/mempool_node/src/com_channels.rs line 10 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Ditto regarding visibility (pub) and getters.

Done.


crates/mempool_node/src/com_channels.rs line 13 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Ignore the previous message, please rename to create_node_channels.

Done.

@lev-starkware lev-starkware force-pushed the pr/lev-starkware/lev_dev/4cc36c62 branch 3 times, most recently from 55cf06e to 7547018 Compare June 20, 2024 16:11
Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @lev-starkware)

Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @lev-starkware)

Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lev-starkware)


crates/mempool_node/src/communication.rs line 14 at r3 (raw file):

    }
    fn get_rx(&mut self) -> Receiver<T> {
        self.rx.take().expect("Receiver already taken")

Please change to the conventional expect format: "X should be Y".
For example, "Receiver should be available".

Code quote:

self.rx.take().expect("Receiver already taken")

Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @lev-starkware)

Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lev-starkware)

@lev-starkware lev-starkware force-pushed the pr/lev-starkware/lev_dev/4cc36c62 branch from 7547018 to e4eff05 Compare June 24, 2024 08:39
@codecov-commenter
Copy link

codecov-commenter commented Jun 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.09%. Comparing base (7ea4039) to head (faf42aa).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #261   +/-   ##
=======================================
  Coverage   85.09%   85.09%           
=======================================
  Files          28       28           
  Lines        1375     1375           
  Branches     1375     1375           
=======================================
  Hits         1170     1170           
  Misses        145      145           
  Partials       60       60           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @lev-starkware)

@lev-starkware lev-starkware force-pushed the pr/lev-starkware/lev_dev/4cc36c62 branch 3 times, most recently from 3faa29a to e185f39 Compare June 25, 2024 14:48
@lev-starkware lev-starkware force-pushed the pr/lev-starkware/lev_dev/4cc36c62 branch 2 times, most recently from 495f4a7 to 6184bbb Compare June 26, 2024 15:22
Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @lev-starkware)

@lev-starkware lev-starkware force-pushed the pr/lev-starkware/lev_dev/4cc36c62 branch from 9fa54a1 to faf42aa Compare July 1, 2024 10:32
@lev-starkware
Copy link
Contributor Author

✓ Commit merged in pull request #308

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