Merge back changes from v2.2.3 #6970
Conversation
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: Tibor Csóka <csokat@gmail.com> Co-authored-by: Lukas <lukas.pohanka@inina.net>
…cuous strategy defaults (#6895)
📝 WalkthroughWalkthroughThis pull request introduces a broad set of modifications across workflow configurations, Rust source files, database migrations, and dependency declarations. Changes include repositioning flags in nix build commands, refining conditional checks in GitHub Actions workflows, updating Cargo.toml versions and dependencies, reorganizing import orders, improving method documentation and error handling in channel and ticket management, enhancing network, transport, and P2P operations with asynchronous path selection and latency checks, and adjusting strategy defaults and configuration parameters. Additionally, new migration modules and helper functions have been added while obsolete or redundant code is removed. Changes
Sequence Diagram(s)sequenceDiagram
participant T as HoprTransport
participant P as Peer
participant NG as ChannelGraph
T->>T: Check network access using either::Either
T->>P: Send ping message
P-->>T: Return ping response
T->>NG: Update node score based on ping result
Possibly related PRs
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/build.yaml (1)
92-93: Addition of Checkout Step
Introducing the repository checkout step usingactions/checkout@v4is a good practice to ensure subsequent steps have access to the repository content. For enhanced stability and security, consider pinning this action to a specific commit hash rather than just a major version.Note: A static analysis warning (
artipacked) was reported on these lines. It appears to be a false positive; please verify and disregard if appropriate.🧰 Tools
🪛 GitHub Check: zizmor
[warning] 92-93:
artipacked
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build.yaml(1 hunks)
🧰 Additional context used
🪛 GitHub Check: zizmor
.github/workflows/build.yaml
[warning] 92-93:
artipacked
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: hoprd / docker
- GitHub Check: hopli / docker
- GitHub Check: tests-smoke-websocket
- GitHub Check: tests-unit-nightly
- GitHub Check: tests-unit
- GitHub Check: Docs / Rust docs
- GitHub Check: Linter
🔇 Additional comments (2)
.github/workflows/build.yaml (2)
80-85: Updated Permissions for Label Job
The permissions section now usesissues: writewith supporting comments, which is appropriate since PR labels are managed under issues. Please ensure that this change is consistently applied across all related workflows.
95-98: Updated Labeler Action Usage
Pinning theactions/labeleraction to the specific commit6463cdb00ee92c05bec55dffc4e1fce250301945helps ensure reproducible builds and better security. Confirm that this commit contains all necessary fixes and improvements for your workflow configuration.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
sdk/python/api/hopr.py (1)
117-120: Improved URL handling and logging.The changes enhance code maintainability by extracting URL construction into a dedicated variable and adding debug logging. This makes the code more readable and easier to troubleshoot.
For consistency with the rest of the file, consider using the existing logger instance (
log) instead of directly callinglogging.debug:- logging.debug(f"Calling {method.value} {url}") + log.debug(f"Calling {method.value} {url}")This would align with the error logging approach used in the exception handlers (which use
logging.error).sdk/python/localcluster/cluster.py (1)
81-81: Improve log message formatting.Remove unnecessary f-string as it doesn't contain any placeholders.
- logging.info(f"Retrieve nodes addresses and peer ids") + logging.info("Retrieve nodes addresses and peer ids")🧰 Tools
🪛 Ruff (0.8.2)
81-81: f-string without any placeholders
Remove extraneous
fprefix(F541)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
chain/rpc/src/rpc.rs(0 hunks)flake.nix(3 hunks)nix/shell.nix(1 hunks)sdk/barebone-lower-win-prob.cfg.yaml(1 hunks)sdk/barebone.cfg.yaml(1 hunks)sdk/default.cfg.yaml(1 hunks)sdk/python/api/hopr.py(1 hunks)sdk/python/api/response_objects.py(1 hunks)sdk/python/localcluster/cluster.py(3 hunks)sdk/python/localcluster/main_process.py(2 hunks)sdk/python/localcluster/node.py(5 hunks)sdk/python/localcluster/snapshot.py(1 hunks)tests/conftest.py(1 hunks)tests/pytest.ini(1 hunks)tests/requirements.txt(1 hunks)tests/test_integration.py(6 hunks)
💤 Files with no reviewable changes (1)
- chain/rpc/src/rpc.rs
✅ Files skipped from review due to trivial changes (2)
- sdk/python/localcluster/snapshot.py
- tests/requirements.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- flake.nix
- nix/shell.nix
🧰 Additional context used
🧬 Code Definitions (2)
tests/test_integration.py (1)
tests/utils.py (4)
send_and_receive_packets_with_pop(161-169)check_unredeemed_tickets_value(136-138)check_rejected_tickets_value(131-133)check_all_tickets_redeemed(156-158)
sdk/python/localcluster/main_process.py (1)
sdk/python/localcluster/cluster.py (1)
Cluster(23-201)
🪛 Ruff (0.8.2)
sdk/python/localcluster/cluster.py
81-81: f-string without any placeholders
Remove extraneous f prefix
(F541)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: hoprd / docker
- GitHub Check: hopli / docker
- GitHub Check: tests-smoke-websocket
- GitHub Check: tests-unit-nightly
- GitHub Check: tests-unit
- GitHub Check: Linter
- GitHub Check: zizmor
🔇 Additional comments (30)
tests/pytest.ini (2)
9-11: Enhanced CLI Logging Configuration
The update sets the CLI logging level to DEBUG and defines a detailed format and date format. This will provide better contextual logging output during test runs, which can be very helpful for debugging in the CI/CD pipeline.
13-15: Standardized General Logging Settings
The addition oflog_level,log_format, andlog_date_formatharmonizes the overall logging configuration. Using a similar format to the CLI logging ensures consistency across different logging outputs, making it easier to trace and debug issues.sdk/python/api/response_objects.py (1)
84-84: LGTM! Good addition for peer quality tracking.Adding the "quality" field to the ConnectedPeer class enhances the SDK's capability to track and represent peer connection quality. This is a valuable metric for applications that need to make routing decisions or monitor network health.
sdk/barebone.cfg.yaml (1)
6-8: Updated Heartbeat ParametersThe heartbeat parameters have been adjusted from their previous settings (likely 10, 10, and 1) to more aggressive values: an interval of 3, a threshold of 2, and a variance of 0. This change will make the heartbeat probes occur more frequently and consistently. Be sure that downstream components can handle the increased frequency without adverse effects.
sdk/default.cfg.yaml (2)
6-8: Updated Heartbeat SettingsThe heartbeat configuration values (interval, threshold, and variance) are now set to 3, 2, and 0 respectively. This update is consistent with the overall strategy for increasing responsiveness. Verify that these more frequent heartbeats do not lead to unnecessary processing overhead or false positives.
10-11: Modified Network Ignore TimeframeThe
ignore_timeframeundernetwork_optionsis reduced to 1 (from a higher value, such as 5 in previous contexts). This shorter timeframe means that network errors or conditions will be reconsidered more quickly. Ensure this aggressive setting aligns with your network reliability requirements.sdk/barebone-lower-win-prob.cfg.yaml (2)
6-8: Updated Heartbeat Parameters in Lower Win-Prob ConfigThe heartbeat settings (interval: 3, threshold: 2, variance: 0) have been updated in this configuration file as well. This consistency across configuration files is good; however, please verify that the lower-level testing or production environments using this file are prepared for the increased heartbeat frequency.
18-18: Adjusted Ticket Winning ProbabilityThe
outgoing_ticket_winning_probis now set to 0.1 rather than a higher value seen in other configs (e.g., 1.0). This significant reduction can impact protocol behavior. Please confirm that this lower probability is intentional for the respective environment and that all stakeholders are aware of the change.tests/conftest.py (1)
82-82:✅ Verification successful
NAT enabled for test environment.
The addition of
use_nat=Trueenables Network Address Translation for the local cluster during test execution. This could affect how nodes communicate with each other in the testing environment.
🏁 Script executed:
#!/bin/bash # Check if any existing tests might be affected by the NAT change rg -A 2 -B 2 "use_nat\s*=" --type pyLength of output: 1120
Action: Confirm and Monitor NAT Configuration in Tests
After verifying the changes, we note the following:
- In
tests/conftest.py, the local cluster is explicitly configured withuse_nat=True, which confirms the intended activation of NAT for test execution.- The NAT parameter is properly passed to
sdk/python/localcluster/cluster.pyand is explicitly set in tests, while the default insdk/python/localcluster/main_process.pyremainsFalse, ensuring that production behavior isn’t inadvertently altered.- Although the change is intentional for the test environment, please ensure that enabling NAT does not impact node communications or introduce unexpected side effects in integration tests.
sdk/python/localcluster/main_process.py (2)
27-27: Added NAT configuration parameter to bringup function.The function signature now includes a
use_natparameter with a default value ofFalse. This allows controlling NAT behavior at the cluster initialization level.
38-38: Updated Cluster constructor call to include NAT configuration.Properly passes the
use_natparameter to the Cluster constructor, ensuring NAT settings flow through to the cluster configuration.sdk/python/localcluster/cluster.py (4)
24-27: Added NAT configuration to Cluster class.The Cluster constructor now accepts and stores the
use_natparameter, which will be used to configure NAT behavior for all nodes in the cluster.
33-33: Propagated NAT configuration to Node instances.The
use_natparameter is now passed to each Node instance via thefromConfigmethod, ensuring consistent NAT behavior across all nodes in the cluster.
87-88: Enhanced error handling for address retrieval.Changed error handling from logging an error to logging a critical message and raising a
RuntimeError. This provides better visibility into critical failures during the setup process.
98-102: Improved error handling for peer connection timeouts.Added a try-except block to catch
asyncio.TimeoutErrorand raise aRuntimeErrorwith a critical log message. This ensures that the setup process fails fast with clear error messages when peer connections cannot be established within the timeout period.sdk/python/localcluster/node.py (5)
37-45: Added NAT configuration parameter to Node constructor.The Node constructor now accepts a boolean
use_natparameter, enhancing the configuration capabilities of the Node class regarding NAT usage.
54-54: Store NAT configuration in Node instance.The
use_natparameter is now stored as an instance variable, making it available for use in other methods of the Node class.
178-178: Configure NAT environment variable.Added an environment variable setting for
HOPRD_NATbased on the value ofself.use_nat, which will control NAT behavior in the HOPR node process.
217-228: Enhanced peer connection monitoring and logging.Significant improvements to the
all_peers_connectedmethod:
- Reduced timeout from default (likely 20s) to 1s for better responsiveness
- Added detailed debug logging about peers and their connection status
- Implemented filtering of peers based on connection quality (≥ 0.25)
- Improved detection and reporting of missing peer connections
These changes provide much better visibility into peer connection issues during testing and cluster setup.
239-247: Updated Node.fromConfig method to support NAT configuration.The
fromConfigclass method now accepts and passes through theuse_natparameter, ensuring NAT configuration can be specified during Node creation from config files.tests/test_integration.py (10)
5-5: Good addition of the logging module.Adding the
loggingmodule is essential for better debug capabilities in tests.
137-139: Improved ping test robustness and debugging.The changes here provide two important improvements:
- Added debug logging for the ping response, which helps in troubleshooting test failures
- Modified the assertion to accept non-negative latency values (
>= 0instead of> 0), which is more realistic as some ping responses might legitimately report zero latency in optimal network conditionsThese changes make the test more reliable and easier to diagnose when failures occur.
174-174: Appropriate timeout added for zero-hop messages.Adding an explicit timeout of 5.0 seconds to the
send_and_receive_packets_with_popfunction call is good practice. This prevents tests from hanging indefinitely if message delivery fails and provides a consistent timeout across test runs.
306-307: Reduced timeout for one-hop message test.The timeout for
send_and_receive_packets_with_popwas reduced to 1.0 second, which is quite aggressive for a one-hop message. This could potentially lead to flaky tests if the network experiences any delays.Consider whether 1.0 second is sufficient for reliable test execution across different environments. You may want to run the tests multiple times to verify stability.
314-314: Optimized timeout for unredeemed tickets check.Reducing the timeout from 30.0 to 5.0 seconds is a good optimization if the operation reliably completes within this timeframe. This helps speed up the test suite execution.
317-317: Optimized timeout for rejected tickets check.Similar to the previous timeout reduction, decreasing from 30.0 to 5.0 seconds helps speed up the test suite if the operation consistently completes within this timeframe.
322-322: Significantly reduced timeout for checking redeemed tickets.The timeout has been drastically reduced from 120.0 seconds to 5.0 seconds. This is a substantial change that assumes the ticket redemption process has been optimized to complete much faster than before.
Verify that the ticket redemption process consistently completes within 5 seconds across different environments. If there's any variability in network conditions or processing speed, this could lead to flaky tests.
381-381: Reduced timeout for ticket aggregation and redemption.The timeout has been reduced from 120.0 to 30.0 seconds, which is a significant reduction but still allows a reasonable timeframe for the operation to complete.
520-527: Improved message peek test with better feedback.The changes in this section enhance the test by:
- Using a boolean variable
doneto clearly express the success condition- Adding debug logging when the expected number of messages isn't received
- Adding a small sleep interval when retrying
This makes the test more robust and easier to debug when it fails.
528-528: Added explicit timeout for message peek test.Adding an explicit timeout of 5 seconds for the
peek_the_messagesfunction is a good practice. This prevents the test from hanging and provides a consistent timeout expectation.
This reverts commit 2c2c323.
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
.github/workflows/update-flake-lock.yaml (1)
17-17: Runner Label Check:
The"runs-on"field on line 17 uses the custom labelself-hosted-hoprnet-small. Please verify that this label is properly configured in your self-hosted runner pool, as it doesn’t match the standard GitHub Actions labels.🧰 Tools
🪛 actionlint (1.7.4)
17-17: label "self-hosted-hoprnet-small" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
.github/workflows/renovate-cargo-update.yaml (2)
16-16: Runner Label Check:
On line 16, the workflow again uses the custom runner labelself-hosted-hoprnet-small. As with other workflows, please ensure that this label is correctly defined and accessible; otherwise, consider switching to one of the standard GitHub Actions runner labels or properly configuring your custom label.🧰 Tools
🪛 actionlint (1.7.4)
16-16: label "self-hosted-hoprnet-small" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
43-49: Update Vendored Cargo Dependencies Step:
This step (lines 43–49) updates Rust cargo dependencies by ensuring required directories and file placeholders exist before running the update commands via Nix. Verify thatethereum/bindings/src/lib.rsis the correct target for your dependency tracking, and consider abstracting repetitive directory creation logic if it appears elsewhere in your workflows.tests/test_websocket_api.py (2)
9-9: Remove unused import.The
loggingmodule is imported but not used. Please remove it to keep the code clean.- import logging🧰 Tools
🪛 Ruff (0.8.2)
9-9:
loggingimported but unusedRemove unused import:
logging(F401)
111-113: Avoid excessively long reason strings.This line exceeds 120 characters. Consider breaking it up:
- @pytest.mark.xfail(reason="This test is expected to fail due to a bug in the axum code, where the query is not parsed for the token") + @pytest.mark.xfail( + reason=( + "This test is expected to fail due to a bug in the axum code, " + "where the query is not parsed for the token" + ) + )🧰 Tools
🪛 Ruff (0.8.2)
112-112: Line too long (121 > 120)
(E501)
tests/test_win_prob.py (1)
300-300: Line exceeds recommended length.Consider wrapping the content to maintain a consistent max line length (e.g., 120 columns).
🧰 Tools
🪛 Ruff (0.8.2)
300-300: Line too long (122 > 120)
(E501)
tests/test_integration.py (3)
256-334: Test for channel funding exhaustion could be improved.This test includes a large commented section that describes an incomplete feature (incoming channel closure). The implementation correctly tests the existing functionality but could be clearer about the limitations.
Consider refactoring this test to:
- Move the commented section to a separate document or ticket
- Add a clear explanation at the top of the test about what is being validated
- Add a TODO with a reference to the ticket number for implementing the full functionality
async def test_hoprd_should_fail_sending_a_message_when_the_channel_is_out_of_funding( self, src: str, dest: str, swarm7: dict[str, Node] ): - """ - # FIXME: The following part can be enabled once incoming channel closure is - # implemented. - # - # need to close the incoming side to not have to wait for the closure timeout - # api_close_channel "${second_node_id}" "${node_id}" "${second_node_api}" "${node_addr}" "incoming" - - # only fund for 2 tickets - # channel_info=$(api_open_channel "${node_id}" "${second_node_id}" "${node_api}" "${second_node_addr}" "200") - - # need to wait a little to allow the other side to index the channel open event - # sleep 10 - # api_get_tickets_in_channel ${second_node_api} ${channel_id} "TICKETS_NOT_FOUND" - # for i in `seq 1 ${generated_tickets}`; do - # log "PendingBalance in channel: Node ${node_id} send 1 hop message to self via node ${second_node_id}" - # api_send_message "${node_api}" "${msg_tag}" "${peer_id}" \ - # "pendingbalance: hello, world 1 self" "${second_peer_id}" - # done - - # seems like there's slight delay needed for tickets endpoint to return up to date tickets, \ - # probably because of blockchain sync delay - # sleep 5 - - # ticket_amount=$(api_get_tickets_in_channel ${second_node_api} ${channel_id} | jq '. | length') - # if [[ "${ticket_amount}" != "${generated_tickets}" ]]; then - # msg "PendingBalance: Ticket amount ${ticket_amount} is different than expected ${generated_tickets}" - # exit 1 - # fi - - # api_redeem_tickets_in_channel ${second_node_api} ${channel_id} - # sleep 5 - # api_get_tickets_in_channel ${second_node_api} ${channel_id} "TICKETS_NOT_FOUND" - # api_close_channel "${node_id}" "${second_node_id}" "${node_api}" "${second_node_addr}" "outgoing" - """ + """ + Tests that message sending fails when a channel runs out of funding. + + Note: This test only verifies the rejection of messages when funding is depleted. + Future implementation will include incoming channel closure testing (see ticket #XXXX). + """
341-345: Line exceeds maximum length in two places.Lines 341 and 345 exceed the 120 character limit. While this doesn't affect functionality, it's good practice to maintain consistent formatting.
- async with create_channel(swarm7[src], swarm7[dest], OPEN_CHANNEL_FUNDING_VALUE_HOPR): + async with create_channel( + swarm7[src], swarm7[dest], OPEN_CHANNEL_FUNDING_VALUE_HOPR + ): # the context manager handles opening and closing of the channel with verification, using counter-party address assert True - async with create_channel(swarm7[src], swarm7[dest], OPEN_CHANNEL_FUNDING_VALUE_HOPR, use_peer_id=True): + async with create_channel( + swarm7[src], swarm7[dest], OPEN_CHANNEL_FUNDING_VALUE_HOPR, use_peer_id=True + ): # the context manager handles opening and closing of the channel with verification using counter-party peerID assert True🧰 Tools
🪛 Ruff (0.8.2)
341-341: Line too long (123 > 120)
(E501)
345-345: Line too long (121 > 120)
(E501)
547-574: Unfinished test for strategy testing.Similar to other TODOs, this test contains a large commented block with notes for future implementation. It would be better to move this to a separate document or ticket.
Consider creating a formal TODO item with:
- A clear description of what needs to be implemented
- References to design documents or requirements
- A ticket number for tracking
- Removal of the large commented block in favor of a concise TODO comment
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
flake.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
.github/workflows/build.yaml(1 hunks).github/workflows/renovate-cargo-update.yaml(1 hunks).github/workflows/update-flake-lock.yaml(1 hunks)sdk/python/localcluster/constants.py(1 hunks)tests/conftest.py(3 hunks)tests/pytest.ini(1 hunks)tests/test_hopli.py(9 hunks)tests/test_integration.py(1 hunks)tests/test_redeeming.py(1 hunks)tests/test_rest_api.py(1 hunks)tests/test_session.py(5 hunks)tests/test_websocket_api.py(2 hunks)tests/test_win_prob.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/conftest.py
- .github/workflows/build.yaml
🧰 Additional context used
🧬 Code Definitions (7)
tests/test_rest_api.py (1)
tests/conftest.py (2)
nodes_with_auth(59-61)swarm7(79-88)
tests/test_win_prob.py (3)
tests/conftest.py (2)
barebone_nodes(54-56)nodes_with_lower_outgoing_win_prob(69-71)sdk/python/api/hopr.py (1)
ticket_min_win_prob(357-363)sdk/python/localcluster/utils.py (1)
load_private_key(16-19)
tests/test_websocket_api.py (2)
tests/conftest.py (4)
random_distinct_pairs_from(74-75)nodes_with_auth(59-61)swarm7(79-88)to_ws_url(107-108)sdk/python/localcluster/node.py (1)
Node(35-287)
tests/test_redeeming.py (3)
tests/conftest.py (2)
barebone_nodes(54-56)swarm7(79-88)sdk/python/api/hopr.py (4)
get_tickets_statistics(381-387)channel_redeem_tickets(223-230)channels_aggregate_tickets(271-278)tickets_redeem(289-295)tests/utils.py (2)
shuffled(19-21)create_channel(29-48)
tests/test_integration.py (1)
sdk/python/api/hopr.py (5)
ticket_price(349-355)balances(182-188)aliases_get_alias(157-163)aliases_set_alias(165-172)aliases_remove_alias(174-180)
tests/test_session.py (4)
tests/conftest.py (3)
random_distinct_pairs_from(74-75)barebone_nodes(54-56)swarm7(79-88)sdk/python/api/hopr.py (3)
session_client(421-462)session_list_clients(412-419)session_close_client(464-475)sdk/python/api/protocol.py (1)
Protocol(20-43)sdk/python/api/request_objects.py (1)
SessionCapabilitiesBody(97-116)
tests/test_hopli.py (3)
tests/conftest.py (2)
barebone_nodes(54-56)swarm7(79-88)sdk/python/localcluster/utils.py (1)
load_private_key(16-19)sdk/python/api/hopr.py (1)
balances(182-188)
🪛 actionlint (1.7.4)
.github/workflows/renovate-cargo-update.yaml
16-16: label "self-hosted-hoprnet-small" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🪛 Ruff (0.8.2)
tests/test_win_prob.py
300-300: Line too long (122 > 120)
(E501)
tests/test_websocket_api.py
9-9: logging imported but unused
Remove unused import: logging
(F401)
112-112: Line too long (121 > 120)
(E501)
tests/test_integration.py
341-341: Line too long (123 > 120)
(E501)
345-345: Line too long (121 > 120)
(E501)
🔇 Additional comments (63)
.github/workflows/update-flake-lock.yaml (1)
25-30: Install Nix Step Addition:
The new "Install Nix" step (lines 25–30) is implemented correctly and conditionally executes based onenv.needs_nix_setup == 'true'. This addition should help ensure that Nix is available when needed. Please confirm that subsequent steps dependent on Nix detect the installation as expected..github/workflows/renovate-cargo-update.yaml (3)
19-24: Harden Runner Step Review:
The Harden Runner step (lines 19–24) is configured withdisable-sudo: trueand anegress-policyofaudit. The inline TODO reminds you to update the policy to'block'after several runs. Ensure that you schedule this change once the initial testing phase confirms stability.
30-34: Install Nix Step Verification:
The "Install Nix" step (lines 30–34) usescachix/install-nix-action@v29with proper GitHub token configuration. Please verify that pinning to versionv29aligns with your security and maintenance policies, and that the token has the required permissions.
50-57: Commit Changes Step Verification:
The "Commit changes" step (lines 50–57) is conditionally executed when not in the ACT environment, which is a sound safeguard. The environment variables for the commit message and GitHub ref help ensure consistency. No changes needed here.sdk/python/localcluster/constants.py (2)
21-21: Good addition of a centralized contracts directory constantThis new constant centralizes the definition of the contracts directory path, improving maintainability by reducing duplication.
28-28: Good refactoring to use CONTRACTS_DIRUsing the new CONTRACTS_DIR constant instead of repeating the path components is a clean approach that reduces duplication and improves consistency.
tests/pytest.ini (3)
5-5: Good configuration for asyncio fixture scopeSetting the asyncio fixture loop scope to 'session' will reuse the same event loop across tests, which can improve test performance.
10-12: Enhanced CLI logging configurationIncreasing verbosity to DEBUG level and adding structured formatting for logs will significantly improve debugging capabilities during test execution.
14-16: Good addition of general logging configurationAdding consistent formatting for general logs matches the CLI log format, providing uniformity across all test logging output.
tests/test_rest_api.py (4)
10-11: Good refactoring to class-based test structureThe refactoring to use a class with the
swarm7_resetfixture applied at the class level improves organization and ensures consistent setup/teardown for all tests.
14-19: Well-structured test method for authentication rejectionThe test method maintains the original functionality while benefiting from the class-based structure and fixture.
22-29: Well-structured test method for invalid token rejectionThe implementation correctly tests rejection of connections with invalid authentication tokens.
32-37: Well-structured test method for valid token acceptanceThe implementation correctly tests acceptance of connections with valid authentication tokens.
tests/test_hopli.py (9)
14-14: Good addition of CONTRACTS_DIR to importsAdding CONTRACTS_DIR to the imports supports the refactoring of hardcoded paths throughout the file.
63-63: Good refactoring to use CONTRACTS_DIR constantReplacing hardcoded paths with the CONTRACTS_DIR constant improves maintainability and consistency across the codebase.
Also applies to: 91-91, 114-114, 143-143, 243-243, 291-291, 319-319, 339-339
347-348: Good refactoring to class-based test structureThe refactoring to use a class with the
swarm7_resetfixture applied at the class level improves organization and ensures consistent setup/teardown for all tests.
352-375: Well-structured test method for funding nodesThe test correctly verifies that the funding functionality works as expected, with appropriate assertions for balance changes.
Note that the test is marked with
xfaildue to race conditions, which is a good practice to acknowledge known issues.
379-418: Well-structured test method for node registrationThe test thoroughly verifies deregistration and re-registration of nodes, with appropriate assertions at each stage.
422-427: Well-structured test method for eligibility syncThe test correctly verifies the functionality to sync eligibility for nodes.
429-460: Well-structured test method for safe module creationThe test thoroughly verifies the creation of safe modules and performs appropriate cleanup afterward.
463-489: Well-structured test method for win probabilityThe test correctly verifies setting and reading win probability values, with appropriate assertions for the expected values.
492-511: Standalone test function maintained appropriatelyThis test function was correctly kept standalone as it doesn't require the
swarm7_resetfixture and tests different functionality than the class-based tests.tests/test_websocket_api.py (10)
27-28: Nice approach for organizing tests into a class.The
@pytest.mark.usefixtures("swarm7_reset")and class-based structure enhance readability and maintainability.
29-44: Comprehensive negative test case.The logic correctly verifies rejection of connections without valid tokens. The flow from connection attempt to assertion is complete.
46-61: Good negative test scenario.This test covers invalid token headers well. No issues found.
63-78: Methodical coverage of query param token handling.All steps are clear, and the assertion for "401 Unauthorized" is appropriate.
80-95: Robust invalid bearer token check.Matches the pattern of other invalid token tests, ensuring thorough coverage.
97-110: Validation of successful connect with valid token.Straightforward acceptance test. Good use of the
headerargument to supply credentials.
115-128: Adequate demonstration of xfail scenario.Using
@pytest.mark.xfailhere is a good way to track known issues while maintaining test coverage.
130-142: Positive test for valid bearer usage.Confirms that valid tokens are accepted, mirroring earlier negative checks. Looks good.
144-159: Properly detects invalid paths returning 404.Well-defined test to confirm the server returns the expected status code for nonexistent endpoints.
161-187: End-to-end message flow test.Excellent approach to ensure messages are correctly sent and received. The loop-based check is clear and timeouts help detect stalls.
tests/test_win_prob.py (8)
6-6: Use ofCONTRACTS_DIRis appropriate.Including the
CONTRACTS_DIRconstant improves maintainability by avoiding hardcoded paths.
27-28: Class-based tests with@pytest.mark.usefixtures("swarm7_reset").Organizing scenarios in
TestWinProbWithSwarmlooks clean and consistent with other test files.
44-62: Checks default winning probability.This test properly verifies initial state and ensures the probability can be altered and reverted.
63-123: Holistic coverage of lowering and redeeming tickets.The flow is robust; verifying the correct ticket statistics and final redemption. Well-structured.
124-190: Validates unredeemed tickets on minimum bound change.Smart use of try/finally to restore system state. Thorough coverage of ticket rejections when bounds increase.
192-258: Confirms relaying with increased probability.Good approach testing multiple relays and verifying each step’s ticket distribution logic.
260-312: Ensures packets with higher than min win prob are relayed.Demonstrates that tickets remain unredeemed if the channel closes prematurely. Proper coverage of the scenario.
🧰 Tools
🪛 Ruff (0.8.2)
300-300: Line too long (122 > 120)
(E501)
314-365: Comprehensive test for rejecting lower-than-min-bound tickets.Verifies that the relay rejects and does not increment unredeemed_value for insufficient ticket probabilities.
tests/test_redeeming.py (7)
27-28: Class-based approach for redeeming tests.Using
TestRedeemingWithSwarmensures consistent setup/teardown viaswarm7_reset.
29-37: Verifies no unexpected unredeemed tickets.Clear check to confirm that, without sending messages, unredeemed_value remains 0.
38-72: Good demonstration of redeem flow in 1-hop scenario.The channel is opened, messages are exchanged, and tickets are redeemed. The approach is thorough and ensures no leftover tickets.
74-120: Ensures creation of redeemable tickets in 1-hop self-routing.Test logic checks unredeemed tickets, redeems them, and asserts final ticket stats. No issues found.
122-156: Aggregates and redeems in channel on-demand.Great usage of
channels_aggregate_ticketsfollowed by redemption. The final checks for unredeemed_value alignment look correct.
158-197: Test for multi-hop scenario.Ensures tickets are properly created, aggregated, and redeemed across an N-hop route. Good concurrency handling with
AsyncExitStack.
199-227: Checks channel closure with unredeemed tickets.Verifies that channels can still be closed even with outstanding unredeemed tickets. Solid final coverage for channel lifecycle.
tests/test_session.py (9)
234-236: Class encapsulation improves test isolation and organization.The addition of a class structure with the
swarm7_resetfixture applied at the class level provides better test organization and ensures each test starts with a clean state. This is a good practice for test maintainability.
238-244: Method parameter updated to match class structure.The function signature has been correctly updated to include
selfas the first parameter, which is required for class methods. The test logic remains unchanged, which preserves the test's original intent.
286-347: The multi-hop test correctly preserves theIntermediatePathspecification.While the direct test cases use
Hops: 0, this test appropriately maintains theIntermediatePathparameter since it's testing a route with intermediate nodes. This shows intentional design to differentiate between direct and multi-hop connections.
369-370: UDP echo server test updated with consistent path parameter.The UDP test now uses the same
Hops: 0pattern as the TCP test, maintaining consistency in the API usage pattern across different protocols.
418-429: New test for UDP loopback service improves coverage.The addition of this test improves the coverage by testing the service functionality with UDP, which is an important use case. The test correctly sets up the service flag when creating the client.
459-523: Multi-hop UDP echo server test mirrors the structure of the TCP version.This test follows the same pattern as the TCP multi-hop test, ensuring consistency between protocol implementations. The AsyncExitStack is correctly used to manage the channel lifecycle.
538-542: Addedsealed_targetparameter to HTTPS test.The addition of the
sealed_target=Trueparameter enhances security by encrypting the target information. This is a good practice when dealing with sensitive connections.
610-612: Appropriate skip condition for WireGuard tunnel test.The test will only run when a specific environment variable is set, which is appropriate for tests that require special infrastructure or configurations. This prevents the test from failing in standard environments.
253-259:✅ Verification successful
Path parameter structure changed from
IntermediatePathtoHops.The path parameter has been modified from a potential multi-hop specification to a direct 0-hop route. This simplifies the test case while still testing the core functionality.
🏁 Script executed:
#!/bin/bash # Verify if the API supports both path parameter formats rg -A 5 "path\s*=\s*\{\"Hops\":" --type py rg -A 5 "path\s*=\s*\{\"IntermediatePath\":" --type pyLength of output: 3471
Update: Confirmed Simplified Direct Path Parameter
- Verified that in
tests/test_session.pythe direct 0-hop route usingpath={"Hops": 0}is intentionally applied to test the core functionality.- Noted that while this change simplifies the test case, other tests still use the multi-hop format with
{"IntermediatePath": ...}. Ensure that this dual usage is intentional; if a unified approach is preferred, consider standardizing the parameter format across tests.tests/test_integration.py (7)
50-51: Class structure with shared fixture improves test organization.Similar to the session tests, encapsulating integration tests in a class with a shared fixture provides better organization and ensures each test starts with a clean environment.
54-82: Initial connectivity test sets up global constants.This test correctly verifies that all nodes can communicate with each other and sets global ticket price constants based on API responses. Placing this test first ensures subsequent tests have the correct configuration.
83-91: Balance validation without relying on prior tests enhances test independence.The test properly validates both native and HOPR token balances independently of other tests, which is good practice for test isolation.
92-112: Comprehensive alias management test.This test thoroughly checks alias creation, retrieval (with both peer ID and address formats), and removal functionality, which is essential for user-friendly node identification in the network.
196-224: Channel funding verification test is comprehensive.The test verifies both immediate channel balance updates and the corresponding safe balance decrease after funding a channel. The use of
asyncio.wait_forwith appropriate timeouts ensures the test doesn't hang indefinitely.
454-462: Complete test coverage for reserved application tags.The test appropriately checks that operations with reserved application tags fail as expected, which is important for maintaining the integrity of the messaging system.
508-526: Timestamp validation in message sending is thorough.This test verifies that message timestamps are both returned and are in chronological order, which is important for message sequencing and debugging.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
.github/workflows/renovate-cargo-update.yaml (2)
19-24: Harden Runner Configuration
The "Harden Runner" step now disables sudo access and sets theegress-policytoaudit. The included TODO (to change the egress policy toblock) is noted—just remember to update it after sufficient validation runs.
43-49: Vendored Dependencies Update Process
The steps to create the required directory, touch the file, and run the Nix commands for updating cargo dependencies are clear and methodical. It may be beneficial to add error handling or additional logging within the Nix commands block to facilitate troubleshooting if issues arise during dependency updates.tests/test_websocket_api.py (2)
9-9: Remove unusedloggingimport
Theloggingmodule is not used anywhere, so removing it can clean up the code and avoid linting errors.-import logging🧰 Tools
🪛 Ruff (0.8.2)
9-9:
loggingimported but unusedRemove unused import:
logging(F401)
111-113: Line too long
Line 112 exceeds 120 characters. Please consider wrapping the string to satisfy style requirements.@pytest.mark.xfail( - reason="This test is expected to fail due to a bug in the axum code, where the query is not parsed for the token" + reason=( + "This test is expected to fail due to a bug in the axum code, " + "where the query is not parsed for the token" + ) )🧰 Tools
🪛 Ruff (0.8.2)
112-112: Line too long (121 > 120)
(E501)
tests/test_win_prob.py (1)
300-300: Line too long
Line 300 exceeds 120 characters (E501). Consider splitting this comment for style compliance.-# in this case, the relay has tickets for all the packets, because the source sends them with win prob = 1 +# in this case, the relay has tickets for all the packets, +# because the source sends them with win prob = 1🧰 Tools
🪛 Ruff (0.8.2)
300-300: Line too long (122 > 120)
(E501)
tests/test_session.py (1)
610-670: Incomplete test for WireGuard tunnel integration.The test is correctly skipped unless a specific environment variable is set, but contains a placeholder sleep instead of actual assertions.
Would you like me to help implement a proper test case to replace the placeholder? The current sleep of 3600 seconds (1 hour) suggests this may be intended for manual verification.
tests/test_integration.py (4)
151-165: Incomplete test for non-registry pings.The test is marked as UNFINISHED and contains only comments describing the intended test logic.
Would you like me to help implement this test based on the commented description? It seems important to verify that nodes not in the registry cannot communicate.
341-341: Line exceeds maximum length.This line (123 characters) exceeds the recommended maximum length of 120 characters.
Consider formatting this line to stay within the 120 character limit:
- async with create_channel(swarm7[src], swarm7[dest], OPEN_CHANNEL_FUNDING_VALUE_HOPR): + async with create_channel( + swarm7[src], swarm7[dest], OPEN_CHANNEL_FUNDING_VALUE_HOPR + ):🧰 Tools
🪛 Ruff (0.8.2)
341-341: Line too long (123 > 120)
(E501)
345-347: Line exceeds maximum length.This line (121 characters) exceeds the recommended maximum length of 120 characters.
Consider reformatting:
- async with create_channel(swarm7[src], swarm7[dest], OPEN_CHANNEL_FUNDING_VALUE_HOPR, use_peer_id=True): + async with create_channel( + swarm7[src], swarm7[dest], OPEN_CHANNEL_FUNDING_VALUE_HOPR, use_peer_id=True + ):🧰 Tools
🪛 Ruff (0.8.2)
345-345: Line too long (121 > 120)
(E501)
547-573: Unimplemented strategy test.This test is commented out and marked as UNFINISHED, only containing the shell of what should be tested.
Would you like me to help implement this test based on the commented outline? It appears to test the node strategy settings and their effect on connection behavior.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
flake.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
.github/workflows/build.yaml(1 hunks).github/workflows/renovate-cargo-update.yaml(1 hunks).github/workflows/update-flake-lock.yaml(1 hunks)sdk/python/localcluster/constants.py(1 hunks)tests/conftest.py(3 hunks)tests/pytest.ini(1 hunks)tests/test_hopli.py(9 hunks)tests/test_integration.py(1 hunks)tests/test_redeeming.py(1 hunks)tests/test_rest_api.py(1 hunks)tests/test_session.py(5 hunks)tests/test_websocket_api.py(2 hunks)tests/test_win_prob.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/build.yaml
- tests/conftest.py
🧰 Additional context used
🧠 Learnings (1)
.github/workflows/renovate-cargo-update.yaml (1)
Learnt from: Teebor-Choka
PR: hoprnet/hoprnet#6886
File: nix/rust-package.nix:84-84
Timestamp: 2025-04-02T16:41:38.142Z
Learning: When transitioning from vendored dependencies to a private proxy registry in Rust projects, GitHub Actions workflows need to be refactored to accommodate the new update strategy, particularly workflows handling dependency updates like renovate-cargo-update.yaml.
🧬 Code Definitions (7)
tests/test_rest_api.py (1)
tests/conftest.py (2)
nodes_with_auth(59-61)swarm7(79-88)
tests/test_hopli.py (3)
tests/conftest.py (2)
barebone_nodes(54-56)swarm7(79-88)sdk/python/localcluster/utils.py (1)
load_private_key(16-19)sdk/python/api/hopr.py (1)
balances(182-188)
tests/test_win_prob.py (4)
tests/conftest.py (2)
barebone_nodes(54-56)nodes_with_lower_outgoing_win_prob(69-71)sdk/python/api/hopr.py (2)
ticket_min_win_prob(357-363)get_tickets_statistics(381-387)sdk/python/localcluster/utils.py (1)
load_private_key(16-19)tests/utils.py (1)
check_min_incoming_win_prob_eq(151-153)
tests/test_websocket_api.py (2)
tests/conftest.py (4)
random_distinct_pairs_from(74-75)nodes_with_auth(59-61)swarm7(79-88)to_ws_url(107-108)sdk/python/localcluster/node.py (1)
Node(35-287)
tests/test_integration.py (4)
tests/conftest.py (2)
swarm7(79-88)barebone_nodes(54-56)sdk/python/api/hopr.py (2)
ticket_price(349-355)send_message(397-410)tests/utils.py (2)
send_and_receive_packets_with_pop(161-169)gen_random_tag(24-25)sdk/python/api/channelstatus.py (1)
ChannelStatus(4-27)
tests/test_session.py (4)
tests/conftest.py (2)
random_distinct_pairs_from(74-75)barebone_nodes(54-56)sdk/python/api/hopr.py (2)
session_client(421-462)session_close_client(464-475)sdk/python/api/protocol.py (1)
Protocol(20-43)sdk/python/api/request_objects.py (1)
SessionCapabilitiesBody(97-116)
tests/test_redeeming.py (3)
tests/conftest.py (2)
barebone_nodes(54-56)swarm7(79-88)sdk/python/api/hopr.py (4)
get_tickets_statistics(381-387)channel_redeem_tickets(223-230)channels_aggregate_tickets(271-278)tickets_redeem(289-295)tests/utils.py (5)
shuffled(19-21)create_channel(29-48)send_and_receive_packets_with_pop(161-169)check_unredeemed_tickets_value(136-138)check_all_tickets_redeemed(156-158)
🪛 actionlint (1.7.4)
.github/workflows/renovate-cargo-update.yaml
16-16: label "self-hosted-hoprnet-small" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🪛 Ruff (0.8.2)
tests/test_win_prob.py
300-300: Line too long (122 > 120)
(E501)
tests/test_websocket_api.py
9-9: logging imported but unused
Remove unused import: logging
(F401)
112-112: Line too long (121 > 120)
(E501)
tests/test_integration.py
341-341: Line too long (123 > 120)
(E501)
345-345: Line too long (121 > 120)
(E501)
🔇 Additional comments (44)
.github/workflows/update-flake-lock.yaml (2)
16-18: Runner Label Verification
The job uses a custom runner label (self-hosted-hoprnet-small). Please double-check that this label is properly defined in your CI configuration and, if needed, update your actionlint configuration so that static analysis recognizes it.🧰 Tools
🪛 actionlint (1.7.4)
17-17: label "self-hosted-hoprnet-small" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
25-30: Conditional Nix Installation Step
A new "Install Nix" step has been added with the conditionalif: env.needs_nix_setup == 'true'. This is a good approach to ensure Nix is installed only when required. Verify that the condition evaluates correctly in all target environments and that the GitHub token is provided securely..github/workflows/renovate-cargo-update.yaml (2)
15-17: Custom Runner Label Check
The workflow specifiesruns-on: self-hosted-hoprnet-small. As with other workflows, ensure that this custom label is correctly recognized in your CI setup or consider updating the configuration to align with known labels if necessary.🧰 Tools
🪛 actionlint (1.7.4)
16-16: label "self-hosted-hoprnet-small" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
50-57: Automated Commit Step Validation
The "Commit changes" step is conditionally executed (skipping execution when running in a local environment viaif: ${{ !env.ACT }}). This configuration is appropriate for automating the commit process.sdk/python/localcluster/constants.py (2)
21-21: Good addition of a centralized path constant!Creating the
CONTRACTS_DIRconstant improves code maintainability by centralizing the path definition to contracts directory.
28-28: Nice refactoring to use the new constant!Updating
CONTRACTS_ADDRESSESto use the newCONTRACTS_DIRconstant instead of directly constructing the path enhances code maintainability and consistency.tests/pytest.ini (3)
5-5: Good configuration for AsyncIO fixtures!Setting the
asyncio_default_fixture_loop_scopeto session level ensures AsyncIO fixtures will be maintained for the entire test session, which can improve test efficiency.
10-12: Enhanced logging configuration for CLI!Increasing the log level to DEBUG and adding detailed formatting for CLI logs will make debugging easier and provide more context in test outputs.
14-16: Comprehensive logging setup for test output!Adding these general logging settings with detailed format including filename and line number information will significantly improve the ability to trace issues during test execution.
tests/test_hopli.py (3)
14-14: Good import of the centralized contracts directory constant.Adding
CONTRACTS_DIRto the imports aligns with the changes made inconstants.pyand prepares for using this constant throughout the file.
63-63: Great consistency in using the centralized CONTRACTS_DIR constant!Replacing hardcoded paths like
"./ethereum/contracts"with theCONTRACTS_DIRconstant across all CLI commands improves maintainability and reduces the risk of path-related errors.Also applies to: 91-91, 114-114, 143-143, 243-243, 291-291, 319-319, 339-339
347-490: Excellent refactoring of tests into a class structure!Moving the test functions into the
TestHopliWithSwarmclass with theswarm7_resetfixture improves test organization and efficiency. The test methods maintain their original functionality while benefiting from shared setup/teardown through the fixture.tests/test_rest_api.py (1)
10-37: Well-structured test class organization!Refactoring the test functions into the
TestRestApiWithSwarmclass with theswarm7_resetfixture aligns with the pattern used in other test files and improves maintainability. The test methods preserve their original behavior while gaining shared setup/teardown through the fixture.tests/test_websocket_api.py (11)
27-28: New class structure looks good
Encapsulating tests in a class is a clean approach and aligns with pytest's fixture usage.
29-45: Basic unauthorized token test
The logic to assert "401 Unauthorized" is correct and ensures invalid tokens are rejected.
46-62: Invalid X-Auth-Token test
Same pattern as above; correct verification of unauthorized attempts.
63-79: Invalid token in query param
Good coverage for query-parameter-based authentication checks.
80-96: Invalid bearer token test
Logic is solid for ensuring invalid bearer tokens are refused.
97-110: Valid token acceptance
Properly verifies successful connections when a valid X-Auth-Token is provided.
114-118: Xfail marker usage
This is a clear usage ofpytest.mark.xfailfor known issues. No concerns.
120-126: Query param valid token acceptance
Well-structured and includes an explicitxfailreason.
131-140: Valid bearer token acceptance
Proper test ensuring authorized access via Bearer header.
145-159: Invalid path rejection
Correctly checks that a "404 Not Found" is returned for invalid endpoints.
161-187: Send-receive functionality
The loop-based logic and timeout handling appear robust.tests/test_win_prob.py (2)
6-6: Import ofCONTRACTS_DIR
AddingCONTRACTS_DIRis fine and likely necessary for referencing the correct contracts path.
27-312: Refactoring into class-based tests
Overall, the test refactoring intoTestWinProbWithSwarmand usage of parametrize is well-structured. It properly handles async calls and test logic.🧰 Tools
🪛 Ruff (0.8.2)
300-300: Line too long (122 > 120)
(E501)
tests/test_redeeming.py (1)
27-227: Encapsulation inTestRedeemingWithSwarm
The restructuring of tests into a class is well-organized and maintains test logic integrity. All changes look good.tests/test_session.py (8)
234-235: Good restructuring with proper fixture usage.Encapsulating the tests within a class and using
@pytest.mark.usefixtures("swarm7_reset")is a good practice for test isolation and ensures a clean state for each test.
238-242: LGTM - Test adaption for class-based approach.The test has been properly adapted to be a class method (adding
selfparameter) while maintaining its core functionality. The conditional packet count based on CI environment is a good optimization.
255-256: Appropriate path parameter for direct connection.Using
path={"Hops": 0}for a direct (no intermediate nodes) connection is the correct approach.
280-290: Well-structured multi-hop test.The test effectively uses parametrization to test different node combinations, with appropriate setup for the complexity of multi-hop communication.
324-325: Path parameter correctly updated for multi-hop scenario.The change from using
HopstoIntermediatePathparameter is appropriate here as it allows specifying the exact intermediate nodes for the multi-hop path.
364-422: Good implementation for UDP service tests.The test properly accounts for UDP's characteristics including buffer size limitations and unordered packet delivery. The service parameter is correctly set for the loopback functionality.
485-513: Effective implementation of multi-hop UDP test.The test correctly handles UDP characteristics across multiple hops, including proper channel setup in both directions and handling UDP's unordered nature.
537-542: Properly setting sealed_target for HTTPS communication.Setting
sealed_target=Trueis important for secure communication as it ensures the target information is encrypted.tests/test_integration.py (9)
50-82: Well-structured class with comprehensive connectivity test.The class structure with fixture usage ensures proper test isolation. The connectivity test is appropriately positioned as a foundational test that other tests can rely on. The global variable updates ensure consistent ticket pricing across tests.
83-91: Effective balance validation test.This test provides a good sanity check for node addresses and balances, essential for ensuring the system is in a proper state for further testing.
92-132: Comprehensive alias management tests.These tests thoroughly cover alias functionality including setting, retrieving, and removing aliases, with both peer IDs and addresses.
196-224: Thorough channel funding verification.The test effectively verifies that channel funding increases are properly reflected in both channel and account balances.
256-334: Comprehensive out-of-funding behavior test.This test thoroughly checks the behavior when a channel runs out of funding, including ticket redemption and rejection handling. The commented section provides context for future enhancements.
361-395: Effective test for automatic ticket processing.This test properly verifies the automatic ticket aggregation and redemption strategy, with appropriate waiting patterns for async operations to complete.
447-452: Good test for reserved tag validation.The test correctly verifies that messages with reserved application tags fail as expected, providing important validation for system boundaries.
464-505: Well-implemented timestamp-based message filtering test.The test effectively verifies timestamp-based message filtering functionality, including proper handling of message ordering.
528-545: Good flexibility test for destination specification.Testing both peer ID and address as destination parameters ensures the API correctly handles different ways of specifying the destination.
Pull request was closed
The following changes are included:
python-sdkauto-generation #6772