Do every API call async #574
Conversation
…oprnet/ct-research into jean/various-code-improvements
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThe changes in this pull request involve updates to configuration files, core components, and test suites. Key modifications include adjustments to channel management parameters in 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 (
|
![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 (1)
ct-app/tests_endurance/test_fund_channels.py (1)
48-51: Simplify the channel lookup logicThe nested list comprehension makes the code harder to read. Consider simplifying this for better maintainability.
- channel = channels[ - [ - c.id for c in channels if c.source_peer_id == self.address.hopr - ].index(id) - ] + channel = next( + (c for c in channels if c.id == id and c.source_peer_id == self.address.hopr), + None + ) + if channel is None: + raise ValueError(f"Channel {id} not found")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
ct-app/tests_endurance/test_fund_channels.py(2 hunks)ct-app/tests_endurance/test_get_channels.py(1 hunks)ct-app/tests_endurance/test_send_messages.py(3 hunks)
🔇 Additional comments (6)
ct-app/tests_endurance/test_get_channels.py (1)
14-15: Verify the API method signature change across the codebase.
The changes reflect a breaking change in how addresses are handled:
- Removal of "hopr" parameter from
get_address() - Change in return type from direct value to an object with
.hoprproperty
Let's verify the API implementation and its usage:
✅ Verification successful
API method signature change verified and consistent across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the API method signature change and its usage
# Check the API implementation
echo "Checking API implementation:"
ast-grep --pattern 'async def get_address($$$)'
# Check all usages of get_address
echo "\nChecking all usages:"
rg -A 2 'get_address\('
Length of output: 1305
ct-app/tests_endurance/test_fund_channels.py (2)
24-26: LGTM! Channel filtering updated correctly
The channel filtering logic has been properly updated to use self.address.hopr for source peer comparison while maintaining the same clear and efficient list comprehension structure.
Line range hint 1-85: Well-structured endurance test implementation
The test follows good practices with:
- Clear separation of setup, execution, and cleanup
- Proper error handling and timeout mechanisms
- Comprehensive metrics collection
ct-app/tests_endurance/test_send_messages.py (3)
42-42: LGTM! Logging changes are consistent with the new object structures
The updated logging statements correctly reflect the new address and channel object structures while maintaining good visibility into the test setup.
Also applies to: 44-44
Line range hint 1-124: Well-structured test with comprehensive metrics
The endurance test is well-designed with:
- Clear setup, execution, and validation phases
- Comprehensive metrics collection
- Appropriate success criteria (90% success rate)
- Good instrumentation to measure the performance improvements mentioned in the PR description
The test structure will effectively validate the async API changes and their performance impact.
18-18: Verify address object structure changes across the codebase
The changes in address handling (removal of "hopr" parameter and addition of .hopr property) suggest a structural change in the API. Let's verify the consistency of these changes.
Also applies to: 24-24
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
ct-app/run.sh (1)
29-29: Consider implementing log rotation.With the increased message processing rate (100 msgs/sec) and continuous logging, the log file could grow significantly. Consider these improvements:
- Add timestamp to log filename
- Implement log rotation
-python -m core --configfile ./.configs/core_${env}_config.yaml 2>&1 | tee logs_core.log +LOG_FILE="logs_core_$(date +%Y%m%d_%H%M%S).log" +python -m core --configfile ./.configs/core_${env}_config.yaml 2>&1 | tee "$LOG_FILE" | rotatelogs -n 5 "$LOG_FILE" 100Mct-app/core/components/api_types.py (3)
6-24: Improve type hints and error handling in _convert functionThe function could benefit from these improvements:
- Add return type hint
- Use
contextlib.suppressfor more idiomatic Python error handling-def _convert(value: Any): +from contextlib import suppress + +def _convert(value: Any) -> Any: if value is None: return None - try: + with suppress(ValueError): value = float(value) - except ValueError: - pass - try: + with suppress(ValueError): integer = int(value) if integer == value: value = integer - except ValueError: - pass return value🧰 Tools
🪛 Ruff
10-13: Use
contextlib.suppress(ValueError)instead oftry-except-passReplace with
contextlib.suppress(ValueError)(SIM105)
26-41: Add type hints and remove redundant None defaultThe base class implementation looks good but could be improved with better typing:
+from typing import Dict, Any, ClassVar class ApiObject: + keys: ClassVar[Dict[str, str]] = {} + - def __init__(self, data: dict): + def __init__(self, data: Dict[str, Any]): for key, value in self.keys.items(): - setattr(self, key, _convert(data.get(value, None))) + setattr(self, key, _convert(data.get(value)))🧰 Tools
🪛 Ruff
29-29: Use
data.get(value)instead ofdata.get(value, None)Replace
data.get(value, None)withdata.get(value)(SIM910)
82-83: Document the magic number in TicketPrice conversionThe division by 1e18 appears to be converting from Wei to Ether, but this should be documented.
def post_init(self): + # Convert from Wei (10^18 units) to Ether self.value = float(self.value) / 1e18
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
ct-app/core/components/api_types.py(1 hunks)ct-app/core/core.py(5 hunks)ct-app/run.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ct-app/core/core.py
🧰 Additional context used
🪛 Ruff
ct-app/core/components/api_types.py
10-13: Use contextlib.suppress(ValueError) instead of try-except-pass
Replace with contextlib.suppress(ValueError)
(SIM105)
29-29: Use data.get(value) instead of data.get(value, None)
Replace data.get(value, None) with data.get(value)
(SIM910)
🔇 Additional comments (2)
ct-app/run.sh (1)
29-29: LGTM! Enhanced logging will help monitor async operations.
The addition of logging to both console and file will be valuable for debugging the async API calls and tracking the performance improvements.
ct-app/core/components/api_types.py (1)
90-94:
Implement missing channel filtering logic
The incoming and outgoing lists are initialized but never populated. They should be filtered based on source and destination addresses.
Additionally, as noted in the previous review, this class should inherit from ApiObject for consistency. Here's the complete fix:
-class Channels:
+class Channels(ApiObject):
+ keys = {"all": "all"}
+
def __init__(self, data: dict):
- self.all = [Channel(channel) for channel in data.get("all", [])]
- self.incoming = []
- self.outgoing = []
+ super().__init__(data)
+
+ def post_init(self):
+ self.all = [Channel(channel) for channel in self.all or []]
+ self.incoming = [c for c in self.all if c.destination_address == self.our_address]
+ self.outgoing = [c for c in self.all if c.source_address == self.our_address]However, this implementation requires access to self.our_address. Please clarify how the node's own address should be obtained.
#!/bin/bash
# Search for how our_address is used in other classes
ast-grep --pattern 'class $_ {
$$$
our_address
$$$
}'
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
ct-app/test/conftest.py (3)
47-47: Consider adding value validation for balance ranges.While the transition to
Balancestype is good, consider if the random ranges (1-10) are representative of real-world scenarios. You might want to add constants for these ranges or make them configurable.
Line range hint
145-161: Consider optimizing the peers mock response construction.The list comprehension creating ConnectedPeer objects could be simplified and made more efficient:
- return_value=[ - ConnectedPeer(peer) for peer in peers_raw[:idx] + peers_raw[idx + 1 :] - ], + return_value=[ConnectedPeer(peer) for peer in peers_raw if peers_raw.index(peer) != idx],
Line range hint
172-200: Consider improving channel mock data generation.A few suggestions to enhance the channels fixture:
- Extract the balance calculation
1*1e18to a named constant- Consider adding variety to channel statuses beyond just "Open"
- Consider parameterizing the balance ranges
+MOCK_CHANNEL_BALANCE = 1e18 +CHANNEL_STATUSES = ["Open", "Pending", "Closing"] def channels(peers: set[Peer]) -> Channels: all_channels = list[Channel]() index = 0 + + def get_mock_balance() -> str: + return f"{MOCK_CHANNEL_BALANCE:.0f}" for src in peers: for dest in peers: if src.address == dest.address: continue all_channels.append( Channel( { - "balance": f"{1*1e18:.0f}", + "balance": get_mock_balance(), "id": f"channel_{index}", "destinationAddress": dest.address.address, "destinationPeerId": dest.address.id, "sourceAddress": src.address.address, "sourcePeerId": src.address.id, - "status": "Open", + "status": choice(CHANNEL_STATUSES), } ) )ct-app/core/components/hoprd_api.py (3)
33-35: Add input validation for URL and token.Consider validating the input parameters to ensure they meet the expected format:
- URL should be a valid HTTP/HTTPS URL
- Token should not be empty
def __init__(self, url: str, token: str): + if not url or not url.startswith(('http://', 'https://')): + raise ValueError("URL must be a valid HTTP/HTTPS URL") + if not token: + raise ValueError("Token cannot be empty") self.host = url self.headers = {"Authorization": f"Bearer {token}"} self.prefix = "/api/v3/"
57-61: Enhance error handling with more specific exception types.The current error handling could be more specific to handle different types of network-related errors:
- except OSError as e: - self.error(f"OSError calling {method.value} {endpoint}: {e}") - - except Exception as e: - self.error(f"Exception calling {method.value} {endpoint}. error is: {e}") + except aiohttp.ClientConnectorError as e: + self.error(f"Connection error calling {method.value} {endpoint}: {e}") + except aiohttp.ServerDisconnectedError as e: + self.error(f"Server disconnected while calling {method.value} {endpoint}: {e}") + except aiohttp.ClientError as e: + self.error(f"Client error calling {method.value} {endpoint}: {e}") + except Exception as e: + self.error(f"Unexpected error calling {method.value} {endpoint}: {e}")
137-138: Use urllib.parse for query parameter construction.Instead of manually constructing query parameters, use
urllib.parse.urlencodefor better safety and maintainability:+from urllib.parse import urlencode - params_str = "&".join([f"{k}={v}" for k, v in params.items()]) + params_str = urlencode(params)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
ct-app/.configs/core_prod_config.yaml(2 hunks)ct-app/.configs/core_staging_config.yaml(2 hunks)ct-app/core/components/hoprd_api.py(5 hunks)ct-app/core/core.py(5 hunks)ct-app/core/node.py(9 hunks)ct-app/test/conftest.py(9 hunks)helm/values-prod.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- ct-app/.configs/core_prod_config.yaml
- ct-app/.configs/core_staging_config.yaml
- ct-app/core/core.py
- ct-app/core/node.py
- helm/values-prod.yaml
🧰 Additional context used
🪛 Ruff
ct-app/core/components/hoprd_api.py
45-45: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
49-54: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
66-66: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
🔇 Additional comments (6)
ct-app/test/conftest.py (4)
7-14: LGTM: Import changes align with the new async API structure.
The new imports from core.components.api_types properly support the transition to async API calls with strongly-typed responses.
108-108: LGTM: Peer object creation updated correctly.
The peer object creation is properly updated to use the new camelCase keys.
231-240: LGTM: Node fixture properly updated for async API.
The fixture correctly uses the new types and properly mocks the API responses.
93-101: Verify API response format matches the new camelCase keys.
The keys have been updated to camelCase format. Let's verify this matches the actual API response structure.
✅ Verification successful
Let me gather more context about the API response format by checking the actual API handler code.
Let me check the actual API response handling code to see how the peer data is processed.
Let me check one final thing to see how the peer data is transformed between API and internal representation.
API response format matches the camelCase keys in test fixtures
The camelCase keys in the test fixtures (peerId, peerAddress, reportedVersion) are correct as they match the API response format defined in ConnectedPeer class in ct-app/core/components/api_types.py. This class explicitly maps the snake_case internal representation to camelCase API format:
keys = {"address": "peerAddress", "peer_id": "peerId", "version": "reportedVersion"}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for API response handling code to verify key format
rg -t py "peerId|peerAddress|reportedVersion" --no-filename
Length of output: 1812
Script:
#!/bin/bash
# Search for API response handling code and route definitions
rg -t py "class.*API|@route|@app.route" -A 5
# Also search for peer-related API endpoints specifically
rg -t py "'/peers|/peer" -A 5
Length of output: 1669
Script:
#!/bin/bash
# Look for peer response handling and data transformation code
ast-grep --pattern 'response.json()' -A 5
# Also check for any peer data structure definitions
rg -t py "class Peer" -A 10
Length of output: 1381
Script:
#!/bin/bash
# Search for code that maps between API response and Peer objects
rg -t py "keys = \{.*peerId" -B 5 -A 5
Length of output: 700
ct-app/core/components/hoprd_api.py (2)
2-15: LGTM! Well-organized imports with proper type definitions.
The imports are properly structured with clear separation between standard library, third-party, and local imports. The comprehensive set of type definitions from api_types indicates good type safety practices.
21-25: LGTM! Good use of enum for HTTP methods.
Using an enum for HTTP methods is a great practice as it provides type safety, prevents typos, and makes refactoring easier. The implementation is clean and follows Python best practices.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
ct-app/core/components/hoprd_api.py (3)
19-19: Document the MESSAGE_TAG constantAdd a docstring explaining the purpose and significance of this magic number (0x1245).
33-35: Consider making the API prefix configurableThe API prefix is hardcoded. Consider making it a constructor parameter with a default value for better flexibility and maintainability.
- def __init__(self, url: str, token: str): + def __init__(self, url: str, token: str, prefix: str = "/api/v3/"): self.host = url self.headers = {"Authorization": f"Bearer {token}"} - self.prefix = "/api/v3/" + self.prefix = prefix
78-86: Update docstring return type and improve error handlingThe docstring return type doesn't match the actual Optional[Balances] return type. Also, consider logging the error response.
async def balances(self) -> Optional[Balances]: """ Returns the balance of the node. - :return: balances: Balances | undefined + :return: Optional[Balances] - None if the request fails """ is_ok, response = await self.__call_api(Method.GET, "account/balances") + if not is_ok: + self.error(f"Failed to fetch balances: {response}") return Balances(response) if is_ok else None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ct-app/core/components/hoprd_api.py(5 hunks)
🧰 Additional context used
🪛 Ruff
ct-app/core/components/hoprd_api.py
45-45: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
49-54: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
66-66: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
🔇 Additional comments (1)
ct-app/core/components/hoprd_api.py (1)
42-56: Fix mutable default argument and optimize context managers
The existing review comments about mutable default arguments and nested with statements are still valid.
🧰 Tools
🪛 Ruff
45-45: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
49-54: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
ct-app/core/components/hoprd_api.py (2)
33-35: Consider making the API prefix configurable.The API prefix is hardcoded to "/api/v3/". Consider making it configurable through the constructor to support different API versions or testing environments.
- def __init__(self, url: str, token: str): - self.host = url - self.headers = {"Authorization": f"Bearer {token}"} - self.prefix = "/api/v3/" + def __init__(self, url: str, token: str, prefix: str = "/api/v3/"): + self.host = url + self.headers = {"Authorization": f"Bearer {token}"} + self.prefix = prefix
149-155: Update docstring to match current implementation.The docstring parameters don't match the current method signature:
""" Returns a list of peers. - :param: param: list or str = "peerId" - :param: status: str = "connected" :param: quality: int = 0..1 :return: peers: list """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ct-app/core/components/hoprd_api.py(5 hunks)
🧰 Additional context used
🪛 Ruff
ct-app/core/components/hoprd_api.py
45-45: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
49-54: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
66-66: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
🔇 Additional comments (4)
ct-app/core/components/hoprd_api.py (4)
2-15: LGTM! Clean implementation of Method enum and imports.
The introduction of the Method enum and structured imports improves type safety and code organization.
Also applies to: 21-25
43-77: LGTM! Solid async implementation.
The async implementation is well-structured with proper error handling and timeout management. The use of asyncio.wait_for ensures that API calls don't hang indefinitely.
🧰 Tools
🪛 Ruff
45-45: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
49-54: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
66-66: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
205-210: 🛠️ Refactor suggestion
Improve error handling in messages_pop_all.
The method should validate the response structure before accessing 'messages':
data = {} if tag is None else {"tag": tag}
is_ok, response = await self.__call_api(
Method.POST, "messages/pop-all", data=data
)
- return response.get("messages", []) if is_ok else []
+ if not is_ok:
+ return []
+ if not isinstance(response, dict):
+ self.error(f"Unexpected response type: {type(response)}")
+ return []
+ return response.get("messages", [])Likely invalid or redundant comment.
57-62: 🛠️ Refactor suggestion
Enhance error handling with specific exception types.
The current error handling could be more specific to help with debugging and recovery:
except OSError as e:
- self.error(f"OSError calling {method.value} {endpoint}: {e}")
+ self.error(f"Network error calling {method.value} {endpoint}: {e}", exc_info=True)
+ return (False, None)
+ except aiohttp.ClientError as e:
+ self.error(f"HTTP client error calling {method.value} {endpoint}: {e}", exc_info=True)
+ return (False, None)
+ except aiohttp.ContentTypeError as e:
+ self.error(f"Invalid JSON response from {method.value} {endpoint}: {e}", exc_info=True)
+ return (False, None)
except Exception as e:
- self.error(f"Exception calling {method.value} {endpoint}. error is: {e}")
+ self.error(f"Unexpected error calling {method.value} {endpoint}: {e}", exc_info=True)
+ return (False, None)
- return (False, None)Likely invalid or redundant comment.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Results
This PR improves message sending rate from ~10 messages/s/node to ~100 messages/s/node.