feat(torii): enable any build (not just wasm) to connect to Slot #2147
Conversation
WalkthroughOhayo, sensei! The recent changes focus on upgrading the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
Client->>Server: Request connection with TLS
Server->>Client: Send certificate info
Client->>Server: Establish SSL/TLS connection
Server->>Client: Confirm connection established
sequenceDiagram
participant Relay
participant Event
Relay->>Event: Handle ServerEvent::Identify
Event-->>Relay: Extract peer_id and other fields
Relay->>Event: Process event with extracted fields
Recent review detailsConfiguration used: .coderabbit.yaml Files ignored due to path filters (1)
Files selected for processing (1)
Additional comments not posted (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (4)
- Cargo.toml (1 hunks)
- crates/torii/grpc/Cargo.toml (1 hunks)
- crates/torii/grpc/src/client.rs (2 hunks)
- crates/torii/libp2p/src/server/mod.rs (1 hunks)
Files skipped from review due to trivial changes (1)
- crates/torii/grpc/Cargo.toml
Additional comments not posted (5)
crates/torii/grpc/src/client.rs (2)
2-3: Imports for SSL/TLS connections.The added imports for handling SSL/TLS connections are necessary and correctly used in the code.
Also applies to: 8-8, 10-10
47-83: Connection handling logic forWorldClient.The changes to the
newfunction in theWorldClientstruct correctly handle different connection scenarios based on the URI, including SSL/TLS configuration.However, ensure that all function calls to
WorldClient::newmatch the new signature.Verification successful
The output shows two instances of
WorldClient::newusage:
crates/torii/grpc/src/client.rscrates/torii/client/src/client/mod.rsWe need to inspect the relevant lines in these files to ensure they match the new function signature
new(dst: String, _world_address: Felt).
Connection handling logic for
WorldClient.The changes to the
newfunction in theWorldClientstruct correctly handle different connection scenarios based on the URI, including SSL/TLS configuration. Additionally, all function calls toWorldClient::newmatch the new signature.
crates/torii/client/src/client/mod.rs: The usage ofWorldClient::newmatches the new signature.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `WorldClient::new` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type rust $'WorldClient::new'Length of output: 293
Script:
#!/bin/bash # Display the relevant lines where `WorldClient::new` is used to verify the function signature. # Fetching the line from crates/torii/grpc/src/client.rs sed -n '47,83p' crates/torii/grpc/src/client.rs # Fetching the line from crates/torii/client/src/client/mod.rs rg --context 5 'WorldClient::new' crates/torii/client/src/client/mod.rsLength of output: 1963
Cargo.toml (2)
211-211: Updatedtonicdependency.The
tonicdependency version has been correctly updated to include TLS features.
211-211: Addedopenssldependency.The
openssldependency has been correctly added to support SSL/TLS functionality.crates/torii/libp2p/src/server/mod.rs (1)
399-399: Destructuringpeer_idinIdentifyvariant.The
peer_idfield is correctly destructured along with other fields in theIdentifyvariant ofServerEvent.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (1)
- Cargo.toml (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
glihm
changed the title
caillef
force-pushed
the
debug-http2
branch
2 times, most recently
from
e544c63
to
15544e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (4)
- Cargo.toml (1 hunks)
- crates/torii/grpc/Cargo.toml (1 hunks)
- crates/torii/grpc/src/client.rs (2 hunks)
- crates/torii/libp2p/src/server/mod.rs (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- Cargo.toml
- crates/torii/grpc/Cargo.toml
- crates/torii/grpc/src/client.rs
- crates/torii/libp2p/src/server/mod.rs
|
I need to remove all the unwrap to avoid crashes. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (4)
- Cargo.toml (2 hunks)
- crates/torii/grpc/Cargo.toml (1 hunks)
- crates/torii/grpc/src/client.rs (4 hunks)
- crates/torii/libp2p/src/server/mod.rs (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- Cargo.toml
- crates/torii/grpc/src/client.rs
- crates/torii/libp2p/src/server/mod.rs
Additional comments not posted (1)
crates/torii/grpc/Cargo.toml (1)
9-9: Addition ofopenssldependency.The addition of
opensslis necessary for handling SSL/TLS connections. Ensure that the version compatibility and security implications are considered.
|
@glihm would like to have your review on this one please. I need to know if importing openssl is ok, and also is openssl import mandatory: I'm not 100% sure fetching the certificates is the right way to do this. Maybe we could use another certs and avoid the openssl import. Also I didn't changed how it worked with http, so that localhost server still works, but we could also handle the "with_origin" like I did with https so that we can handle http url where torii is not exposed at the root domain (localhost:8080/torii for example) I am currently trying to use 1.0 with dojo.c but the FieldElement/Felt changes is a lot of work, so I didn't test using dojo.c, I only had a temporary test to run from the dojo repo: #[cfg(test)]
mod tests {
use super::*;
#[tokio::test]
async fn test_client_new() {
let torii_url = "https://api.cartridge.gg/x/spawn-and-move-cubzh/torii".to_string();
let rpc_url = "https://api.cartridge.gg/x/spawn-and-move-cubzh/katana".to_string();
// let torii_url = "http://localhost:8080".to_string();
// let rpc_url = "http://localhost:5050".to_string();
let relay_url = "/ip4/127.0.0.1/tcp/9090".to_string();
let world = FieldElement::from_hex_be("0x7efebb0c2d4cc285d48a97a7174def3be7fdd6b7bd29cca758fa2e17e03ef30").unwrap();
let models_keys = None; // or Some(Vec<KeysClause>) if you have keys
let client = Client::new(torii_url, rpc_url, relay_url, world, models_keys).await;
assert!(client.is_ok(), "Error creating client");
}
}The test was not passing with the api.cartridge.gg domain at the beginning and now it works. So I guess it will also work when used with dojo.c. |
Cargo.toml
Outdated
| @@ -162,6 +162,7 @@ log = "0.4.21" | |||
| metrics = "0.21.1" | |||
| num-traits = { version = "0.2", default-features = false } | |||
| once_cell = "1.0" | |||
| openssl = "0.10.64" | |||
There was a problem hiding this comment.
We don't want to use openssl for the requirement it has on the host, but rustls instead. You can achieve a similar syntax using tokio_rustls. 👍
Cargo.toml
Outdated
| tonic-build = "0.10.1" | ||
| tonic-reflection = "0.10.1" | ||
| tonic-web = "0.10.1" | ||
| tonic = { version = "0.11", features = [ "tls" ] } |
There was a problem hiding this comment.
This feature is actually using rustls under the hood.
crates/torii/grpc/src/client.rs
Outdated
| _world_address, | ||
| inner: world_client::WorldClient::connect(dst).await.map_err(Error::Transport)?, | ||
| }) | ||
| pub async fn new(dst: String, _world_address: Felt) -> Result<Self, Error> { |
There was a problem hiding this comment.
| pub async fn new(dst: String, _world_address: Felt) -> Result<Self, Error> { | |
| pub async fn new(dst: String, world_address: Felt) -> Result<Self, Error> { |
World address is actually used, we can remove the _ at this occasion. :)
|
@glihm Thanks for the review, I found another way to do it without openssl with a feature of tonis |
Nice! Happy to discuss on discord about that if you need support for the alpha integration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2147 +/- ##
=======================================
Coverage 69.60% 69.60%
=======================================
Files 352 352
Lines 46034 46034
=======================================
Hits 32040 32040
Misses 13994 13994 ☔ View full report in Codecov by Sentry. |
crates/torii/grpc/src/client.rs
Outdated
| pub async fn new(dst: String, world_address: Felt) -> Result<Self, Error> { | ||
| let endpoint = | ||
| Endpoint::from_shared(dst.clone()).map_err(|e| Error::Endpoint(e.to_string()))?; | ||
| println!("{}", dst.clone()); | ||
| let channel = endpoint.connect().await.map_err(Error::Transport)?; | ||
| Ok(Self { | ||
| _world_address, | ||
| inner: world_client::WorldClient::connect(dst).await.map_err(Error::Transport)?, | ||
| _world_address: world_address, | ||
| inner: world_client::WorldClient::with_origin(channel, endpoint.uri().clone()), |
There was a problem hiding this comment.
Ohayo, sensei! Connection handling improvements look solid.
The refactoring of the new function to use a String for dst and the improved error handling are great enhancements. However, consider removing the println! statement used for debugging to keep the code clean.
- println!("{}", dst.clone());Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub async fn new(dst: String, world_address: Felt) -> Result<Self, Error> { | |
| let endpoint = | |
| Endpoint::from_shared(dst.clone()).map_err(|e| Error::Endpoint(e.to_string()))?; | |
| println!("{}", dst.clone()); | |
| let channel = endpoint.connect().await.map_err(Error::Transport)?; | |
| Ok(Self { | |
| _world_address, | |
| inner: world_client::WorldClient::connect(dst).await.map_err(Error::Transport)?, | |
| _world_address: world_address, | |
| inner: world_client::WorldClient::with_origin(channel, endpoint.uri().clone()), | |
| pub async fn new(dst: String, world_address: Felt) -> Result<Self, Error> { | |
| let endpoint = | |
| Endpoint::from_shared(dst.clone()).map_err(|e| Error::Endpoint(e.to_string()))?; | |
| let channel = endpoint.connect().await.map_err(Error::Transport)?; | |
| Ok(Self { | |
| _world_address: world_address, | |
| inner: world_client::WorldClient::with_origin(channel, endpoint.uri().clone()), |
crates/torii/grpc/src/client.rs
Outdated
| pub async fn new(dst: String, world_address: Felt) -> Result<Self, Error> { | ||
| let endpoint = | ||
| Endpoint::from_shared(dst.clone()).map_err(|e| Error::Endpoint(e.to_string()))?; | ||
| println!("{}", dst.clone()); | ||
| let channel = endpoint.connect().await.map_err(Error::Transport)?; | ||
| println!("{}", "channel done"); | ||
| Ok(Self { | ||
| _world_address, | ||
| inner: world_client::WorldClient::connect(dst).await.map_err(Error::Transport)?, | ||
| _world_address: world_address, | ||
| inner: world_client::WorldClient::with_origin(channel, endpoint.uri().clone()), |
There was a problem hiding this comment.
Refactor suggestion: Extract TLS initialization into a separate function.
Consider refactoring the TLS initialization logic into a separate function. This will improve code clarity and facilitate reuse, especially if similar logic is needed elsewhere.
// Example of a possible refactor
async fn initialize_tls_channel(dst: String) -> Result<tonic::transport::Channel, Error> {
let endpoint = Endpoint::from_shared(dst.clone()).map_err(|e| Error::Endpoint(e.to_string()))?;
endpoint.connect().await.map_err(Error::Transport)
}
// Usage in `new` function
let channel = initialize_tls_channel(dst.clone()).await?;Remove debugging println! statements.
The println! statements used for debugging should be removed to keep the code clean for production.
- println!("{}", dst.clone());
- println!("{}", "channel done");Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub async fn new(dst: String, world_address: Felt) -> Result<Self, Error> { | |
| let endpoint = | |
| Endpoint::from_shared(dst.clone()).map_err(|e| Error::Endpoint(e.to_string()))?; | |
| println!("{}", dst.clone()); | |
| let channel = endpoint.connect().await.map_err(Error::Transport)?; | |
| println!("{}", "channel done"); | |
| Ok(Self { | |
| _world_address, | |
| inner: world_client::WorldClient::connect(dst).await.map_err(Error::Transport)?, | |
| _world_address: world_address, | |
| inner: world_client::WorldClient::with_origin(channel, endpoint.uri().clone()), | |
| pub async fn new(dst: String, world_address: Felt) -> Result<Self, Error> { | |
| let endpoint = | |
| Endpoint::from_shared(dst.clone()).map_err(|e| Error::Endpoint(e.to_string()))?; | |
| let channel = endpoint.connect().await.map_err(Error::Transport)?; | |
| Ok(Self { | |
| _world_address: world_address, | |
| inner: world_client::WorldClient::with_origin(channel, endpoint.uri().clone()), |
| pub async fn new(dst: String, world_address: Felt) -> Result<Self, Error> { | ||
| let endpoint = | ||
| Endpoint::from_shared(dst.clone()).map_err(|e| Error::Endpoint(e.to_string()))?; | ||
| let channel = endpoint.connect().await.map_err(Error::Transport)?; | ||
| Ok(Self { | ||
| _world_address, | ||
| inner: world_client::WorldClient::connect(dst).await.map_err(Error::Transport)?, | ||
| _world_address: world_address, | ||
| inner: world_client::WorldClient::with_origin(channel, endpoint.uri().clone()), |
There was a problem hiding this comment.
Refactor suggestion: Extract TLS initialization into a separate function.
Consider refactoring the TLS initialization logic into a separate function. This will improve code clarity and facilitate reuse, especially if similar logic is needed elsewhere.
// Example of a possible refactor
async fn initialize_tls_channel(dst: String) -> Result<tonic::transport::Channel, Error> {
let endpoint = Endpoint::from_shared(dst.clone()).map_err(|e| Error::Endpoint(e.to_string()))?;
endpoint.connect().await.map_err(Error::Transport)
}
// Usage in `new` function
let channel = initialize_tls_channel(dst.clone()).await?;
Description
We couldn't connect to a remote Slot using dojo.c. Only the Unity game built with a web build could connect.
This PR fix this issue. Thanks @steebchen for the help to understand what was wrong.
The issue was that GRPC didn't handled the right uri, we are now using with_origin method to connect to GRPC.
There is also a part of the code retrieving the public certs to handle HTTPS.
Added to documentation?
Once dojo.c uses this commit, we can update the doc to say that any build can connect to Slot, not just wasm.
Checklist
scripts/prettier.sh,scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit
New Features
0.11, enhancing security for communication.Chores
Refactor