-
Notifications
You must be signed in to change notification settings - Fork 147
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
Remove protocol deps from pool
crate
#1483
base: main
Are you sure you want to change the base?
Remove protocol deps from pool
crate
#1483
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1483 +/- ##
==========================================
+ Coverage 18.90% 19.32% +0.41%
==========================================
Files 134 131 -3
Lines 9521 9310 -211
==========================================
- Hits 1800 1799 -1
+ Misses 7721 7511 -210
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
||
pub use binary_sv2::Error as BinaryError; | ||
pub use binary_sv2::U256; | ||
pub use binary_sv2::B064K; | ||
pub use binary_sv2::B0255; | ||
|
||
pub use codec_sv2::Error as CodecError; | ||
pub use codec_sv2::framing_sv2::Error as FramingError; | ||
|
||
pub use codec_sv2::Initiator; | ||
pub use codec_sv2::Responder; | ||
pub use codec_sv2::HandshakeRole; | ||
pub use codec_sv2::noise_sv2::Error as NoiseError; | ||
|
||
pub use codec_sv2::StandardEitherFrame; | ||
pub use codec_sv2::StandardSv2Frame; | ||
|
||
pub use const_sv2::MESSAGE_TYPE_CHANNEL_ENDPOINT_CHANGED; | ||
|
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.
I have a slightly different opinion on this—let me know what you think. Although roles-logic-sv2
is part of the protocol, it should have been placed in roles
, considering that it primarily contains application-layer logic. The codec
crate is the protocol crate, encapsulating lower-level foundational crates like binary
, framing
, noise
, and const
. To me, it makes sense to expose these from codec
and then use them in roles-logic-sv2
. That way, the hierarchy remains structured correctly.
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.
Yea I agree with you. But roles logic is not part of the scope for this pr..
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.
referenced #1268 (comment)
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.
@jbesraa, my previous comment focused on exposing the binary, framing, noise, and const modules through the codec_sv2 crate, rather than the roles_logic_sv2 crate. This is independent of the roles_logic_sv2 refactoring. I've noticed other PR's relying on the assumption that these modules should be exposed via roles_logic_sv2. I believe we should carefully consider this design decision. My preference remains to expose them through codec_sv2.
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.
Can you say why? roles_logic is the 'window' to the lower level crates, why would we fetch code from codec directly then?
59e0e20
to
3fc68a9
Compare
aa0446d
to
b582577
Compare
This is ready for review |
ef80814
to
826d1d1
Compare
This also exports the needed structs/fns through `roles-logic-sv2`.
826d1d1
to
11c5c6f
Compare
Partially addresses #1458