-
Notifications
You must be signed in to change notification settings - Fork 44
feat(ton-client-util): redesign Routed trait #1302
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR redesigns the Routed trait to return a BlockAvailability enum instead of separate boolean flags, enabling richer routing decisions.
- Refactors
ShardBounds::contains_seqnoandcontains_ltto returnBlockAvailability. - Replaces
Registry::contains/contains_not_availablewith a unifiedRegistry::available, merging per-shard availability and adding unit tests. - Updates all
Routedimplementations (incursor/client,tracked_client, androute.rs) and defines the newBlockAvailabilityenum inrouter/mod.rs.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tonlibjson-client/src/cursor/shard_bounds.rs | Refactored contains_seqno & contains_lt to return BlockAvailability |
| tonlibjson-client/src/cursor/registry.rs | Replaced contains* with available(), introduced merge logic, added tests |
| tonlibjson-client/src/cursor/client.rs | Updated Routed impl to call available() |
| ton-liteserver-client/src/tracked_client.rs | Updated Routed impl to return BlockAvailability |
| ton-client-util/src/router/route.rs | Switched route filtering to use the new availability enum |
| ton-client-util/src/router/mod.rs | Added BlockAvailability enum and updated the Routed trait |
Comments suppressed due to low confidence (3)
tonlibjson-client/src/cursor/shard_bounds.rs:69
- [nitpick] The method name
contains_seqnonow returns aBlockAvailabilityenum rather than a boolean. Consider renaming it toavailability_for_seqnoorget_seqno_availabilityfor clearer intent.
pub fn contains_seqno(&self, seqno: Seqno) -> BlockAvailability {
tonlibjson-client/src/cursor/registry.rs:151
- There are unit tests for the
contains_ltpath but none for thecontains_seqno/available–Seqnobranch. Consider adding tests covering available, not available, and unknown cases for sequence numbers.
bounds.contains_seqno(*seqno)
ton-client-util/src/router/mod.rs:18
- [nitpick] The new
BlockAvailabilityenum lacks documentation comments. Adding a brief docstring for each variant (e.g., whatNotPresentvsUnknownmeans) will help future maintainers.
pub enum BlockAvailability {
| } | ||
|
|
||
| impl<S> Routed for TrackedClient<S> { | ||
| fn available(&self, chain: &i32, criteria: &BlockCriteria) -> BlockAvailability { |
Copilot
AI
Jul 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation only returns Available or NotPresent. To fully support the BlockAvailability enum, handle cases where blocks are known but missing (NotAvailable) and when no information is available (Unknown).
| if matches!(result, BlockAvailability::Available) { | ||
| return BlockAvailability::Available; | ||
| } | ||
|
|
||
| let availability = bounds.contains_lt(*lt); | ||
|
|
||
| if matches!(result, BlockAvailability::NotAvailable) { | ||
| return match availability { | ||
| BlockAvailability::Available => BlockAvailability::Available, | ||
| _ => BlockAvailability::NotAvailable, | ||
| }; | ||
| } | ||
|
|
||
| if matches!(result, BlockAvailability::Unknown) { | ||
| return match availability { | ||
| BlockAvailability::Available => BlockAvailability::Available, | ||
| BlockAvailability::NotAvailable => { | ||
| BlockAvailability::NotAvailable | ||
| } | ||
| _ => BlockAvailability::Unknown, | ||
| }; | ||
| } | ||
|
|
||
| availability |
Copilot
AI
Jul 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The fold logic for merging BlockAvailability values is quite dense. Consider extracting a helper function (e.g., merge_availability) to improve readability and reduce nested match blocks.
| if matches!(result, BlockAvailability::Available) { | |
| return BlockAvailability::Available; | |
| } | |
| let availability = bounds.contains_lt(*lt); | |
| if matches!(result, BlockAvailability::NotAvailable) { | |
| return match availability { | |
| BlockAvailability::Available => BlockAvailability::Available, | |
| _ => BlockAvailability::NotAvailable, | |
| }; | |
| } | |
| if matches!(result, BlockAvailability::Unknown) { | |
| return match availability { | |
| BlockAvailability::Available => BlockAvailability::Available, | |
| BlockAvailability::NotAvailable => { | |
| BlockAvailability::NotAvailable | |
| } | |
| _ => BlockAvailability::Unknown, | |
| }; | |
| } | |
| availability | |
| merge_availability(result, bounds.contains_lt(*lt)) |
No description provided.