Add auto-redeem strategy changes missed in the merge-back #6612
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve modifications to the HOPR protocol's configuration and logic related to ticket redemption strategies. A new parameter, Changes
Possibly related issues
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
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
logic/strategy/src/auto_redeeming.rs (3)
31-35: Consider defining constants instead of functions for fixed valuesSince
single_ticket_redeemandmin_redeem_hoprreturn fixed values without performing any computation, consider defining them as constants. This can improve readability and eliminate unnecessary function calls.Apply this diff to refactor:
-fn single_ticket_redeem() -> Balance { - Balance::new_from_str("2000000000000000000", BalanceType::HOPR) -} +const SINGLE_TICKET_REDEEM: Balance = Balance::new_from_str("2000000000000000000", BalanceType::HOPR); -fn min_redeem_hopr() -> Balance { - Balance::new_from_str("90000000000000000", BalanceType::HOPR) -} +const MIN_REDEEM_HOPR: Balance = Balance::new_from_str("90000000000000000", BalanceType::HOPR);Then update references accordingly:
-#[serde(default = "single_ticket_redeem")] +#[serde(default = "|| SINGLE_TICKET_REDEEM")]-#[serde(default = "min_redeem_hopr")] +#[serde(default = "|| MIN_REDEEM_HOPR")]
49-57: Ensure consistency in default value definitionsThe default value for
minimum_redeem_ticket_valueis specified in both themin_redeem_hoprfunction and the#[default(...)]attribute. To avoid duplication and potential inconsistencies, consider defining the default value in one place.You can remove the
#[default(...)]attribute and rely on#[serde(default = "min_redeem_hopr")](or|| MIN_REDEEM_HOPRif converted to a constant):-#[default(Balance::new_from_str("90000000000000000", BalanceType::HOPR))] pub minimum_redeem_ticket_value: Balance,Alternatively, if you switch to constants:
#[serde(default = "|| MIN_REDEEM_HOPR")] pub minimum_redeem_ticket_value: Balance,
285-285: Consider adding tests with non-zerominimum_redeem_ticket_valueIn several tests,
minimum_redeem_ticket_valueis set to zero. Adding tests with non-zero values can help ensure consistent behavior across different configurations and catch potential edge cases.Also applies to: 316-316, 397-397, 441-441
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
hoprd/hoprd/example_cfg.yaml(1 hunks)logic/strategy/src/auto_redeeming.rs(8 hunks)logic/strategy/src/lib.rs(5 hunks)
🔇 Additional comments (8)
logic/strategy/src/lib.rs (4)
19-21: Documentation improvements look good!
The clarifications about CLI configuration limitations and the relationship with disableTicketAutoRedeem are accurate and helpful.
62-62: LGTM!
The documentation wording improvement maintains accuracy while being more straightforward.
121-121: Type alias addition improves code clarity!
The StrategyConfig alias provides a clear, descriptive name for the configuration type.
105-109: Please verify the significant difference in minimum redeem values
There's a notable difference between:
- Normal operation minimum: 0.09 HOPR (
minimum_redeem_ticket_value) - Channel close minimum: 2 HOPR (
on_close_redeem_single_tickets_value_min)
This 22x difference in thresholds could impact ticket redemption behavior significantly.
hoprd/hoprd/example_cfg.yaml (2)
120-120: Verify the significant increase in minimum redemption value.
The value for on_close_redeem_single_tickets_value_min has been increased significantly from 90000000000000000 to 2000000000000000000 HOPR. This represents more than a 20x increase in the minimum value required for redeeming single tickets when a channel is closing.
Let's verify if this value aligns with other protocol configurations and documentation:
#!/bin/bash
# Search for documentation or test files that might reference these values
rg -B 2 -A 2 "2000000000000000000" --glob "*.{md,rs,js,ts}"111-116: Consider aligning example values for consistency.
The commented example value for minimum_redeem_ticket_value (90000000000000000 HOPR) differs from the value used in on_close_redeem_single_tickets_value_min (2000000000000000000 HOPR). Consider aligning these values for consistency in the example configuration, as they serve similar purposes in different contexts.
Let's verify if these values are consistent with other configuration files:
logic/strategy/src/auto_redeeming.rs (2)
329-361: Excellent addition of tests for minimum_redeem_ticket_value logic
The test test_auto_redeeming_strategy_redeem_minimum_ticket_amount effectively verifies that tickets below the threshold are not redeemed and those at or above the threshold are redeemed. This enhances confidence in the correctness of the implementation.
104-106: Validate logical conditions for ticket redemption
The updated condition combines redeem_only_aggregated and minimum_redeem_ticket_value checks. Ensure that the parentheses accurately reflect the intended logic and consider adding unit tests for edge cases to confirm the behavior.
Run the following script to review logical conditions and their usage:
✅ Verification successful
The previous ast-grep pattern might have failed due to syntax. Let's search for the method implementation and test cases to verify the logical conditions.
Let's look at both the implementation and test cases to verify the logical conditions and their coverage.
Logical conditions in ticket redemption are correctly implemented
The code and tests confirm that the logical conditions for ticket redemption are properly implemented:
-
The parentheses in the condition
(!self.cfg.redeem_only_aggregated || ack.verified_ticket().is_aggregated()) && ack.verified_ticket().amount.ge(&self.cfg.minimum_redeem_ticket_value)correctly reflect the intended logic:- Either redeem all tickets (when
redeem_only_aggregatedis false) OR the ticket is aggregated - AND the ticket amount meets the minimum value threshold
- Either redeem all tickets (when
-
The test suite thoroughly verifies this logic through:
test_auto_redeeming_strategy_redeem_agg_only: Verifies aggregated-only redemptiontest_auto_redeeming_strategy_redeem_minimum_ticket_amount: Validates minimum amount threshold- Additional edge cases for channel closing scenarios
The implementation is well-tested and the logical conditions are correctly structured.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the usage of logical conditions in 'on_acknowledged_winning_ticket' method.
# Test: Extract the condition and print it for review.
ast-grep --pattern $'fn on_acknowledged_winning_ticket($_) {
if ($_ && $_) {
$$$
}
$$$
}'
Length of output: 100
Script:
#!/bin/bash
# Let's try a different approach to find the method implementation
rg -U "fn on_acknowledged_winning_ticket.*?\{.*?\}" -A 20 --multiline
# Also search for related test cases
rg -l "test.*redeem"
Length of output: 565
Script:
#!/bin/bash
# Let's examine the test file for redeeming
cat tests/test_redeeming.py
# And check the implementation in auto_redeeming.rs
cat logic/strategy/src/auto_redeeming.rs
Length of output: 28829
The following changes were missed in the merge-back PRs from saint-louis: