feat(bens): Added TLD-based fallback for name search in networks #1175
Conversation
|
Warning Rate limit exceeded@Ilyak777 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 38 minutes and 12 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe changes modify the Assessment against linked issues
Poem
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 using PR comments)
Other keywords and placeholders
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
🧹 Nitpick comments (3)
blockscout-ens/bens-logic/src/protocols/protocoler.rs (3)
69-69: Consider making the limit configurable or documenting its rationale.Currently,
MAX_NAMES_LIMITis a hardcoded magic number. Labeling it as a constant is good, but consider either making it configurable (e.g., via environment variables or config files) or at least documenting why 5 is an appropriate limit for your use case.
207-241: Consider deduplicating or short-circuiting logic for performance.When appending TLDs in a loop and collecting names, consider:
- Maintaining a set to avoid duplicates if multiple protocols yield the same name.
- Short-circuiting earlier once the limit is reached, both outside and inside
find_names_with_tld, to reduce unnecessary calls.This can improve performance and clarity.
243-258: Reassess error handling infilter_mapto avoid silently discarding errors.Using
.filter_map(|p| DomainNameOnProtocol::new(name_with_tld, p).ok())silently drops errors. If legitimate errors occur, they won't be surfaced, making debugging more difficult. Consider handling or logging errors before discarding them.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
blockscout-ens/bens-logic/src/protocols/protocoler.rs (4)
293-305: Consider using HashSet for TLD collection.The current implementation might collect duplicate TLDs. Using a HashSet would be more efficient:
- .collect::<Vec<Tld>>(); + .collect::<std::collections::HashSet<Tld>>();
322-324: Enhance error message for invalid names.The current error message doesn't provide enough context about why the name is invalid.
- return Err(ProtocolError::InvalidName(name.to_string())); + return Err(ProtocolError::InvalidName { + name: name.to_string(), + reason: "no valid TLD combinations found".to_string(), + });
329-344: Add documentation for the helper method.This public method would benefit from documentation explaining its purpose, parameters, and return value.
+ /// Attempts to find domain names with the specified TLD in the given network. + /// + /// # Arguments + /// * `name_with_tld` - The domain name including TLD + /// * `network_id` - The network to search in + /// * `maybe_filter` - Optional protocol filter + /// + /// # Returns + /// A vector of valid domain names on their respective protocols fn find_names_with_tld(
293-344: Well-structured implementation of TLD-based fallback.The implementation successfully achieves the PR objective of enhancing name resolution with TLD fallback. The separation of concerns between
names_options_in_networkandfind_names_with_tldis clean and maintainable.A few architectural considerations:
- The solution gracefully handles both TLD and non-TLD cases
- The result limiting ensures reasonable response sizes
- The error handling provides a good foundation for debugging
Consider adding metrics/logging to track:
- Number of TLD fallbacks attempted
- Success rate of TLD combinations
- Performance impact of TLD collection
🧰 Tools
🪛 GitHub Actions: Test, lint and docker (bens)
[error] 303-341: Code formatting issues detected by cargo fmt. Multiple formatting inconsistencies found including incorrect spacing and line breaks. Run 'cargo fmt' to fix these issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
blockscout-ens/bens-logic/src/protocols/protocoler.rs(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Test, lint and docker (bens)
blockscout-ens/bens-logic/src/protocols/protocoler.rs
[error] 303-341: Code formatting issues detected by cargo fmt. Multiple formatting inconsistencies found including incorrect spacing and line breaks. Run 'cargo fmt' to fix these issues.
🔇 Additional comments (1)
blockscout-ens/bens-logic/src/protocols/protocoler.rs (1)
148-148: LGTM! Good use of a constant for limiting results.The constant helps prevent excessive name variations while providing a reasonable limit for user queries.
| let tlds = self | ||
| .networks | ||
| .get(&network_id) | ||
| .ok_or_else(|| ProtocolError::NetworkNotFound(network_id))? | ||
| .use_protocols | ||
| .iter() | ||
| .filter_map(|protocol_name| { | ||
| self.protocols | ||
| .get(protocol_name) | ||
| .map(|protocol| protocol.info.tld_list.iter().cloned()) | ||
| }) | ||
| .flatten() | ||
| .collect::<Vec<Tld>>(); | ||
|
|
||
| if name.contains('.') { | ||
| return self.find_names_with_tld(name, network_id, maybe_filter); | ||
| } | ||
|
|
||
| let mut all_names_with_protocols = Vec::new(); | ||
| for tld in tlds { | ||
| if all_names_with_protocols.len() >= MAX_NAMES_LIMIT { | ||
| break; | ||
| } | ||
| let name_with_tld = format!("{}.{}", name, tld.0); | ||
| if let Ok(mut names) = self.find_names_with_tld(&name_with_tld, network_id, maybe_filter.clone()) { | ||
| all_names_with_protocols.append(&mut names); | ||
| } | ||
| } | ||
|
|
||
| if all_names_with_protocols.is_empty() { | ||
| return Err(ProtocolError::InvalidName(name.to_string())); | ||
| } | ||
|
|
||
| Ok(all_names_with_protocols.into_iter().take(MAX_NAMES_LIMIT).collect()) | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Formatting issues confirmed in protocoler.rs
- Line 317 exceeds Rust's standard 100-character line length limit
- Multiple consecutive empty lines found between function definitions
Run cargo fmt to automatically fix these issues.
🔗 Analysis chain
Fix code formatting issues.
The pipeline has detected formatting inconsistencies. Please run cargo fmt to fix these issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify formatting issues
cargo fmt --all -- --check
Length of output: 124
Script:
#!/bin/bash
# Check for long lines (Rust's default is 100 characters)
rg -l "^.{101,}$" blockscout-ens/bens-logic/src/protocols/protocoler.rs
# Check for multiple consecutive empty lines
rg -l "\n\n\n+" blockscout-ens/bens-logic/src/protocols/protocoler.rs
# Verify code structure
ast-grep --pattern 'fn $_ ($$$) -> Result<$_, $_> {
$$$
}' blockscout-ens/bens-logic/src/protocols/protocoler.rs
Length of output: 2026
Script:
#!/bin/bash
# Show the actual long lines with line numbers
rg -n "^.{101,}$" blockscout-ens/bens-logic/src/protocols/protocoler.rs
# Check for multiple empty lines using a different approach
rg -U "(\n\s*){3,}" blockscout-ens/bens-logic/src/protocols/protocoler.rs
Length of output: 310
….com:blockscout/blockscout-rs into feat/bens/add-tld-for-name-in-network-search
| if name.contains('.') { | ||
| return self.find_names_with_tld(name, network_id, maybe_filter); | ||
| } |
There was a problem hiding this comment.
try to avoid if {return} statements because it makes the code harder to read. consider refactoring to reduce nesting and improve flow
| if all_names_with_protocols.is_empty() { | ||
| return Err(ProtocolError::InvalidName(name.to_string())); | ||
| } |
There was a problem hiding this comment.
also try to rearrange code in such way that you don't need this if-return statement
Closes #917
This PR enhances the names_options_in_network function by introducing a fallback mechanism for names without TLD. If the input name does not include a TLD, the function appends possible TLDs associated with the network's protocols, generating up to a defined maximum number of results (MAX_NAMES_LIMIT).
Summary by CodeRabbit
New Features
Bug Fixes