Remove single ticket redeem bound in AutoRedeem strategy #6614
Conversation
- Remove `on_close_redeem_single_tickets_value_min` config option in the AutoRedeem strategy. The only limiting factor for ticket redemptions is `minimum_redeem_ticket_value`. - Refactored ticket selector methods to use range bounds instead of strict comparisons. Modified selector criteria for indices, winning probability, and amounts.
|
Warning Rate limit exceeded@NumberFour8 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 39 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant modifications to event handling and ticket selection in the codebase. Key changes include updates to 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
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
logic/strategy/src/lib.rs (1)
106-106: Consider documenting the minimum redemption value.The
minimum_redeem_ticket_valueof 0.09 HOPR seems to be a critical threshold. Consider adding a comment explaining why this specific value was chosen and what implications it has for the network.db/api/src/tickets.rs (3)
37-37: Ensure consistent formatting inDisplayimplementationThe
Displayimplementation forTicketIndexSelectorhandles the newRangevariant, but consider formatting the output for clarity. For example, using standard range notation:-TicketIndexSelector::Range((lb, ub)) => write!(f, "with indices in {lb:?}..{ub:?}"), +TicketIndexSelector::Range((lb, ub)) => write!(f, "with indices in {:?}..{:?}", lb, ub),Alternatively, implement custom formatting for
Bound<u64>to improve readability.
74-74: Includewin_probandamountinDisplayimplementation ofTicketSelectorThe
Displayimplementation currently omits thewin_probandamountfields. Including these fields will provide a more complete representation, which is helpful for debugging and logging.Consider updating the
fmtmethod:impl Display for TicketSelector { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { let out = format!( "ticket selector in {:?} {}{}{}{}{}", self.channel_identifiers, self.index, self.state .map(|state| format!(" in state {state}")) .unwrap_or("".into()), if self.only_aggregated { " only aggregated" } else { "" }, + match &self.win_prob { + (Bound::Unbounded, Bound::Unbounded) => "".to_string(), + bounds => format!(" with win_prob in {:?}", bounds), + }, + match &self.amount { + (Bound::Unbounded, Bound::Unbounded) => "".to_string(), + bounds => format!(" with amount in {:?}", bounds), + }, ); write!(f, "{}", out.trim()) } }
86-87: Reduce code duplication by implementingDefaultforTicketSelectorThe initialization of
win_probandamountto(Bound::Unbounded, Bound::Unbounded)occurs multiple times. Implementing theDefaulttrait can help reduce code duplication and improve maintainability.For example:
impl Default for TicketSelector { fn default() -> Self { Self { channel_identifiers: Vec::new(), index: TicketIndexSelector::None, win_prob: (Bound::Unbounded, Bound::Unbounded), amount: (Bound::Unbounded, Bound::Unbounded), state: None, only_aggregated: false, } } }Then, you can modify the constructors:
pub fn new<T: Into<U256>>(channel_id: Hash, epoch: T) -> Self { - Self { - channel_identifiers: vec![(channel_id, epoch.into())], - index: TicketIndexSelector::None, - win_prob: (Bound::Unbounded, Bound::Unbounded), - amount: (Bound::Unbounded, Bound::Unbounded), - state: None, - only_aggregated: false, - } + Self::default().just_on_channel(channel_id, epoch) }Also applies to: 197-198, 213-214, 226-227
db/sql/src/tickets.rs (1)
93-103: Consider refactoring duplicate bound handling logicThe code for handling lower and upper bounds in
TicketIndexSelector::Rangeis similar to the logic used forwin_prob(lines 115-127) andamount(lines 129-141). To improve maintainability and reduce code duplication, consider abstracting this pattern into a helper function.For example, you could create a helper function:
fn apply_bound_condition<T>( expr: SimpleExpr, column: ticket::Column, bounds: (Bound<T>, Bound<T>), to_bytes: impl Fn(T) -> Vec<u8>, ) -> SimpleExpr { let expr = match bounds.0 { Bound::Included(gte) => expr.and(column.gte(to_bytes(gte))), Bound::Excluded(gt) => expr.and(column.gt(to_bytes(gt))), Bound::Unbounded => expr, }; match bounds.1 { Bound::Included(lte) => expr.and(column.lte(to_bytes(lte))), Bound::Excluded(lt) => expr.and(column.lt(to_bytes(lt))), Bound::Unbounded => expr, } }Then, you can replace the repeated code with calls to this function.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
chain/indexer/src/handlers.rs(2 hunks)db/api/Cargo.toml(1 hunks)db/api/src/tickets.rs(14 hunks)db/sql/Cargo.toml(1 hunks)db/sql/src/tickets.rs(3 hunks)logic/strategy/Cargo.toml(2 hunks)logic/strategy/src/auto_redeeming.rs(10 hunks)logic/strategy/src/lib.rs(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- db/api/Cargo.toml
- db/sql/Cargo.toml
- logic/strategy/Cargo.toml
🧰 Additional context used
📓 Learnings (1)
chain/indexer/src/handlers.rs (2)
Learnt from: NumberFour8
PR: hoprnet/hoprnet#6494
File: chain/indexer/src/handlers.rs:653-681
Timestamp: 2024-10-09T06:16:18.170Z
Learning: The `on_ticket_winning_probability_oracle_event` function will be implemented later once the relevant tooling is added.
Learnt from: NumberFour8
PR: hoprnet/hoprnet#6494
File: chain/indexer/src/handlers.rs:653-681
Timestamp: 2024-09-18T18:34:56.153Z
Learning: The `on_ticket_winning_probability_oracle_event` function will be implemented later once the relevant tooling is added.
🔇 Additional comments (25)
logic/strategy/src/lib.rs (1)
105-107: Verify the impact of enabling redeem_all_on_close.
The addition of redeem_all_on_close: true replaces the removed on_close_redeem_single_tickets_value_min configuration. This change means that all tickets will be redeemed upon channel closure, regardless of their individual values, as long as they meet the minimum_redeem_ticket_value threshold of 0.09 HOPR.
Let's verify if this change is properly handled in the auto redeeming implementation:
✅ Verification successful
Based on the search results, I can now generate the final response as the implementation details are clear:
The redeem_all_on_close flag is properly implemented and safe to use
The implementation in auto_redeeming.rs shows that:
- The flag is correctly used in channel closure logic to determine redemption behavior
- It's properly guarded with direction checks (
direction != ChannelDirection::Incoming) - The change aligns with the removal of
on_close_redeem_single_tickets_value_min - Test cases validate the behavior with the flag set to
true
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of redeem_all_on_close in auto_redeeming.rs
# Expect: Logic that uses this flag for redemption decisions
# Search for redeem_all_on_close usage
rg "redeem_all_on_close" -A 5
Length of output: 2706
db/api/src/tickets.rs (5)
27-28: Range variant added to TicketIndexSelector enhances flexibility
The introduction of the Range((Bound<u64>, Bound<u64>)) variant allows for selecting multiple tickets within a specified index range. This enhancement is well-implemented and aligns with the intended functionality.
54-58: win_prob and amount ranges added to TicketSelector
The addition of win_prob and amount fields as ranges enhances the selector's flexibility for filtering tickets based on winning probability and amount. This change is well-integrated.
137-137: Handle all variants in with_index method
In the with_index method, when replacing the index, consider whether overwriting the existing Range variant directly with Single(index) is the intended behavior. If preservation of existing selections is desired, you may need to handle this differently.
Please ensure that overwriting is acceptable in this context and aligns with the overall design.
152-154: with_index_range method correctly implements range selection
The with_index_range method effectively updates the selector to use the specified index range. This implementation correctly utilizes start_bound() and end_bound() with cloning for ownership.
176-184: Addition of with_winning_probability and with_amount methods enhances flexibility
The new methods with_winning_probability and with_amount allow setting ranges for winning probability and amount, providing greater flexibility in ticket selection. The use of RangeBounds is appropriate and well-executed.
logic/strategy/src/auto_redeeming.rs (11)
6-7: Imports for CriteriaNotSatisfied and SingularStrategy are appropriately added
The imports of CriteriaNotSatisfied and SingularStrategy are necessary for error handling and trait implementation.
12-12: Import of TicketSelector is correctly included
Including TicketSelector enables the selection of tickets based on specific criteria, which is essential for the updated ticket redemption logic.
19-19: Logging imports from tracing crate are properly added
The addition of debug, error, and info log levels enhances the observability of the strategy's operations.
31-32: Helper function just_true is correctly implemented
The just_true function provides a default value of true for boolean configuration fields using serde.
47-57: New configuration fields are properly defined with defaults
The fields redeem_only_aggregated and redeem_all_on_close are added with appropriate default values and serde annotations, enhancing the configurability of the strategy.
126-126: Confirm if only incoming channels should trigger redemption on close
The condition direction != ChannelDirection::Incoming restricts the redemption logic to incoming channels. Please verify if outgoing channels should also be considered for ticket redemption when closing.
138-140: TicketSelector is effectively used for ticket filtering
The TicketSelector is configured to select untouched tickets with amounts greater than or equal to the minimum redeem ticket value, ensuring only eligible tickets are redeemed.
154-154: Error handling in redemption submission is properly logged
Errors encountered during ticket redemption are logged using error!, which is important for diagnosing issues during execution.
400-401: Test configuration accurately reflects the new settings
The test sets redeem_all_on_close to true and defines a minimum_redeem_ticket_value, which aligns with the updated strategy behavior.
444-446: Test cases include various configurations to ensure robustness
By setting different values for redeem_only_aggregated, minimum_redeem_ticket_value, and redeem_all_on_close, the tests cover multiple scenarios to validate the strategy's correctness.
Line range hint 464-520: Tests effectively validate ticket redemption logic on channel close
The test test_auto_redeeming_strategy_should_not_redeem_unworthy_tickets_on_close checks that only tickets meeting the minimum value are redeemed when a channel closes, ensuring the strategy behaves as expected.
chain/indexer/src/handlers.rs (2)
419-419: Verify correctness of ticket index range selection
The change from with_index_lt(ticket_redeemed.new_ticket_index) to with_index_range(..ticket_redeemed.new_ticket_index) alters how tickets are selected for marking as neglected. Ensure that this change correctly includes all tickets with indices less than ticket_redeemed.new_ticket_index and does not introduce any off-by-one errors or unintended inclusions.
704-704: Confirm accurate ticket selection based on winning probability range
Updating from with_winning_probability_lt(encoded_new) to with_winning_probability(..encoded_new) modifies the selection of tickets to be rejected when the minimum winning probability increases. Please verify that this change correctly identifies all tickets with winning probabilities less than the new minimum and does not affect tickets that should remain valid.
db/sql/src/tickets.rs (6)
5-5: Importing Add and Bound for range-based comparisons
The addition of Add and Bound from std::ops is appropriate for implementing range-based selectors and calculations.
93-103: Correct implementation of range-based index selector
The handling of range bounds for ticket indices using Bound types is correctly implemented, and the SQL conditions are appropriately constructed.
115-127: Duplicate bound handling logic for win_prob
The bound handling logic for win_prob duplicates the pattern used in TicketIndexSelector::Range. Refer to the earlier refactoring suggestion to abstract this logic.
115-127: Accurate application of win_prob bounds
The lower and upper bounds for win_prob are correctly applied using Bound types, ensuring proper filtering in the SQL query.
129-141: Duplicate bound handling logic for amount
The bound handling logic for amount mirrors that of TicketIndexSelector::Range. Consider refactoring as previously suggested to reduce code duplication.
129-141: Correct implementation of amount bounds
The code accurately applies the lower and upper bounds for amount using Bound types, which correctly filters the tickets based on their amounts.
Updated the `TicketSelector` `fmt` implementation to include optional bounds for winning probability and amount in the output. This provides more detailed information for debugging and logging purposes.
Remove
on_close_redeem_single_tickets_value_minconfig option in the AutoRedeem strategy. The only limiting factor for ticket redemptions isminimum_redeem_ticket_value.Refactored ticket selector methods to use range bounds instead of strict comparisons. Modified selector criteria for indices, winning probability, and amounts.