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: client webtransport-websys feat #1224

Merged
merged 3 commits into from Jan 25, 2024
Merged

Conversation

joshuef
Copy link
Contributor

@joshuef joshuef commented Jan 24, 2024

Description

Summary generated by Reviewpad on 24 Jan 24 15:33 UTC

This pull request includes multiple file diffs. Here is a summarized overview of the changes:

  1. The cmd.rs file in the sn_networking module has several modifications, including new imports, changes in enum variants and struct methods, variable updates, and code formatting changes.

  2. The replication_fetcher.rs file has changes related to imports, removing and adding features, and updating comments and code related to parallel fetches.

  3. The Cargo.toml file removes the "quic" feature and includes only the "metrics" feature.

  4. The wallet.rs file has changes in imports related to time and duration, along with a comment explaining the usage of a wallet client for token transfers.

  5. The lib.rs file has modifications in the genesis_multi_addr variable and related assertions in tests.

  6. The README.md file has updates regarding transport protocols, architectures, and building for wasm32.

  7. The diff adds a new target_arch.rs file with conditionally imported modules based on the target architecture. Changes also include importing spawn_local for handling asynchronous tasks.

  8. The lib.rs file includes changes in importing the Duration struct from the tokio::time module, replacing the previous import from the std::time module.

  9. The diff for the file download.rs includes import changes related to time, removing and replacing import statements.

  10. The Cargo.toml file has changes in dependency versions, feature modifications, and wasm compilation options.

  11. The file client.rs in tests/common has changes in imports related to time.

  12. The upload.rs file has modifications related to logging and changing the method of reading chunk bytes.

  13. The diff includes changes in importing the sleep function and related replacements for time-related functions.

  14. The Cargo.toml file includes changes in features, dependencies, and versions.

  15. The driver.rs file within the sn_networking module has various changes, including conditional compilation, transport updates, logging, and configurations based on build and target architectures.

  16. The node_registry.rs file has changes in constructing the addr variable based on feature flags for different build configurations.

  17. The diff introduces modifications related to imports, including the addition and removal of import statements for various modules, types, and libraries.

  18. The Cargo.toml file includes changes in dependencies, removing and adding features.

  19. The diff includes changes in importing modules, types, and constants in the file.

  20. The Cargo.toml file has changes in dependencies, including the removal of the "default" and "quic" features.

Please let me know if you need more details or specific information for any of the file diffs.

@reviewpad reviewpad bot added the Large Large sized PR label Jan 24, 2024
assert_eq!(format!("/ip4/127.0.0.1/tcp/11101/p2p/{peer_id}"), multiaddr);
if !cfg!(feature = "websockets") {
assert_eq!(
format!("/ip4/127.0.0.1/tcp/11101/ws/p2p/{peer_id}"),

Check notice

Code scanning / devskim

Accessing localhost could indicate debug code, or could hinder scaling. Note

Do not leave debug code in production
assert_eq!(format!("/ip4/127.0.0.1/tcp/11101/p2p/{peer_id}"), multiaddr);
if cfg!(feature = "websockets") {
assert_eq!(
format!("/ip4/127.0.0.1/tcp/11101/ws/p2p/{peer_id}"),

Check notice

Code scanning / devskim

Accessing localhost could indicate debug code, or could hinder scaling. Note

Do not leave debug code in production
let genesis_multi_addr = if cfg!(any(feature = "websockets", target_arch = "wasm32")) {
format!("/ip4/127.0.0.1/tcp/{genesis_port:?}/ws/p2p/{peer_id}")
} else {
format!("/ip4/127.0.0.1/udp/{genesis_port:?}/quic-v1/p2p/{peer_id}")

Check notice

Code scanning / devskim

Accessing localhost could indicate debug code, or could hinder scaling. Note

Do not leave debug code in production
format!("/ip4/127.0.0.1/udp/{genesis_port:?}/quic-v1/p2p/{peer_id}");

let genesis_multi_addr = if cfg!(any(feature = "websockets", target_arch = "wasm32")) {
format!("/ip4/127.0.0.1/tcp/{genesis_port:?}/ws/p2p/{peer_id}")

Check notice

Code scanning / devskim

Accessing localhost could indicate debug code, or could hinder scaling. Note

Do not leave debug code in production
)));


// TODO: We need to tidy this up, the client loops forever in the browser, and eventually crashes

Check notice

Code scanning / devskim

A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note

Suspicious comment
console::log_1(&JsValue::from_str("Hello safe world!"));

// Tracing
// TODO: dont log _everything_

Check notice

Code scanning / devskim

A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note

Suspicious comment
@joshuef joshuef force-pushed the WebTransport4 branch 4 times, most recently from 0896de4 to 2ca735f Compare January 24, 2024 15:04
.github/workflows/merge.yml Outdated Show resolved Hide resolved
sn_client/src/api.rs Outdated Show resolved Hide resolved
sn_client/src/api.rs Outdated Show resolved Hide resolved
sn_client/src/api.rs Outdated Show resolved Hide resolved
sn_client/src/lib.rs Outdated Show resolved Hide resolved
#[cfg(feature = "tcp")]
let main_transport = TokioTransport::new(TransportConfig::default())
#[cfg(target_arch = "wasm32")]
let main_transport = WebSocketTransport::default()
Copy link
Member

Choose a reason for hiding this comment

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

same idea as before, move to separate file and create a create_main_transport function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did look at this, but things got unwieldy fast.

I'm not against it, but I don't think it's a blocker for this PR.

@@ -87,6 +95,8 @@ pub const fn close_group_majority() -> usize {

/// Max duration for all GET attempts
const MAX_GET_RETRY_DURATION_MS: u64 = 6800;

#[cfg(not(target_arch = "wasm32"))]
Copy link
Member

Choose a reason for hiding this comment

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

wasm doesn't need MAX_GET_RETRY_DURATION ?
I thought this is a non-transport-dependent retry config ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we cannot (at the moment) easily have backoff with wasm32. backoff uses tokio right now.

We can probably rejig this, but i'd say that's an optimisation atm. The main aim here (i think) is to get the wasm code building and checked?

@joshuef joshuef force-pushed the WebTransport4 branch 2 times, most recently from e7cdd2a to 9d8c040 Compare January 24, 2024 15:33
@joshuef joshuef marked this pull request as ready for review January 25, 2024 07:58
@joshuef joshuef merged commit 8d7cc12 into maidsafe:main Jan 25, 2024
31 of 32 checks passed
@joshuef joshuef deleted the WebTransport4 branch January 25, 2024 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Large Large sized PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants