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

Reorganize #1

Merged
merged 9 commits into from
Oct 18, 2023
Merged

Reorganize #1

merged 9 commits into from
Oct 18, 2023

Conversation

jmwample
Copy link
Member

This PR is an attempt to centralize our efforts to start reducing the amount of code that we re-write wrt. the rust runner (and example client applications), the water bin library, and the water binaries themselves.

This will help as we start settling in on the API so we can reduce the code required to implement each part.

I am making this PR first in preparation for future PRs like adding the hexencoder water bin example, a macro library to simplify writing water bins, and other example clients in rust.


One piece that I am not entirely clear on - several of the packages import an unknown trait std::os::wasi::Prelude::FromRawFd which my compiler did not recognize and which worked fine when replaced with an import for the std::os::fd::FromRawFd trait.

// Developer Guide: Logic for packaging

// A trait for a encoder, developers should implement this trait and pass it to _write_to_outbound
pub trait Encoder {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we planning on leaving encoder a trait(Go: interface) for the developers to implement? Should this be a V0 specific thing or is it good for future versions as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also encoder sounds a little bit too primitive (For me this name implies a stateless algorithm implementation). TLS, Shadowsocks (as transport), SSH and more, these all could be implemented with this model, so I would suggest a name change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I was having a hard time coming up with a good descriptor for this as well. My issue is similar in that things like encode/decode or obscure/reveal do not capture the ability to handshake and maintain a stream. We could do something like StreamEncoder or StatefulEncoder but those sound like the require more.

Copy link
Contributor

Choose a reason for hiding this comment

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

Naming is so hard 😄

Copy link
Contributor

@gaukas gaukas left a comment

Choose a reason for hiding this comment

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

Overall looks good to me.

Btw here's the proposed API for v0plus: https://github.com/gaukas/water_wasm-rs/tree/v0plus

I think it would be meaningful for us to also include v0plus, which is more transport-ful (instead of v0) in addition to v1, which is very much automated.

@gaukas
Copy link
Contributor

gaukas commented Oct 18, 2023

I can start a branch for v0 API (in fact v0plus) if everyone's okay with it. Perhaps back-port into this branch or master (if this one PR get merged sooner).

@erikziyunchi
Copy link
Member

erikziyunchi commented Oct 18, 2023

Thank you so much for doing this! It looks great! If we all like this I guess we can merge it and I'll add some local changes I made previously to the new structure.

@jmwample jmwample merged commit aad0812 into main Oct 18, 2023
4 checks passed
@jmwample jmwample deleted the jmwample/re-org branch October 18, 2023 18:24
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