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

Web client #1738

Merged
merged 8 commits into from
Mar 22, 2024
Merged

Web client #1738

merged 8 commits into from
Mar 22, 2024

Conversation

Twey
Copy link
Contributor

@Twey Twey commented Mar 1, 2024

Atop #1801, #1802.

Motivation

The client half of #1609.

Proposal

When compiling with web, replace our existing gRPC client with a Wasm-compatible gRPC-Web–based client using tonic-web-wasm-client.

Since browser objects are neither Send nor Sync, a large part of this work is then to enable non–Send equivalents of various traits for structs containing non–Send objects. linera-core in particular now never assumes that the client struct is Send. linera-service, however, does. In order to enable this, I have used trait-variant to construct, as automatically as possible, local and non-local variants of relevant traits.

Task list:

  • convert relevant traits to use AFIT and trait-variant
  • relax the Sendness bound that permeates linera-core and linera-rpc
  • implement a gRPC-Web client when compiling for the browser
  • deduplicate the implementations for the Web and non–Web clients
  • add tests in which the Wasm client connects to the gRPC-Web endpoint implemented in linera-service: export a grpc-web–compatible endpoint #1735

Test Plan

As part of this PR, I shall add tests to be run in a headless browser using wasm-pack test.

Due to #1799 and #1800 these are not currently part of the standard test cargo test test suite, but can be run manually:

$ eval "$(linera net helper)"
$ linera_spawn_and_read_wallet_variables linera net up
$ cd linera-rpc
$ wasm-pack test --chrome --headless --features web -- --include-ignored

The above assumes you are using the provided Nix flake for a development environment; if not, first you will need to install wasm-pack, chromium, and chromedriver through your usual package distribution channels. Tests also pass on Firefox, if you have Firefox installed — substitute --firefox for --chrome above.

Release Plan

  • Need to bump the major/minor version number in the next release of the crates.

Links

@Twey Twey force-pushed the 03-01-Web_client branch 5 times, most recently from 54a87b3 to 0dcda6a Compare March 7, 2024 00:29
@Twey Twey force-pushed the 03-01-Web_client branch from 1de3dff to f8fb3cb Compare March 8, 2024 16:25
@Twey Twey force-pushed the 03-01-Web_client branch from 3636963 to 141f6bb Compare March 20, 2024 13:10
@Twey Twey force-pushed the 03-01-Web_client branch from 141f6bb to 3d7bb56 Compare March 20, 2024 14:42
@Twey Twey marked this pull request as ready for review March 20, 2024 14:59
@Twey Twey force-pushed the 03-01-Web_client branch 4 times, most recently from 4b043a1 to be520e9 Compare March 21, 2024 19:39
linera-rpc/src/mass_client.rs Outdated Show resolved Hide resolved
linera-rpc/src/simple/client.rs Outdated Show resolved Hide resolved
linera-rpc/src/grpc/transport.rs Outdated Show resolved Hide resolved
linera-rpc/src/grpc/transport.rs Outdated Show resolved Hide resolved
linera-rpc/src/grpc/transport.rs Outdated Show resolved Hide resolved
@Twey Twey force-pushed the 03-01-Web_client branch from 74c5482 to b3c61d7 Compare March 22, 2024 14:09
@@ -2,6 +2,7 @@
// SPDX-License-Identifier: Apache-2.0

use crate::RpcMessage;

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: No need for the whitespace here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes indeed, we're generally trying to maximize the normalization opportunities for Rustfmt

Copy link
Contributor Author

@Twey Twey Mar 22, 2024

Choose a reason for hiding this comment

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

The blocks here are ordered by scope (ancestor < crate < workspace < external < std), so there shouldn't be any normalization opportunities between them (different crates will always end up in different blocks). Unfortunately rustfmt doesn't understand anything about the scope of crates, so while it's smart enough to order super and crate dependencies at the top, without the newlines it will mix std and linera-* crates in with third-party dependencies :-\

Copy link
Contributor Author

@Twey Twey Mar 22, 2024

Choose a reason for hiding this comment

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

rustfmt has added preliminary support for this, and since we're already using nightly rustfmt I've created PR #1821 to codify import grouping behaviour — it's not quite what's done here, but at least moves a standard for crate grouping into CI to increase normalization opportunities.

@@ -3,24 +3,26 @@
// SPDX-License-Identifier: Apache-2.0

use super::{codec, transport::TransportProtocol};

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: same here, following the convention in the rest of the code of just separating imports if they have different visibility modifiers or feature gates.

use crate::{
config::ValidatorPublicNetworkPreConfig, mass_client, HandleCertificateRequest,
HandleLiteCertificateRequest, RpcMessage,
};
use async_trait::async_trait;
use futures::{sink::SinkExt, stream::StreamExt};

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: same here

use linera_version::VersionInfo;

use std::time::Duration;
use async_trait::async_trait;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: if possible, could you also remove the other newlines above and below?

@Twey
Copy link
Contributor Author

Twey commented Mar 22, 2024

Remaining nits should be addressed by #1821 — so I'm going to go ahead and merge this one for now :)

@Twey Twey merged commit 8ce69b8 into linera-io:main Mar 22, 2024
3 checks passed
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.

Tunnel the RPC protocol over a browser-compatible transport
3 participants