fix(sdk)!: create channel error due to empty address #2317
Conversation
WalkthroughThe changes in this pull request primarily focus on enhancing the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
shumkov
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (10)
packages/rs-sdk/tests/fetch/evonode.rs (2)
65-69: Add test coverage for invalid address creationWhile this test covers the connection refusal scenario, given the PR's focus on preventing invalid
Addresscreation, consider adding explicit test cases for:
- Invalid URI to Address conversion scenarios
- Empty address cases that were causing issues on Android
Example addition:
#[test] fn test_invalid_address_creation() { // Test empty host let result = Address::try_from(Uri::from_static("http:///path")); assert!(result.is_err()); // Test missing scheme let result = "localhost:3000".parse::<Address>(); assert!(result.is_err()); }
Based on the code inspection, I can now generate the final response:
Add unit tests for breaking type conversion changes
The code shows the implementation of the breaking changes but lacks dedicated unit tests. Please add tests for:
TryFrom<Uri>forAddressto verify:
- Success case with valid URI containing host
- Error case with URI missing host
FromStrforAddressListto verify:
- Success case with comma-separated valid addresses
- Error case with invalid URI strings
FromIterator<Address>forAddressListto verify:
- Collection creation from iterator of valid addresses
These tests should be added in a new
mod testsblock inpackages/rs-dapi-client/src/address_list.rs.🔗 Analysis chain
Line range hint
14-72: Enhance test coverage for breaking changesThe PR introduces several breaking changes in type conversions that should be explicitly tested:
UritoAddress:From→TryFrom&strtoAddressList:From→FromStrUritoAddressList:FromIterator<Uri>→FromIterator<Address>Let's check if these conversions are tested elsewhere:
Consider adding comprehensive test coverage for these breaking changes if not already covered elsewhere.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing tests of the breaking changes rg -t rust "test.*(?:try_)?from.*uri|test.*address.*list.*from|test.*from.*str" ./packages/rs-dapi-client/src/Length of output: 112
Script:
#!/bin/bash # Let's search for test files and type conversion tests with a broader pattern rg -t rust "(?m)^#\[test\].*\n.*(?:try_)?from|test.*address.*list|test.*uri.*address" --context 2Length of output: 291
Script:
#!/bin/bash # Let's also check for the actual type conversion implementations ast-grep --pattern 'impl $_ for Address { $$$ }'Length of output: 3255
Script:
#!/bin/bash # And check for AddressList implementations ast-grep --pattern 'impl $_ for AddressList { $$$ }'Length of output: 2602
Script:
#!/bin/bash # Let's also check all test files in the repository fd "test.*\.rs$"Length of output: 2278
Script:
#!/bin/bash # Let's check for existing tests in address_list.rs rg -U "(?s)#\[test\].*?\}" packages/rs-dapi-client/src/address_list.rsLength of output: 72
Script:
#!/bin/bash # Let's also check the test file where the review comment was made rg -U "(?s)#\[test\].*?\}" packages/rs-sdk/tests/fetch/evonode.rsLength of output: 67
Script:
#!/bin/bash # Let's try a simpler approach to find tests rg "#\[test\]" packages/rs-dapi-client/src/address_list.rs -A 1Length of output: 65
Script:
#!/bin/bash # Let's also check if there's a dedicated test module rg "mod tests" packages/rs-dapi-client/src/address_list.rs -A 2Length of output: 65
Script:
#!/bin/bash # And check the actual content of address_list.rs cat packages/rs-dapi-client/src/address_list.rsLength of output: 6806
packages/rs-sdk/examples/read_contract.rs (2)
83-85: Consider improving error handling in this exampleWhile the implementation correctly uses the new
Addresstype, usingexpect()in an example might not demonstrate best practices for error handling.Consider using proper error handling to make the example more educational:
- let address = Address::from_str(&format!( - "https://{}:{}", - config.server_address, config.platform_port - )) - .expect("parse uri"); + let address = Address::from_str(&format!( + "https://{}:{}", + config.server_address, config.platform_port + )) + .map_err(|e| format!("Failed to parse address: {}", e))?;
83-90: Add documentation about Address usageSince this is an example file and the PR introduces breaking changes in how addresses are handled, it would be helpful to add comments explaining the new
Addresstype usage and why it's preferred over the previous URI approach.Add a comment block before the address creation:
+ // Create a new Address instance using HTTPS scheme + // Note: We use Address instead of URI for better validation and type safety + // The Address type ensures that only valid addresses are created let address = Address::from_str(&format!(packages/rs-sdk/src/platform/types/evonode.rs (1)
Line range hint
82-86: Consider enhancing error handling specificity.Given the PR's focus on improving error handling (changing from failed precondition to invalid argument errors), consider making the error handling in
execute_transportmore specific. Instead of using a generic error conversion withmap_err(TransportError::Grpc), you could map specific gRPC errors to more descriptive transport errors.Here's a suggested improvement:
let response = client .get_status(grpc_request) - .map_err(TransportError::Grpc) + .map_err(|e| match e.code() { + tonic::Code::InvalidArgument => TransportError::InvalidAddress(e.message().to_string()), + _ => TransportError::Grpc(e), + }) .map_ok(|response| response.into_inner()) .await;packages/rs-sdk/tests/fetch/config.rs (1)
134-136: Consider more robust error handling for address parsing.While the implementation correctly uses the new
Addresstype and parsing mechanism, the use ofexpect()could make debugging harder if the configuration is invalid. Consider either:
- Propagating the error to provide more context about the invalid configuration, or
- Adding a more descriptive expect message that includes the actual address being parsed
Example improvement:
- let address: Address = format!("{}://{}:{}", scheme, self.platform_host, self.platform_port).parse().expect("valid address"); + let addr_str = format!("{}://{}:{}", scheme, self.platform_host, self.platform_port); + let address: Address = addr_str.parse().expect(&format!("Invalid address: {}", addr_str));packages/rs-dapi-client/src/address_list.rs (3)
28-29: Consider preserving original error types for better error handlingConverting the error into a
Stringwithe.to_string()may lose valuable error information. Consider preserving the original error type to retain detailed context for debugging purposes.You could modify the
InvalidAddressUrivariant to include the original error:- InvalidAddressUri(String), + InvalidAddressUri(http::uri::InvalidUri),And adjust the error mapping accordingly:
.map_err(|e| AddressListError::InvalidAddressUri(e))
55-57: Include URI in error message for better debuggingIncluding the problematic URI in the error message can aid in diagnosing issues by providing more context.
Apply this change:
if value.host().is_none() { return Err(AddressListError::InvalidAddressUri( - "uri must contain host".to_string() + format!("URI '{}' must contain host", value) )); }
228-232: Rename variableuritoaddressfor clarityIn the
FromIterator<Address>implementation, the variableurirepresents anAddress. Renaming it toaddresswould improve code readability and reduce confusion.Apply this diff:
fn from_iter<T: IntoIterator<Item = Address>>(iter: T) -> Self { let mut address_list = Self::new(); - for uri in iter { - address_list.add(uri); + for address in iter { + address_list.add(address); } address_list }packages/rs-sdk/src/sync.rs (1)
200-200: Fix typographical error in commentThere's a misspelling in the comment: "preceeding" should be "preceding".
Apply this diff to correct the typo:
- // requests sent in all preceeding attempts; user expects `settings.retries +1` + // requests sent in all preceding attempts; user expects `settings.retries +1`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
packages/rs-dapi-client/src/address_list.rs(5 hunks)packages/rs-dapi-client/src/transport/grpc.rs(1 hunks)packages/rs-sdk/examples/read_contract.rs(2 hunks)packages/rs-sdk/src/platform/types/evonode.rs(1 hunks)packages/rs-sdk/src/sync.rs(2 hunks)packages/rs-sdk/tests/fetch/config.rs(2 hunks)packages/rs-sdk/tests/fetch/evonode.rs(2 hunks)
🔇 Additional comments (6)
packages/rs-sdk/tests/fetch/evonode.rs (2)
11-11: LGTM: Address import aligns with PR objectives
The addition of rs_dapi_client::Address import supports the transition from Uri to Address type as outlined in the PR objectives.
Line range hint 14-61: LGTM: Robust test implementation with proper error handling
The test case properly implements the new Address type while maintaining comprehensive error handling, timeouts, and assertions. The logging is informative and will help with debugging.
packages/rs-sdk/examples/read_contract.rs (1)
6-6: LGTM: Import statement updated correctly
The addition of Address to the import statement aligns with the PR's objective to handle addresses more safely.
packages/rs-sdk/tests/fetch/config.rs (1)
11-11: LGTM: Import aligns with new Address type usage.
The addition of the Address import is consistent with the PR's objective to prevent invalid address instances.
packages/rs-dapi-client/src/address_list.rs (1)
104-104: Ensure error messages provide sufficient context
By changing InvalidAddressUri to accept a String, ensure that error messages contain enough detail to help debug issues, especially if original error types are not preserved.
Consider reviewing all error messages to confirm they provide adequate information.
packages/rs-sdk/src/sync.rs (1)
344-344: LGTM!
The code change in the test function looks good.
| /// let address = "http://127.0.0.1:1".parse().expect("valid address"); | ||
| /// let node = EvoNode::new(address); |
There was a problem hiding this comment.
💡 Codebase verification
Example code should use try_from instead of expect for proper error handling
The verification reveals that the example code uses expect() which can cause runtime panics, while the codebase has proper error handling patterns using TryFrom trait. The example should be updated to demonstrate proper error handling:
- Current example:
let address = "http://127.0.0.1:1".parse().expect("valid address");- Should be changed to:
let address = Address::try_from("http://127.0.0.1:1".parse()?)?;This aligns with:
- The codebase's error handling patterns using
TryFromtrait for Address parsing - The PR's objective of fixing SDK issues related to invalid addresses
- Best practices of propagating errors rather than panicking
🔗 Analysis chain
Example code correctly demonstrates the new Address parsing approach.
The updated example aligns with the breaking changes, showing the transition from Uri to direct Address parsing. The error handling with expect() provides clear feedback for invalid addresses.
Let's verify the error handling for invalid addresses:
Address parsing error handling needs improvement across the codebase
The verification reveals several instances where expect() is used with address parsing, which can lead to runtime panics:
packages/rs-sdk/src/sync.rs: Usingexpect()in address parsingpackages/rs-sdk/tests/fetch/config.rs: Usingexpect()in address formatting and parsingpackages/rs-sdk/src/mock/sdk.rs: Usingexpect()in mock address creationpackages/rs-dapi-client/src/executor.rs: Usingexpect()in mock address creation
Let's verify if there are better error handling patterns available:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that invalid addresses are properly handled across the codebase
# Test: Search for error handling patterns around address parsing
rg -A 3 'parse\(\).*expect\('
# Test: Look for other address validation patterns
ast-grep --pattern 'TryFrom<Uri> for Address {
$$$
fn try_from($_: Uri) -> Result<Self, Self::Error> {
$$$
}
}'
Length of output: 1983
Script:
#!/bin/bash
# Check for proper error handling patterns using Result or Option
rg -A 3 'parse\(\)\?|try_from|TryFrom'
# Look for Address struct implementation
ast-grep --pattern 'impl Address {
$$$
}'
Length of output: 441675
| Err(e) => Err(dapi_grpc::tonic::Status::invalid_argument(format!( | ||
| "channel creation failed: {}", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve consistency in error handling across methods.
The change from failed_precondition to invalid_argument is appropriate for invalid address scenarios. However, there are some inconsistencies to address:
-
The same error scenario in other methods still uses
failed_precondition:PlatformGrpcClient::with_uri_and_settingsCoreGrpcClient::with_uriCoreGrpcClient::with_uri_and_settings
-
The error message format differs from other occurrences ("channel creation failed" vs "Channel creation failed").
Apply these changes to maintain consistency:
- Err(e) => Err(dapi_grpc::tonic::Status::invalid_argument(format!(
- "channel creation failed: {}",
+ Err(e) => Err(dapi_grpc::tonic::Status::invalid_argument(format!(
+ "Channel creation failed: {}",
e
))),Also update the error type in other methods:
- Err(dapi_grpc::tonic::Status::failed_precondition(
+ Err(dapi_grpc::tonic::Status::invalid_argument(Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/rs-dapi-client/src/address_list.rs (1)
223-232: Consider handling whitespace in address parsingThe implementation is correct, but consider trimming whitespace for better usability:
fn from_str(s: &str) -> Result<Self, Self::Err> { let uri_list: Vec<Address> = s .split(',') + .map(str::trim) + .filter(|s| !s.is_empty()) .map(Address::from_str) .collect::<Result<_, _>>()?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/rs-dapi-client/src/address_list.rs(6 hunks)
🔇 Additional comments (4)
packages/rs-dapi-client/src/address_list.rs (4)
28-29: LGTM: Improved error handling in FromStr implementation
The change improves error handling by providing more descriptive error messages and properly chaining the conversions through try_from.
104-104: LGTM: Simplified error variant
The change to use String for InvalidAddressUri makes the error handling more flexible and readable.
236-243: LGTM: Safe FromIterator implementation
The change from FromIterator<Uri> to FromIterator<Address> properly aligns with the new validation approach, ensuring all addresses are valid.
51-63: LGTM: Essential validation in TryFrom implementation
The change from From to TryFrom with host validation is crucial for preventing invalid Address instances. This directly addresses the root cause of the channel creation errors.
Let's verify that there are no direct From<Uri> usages that need to be updated:
✅ Verification successful
LGTM: No direct From usages found
The codebase only contains a TryFrom<Uri> implementation for Address, with no direct From<Uri> usages. The change from From to TryFrom is safe since there are no direct .into() calls or From trait implementations that need to be updated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining From<Uri> implementations or direct .into() calls
rg -l "impl\s+From<Uri>"
rg "\.into\(\)\s*//.*Uri.*Address"
Length of output: 63
Script:
#!/bin/bash
# Let's search more broadly for potential .into() calls and Uri to Address conversions
rg -A 2 "Uri.*into.*Address|Address.*from.*Uri"
# Also search for any direct Uri to Address conversions in tests
rg -A 2 "Uri.*Address" "**/*test*.rs"
# Search for any From trait usage with Uri
ast-grep --pattern 'impl From<$_> for $_'
Length of output: 486521
Script:
#!/bin/bash
# Search for any direct Uri to Address conversions or .into() calls
rg -A 2 "Uri.*into.*Address|Address.*from.*Uri|impl From<Uri> for Address"
# Search for any Uri type usage in the codebase
rg -A 2 "Uri.*Address"
Length of output: 537
| #[deprecated] | ||
| // TODO: Remove in favor of add | ||
| /// Add a node [Address] to [AddressList] by [Uri]. | ||
| /// Returns false if the address is already in the list. | ||
| pub fn add_uri(&mut self, uri: Uri) -> bool { | ||
| self.addresses.insert(uri.into()) | ||
| self.addresses.insert(uri.try_into().expect("valid uri")) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider safer implementation for deprecated method
While it's good that this method is deprecated, it currently uses expect() which could cause panics. Consider implementing a safer version that returns a Result while the method is still available:
- pub fn add_uri(&mut self, uri: Uri) -> bool {
- self.addresses.insert(uri.try_into().expect("valid uri"))
+ pub fn add_uri(&mut self, uri: Uri) -> Result<bool, AddressListError> {
+ let address = Address::try_from(uri)?;
+ Ok(self.addresses.insert(address))Committable suggestion skipped: line range outside the PR's diff.
Issue being fixed or feature implemented
New SDK 1.5.1 fails on android with:
What was done?
AddressTransportClient::new_with_uriAddressList::add_uriis deprecatedHow Has This Been Tested?
Running tests
Breaking Changes
From<Uri> for Addressreplaced withTryFrom<Uri> for AddressFrom<&str> for AddressListreplaced withFromStr for AddressListFromIterator<Uri> for AddressListreplaced withFromIterator<Address> for AddressListChecklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Release Notes
New Features
Addresstype for improved type safety.Bug Fixes
Documentation
Addresstype usage.Tests