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

Use external transport crate #366 #367

Merged
merged 14 commits into from
Apr 20, 2022
Merged

Use external transport crate #366 #367

merged 14 commits into from
Apr 20, 2022

Conversation

gijsvl
Copy link
Contributor

@gijsvl gijsvl commented Mar 25, 2022

The extracted transport crate can be found here: https://github.com/boltlabs-inc/transport

We should make sure it all still makes sense, including the level of abstraction.
Specifically, the feature allow_explicit_certificate_trust, does that make sense to also live inside the transport crate, I do believe so, but just checking. Also, the abstraction for ZkChannelAddress that now implements the trait Address, which is part of the transport crate.

@gijsvl gijsvl requested a review from marsella March 25, 2022 13:40
Copy link
Contributor

@marsella marsella left a comment

Choose a reason for hiding this comment

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

This looks good to me. Waiting to approve until I have a chance to double-check the transport crate, but it seems like you have a good abstraction. My comments are almost all formatting.

src/lib.rs Outdated Show resolved Hide resolved
src/transport.rs Outdated Show resolved Hide resolved
src/zkchannels/customer.rs Show resolved Hide resolved
src/customer.rs Outdated Show resolved Hide resolved
@marsella
Copy link
Contributor

Review of the transport crate:

As discussed on slack, we might want to add a callback to the serve_while function (e.g. a FnOnce parameter) in order to allow custom logging or other administrative work once the server is running. However, I'm not sure if this would fix the logging issue we're seeing in tests (where we're currently filtering out non-zeekoe logs) because the log itself would be called from transport -- you will have to check the log and see. Options if the filtering is still a problem:

  • Add the transport crate to the log filter
  • Return to the original solution of sleeping for 5-10 seconds and assuming the server gets set up in that time

It looks to me like there aren't major changes in the transport crate except maybe adding an Address trait. Is there anything else specific that you'd like me to look at?

@gijsvl
Copy link
Contributor Author

gijsvl commented Mar 31, 2022

With regard to the transport crate, indeed, not much changed, it's basically the same with the exception of the abstract Trait Address, as you've mentioned.

@gijsvl gijsvl requested a review from marsella March 31, 2022 10:59
Copy link
Contributor

@marsella marsella left a comment

Choose a reason for hiding this comment

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

Thank you for the fixes. My only remaining recommendation is to update the rustfmt.toml with the imports_granularity specification - it looks like that file already supports unstable features, although I'm not sure which ones. Mukund may have mentioned something about line-wrapping documentation, but I think that is an issue for a different PR.

@gijsvl
Copy link
Contributor Author

gijsvl commented Apr 8, 2022

Before merging, we should merge boltlabs-inc/transport#1, and change the transport dependency back to "main" branch

@marsella marsella self-requested a review April 8, 2022 14:59
Copy link
Contributor

@marsella marsella left a comment

Choose a reason for hiding this comment

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

Looks good, approving pending the transport fix.

@gijsvl gijsvl merged commit 1509be8 into main Apr 20, 2022
@gijsvl gijsvl deleted the extract_transport branch April 20, 2022 09:03
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.

2 participants