feat(bens): Added TLD-based fallback for name search in networks #1175
Conversation
WalkthroughThe pull request introduces modifications to the Assessment against linked issues
Poem
Tip 🌐 Web search-backed reviews and chat
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.
….com:blockscout/blockscout-rs into feat/bens/add-tld-for-name-in-network-search
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
blockscout-ens/bens-logic/src/protocols/protocoler.rs (2)
148-148: Add documentation for the constant.Consider adding a doc comment explaining why the limit is set to 5 and what factors influenced this choice.
+/// Maximum number of name variations to return when generating TLD-based suggestions. +/// This limit prevents excessive variations while providing sufficient options for users. const MAX_NAMES_LIMIT: usize = 5;
323-326: Consider preserving error context.The current implementation silently ignores errors during name suggestion generation. Consider collecting errors or at least logging them for debugging purposes.
-match self.names_options_in_network_with_suggestions(&name_with_tld, network_id, maybe_filter.clone()) { - Ok(vec) => vec, - Err(_) => Vec::new(), +match self.names_options_in_network_with_suggestions(&name_with_tld, network_id, maybe_filter.clone()) { + Ok(vec) => vec, + Err(e) => { + tracing::debug!("Failed to generate suggestion for {}: {}", name_with_tld, e); + Vec::new() + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
blockscout-ens/bens-logic/src/protocols/protocoler.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Unit, doc and integration tests
🔇 Additional comments (3)
blockscout-ens/bens-logic/src/protocols/protocoler.rs (3)
319-329: Well-implemented functional approach!The code effectively uses functional programming concepts (map, flat_map, take) to handle name variations, making it more readable and maintainable.
342-359: Clean and well-structured implementation!The helper method effectively:
- Validates input and provides clear error messages
- Handles domain name creation and TLD extraction
- Generates protocol-specific name variations
292-340: Verify backward compatibility.The changes modify the behavior of
names_options_in_network. Let's verify that existing callers handle both the new limit and potential errors correctly.✅ Verification successful
Changes maintain backward compatibility ✅
The implementation is safe as all callers properly handle both the new result limit and potential errors:
- Internal callers either use first result or iterate results
- External caller in
reader.rsgracefully handles errors by returning empty list🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all callers of names_options_in_network to verify compatibility # Find direct function calls rg -A 2 "names_options_in_network\(" --type rust # Find error handling patterns in callers ast-grep --pattern 'match $_.names_options_in_network($$$) { $$$ }'Length of output: 2034
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
blockscout-ens/bens-logic/src/protocols/protocoler.rs (2)
148-148: Document the MAX_NAMES_LIMIT constant.Add documentation explaining the purpose of this constant and the rationale behind choosing 5 as the limit.
+/// Maximum number of name variations to return when generating TLD-based suggestions. +/// Limited to 5 to prevent excessive resource usage while providing sufficient options. const MAX_NAMES_LIMIT: usize = 5;
293-305: Optimize TLD collection for better performance.The current implementation creates unnecessary intermediate collections. Consider using a more efficient chain of iterator operations.
- 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>>(); + let tlds = self + .networks + .get(&network_id) + .ok_or_else(|| ProtocolError::NetworkNotFound(network_id))? + .use_protocols + .iter() + .flat_map(|protocol_name| { + self.protocols + .get(protocol_name) + .into_iter() + .flat_map(|protocol| protocol.info.tld_list.iter().cloned()) + }) + .collect::<Vec<Tld>>();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
blockscout-ens/bens-logic/src/protocols/protocoler.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Unit, doc and integration tests
🔇 Additional comments (1)
blockscout-ens/bens-logic/src/protocols/protocoler.rs (1)
347-365: Rename function to better reflect its purpose.The function name
names_options_in_network_with_suggestionsis misleading as it doesn't find names but rather returns name options for a given TLD.Consider renaming the function to better reflect its purpose, for example:
- fn names_options_in_network_with_suggestions( + fn get_name_options_for_tld(
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
blockscout-ens/bens-logic/src/protocols/protocoler.rs (1)
307-342:⚠️ Potential issueImprove code structure and error handling.
The current implementation has several issues:
- Deep nesting with match statements makes the code harder to follow
- Silently ignoring errors during name creation could hide important issues
Consider restructuring the code to reduce nesting and improve error handling as suggested in the previous review.
🧹 Nitpick comments (2)
blockscout-ens/bens-logic/src/protocols/protocoler.rs (2)
148-148: Document the rationale for MAX_NAMES_LIMIT value.Consider adding a doc comment explaining why 5 was chosen as the limit and what factors might influence this value.
+/// Maximum number of name variations to return when generating TLD-based suggestions. +/// This limit prevents overwhelming users with too many options while ensuring +/// the most relevant TLDs are included. const MAX_NAMES_LIMIT: usize = 5;
293-305: Simplify TLD collection using iterator chaining.The nested iterator transformations can be simplified for better readability.
- 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>>(); + let network = self.networks + .get(&network_id) + .ok_or_else(|| ProtocolError::NetworkNotFound(network_id))?; + + let tlds: Vec<Tld> = network.use_protocols.iter() + .filter_map(|name| self.protocols.get(name)) + .flat_map(|protocol| protocol.info.tld_list.iter().cloned()) + .collect();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
blockscout-ens/bens-logic/src/protocols/protocoler.rs(4 hunks)blockscout-ens/bens-server/tests/data/domains/levvv_gno/detailed.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- blockscout-ens/bens-server/tests/data/domains/levvv_gno/detailed.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Unit, doc and integration tests
🔇 Additional comments (2)
blockscout-ens/bens-logic/src/protocols/protocoler.rs (2)
345-363: Well-structured helper method with proper error handling!The implementation is clean, with clear error messages and appropriate error handling.
417-452: Add tests for TLD fallback functionality.While the current tests cover basic TLD operations, consider adding tests for:
- Name resolution with TLD fallback
- Cases where MAX_NAMES_LIMIT is reached
- Error cases in TLD fallback path
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
blockscout-ens/README.md (3)
49-56: Consider adding more context to the test run section.While the instructions are clear, it would be helpful to add:
- What types of tests are included (e.g., unit tests, integration tests)
- What components are being tested
- Expected test output or success criteria
78-93: Improve grammar and precision in contribution guidelines.Consider these improvements:
- Line 78: "connected to a common database"
- Line 85: "Add your protocol to the list of"
- Line 86: "Update the default config"
- Line 87: "create a PR"
- Line 92: "contains the correct number of domains"
Apply this diff to improve the text:
-connected to common database +connected to a common database -Add your protocol to list of +Add your protocol to the list of -Update default config +Update the default config -create PR with: +create a PR with: -contains correct amount of domains +contains the correct number of domains🧰 Tools
🪛 LanguageTool
[uncategorized] ~78-~78: You might be missing the article “a” here.
Context: ...ig and startbens-serverconnected to common database (read more in [how to start be...(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~85-~85: You might be missing the article “the” here.
Context: ...ain/index.html) 9. Add your protocol to list of [supported domains](#current-support...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~86-~86: You might be missing the article “the” here.
Context: ...(#current-supported-domains) 10. Update default config of BENS server for [production](...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~87-~87: You might be missing the article “a” here.
Context: ...onfig/staging.json) 11. Finally, create PR with: - New directory inside `bloc...(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~92-~92: You might be missing the article “the” here.
Context: ...oof that your indexed subgraph contains correct amount of domains, resolved_addresses a...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~92-~92: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ... your indexed subgraph contains correct amount of domains, resolved_addresses and so o...(AMOUNTOF_TO_NUMBEROF)
94-94: Remove unused anchor reference.The line
[anchor]: [anchor]:appears to be unused and should be removed.🧰 Tools
🪛 Markdownlint (0.37.0)
94-94: Unused link or image reference definition: "anchor"
Link and image reference definitions should be needed(MD053, link-image-reference-definitions)
blockscout-ens/bens-server/tests/domains.rs (1)
268-271: LGTM! Consider adding more cross-protocol test cases.The changes correctly verify TLD-based fallback in a multi-protocol environment. However, consider adding test cases for:
- Ambiguous domain names that could resolve to multiple TLDs
- Invalid domain names that don't match any protocol's TLD pattern
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
blockscout-ens/README.md(2 hunks)blockscout-ens/bens-logic/src/subgraph/reader.rs(1 hunks)blockscout-ens/bens-logic/src/subgraph/sql/domain.rs(2 hunks)blockscout-ens/bens-logic/tests/migrations/20231219085631_push_mock_gnosis_data.sql(1 hunks)blockscout-ens/bens-server/tests/domains.rs(4 hunks)
✅ Files skipped from review due to trivial changes (2)
- blockscout-ens/bens-logic/src/subgraph/sql/domain.rs
- blockscout-ens/bens-logic/src/subgraph/reader.rs
🧰 Additional context used
🪛 LanguageTool
blockscout-ens/README.md
[uncategorized] ~78-~78: You might be missing the article “a” here.
Context: ...ig and start bens-server connected to common database (read more in [how to start be...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~85-~85: You might be missing the article “the” here.
Context: ...ain/index.html) 9. Add your protocol to list of [supported domains](#current-support...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~86-~86: You might be missing the article “the” here.
Context: ...(#current-supported-domains) 10. Update default config of BENS server for [production](...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~87-~87: You might be missing the article “a” here.
Context: ...onfig/staging.json) 11. Finally, create PR with: - New directory inside `bloc...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~92-~92: You might be missing the article “the” here.
Context: ...oof that your indexed subgraph contains correct amount of domains, resolved_addresses a...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~92-~92: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ... your indexed subgraph contains correct amount of domains, resolved_addresses and so o...
(AMOUNTOF_TO_NUMBEROF)
🪛 Markdownlint (0.37.0)
blockscout-ens/README.md
94-94: Unused link or image reference definition: "anchor"
Link and image reference definitions should be needed
(MD053, link-image-reference-definitions)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Unit, doc and integration tests
🔇 Additional comments (5)
blockscout-ens/README.md (2)
13-22: LGTM! Well-formatted table of supported domains.The table clearly presents the supported domain services with their respective networks, TLDs, and notes.
26-36: LGTM! Well-structured environment variables documentation.The table clearly presents the environment variables with their requirements, descriptions, and default values.
blockscout-ens/bens-server/tests/domains.rs (2)
Line range hint
73-85: LGTM! Event endpoints correctly updated for TLD-based fallback.The changes maintain consistency in the API by removing TLD suffixes from event endpoints while preserving the existing functionality and response format.
58-64: LGTM! Test coverage aligns with TLD-based fallback implementation.The changes correctly verify that domain names without TLD suffixes are properly resolved in the ETH network, while maintaining backward compatibility with the existing response format.
✅ Verification successful
Verification successful: TLD-based fallback implementation is properly tested
The test suite comprehensively covers:
- TLD-less API endpoints across different protocols (ETH, GNO)
- Special character handling (emoji domains)
- Proper response format maintaining TLD information
- Multiple network configurations (1, 1337)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify test coverage for various TLD scenarios rg -A 2 "domains/[^/]+/detailed.json" blockscout-ens/bens-server/tests/Length of output: 1071
Script:
#!/bin/bash # Check test data files content and API endpoint patterns echo "=== Test Data Files ===" fd "detailed.json" blockscout-ens/bens-server/tests/data/domains -x cat {} echo -e "\n=== API Endpoint Patterns ===" rg "/api/v1/\d+/domains/[^\"']+" blockscout-ens/bens-server/tests/domains.rsLength of output: 3894
blockscout-ens/bens-logic/tests/migrations/20231219085631_push_mock_gnosis_data.sql (1)
275-275: Avoid using MAX_INT (2147483647) for expiry dates.While this is test data, using MAX_INT (2^31 - 1) for dates can lead to:
- Y2K38 problems on 32-bit systems
- Potential overflow issues when performing date arithmetic
- Difficulty in distinguishing between "never expires" and actual expiry dates
Let's verify if this pattern is used in production:
Consider using a more reasonable future date for test data, such as 5-10 years from now, or introduce a proper "no expiry" representation in the schema if that's the intent.
-INSERT INTO sgd2.domain (..., expiry_date) VALUES (..., 2147483647); +INSERT INTO sgd2.domain (..., expiry_date) VALUES (..., 1893456000); -- 2030-01-01✅ Verification successful
Using MAX_INT for expiry dates is isolated to test data only
The codebase search confirms that the use of MAX_INT (2147483647) for expiry dates:
- Is limited to mock data in test files
- Is not present in production migrations or schema definitions
- Production code properly handles expiry dates with timestamp conversions
While the suggestion to use a more reasonable future date in test data is a good practice, this is not a critical issue as it doesn't affect production code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for MAX_INT or similar large timestamps in production SQL files rg -g '!*test*' -g '!*mock*' '2147483647|9999999999' --type sql # Search for large expiry dates in migrations rg -g 'migrations/*.sql' 'expiry_date.*[12][90][0-9]{8}'Length of output: 273
Script:
#!/bin/bash # Find SQL files excluding test/mock directories fd -e sql -E '*test*' -E '*mock*' . | while read -r file; do echo "=== $file ===" rg -U '(2147483647|9999999999|expiry_date.*20[34][0-9])' "$file" || true done # Specifically search for expiry_date column definitions fd -e sql . | while read -r file; do echo "=== $file ===" rg -U 'expiry_date.*timestamp|create table.*expiry_date' "$file" || true doneLength of output: 8393
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
blockscout-ens/bens-logic/src/protocols/protocoler.rs (2)
149-149: Consider making MAX_NAMES_LIMIT configurable.While 5 is a reasonable default, consider making this limit configurable through environment variables or configuration files to allow flexibility in different deployment scenarios.
-const MAX_NAMES_LIMIT: usize = 5; +const MAX_NAMES_LIMIT: usize = std::env::var("BENS_MAX_NAMES_LIMIT") + .ok() + .and_then(|v| v.parse().ok()) + .unwrap_or(5);
381-414: Reduce code duplication with names_options_in_network_exact.The function shares significant logic with
names_options_in_network_exact. Consider extracting the common logic into a private helper function.+ fn create_domain_names( + &self, + name: &str, + protocols: NonEmpty<DeployedProtocol<'_>>, + ) -> Result<Vec<DomainNameOnProtocol>, ProtocolError> { + protocols + .into_iter() + .map(|deployed_protocol| { + let empty_label_hash = match &deployed_protocol.protocol.info.protocol_specific { + ProtocolSpecific::EnsLike(ens_like) => ens_like.empty_label_hash, + ProtocolSpecific::D3Connect(d3_connect) => d3_connect.empty_label_hash, + }; + + log::debug!( + "Creating DomainName with name: {}, empty_label_hash: {:?}", + name, empty_label_hash + ); + + DomainName::new(name, empty_label_hash).map(|domain_name| DomainNameOnProtocol { + inner: domain_name, + deployed_protocol, + }) + }) + .collect() + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
blockscout-ens/bens-logic/src/protocols/protocoler.rs(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Unit, doc and integration tests
🔇 Additional comments (3)
blockscout-ens/bens-logic/src/protocols/protocoler.rs (3)
123-123: LGTM! Consistent field addition.The addition of
empty_label_hashtoD3ConnectProtocolmaintains consistency withEnsLikeProtocoland supports the protocol-agnostic handling of empty label hashes.
478-513: LGTM! Comprehensive test coverage.The test module provides good coverage of the
Tldstruct's functionality, including edge cases and error conditions.
293-342: 🛠️ Refactor suggestionRefactor to reduce nesting and improve maintainability.
The current implementation has deep nesting and duplicated error handling logic. Consider restructuring the code to be more functional and easier to follow.
- if name.contains('.') { - let direct = self.names_options_in_network_exact(name, network_id, maybe_filter)?; - if direct.is_empty() { - Err(ProtocolError::InvalidName { - name: name.to_string(), - reason: "No matching protocols for given TLD".to_string(), - }) - } else { - Ok(direct.into_iter().take(MAX_NAMES_LIMIT).collect()) - } - } else { - let all_names_with_protocols: Vec<_> = tlds - .into_iter() - .map(|tld| format!("{}.{}", name, tld.0)) - .flat_map(|name_with_tld| { - self.names_options_in_network_with_suggestions( - &name_with_tld, - network_id, - maybe_filter.clone(), - ) - .unwrap_or_default() - }) - .take(MAX_NAMES_LIMIT) - .collect(); - - if all_names_with_protocols.is_empty() { - Err(ProtocolError::InvalidName { - name: name.to_string(), - reason: "No valid TLDs".to_string(), - }) - } else { - Ok(all_names_with_protocols) - } - } + let results = if name.contains('.') { + self.names_options_in_network_exact(name, network_id, maybe_filter)? + } else { + tlds.into_iter() + .map(|tld| format!("{}.{}", name, tld.0)) + .flat_map(|name_with_tld| { + self.names_options_in_network_with_suggestions( + &name_with_tld, + network_id, + maybe_filter.clone(), + ) + .map_err(|e| { + log::debug!("Failed to get suggestions for {}: {}", name_with_tld, e); + e + }) + .unwrap_or_default() + }) + .collect() + }; + + if results.is_empty() { + Err(ProtocolError::InvalidName { + name: name.to_string(), + reason: if name.contains('.') { + "No matching protocols for given TLD" + } else { + "No valid TLDs" + } + .to_string(), + }) + } else { + Ok(results.into_iter().take(MAX_NAMES_LIMIT).collect()) + }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
blockscout-ens/bens-logic/src/protocols/protocoler.rs (2)
293-342:⚠️ Potential issueImprove error handling and code structure in names_options_in_network.
The current implementation has several issues:
- Deep nesting with if-else blocks makes the code harder to follow
- Silent error handling with
unwrap_or_default()could hide important issuesAdditionally, consider using early returns to reduce nesting:
- if name.contains('.') { - let direct = self.names_options_in_network_exact(name, network_id, maybe_filter)?; - if direct.is_empty() { - Err(ProtocolError::InvalidName { - name: name.to_string(), - reason: "No matching protocols for given TLD".to_string(), - }) - } else { - Ok(direct.into_iter().take(MAX_NAMES_LIMIT).collect()) - } - } else { + if name.contains('.') { + let direct = self.names_options_in_network_exact(name, network_id, maybe_filter)?; + return if direct.is_empty() { + Err(ProtocolError::InvalidName { + name: name.to_string(), + reason: "No matching protocols for given TLD".to_string(), + }) + } else { + Ok(direct.into_iter().take(MAX_NAMES_LIMIT).collect()) + }; + }
385-388:⚠️ Potential issueReplace println with proper logging.
Using
println!for debugging is not suitable for production code. Use a proper logging framework instead.Also applies to: 403-406, 425-428
🧹 Nitpick comments (1)
blockscout-ens/bens-logic/src/protocols/protocoler.rs (1)
149-149: Consider making MAX_NAMES_LIMIT configurable.The hardcoded limit of 5 might be too restrictive for some use cases. Consider making this configurable through environment variables or configuration files.
-const MAX_NAMES_LIMIT: usize = 5; +const MAX_NAMES_LIMIT: usize = std::env::var("MAX_NAMES_LIMIT") + .ok() + .and_then(|v| v.parse().ok()) + .unwrap_or(5);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
blockscout-ens/bens-logic/src/protocols/protocoler.rs(6 hunks)blockscout-ens/bens-logic/src/subgraph/sql/domain.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- blockscout-ens/bens-logic/src/subgraph/sql/domain.rs
🔇 Additional comments (2)
blockscout-ens/bens-logic/src/protocols/protocoler.rs (2)
478-513: Well-structured unit tests added.The new test module for the
Tldstruct provides good coverage of edge cases and core functionality.
123-123: Ensure consistent field initialization across protocol types.The
empty_label_hashfield has been added toD3ConnectProtocolto matchEnsLikeProtocol. This is good for consistency, but ensure all new instances are properly initialized.✅ Verification successful
Field initialization is properly handled
The
empty_label_hashfield is safely initialized through the derived Default trait, whereOption<B256>defaults toNone. The struct's configuration ensures consistent initialization across all usage patterns.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for D3ConnectProtocol instantiations to verify initialization ast-grep --pattern 'D3ConnectProtocol { $$$ }'Length of output: 50
Script:
#!/bin/bash # First verify the file exists and is accessible echo "=== File check ===" ls -l blockscout-ens/bens-logic/src/protocols/protocoler.rs echo -e "\n=== Struct initialization check ===" # Try different initialization patterns ast-grep --pattern 'D3ConnectProtocol { $$$' ast-grep --pattern 'D3ConnectProtocol::new($$$)' echo -e "\n=== Fallback grep search ===" rg "D3ConnectProtocol" blockscout-ens/bens-logic/src/protocols/protocoler.rs -A 5Length of output: 1369
Script:
#!/bin/bash echo "=== Default implementation check ===" rg "impl .* for D3ConnectProtocol" -A 10 blockscout-ens/bens-logic/src/protocols/protocoler.rs echo -e "\n=== Test initialization check ===" rg "#\[test\]" -A 20 blockscout-ens/bens-logic/src/protocols/protocoler.rs echo -e "\n=== Constructor check ===" rg "impl D3ConnectProtocol" -A 10 blockscout-ens/bens-logic/src/protocols/protocoler.rsLength of output: 1186
Script:
#!/bin/bash # Extract the complete struct definition with attributes echo "=== Struct definition with attributes ===" rg "struct D3ConnectProtocol" -B 5 -A 10 blockscout-ens/bens-logic/src/protocols/protocoler.rs # Check for any derive macros in the file echo -e "\n=== Derive macros check ===" rg "#\[derive" -A 1 blockscout-ens/bens-logic/src/protocols/protocoler.rsLength of output: 1800
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
blockscout-ens/bens-logic/src/protocols/protocoler.rs (1)
293-342: 🛠️ Refactor suggestionImprove error handling and code structure.
The current implementation has deep nesting and silently ignores errors during name creation.
Consider restructuring the code to reduce nesting and improve error handling as suggested in the previous review.
🧹 Nitpick comments (3)
blockscout-ens/README.md (1)
86-89: Refine the wording in the PR requirements.Consider replacing "amount of domains" with "number of domains" for better grammar.
- - Result of indexed data: proof that your indexed subgraph contains correct amount of domains, resolved_addresses and so on + - Result of indexed data: proof that your indexed subgraph contains correct number of domains, resolved_addresses and so on🧰 Tools
🪛 LanguageTool
[uncategorized] ~89-~89: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ... your indexed subgraph contains correct amount of domains, resolved_addresses and so o...(AMOUNTOF_TO_NUMBEROF)
blockscout-ens/bens-logic/src/protocols/protocoler.rs (2)
149-149: Consider making MAX_NAMES_LIMIT configurable.The hardcoded limit of 5 might need to be adjusted based on different use cases.
Consider making this a configurable parameter in the environment variables:
-const MAX_NAMES_LIMIT: usize = 5; +const MAX_NAMES_LIMIT: usize = std::env::var("BENS__MAX_NAMES_LIMIT") + .ok() + .and_then(|v| v.parse().ok()) + .unwrap_or(5);
374-400: Add debug logging for better observability.The method would benefit from debug logging to track name suggestions.
Add debug logging:
+ log::debug!( + "Generating name suggestions for {} in network {}", + name_with_tld, + network_id + ); let protocols = self.protocols_of_network_for_tld( network_id, Tld::from_domain_name(name_with_tld).unwrap(), maybe_filter, )?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
blockscout-ens/README.md(4 hunks)blockscout-ens/bens-logic/src/protocols/protocoler.rs(6 hunks)
🧰 Additional context used
🪛 Markdownlint (0.37.0)
blockscout-ens/README.md
26-26: Unused link or image reference definition: "anchor"
Link and image reference definitions should be needed
(MD053, link-image-reference-definitions)
🪛 LanguageTool
blockscout-ens/README.md
[uncategorized] ~89-~89: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ... your indexed subgraph contains correct amount of domains, resolved_addresses and so o...
(AMOUNTOF_TO_NUMBEROF)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Linting
- GitHub Check: Unit, doc and integration tests
🔇 Additional comments (7)
blockscout-ens/README.md (3)
13-22: LGTM! Well-structured table of supported domains.The table is properly formatted with consistent column widths and clear headers.
28-38: LGTM! Clear and well-organized environment variables table.The table provides comprehensive information about configuration options with proper alignment and descriptions.
47-51: LGTM! Clear and consistent code block formatting.The bash commands are properly indented and well-organized.
Also applies to: 69-71, 75-77
blockscout-ens/bens-logic/src/protocols/protocoler.rs (4)
123-123: LGTM! Consistent empty_label_hash field addition.The addition of
empty_label_hashtoD3ConnectProtocolaligns with the field inEnsLikeProtocol.
344-372: LGTM! Clear error messages and proper empty_label_hash handling.The method properly extracts TLD and handles empty_label_hash from both protocol types.
410-410: LGTM! Proper error handling in main_name_in_network.The method correctly handles the case when no names are found.
454-489: LGTM! Comprehensive test coverage for Tld struct.The test module provides good coverage of Tld functionality including edge cases.
| @@ -55,11 +55,11 @@ async fn eth_protocol_scenario(base: Url, settings: &Settings) { | |||
| let context = utils::settings_context(settings); | |||
|
|
|||
| // get detailed domain | |||
| let request: Value = send_get_request(&base, "/api/v1/1/domains/vitalik.eth").await; | |||
There was a problem hiding this comment.
Shouldn't we leave at least one test case where tld is preserved?
| @@ -55,11 +55,11 @@ async fn eth_protocol_scenario(base: Url, settings: &Settings) { | |||
| let context = utils::settings_context(settings); | |||
|
|
|||
| // get detailed domain | |||
| let request: Value = send_get_request(&base, "/api/v1/1/domains/vitalik.eth").await; | |||
| let request: Value = send_get_request(&base, "/api/v1/1/domains/vitalik").await; | |||
There was a problem hiding this comment.
adding another test case seems better, as the previous is still useful + more tests is better than less :)
Co-authored-by: Kirill Ivanov <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
blockscout-ens/bens-logic/src/protocols/protocoler.rs (3)
149-149: Add documentation for MAX_NAMES_LIMIT constant.Consider adding a doc comment explaining why the limit is set to 5 and what it represents.
+/// Maximum number of name suggestions to return when searching for names in a network. +/// This limit helps prevent excessive results when generating TLD-based suggestions. const MAX_NAMES_LIMIT: usize = 5;
344-372: Extract empty label hash logic to a helper method.The empty label hash extraction logic is duplicated. Consider extracting it to a helper method.
+ fn get_empty_label_hash(protocol_specific: &ProtocolSpecific) -> Option<B256> { + match protocol_specific { + ProtocolSpecific::EnsLike(ens_like) => ens_like.empty_label_hash, + ProtocolSpecific::D3Connect(d3_connect) => d3_connect.empty_label_hash, + } + } fn names_options_in_network_exact( &self, name: &str, network_id: i64, maybe_filter: Option<NonEmpty<String>>, ) -> Result<Vec<DomainNameOnProtocol>, ProtocolError> { let tld = Tld::from_domain_name(name).ok_or_else(|| ProtocolError::InvalidName { name: name.to_string(), - reason: "Invalid TLD".to_string(), + reason: format!("Invalid or missing TLD in domain name: {}", name), })?; let protocols = self.protocols_of_network_for_tld(network_id, tld, maybe_filter)?; let mut results = Vec::new(); for deployed_protocol in protocols { - let empty_label_hash = match &deployed_protocol.protocol.info.protocol_specific { - ProtocolSpecific::EnsLike(ens_like) => ens_like.empty_label_hash, - ProtocolSpecific::D3Connect(d3_connect) => d3_connect.empty_label_hash, - }; + let empty_label_hash = Self::get_empty_label_hash( + &deployed_protocol.protocol.info.protocol_specific + ); let domain_name = DomainName::new(name, empty_label_hash)?; results.push(DomainNameOnProtocol { inner: domain_name, deployed_protocol, }); } Ok(results) }
410-410: Remove unnecessary clone.The
clone()operation is not needed here asmaybe_filteris only used once.- .names_options_in_network(name, network_id, maybe_filter.clone()) + .names_options_in_network(name, network_id, maybe_filter)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
blockscout-ens/bens-logic/src/protocols/protocoler.rs(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Unit, doc and integration tests
🔇 Additional comments (2)
blockscout-ens/bens-logic/src/protocols/protocoler.rs (2)
123-123: LGTM! Good addition of empty_label_hash field.The addition of
empty_label_hashtoD3ConnectProtocolbrings consistency withEnsLikeProtocoland enables unified empty label hash handling across protocols.
454-489: LGTM! Comprehensive test coverage for Tld struct.The test module provides good coverage of the Tld functionality, including edge cases like empty TLDs and dot handling.
| fn names_options_in_network_with_suggestions( | ||
| &self, | ||
| name_with_tld: &str, | ||
| network_id: i64, | ||
| maybe_filter: Option<NonEmpty<String>>, | ||
| ) -> Result<Vec<DomainNameOnProtocol>, ProtocolError> { | ||
| let protocols = self.protocols_of_network_for_tld( | ||
| network_id, | ||
| Tld::from_domain_name(name_with_tld).unwrap(), | ||
| maybe_filter, | ||
| )?; | ||
|
|
||
| let mut results = Vec::new(); | ||
| for deployed_protocol in protocols { | ||
| let empty_label_hash = match &deployed_protocol.protocol.info.protocol_specific { | ||
| ProtocolSpecific::EnsLike(ens_like) => ens_like.empty_label_hash, | ||
| ProtocolSpecific::D3Connect(d3_connect) => d3_connect.empty_label_hash, | ||
| }; | ||
|
|
||
| let domain_name = DomainName::new(name_with_tld, empty_label_hash)?; | ||
| results.push(DomainNameOnProtocol { | ||
| inner: domain_name, | ||
| deployed_protocol, | ||
| }); | ||
| } | ||
|
|
||
| Ok(results) |
There was a problem hiding this comment.
Remove unsafe unwrap and reduce code duplication.
The method has an unsafe unwrap and duplicates logic from names_options_in_network_exact.
Consider this safer implementation that reuses existing logic:
fn names_options_in_network_with_suggestions(
&self,
name_with_tld: &str,
network_id: i64,
maybe_filter: Option<NonEmpty<String>>,
) -> Result<Vec<DomainNameOnProtocol>, ProtocolError> {
- let protocols = self.protocols_of_network_for_tld(
- network_id,
- Tld::from_domain_name(name_with_tld).unwrap(),
- maybe_filter,
- )?;
-
- let mut results = Vec::new();
- for deployed_protocol in protocols {
- let empty_label_hash = match &deployed_protocol.protocol.info.protocol_specific {
- ProtocolSpecific::EnsLike(ens_like) => ens_like.empty_label_hash,
- ProtocolSpecific::D3Connect(d3_connect) => d3_connect.empty_label_hash,
- };
-
- let domain_name = DomainName::new(name_with_tld, empty_label_hash)?;
- results.push(DomainNameOnProtocol {
- inner: domain_name,
- deployed_protocol,
- });
- }
-
- Ok(results)
+ self.names_options_in_network_exact(name_with_tld, network_id, maybe_filter)
}📝 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.
| fn names_options_in_network_with_suggestions( | |
| &self, | |
| name_with_tld: &str, | |
| network_id: i64, | |
| maybe_filter: Option<NonEmpty<String>>, | |
| ) -> Result<Vec<DomainNameOnProtocol>, ProtocolError> { | |
| let protocols = self.protocols_of_network_for_tld( | |
| network_id, | |
| Tld::from_domain_name(name_with_tld).unwrap(), | |
| maybe_filter, | |
| )?; | |
| let mut results = Vec::new(); | |
| for deployed_protocol in protocols { | |
| let empty_label_hash = match &deployed_protocol.protocol.info.protocol_specific { | |
| ProtocolSpecific::EnsLike(ens_like) => ens_like.empty_label_hash, | |
| ProtocolSpecific::D3Connect(d3_connect) => d3_connect.empty_label_hash, | |
| }; | |
| let domain_name = DomainName::new(name_with_tld, empty_label_hash)?; | |
| results.push(DomainNameOnProtocol { | |
| inner: domain_name, | |
| deployed_protocol, | |
| }); | |
| } | |
| Ok(results) | |
| fn names_options_in_network_with_suggestions( | |
| &self, | |
| name_with_tld: &str, | |
| network_id: i64, | |
| maybe_filter: Option<NonEmpty<String>>, | |
| ) -> Result<Vec<DomainNameOnProtocol>, ProtocolError> { | |
| self.names_options_in_network_exact(name_with_tld, network_id, maybe_filter) | |
| } |
| ) -> Result<Vec<DomainNameOnProtocol>, ProtocolError> { | ||
| 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('.') { | ||
| let direct = self.names_options_in_network_exact(name, network_id, maybe_filter)?; | ||
| if direct.is_empty() { | ||
| Err(ProtocolError::InvalidName { | ||
| name: name.to_string(), | ||
| reason: "No matching protocols for given TLD".to_string(), | ||
| }) | ||
| } else { | ||
| Ok(direct.into_iter().take(MAX_NAMES_LIMIT).collect()) | ||
| } | ||
| } else { | ||
| let all_names_with_protocols: Vec<_> = tlds | ||
| .into_iter() | ||
| .map(|tld| format!("{}.{}", name, tld.0)) | ||
| .flat_map(|name_with_tld| { | ||
| self.names_options_in_network_with_suggestions( | ||
| &name_with_tld, | ||
| network_id, | ||
| maybe_filter.clone(), | ||
| ) | ||
| .unwrap_or_default() | ||
| }) | ||
| .take(MAX_NAMES_LIMIT) | ||
| .collect(); | ||
|
|
||
| if all_names_with_protocols.is_empty() { | ||
| Err(ProtocolError::InvalidName { | ||
| name: name.to_string(), | ||
| reason: "No valid TLDs".to_string(), | ||
| }) | ||
| } else { | ||
| Ok(all_names_with_protocols) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and code structure.
The current implementation has several issues that should be addressed:
- Silent error handling with
unwrap_or_default() - Complex nested structure
- Unnecessary
clone()operations
Consider this more functional approach with better error handling:
fn names_options_in_network(
&self,
name: &str,
network_id: i64,
maybe_filter: Option<NonEmpty<String>>,
) -> Result<Vec<DomainNameOnProtocol>, ProtocolError> {
let tlds: Vec<Tld> = self
.networks
.get(&network_id)
.ok_or_else(|| ProtocolError::NetworkNotFound(network_id))?
.use_protocols
.iter()
.flat_map(|protocol_name| {
self.protocols
.get(protocol_name)
.map(|protocol| protocol.info.tld_list.iter().cloned())
})
.flatten()
.collect();
if name.contains('.') {
self.names_options_in_network_exact(name, network_id, maybe_filter)
.and_then(|results| {
if results.is_empty() {
Err(ProtocolError::InvalidName {
name: name.to_string(),
reason: "No matching protocols for given TLD".to_string(),
})
} else {
Ok(results.into_iter().take(MAX_NAMES_LIMIT).collect())
}
})
} else {
let suggestions: Vec<_> = tlds
.into_iter()
.map(|tld| format!("{}.{}", name, tld.0))
.filter_map(|name_with_tld| {
self.names_options_in_network_with_suggestions(
&name_with_tld,
network_id,
maybe_filter.as_ref().cloned(),
)
.map_err(|e| {
log::debug!("Failed to get suggestions for {}: {}", name_with_tld, e);
e
})
.ok()
})
.flatten()
.take(MAX_NAMES_LIMIT)
.collect();
if suggestions.is_empty() {
Err(ProtocolError::InvalidName {
name: name.to_string(),
reason: "No valid TLDs".to_string(),
})
} else {
Ok(suggestions)
}
}
}📝 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.
| ) -> Result<Vec<DomainNameOnProtocol>, ProtocolError> { | |
| 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('.') { | |
| let direct = self.names_options_in_network_exact(name, network_id, maybe_filter)?; | |
| if direct.is_empty() { | |
| Err(ProtocolError::InvalidName { | |
| name: name.to_string(), | |
| reason: "No matching protocols for given TLD".to_string(), | |
| }) | |
| } else { | |
| Ok(direct.into_iter().take(MAX_NAMES_LIMIT).collect()) | |
| } | |
| } else { | |
| let all_names_with_protocols: Vec<_> = tlds | |
| .into_iter() | |
| .map(|tld| format!("{}.{}", name, tld.0)) | |
| .flat_map(|name_with_tld| { | |
| self.names_options_in_network_with_suggestions( | |
| &name_with_tld, | |
| network_id, | |
| maybe_filter.clone(), | |
| ) | |
| .unwrap_or_default() | |
| }) | |
| .take(MAX_NAMES_LIMIT) | |
| .collect(); | |
| if all_names_with_protocols.is_empty() { | |
| Err(ProtocolError::InvalidName { | |
| name: name.to_string(), | |
| reason: "No valid TLDs".to_string(), | |
| }) | |
| } else { | |
| Ok(all_names_with_protocols) | |
| } | |
| } | |
| } | |
| fn names_options_in_network( | |
| &self, | |
| name: &str, | |
| network_id: i64, | |
| maybe_filter: Option<NonEmpty<String>>, | |
| ) -> Result<Vec<DomainNameOnProtocol>, ProtocolError> { | |
| let tlds: Vec<Tld> = self | |
| .networks | |
| .get(&network_id) | |
| .ok_or_else(|| ProtocolError::NetworkNotFound(network_id))? | |
| .use_protocols | |
| .iter() | |
| .flat_map(|protocol_name| { | |
| self.protocols | |
| .get(protocol_name) | |
| .map(|protocol| protocol.info.tld_list.iter().cloned()) | |
| }) | |
| .flatten() | |
| .collect(); | |
| if name.contains('.') { | |
| self.names_options_in_network_exact(name, network_id, maybe_filter) | |
| .and_then(|results| { | |
| if results.is_empty() { | |
| Err(ProtocolError::InvalidName { | |
| name: name.to_string(), | |
| reason: "No matching protocols for given TLD".to_string(), | |
| }) | |
| } else { | |
| Ok(results.into_iter().take(MAX_NAMES_LIMIT).collect()) | |
| } | |
| }) | |
| } else { | |
| let suggestions: Vec<_> = tlds | |
| .into_iter() | |
| .map(|tld| format!("{}.{}", name, tld.0)) | |
| .filter_map(|name_with_tld| { | |
| self.names_options_in_network_with_suggestions( | |
| &name_with_tld, | |
| network_id, | |
| maybe_filter.as_ref().cloned(), | |
| ) | |
| .map_err(|e| { | |
| log::debug!("Failed to get suggestions for {}: {}", name_with_tld, e); | |
| e | |
| }) | |
| .ok() | |
| }) | |
| .flatten() | |
| .take(MAX_NAMES_LIMIT) | |
| .collect(); | |
| if suggestions.is_empty() { | |
| Err(ProtocolError::InvalidName { | |
| name: name.to_string(), | |
| reason: "No valid TLDs".to_string(), | |
| }) | |
| } else { | |
| Ok(suggestions) | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
blockscout-ens/bens-logic/src/protocols/protocoler.rs (1)
288-318: Consider making the implementation more functional.The implementation could be more concise by using functional patterns:
pub fn fetch_domain_options( &self, name_with_tld: &str, network_id: i64, maybe_filter: Option<NonEmpty<String>>, ) -> Result<Vec<DomainNameOnProtocol>, ProtocolError> { let tld = Tld::from_domain_name(name_with_tld).ok_or_else(|| ProtocolError::InvalidName { name: name_with_tld.to_string(), reason: "Invalid TLD".to_string(), })?; - let protocols = self.protocols_of_network_for_tld(network_id, tld, maybe_filter)?; - - let mut results = Vec::new(); - for deployed_protocol in protocols { - let empty_label_hash = deployed_protocol - .protocol - .info - .protocol_specific - .empty_label_hash(); - - let domain_name = DomainName::new(name_with_tld, empty_label_hash)?; - results.push(DomainNameOnProtocol { - inner: domain_name, - deployed_protocol, - }); - } - - Ok(results) + self.protocols_of_network_for_tld(network_id, tld, maybe_filter)? + .into_iter() + .map(|deployed_protocol| { + let empty_label_hash = deployed_protocol + .protocol + .info + .protocol_specific + .empty_label_hash(); + DomainName::new(name_with_tld, empty_label_hash) + .map(|domain_name| DomainNameOnProtocol { + inner: domain_name, + deployed_protocol, + }) + }) + .collect()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
blockscout-ens/bens-logic/src/protocols/protocoler.rs(7 hunks)blockscout-ens/bens-server/tests/data/domains/abcnews_eth/detailed.json(1 hunks)blockscout-ens/bens-server/tests/domains.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- blockscout-ens/bens-server/tests/domains.rs
🧰 Additional context used
🪛 Biome (1.9.4)
blockscout-ens/bens-server/tests/data/domains/abcnews_eth/detailed.json
[error] 20-20: Expected a property but instead found '{'.
Expected a property here.
(parse)
[error] 20-20: Property key must be double quoted
(parse)
[error] 20-20: unexpected character |
(parse)
[error] 20-20: expected , but instead found json_encode
Remove json_encode
(parse)
[error] 20-20: unexpected character (
(parse)
[error] 20-20: unexpected character )
(parse)
[error] 20-20: unexpected character |
(parse)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Unit, doc and integration tests
- GitHub Check: Linting
🔇 Additional comments (6)
blockscout-ens/bens-logic/src/protocols/protocoler.rs (6)
86-87: LGTM! Improved variable naming.The variable names in pattern matching are now more descriptive and aligned with the protocol types.
123-123: LGTM! Added empty_label_hash support for D3ConnectProtocol.The addition of
empty_label_hashfield brings parity with EnsLikeProtocol's functionality.
149-149: Verify the MAX_NAMES_LIMIT value.The limit of 5 seems reasonable but should be verified against:
- User experience requirements
- Performance implications
- API consumer expectations
334-334: Verify the take(1) limit.The method uses
take(1)for direct matches buttake(MAX_NAMES_LIMIT)for suggestions. This inconsistency might affect user experience.
419-454: LGTM! Comprehensive test coverage.The test module provides good coverage for the Tld struct's functionality:
- Dot trimming
- Empty TLD handling
- Reverse TLD
- Domain name parsing
351-352:⚠️ Potential issueImprove error handling.
Using
unwrap_or_default()silently ignores errors, which could hide important issues.- self.fetch_domain_options(&name_with_tld, network_id, maybe_filter.clone()) - .unwrap_or_default() + match self.fetch_domain_options(&name_with_tld, network_id, maybe_filter.as_ref().cloned()) { + Ok(options) => options, + Err(e) => { + log::debug!("Failed to fetch domain options for {}: {}", name_with_tld, e); + Vec::new() + } + }Likely invalid or redundant comment.
| ) | ||
| .unwrap_or_default() | ||
| self.fetch_domain_options(&name_with_tld, network_id, maybe_filter.clone()) | ||
| .unwrap_or_default() |
There was a problem hiding this comment.
Does it make sense to log an error or all Err(_) variants represent a normal program flow?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
blockscout-ens/bens-logic/src/protocols/protocoler.rs (1)
288-318: LGTM! Well-structured domain options fetching logic.Good separation of concerns and proper error handling.
Replace
println!statements with proper logging using thelogcrate:- println!( + log::debug!( "name {}", name );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
blockscout-ens/bens-logic/src/protocols/protocoler.rs(7 hunks)blockscout-ens/bens-logic/tests/migrations/20231019103631_push_mock_data.sql(1 hunks)blockscout-ens/bens-server/tests/domains.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- blockscout-ens/bens-server/tests/domains.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Unit, doc and integration tests
🔇 Additional comments (4)
blockscout-ens/bens-logic/src/protocols/protocoler.rs (3)
86-87: LGTM! Improved variable naming.The variable names are now more descriptive and consistent with their types.
149-149: LGTM! Good use of a named constant.Using a constant improves maintainability and makes the limit value configurable.
426-461: LGTM! Comprehensive test coverage.The test module provides good coverage of the
Tldstruct functionality, including edge cases.blockscout-ens/bens-logic/tests/migrations/20231019103631_push_mock_data.sql (1)
37-37: LGTM! Well-structured test data.The new test domain entry maintains consistency with the existing schema and includes all required fields.
| if name.contains('.') { | ||
| println!( | ||
| "name {}", name | ||
| ); | ||
| let direct = self.fetch_domain_options(name, network_id, maybe_filter)?; | ||
| if direct.is_empty() { | ||
| Err(ProtocolError::InvalidName { | ||
| name: name.to_string(), | ||
| reason: "No matching protocols for given TLD".to_string(), | ||
| }) | ||
| } else { | ||
| Ok(direct.into_iter().take(1).collect()) | ||
| } | ||
| } else { | ||
| 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)) | ||
| .flat_map(|protocol| protocol.info.tld_list.iter().cloned()) | ||
| .collect::<Vec<Tld>>(); | ||
|
|
||
| println!( | ||
| "tlds {:?}", tlds | ||
| ); | ||
|
|
||
| let all_names_with_protocols: Vec<_> = tlds | ||
| .into_iter() | ||
| .map(|tld| format!("{}.{}", name, tld.0)) | ||
| .flat_map(|name_with_tld| { | ||
| self.fetch_domain_options(&name_with_tld, network_id, maybe_filter.clone()) | ||
| .unwrap_or_default() | ||
| }) | ||
| .take(MAX_NAMES_LIMIT) | ||
| .collect(); | ||
|
|
||
| if all_names_with_protocols.is_empty() { | ||
| Err(ProtocolError::InvalidName { | ||
| name: name.to_string(), | ||
| reason: "No valid TLDs".to_string(), | ||
| }) | ||
| } else { | ||
| Ok(all_names_with_protocols) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and logging.
- Silent error handling with
unwrap_or_default()could hide important issues - Debug statements should use proper logging
Consider this improved implementation:
- println!(
+ log::debug!(
"name {}", name
);
- println!(
+ log::debug!(
"tlds {:?}", tlds
);
let all_names_with_protocols: Vec<_> = tlds
.into_iter()
.map(|tld| format!("{}.{}", name, tld.0))
.flat_map(|name_with_tld| {
- self.fetch_domain_options(&name_with_tld, network_id, maybe_filter.clone())
- .unwrap_or_default()
+ match self.fetch_domain_options(&name_with_tld, network_id, maybe_filter.clone()) {
+ Ok(options) => options,
+ Err(e) => {
+ log::debug!("Failed to fetch options for {}: {}", name_with_tld, e);
+ Vec::new()
+ }
+ }
})📝 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.
| if name.contains('.') { | |
| println!( | |
| "name {}", name | |
| ); | |
| let direct = self.fetch_domain_options(name, network_id, maybe_filter)?; | |
| if direct.is_empty() { | |
| Err(ProtocolError::InvalidName { | |
| name: name.to_string(), | |
| reason: "No matching protocols for given TLD".to_string(), | |
| }) | |
| } else { | |
| Ok(direct.into_iter().take(1).collect()) | |
| } | |
| } else { | |
| 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)) | |
| .flat_map(|protocol| protocol.info.tld_list.iter().cloned()) | |
| .collect::<Vec<Tld>>(); | |
| println!( | |
| "tlds {:?}", tlds | |
| ); | |
| let all_names_with_protocols: Vec<_> = tlds | |
| .into_iter() | |
| .map(|tld| format!("{}.{}", name, tld.0)) | |
| .flat_map(|name_with_tld| { | |
| self.fetch_domain_options(&name_with_tld, network_id, maybe_filter.clone()) | |
| .unwrap_or_default() | |
| }) | |
| .take(MAX_NAMES_LIMIT) | |
| .collect(); | |
| if all_names_with_protocols.is_empty() { | |
| Err(ProtocolError::InvalidName { | |
| name: name.to_string(), | |
| reason: "No valid TLDs".to_string(), | |
| }) | |
| } else { | |
| Ok(all_names_with_protocols) | |
| } | |
| } | |
| if name.contains('.') { | |
| - println!( | |
| - "name {}", name | |
| - ); | |
| + log::debug!( | |
| + "name {}", name | |
| + ); | |
| let direct = self.fetch_domain_options(name, network_id, maybe_filter)?; | |
| if direct.is_empty() { | |
| Err(ProtocolError::InvalidName { | |
| name: name.to_string(), | |
| reason: "No matching protocols for given TLD".to_string(), | |
| }) | |
| } else { | |
| Ok(direct.into_iter().take(1).collect()) | |
| } | |
| } else { | |
| 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)) | |
| .flat_map(|protocol| protocol.info.tld_list.iter().cloned()) | |
| .collect::<Vec<Tld>>(); | |
| - println!( | |
| - "tlds {:?}", tlds | |
| - ); | |
| + log::debug!( | |
| + "tlds {:?}", tlds | |
| + ); | |
| let all_names_with_protocols: Vec<_> = tlds | |
| .into_iter() | |
| .map(|tld| format!("{}.{}", name, tld.0)) | |
| .flat_map(|name_with_tld| { | |
| - self.fetch_domain_options(&name_with_tld, network_id, maybe_filter.clone()) | |
| - .unwrap_or_default() | |
| + match self.fetch_domain_options(&name_with_tld, network_id, maybe_filter.clone()) { | |
| + Ok(options) => options, | |
| + Err(e) => { | |
| + log::debug!("Failed to fetch options for {}: {}", name_with_tld, e); | |
| + Vec::new() | |
| + } | |
| + } | |
| }) | |
| .take(MAX_NAMES_LIMIT) | |
| .collect(); | |
| if all_names_with_protocols.is_empty() { | |
| Err(ProtocolError::InvalidName { | |
| name: name.to_string(), | |
| reason: "No valid TLDs".to_string(), | |
| }) | |
| } else { | |
| Ok(all_names_with_protocols) | |
| } | |
| } |
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
Release Notes
New Features
Bug Fixes
Documentation
Chores