feat(torii-libp2p): inter torii messaging for offchain messages #3082
Conversation
|
Ohayo, sensei! Below is the detailed breakdown of the changes in this pull request. WalkthroughThis pull request updates two parts of the torii libp2p crate. In the client module, the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (4)
🪧 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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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
🧹 Nitpick comments (2)
crates/torii/libp2p/src/client/mod.rs (2)
64-64: Ohayo sensei, consider gracefully handling errors instead of unwrapping.Using
unwrap()might panic. A more robust approach:- let cert = Certificate::generate(&mut thread_rng()).unwrap(); + let cert = Certificate::generate(&mut thread_rng()) + .map_err(Error::CertificateGeneration)?;
74-87: Ohayo sensei, WebSocket transport creation is solid.Your combination of Noise + Yamux is a common, reliable pattern. Watch for
expect()usage if you prefer recovery over panic.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/torii/libp2p/src/client/mod.rs(2 hunks)crates/torii/libp2p/src/server/mod.rs(1 hunks)
🔇 Additional comments (4)
crates/torii/libp2p/src/server/mod.rs (1)
288-288: Ohayo sensei, be mindful of overshadowing other pattern matches.Changing
None => ...to_ => ...risks swallowing other potential branches. If you only intended to handleNone, revert to a more explicit match. Otherwise, confirm no other variants need separate handling.crates/torii/libp2p/src/client/mod.rs (3)
15-15: Ohayo sensei, nice new import.Bringing in
libp2p_webrtcis essential for the new transport. Looks good!
54-58: Ohayo sensei, these imports look well-organized.They’re properly used below and align with your new transport setup.
70-73: Ohayo sensei, good job integrating the WebRTC transport.Be sure the ephemeral certificate usage meets security requirements. Otherwise, everything here looks correct.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/torii/cli/src/options.rs (1)
79-87: Looks good! The new peer sync option is well-structured.Ohayo sensei! The addition of this feature looks well-implemented with proper documentation and CLI argument configuration. This will enable inter-torii relay communication for offchain messages as intended.
You might consider clarifying the expected format of peer addresses in the help message, such as specifying if these should be LibP2P multiaddresses (e.g., "/ip4/127.0.0.1/tcp/9090/p2p/QmHash...").
- help = "A list of other torii relays to connect to and sync with." + help = "A list of other torii relays to connect to and sync with. Format: multiaddr (e.g., \"/ip4/127.0.0.1/tcp/9090/p2p/QmHash...\")."
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/cli/src/options.rs(2 hunks)
🔇 Additional comments (1)
crates/torii/cli/src/options.rs (1)
98-98: Default implementation updated correctly!The default implementation properly initializes the new peers field with an empty vector, maintaining consistency with how other collection fields are handled in this codebase.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/torii/libp2p/src/server/mod.rs (2)
295-295: Maintain explicit matching onNone
Ohayo sensei, replacingNone =>with_ =>can obscure future changes or unexpected variants. Retaining explicit matching onNonehelps catch potential edge cases more clearly.
365-382: Remove theunwrap()on serialization
Ohayo sensei, relying onunwrap()here can panic if serialization fails unexpectedly. It might be safer to handle the error in a consistent manner:-if let Err(e) = self - .swarm - .behaviour_mut() - .gossipsub - .publish( - IdentTopic::new(constants::MESSAGING_TOPIC), - serde_json::to_string(&data).unwrap(), - ) - .map_err(Error::PublishError) -{ +if let Err(e) = serde_json::to_string(&data) + .map_err(Error::from) + .and_then(|serialized| { + self.swarm + .behaviour_mut() + .gossipsub + .publish(IdentTopic::new(constants::MESSAGING_TOPIC), serialized) + .map_err(Error::PublishError) + }) +{ info!( target: LOG_TARGET, error = %e, "Publishing message to peers." ); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/libp2p/src/server/mod.rs(4 hunks)
🔇 Additional comments (1)
crates/torii/libp2p/src/server/mod.rs (1)
72-72: Nice addition of thepeersparameter
Ohayo sensei, this provides a straightforward way to initialize peer connections for offchain message broadcasting.
| // We dial all of our peers. Our server will then broadcast | ||
| // all incoming offchain messages to all of our peers. | ||
| for peer in peers { | ||
| swarm.dial(peer.parse::<Multiaddr>().unwrap())?; | ||
| } |
There was a problem hiding this comment.
Handle potential parse failures
Ohayo sensei, using unwrap() when parsing Multiaddr may crash the server if the input is invalid. Consider gracefully handling parse errors:
-for peer in peers {
- swarm.dial(peer.parse::<Multiaddr>().unwrap())?;
-}
+for peer in peers {
+ if let Ok(addr) = peer.parse::<Multiaddr>() {
+ if let Err(e) = swarm.dial(addr) {
+ warn!(target: LOG_TARGET, error = ?e, "Dialing peer.");
+ }
+ } else {
+ warn!(target: LOG_TARGET, peer = %peer, "Invalid Multiaddr, skipping dial.");
+ }
+}📝 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.
| // We dial all of our peers. Our server will then broadcast | |
| // all incoming offchain messages to all of our peers. | |
| for peer in peers { | |
| swarm.dial(peer.parse::<Multiaddr>().unwrap())?; | |
| } | |
| // We dial all of our peers. Our server will then broadcast | |
| // all incoming offchain messages to all of our peers. | |
| for peer in peers { | |
| if let Ok(addr) = peer.parse::<Multiaddr>() { | |
| if let Err(e) = swarm.dial(addr) { | |
| warn!(target: LOG_TARGET, error = ?e, "Dialing peer."); | |
| } | |
| } else { | |
| warn!(target: LOG_TARGET, peer = %peer, "Invalid Multiaddr, skipping dial."); | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/torii/libp2p/src/server/mod.rs (1)
171-175:⚠️ Potential issueHandle potential parse failures
Ohayo sensei, using
unwrap()when parsingMultiaddrmay crash the server if the input is invalid. Consider gracefully handling parse errors:-for peer in peers { - swarm.dial(peer.parse::<Multiaddr>().unwrap())?; -} +for peer in peers { + if let Ok(addr) = peer.parse::<Multiaddr>() { + if let Err(e) = swarm.dial(addr) { + warn!(target: LOG_TARGET, error = ?e, "Dialing peer."); + } + } else { + warn!(target: LOG_TARGET, peer = %peer, "Invalid Multiaddr, skipping dial."); + } +}
🧹 Nitpick comments (1)
crates/torii/libp2p/src/server/mod.rs (1)
301-301: More robust pattern matching with wildcard.Ohayo sensei! Good improvement changing from specifically matching
Noneto using a wildcard pattern. This makes the code more flexible to handle any case where entity_identity isn't a Some value.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/torii/libp2p/src/constants.rs(1 hunks)crates/torii/libp2p/src/server/mod.rs(5 hunks)
🔇 Additional comments (3)
crates/torii/libp2p/src/constants.rs (1)
3-3: LGTM! New messaging topic for peer communication.Ohayo sensei! The new constant for peer-to-peer communication is well-named and follows the same pattern as the existing message topic.
crates/torii/libp2p/src/server/mod.rs (2)
72-72: Well-designed parameter addition for peer connections.Ohayo sensei! Nice work adding support for connecting to other Torii relays.
187-191: LGTM! Subscription to peer messaging topic.Ohayo sensei! Excellent work setting up the subscription to the peer messaging topic, consistent with how other topics are handled.
| // We only want to publish messages from our clients. Not from our peers | ||
| // otherwise recursion hell :< | ||
| if message.topic != IdentTopic::new(constants::MESSAGING_TOPIC).hash() { | ||
| continue; | ||
| } | ||
|
|
||
| // Publish message to all peers | ||
| if let Err(e) = self | ||
| .swarm | ||
| .behaviour_mut() | ||
| .gossipsub | ||
| .publish( | ||
| IdentTopic::new(constants::PEERS_MESSAGING_TOPIC), | ||
| serde_json::to_string(&data).unwrap(), | ||
| ) | ||
| .map_err(Error::PublishError) | ||
| { | ||
| info!( | ||
| target: LOG_TARGET, | ||
| error = %e, | ||
| "Publishing message to peers." | ||
| ); | ||
| } |
There was a problem hiding this comment.
Handle potential serialization failure
Ohayo sensei! The code has a robust check to prevent recursion when forwarding messages, but there's an unwrap() call that could cause the server to crash if serialization fails.
if let Err(e) = self
.swarm
.behaviour_mut()
.gossipsub
.publish(
IdentTopic::new(constants::PEERS_MESSAGING_TOPIC),
- serde_json::to_string(&data).unwrap(),
+ match serde_json::to_string(&data) {
+ Ok(serialized) => serialized,
+ Err(e) => {
+ info!(
+ target: LOG_TARGET,
+ error = %e,
+ "Serializing message for peers."
+ );
+ continue;
+ }
+ },
)
.map_err(Error::PublishError)📝 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.
| // We only want to publish messages from our clients. Not from our peers | |
| // otherwise recursion hell :< | |
| if message.topic != IdentTopic::new(constants::MESSAGING_TOPIC).hash() { | |
| continue; | |
| } | |
| // Publish message to all peers | |
| if let Err(e) = self | |
| .swarm | |
| .behaviour_mut() | |
| .gossipsub | |
| .publish( | |
| IdentTopic::new(constants::PEERS_MESSAGING_TOPIC), | |
| serde_json::to_string(&data).unwrap(), | |
| ) | |
| .map_err(Error::PublishError) | |
| { | |
| info!( | |
| target: LOG_TARGET, | |
| error = %e, | |
| "Publishing message to peers." | |
| ); | |
| } | |
| // We only want to publish messages from our clients. Not from our peers | |
| // otherwise recursion hell :< | |
| if message.topic != IdentTopic::new(constants::MESSAGING_TOPIC).hash() { | |
| continue; | |
| } | |
| // Publish message to all peers | |
| if let Err(e) = self | |
| .swarm | |
| .behaviour_mut() | |
| .gossipsub | |
| .publish( | |
| IdentTopic::new(constants::PEERS_MESSAGING_TOPIC), | |
| match serde_json::to_string(&data) { | |
| Ok(serialized) => serialized, | |
| Err(e) => { | |
| info!( | |
| target: LOG_TARGET, | |
| error = %e, | |
| "Serializing message for peers." | |
| ); | |
| continue; | |
| } | |
| }, | |
| ) | |
| .map_err(Error::PublishError) | |
| { | |
| info!( | |
| target: LOG_TARGET, | |
| error = %e, | |
| "Publishing message to peers." | |
| ); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
crates/torii/libp2p/src/server/mod.rs (2)
183-187:⚠️ Potential issueHandle potential parse failures
Ohayo sensei, using
unwrap()when parsingMultiaddrmay crash the server if the input is invalid. Consider gracefully handling parse errors:-for peer in peers { - swarm.dial(peer.parse::<Multiaddr>().unwrap())?; -} +for peer in peers { + if let Ok(addr) = peer.parse::<Multiaddr>() { + if let Err(e) = swarm.dial(addr) { + warn!(target: LOG_TARGET, error = ?e, "Dialing peer."); + } + } else { + warn!(target: LOG_TARGET, peer = %peer, "Invalid Multiaddr, skipping dial."); + } +}
384-406:⚠️ Potential issueHandle potential serialization failure
Ohayo sensei! The code has a robust check to prevent recursion when forwarding messages, but there's an unwrap() call that could cause the server to crash if serialization fails.
if let Err(e) = self .swarm .behaviour_mut() .gossipsub .publish( IdentTopic::new(constants::PEERS_MESSAGING_TOPIC), - serde_json::to_string(&data).unwrap(), + match serde_json::to_string(&data) { + Ok(serialized) => serialized, + Err(e) => { + info!( + target: LOG_TARGET, + error = %e, + "Serializing message for peers." + ); + continue; + } + }, ) .map_err(Error::PublishError)
🧹 Nitpick comments (1)
crates/torii/libp2p/src/server/mod.rs (1)
76-85: Keep an eye on the code formatting issue on line 70.Ohayo sensei! The pipeline is reporting a code formatting issue on line 70 related to "inconsistent use of function call formatting." This might be related to how the parameters are aligned between the two function declarations.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/torii/libp2p/src/server/mod.rs(5 hunks)crates/torii/runner/src/lib.rs(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/libp2p/src/server/mod.rs
[error] 70-70: Code formatting issues detected: inconsistent use of function call formatting.
🔇 Additional comments (5)
crates/torii/runner/src/lib.rs (1)
225-234: LGTM! Relay server now connects to specified peers.Ohayo sensei! You've successfully updated the
runmethod to use the newnew_with_peersfunction, allowing the relay server to connect to a specified list of peers. This change enables inter-torii messaging for offchain messages as mentioned in the PR objectives.crates/torii/libp2p/src/server/mod.rs (4)
63-74: Good refactoring of the Relay constructor!Ohayo sensei! You've nicely refactored the original
newmethod to delegate to the newnew_with_peersmethod with an empty peer list. This maintains backward compatibility while adding new functionality.🧰 Tools
🪛 GitHub Actions: ci
[error] 70-70: Code formatting issues detected: inconsistent use of function call formatting.
199-204: Good subscription to the peer messaging topic!Ohayo sensei! You've properly subscribed to the new peer messaging topic, which is necessary for inter-torii communication.
313-324: Pattern matching improvement with wildcard.The change from
Noneto wildcard match (_) improves the code by catching any case where entity_identity isn't present or can't be parsed from the database. This is more robust and will handle unexpected states better.
384-389: Great recursion prevention logic!Ohayo sensei! The check to ensure messages don't recursively propagate between peers is essential for preventing potential infinite loops or message storms in the network. This is a very important safety measure.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3082 +/- ##
==========================================
+ Coverage 57.22% 57.26% +0.04%
==========================================
Files 441 441
Lines 60098 60182 +84
==========================================
+ Hits 34390 34464 +74
- Misses 25708 25718 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary by CodeRabbit
New Features
Refactor