-
Notifications
You must be signed in to change notification settings - Fork 11
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
Conversation
5bc1d25
to
da5398c
Compare
There was a problem hiding this 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
There was a problem hiding this 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)
There was a problem hiding this 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
There was a problem hiding this 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)
Previously, Itay-Tsabary-Starkware wrote…
Please rename the file to
communication.rs
Can be in a refactoring pr, if so, add a todo.
da5398c
to
ca086af
Compare
There was a problem hiding this 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
.
There was a problem hiding this 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)
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
andget_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 toOptional
and use thetake()
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.
55cf06e
to
7547018
Compare
There was a problem hiding this 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: complete! all files reviewed, all discussions resolved (waiting on @lev-starkware)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @lev-starkware)
There was a problem hiding this 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")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @lev-starkware)
There was a problem hiding this 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)
7547018
to
e4eff05
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @lev-starkware)
3faa29a
to
e185f39
Compare
495f4a7
to
6184bbb
Compare
6184bbb
to
eaf9c92
Compare
eaf9c92
to
6f730ee
Compare
a3d106f
to
20da2db
Compare
20da2db
to
25434fb
Compare
956d75c
to
9fa54a1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @lev-starkware)
commit-id:4cc36c62
9fa54a1
to
faf42aa
Compare
✓ Commit merged in pull request #308 |
Stack:
This change is