Improve the tag organization for v3 packet payload format #7203
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
📝 Walkthrough## Walkthrough
The changes refactor the tag system in the transport packet module, replacing the old `Tag` struct and related types with a new `Tag` enum distinguishing reserved and application tags. All related logic, tests, and APIs are updated to use the new model. Test code and session management are adjusted for compatibility, and error handling is improved for tag-related issues.
## Changes
| File(s) | Change Summary |
|-------------------------------------------------------------------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| transport/packet/src/v1.rs | Refactored `Tag` from struct to enum (`Reserved(u64)`, `Application(u64)`), expanded `ReservedTag`, unified conversions, updated serialization, removed obsolete types, added unit tests. |
| transport/packet/src/lib.rs | Stopped re-exporting `CustomTag` and `ResolvedTag` from prelude; only `ApplicationData`, `ReservedTag`, and `Tag` are now re-exported. |
| transport/packet/src/errors.rs | Added `TagError(String)` variant to `PacketError` enum. |
| transport/packet/Cargo.toml | Added `strum` as a workspace dependency. |
| transport/api/src/lib.rs | Renamed `USABLE_TAG_RANGE` to `APPLICATION_TAG_RANGE`, updated range logic, changed session tag handling to use new `Tag` enum, updated `send_message` API and error messages, adjusted imports. |
| transport/session/src/manager.rs | Changed session tag range from `Range<Tag>` to `Range<u64>`, updated related logic, removed `MIN_SESSION_TAG_RANGE_RESERVATION`, used `ReservedTag::range().end`. |
| transport/session/src/initiation.rs | Unified encoding/decoding of `StartProtocol<T>` variants under a fixed reserved tag; added versioning and discriminant bytes in payload; updated tag usage and test assertions accordingly. |
| transport/session/src/errors.rs | Removed `Tag` variant from `TransportSessionError` enum, added `StartProtocolError(String)`. |
| transport/session/src/lib.rs | Removed export of `USABLE_PAYLOAD_CAPACITY_FOR_SESSION`, changed `SESSION_PAYLOAD_SIZE` to use `ApplicationData::PAYLOAD_SIZE`. |
| transport/session/src/types.rs | Generalized `SessionId::new` to accept any `Into<Tag>`, replaced `USABLE_PAYLOAD_CAPACITY_FOR_SESSION` with `ApplicationData::PAYLOAD_SIZE`, fixed deserialization and duplicate field error message. |
| hopr/hopr-lib/src/lib.rs | Removed export of `USABLE_PAYLOAD_CAPACITY_FOR_SESSION` constant. |
| hopr/hopr-lib/tests/session_integration_tests.rs<br>hoprd/rest-api/src/session.rs | Updated `SessionId` and `HoprSessionId` construction in tests to use `u64` directly instead of `Tag`, replaced payload size constants with `ApplicationData::PAYLOAD_SIZE`. |
| transport/probe/src/probe.rs | Changed tag comparisons and construction to use `.into()` for `ReservedTag` to `Tag` conversion, adjusted imports and test code accordingly. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant App as Application
participant HoprTransport
participant Packet as Packet Layer
App->>HoprTransport: send_message(msg, routing, tag: Tag)
HoprTransport->>HoprTransport: Check if tag is reserved (Tag::Reserved)
alt Tag is reserved
HoprTransport-->>App: Error(TagError)
else Tag is application
HoprTransport->>Packet: Create ApplicationData(tag)
Packet-->>HoprTransport: ApplicationData
HoprTransport-->>App: Ok
endPossibly related issues
Possibly related PRs
Suggested reviewers
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the tagging system used across the packet payload format and session management to improve clarity and efficiency. Key changes include:
- Updating tag types and conversions (e.g. replacing Tag with ReservedTag where appropriate) to better enforce reserved vs. application tag ranges.
- Modifying encoding/decoding logic for StartProtocol messages and adjusting related tests.
- Refactoring error handling and API interfaces to reflect the new tag system.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| transport/session/src/manager.rs | Updated tag range type and conversions using ReservedTag and u64. |
| transport/session/src/initiation.rs | Adjusted StartProtocol tag encoding/decoding and added new conversion implementations. |
| transport/session/src/errors.rs | Removed the obsolete Tag error variant. |
| transport/probe/src/probe.rs | Updated tag comparisons to use ReservedTag conversions. |
| transport/packet/src/v1.rs | Overhauled the Tag API by introducing a new enum differentiating Reserved and Application tags. |
| transport/api/src/lib.rs | Adjusted API interfaces to use the new tag ranges and improved error message details. |
| hoprd/rest-api/src/session.rs & hopr-lib tests | Updated tag construction using the new conversion syntax. |
| transport/packet/src/errors.rs & Cargo.toml | Added new error variant for tag errors and included a Strum dependency. |
Comments suppressed due to low confidence (1)
transport/api/src/lib.rs:631
- The error message wording is unclear; consider rephrasing it to something like "Application tag must not be from the reserved range: {:?}, but received {reserved_tag:?}" to improve clarity.
return Err(HoprTransportError::Api(format!(
"Application tag must not from range: {:?}, but was {reserved_tag:?}",
Tag::APPLICATION_TAG_RANGE
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
transport/session/src/initiation.rs (1)
79-79: Consider the future protocol evolution strategyThe TODO comment suggests using a dedicated tag with the entire object as payload. This current approach of embedding version and discriminant bytes provides good flexibility for protocol evolution.
Consider documenting the protocol versioning strategy to help future maintainers understand:
- When to increment the version byte
- How to handle backwards compatibility
- Migration path to the suggested dedicated tag approach
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
hopr/hopr-lib/src/lib.rs(1 hunks)hopr/hopr-lib/tests/session_integration_tests.rs(3 hunks)hoprd/rest-api/src/session.rs(3 hunks)transport/api/src/lib.rs(5 hunks)transport/session/Cargo.toml(1 hunks)transport/session/src/errors.rs(1 hunks)transport/session/src/initiation.rs(8 hunks)transport/session/src/lib.rs(1 hunks)transport/session/src/manager.rs(8 hunks)transport/session/src/types.rs(12 hunks)
✅ Files skipped from review due to trivial changes (2)
- hopr/hopr-lib/src/lib.rs
- transport/session/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (5)
- transport/session/src/errors.rs
- hopr/hopr-lib/tests/session_integration_tests.rs
- hoprd/rest-api/src/session.rs
- transport/api/src/lib.rs
- transport/session/src/manager.rs
🧰 Additional context used
🧠 Learnings (1)
transport/session/src/lib.rs (1)
Learnt from: Teebor-Choka
PR: hoprnet/hoprnet#6393
File: hoprd/rest-api/src/session.rs:91-91
Timestamp: 2024-07-31T10:24:08.332Z
Learning: The current implementation in `hoprd/rest-api/src/session.rs` assumes all sessions are TCP-like due to the missing implementation of UDP behavior. The commented-out code suggests an intention to support different session types based on `capabilities`.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: hoprd / docker
- GitHub Check: hopli / docker
- GitHub Check: Linter
- GitHub Check: tests-smoke-hopli
- GitHub Check: tests-unit
- GitHub Check: tests-unit-nightly
- GitHub Check: zizmor
🔇 Additional comments (14)
transport/session/src/lib.rs (2)
20-20: LGTM: Import addition aligns with payload size refactoringThe addition of
ApplicationDataimport is consistent with the broader refactoring to unify payload size constants across the transport layers.
31-31: LGTM: Const generic usage correctly references ApplicationData payload sizeThe update to use
ApplicationData::PAYLOAD_SIZEas a const generic parameter properly unifies the session payload size with the packet layer definition. This ensures consistency across the transport stack.transport/session/src/types.rs (8)
73-74: LGTM: Generic constructor improves type flexibilityThe change to accept
Into<Tag>instead of a strictTagparameter improves usability by allowing direct construction withu64values while maintaining type safety through the conversion.
141-142: LGTM: Explicit type annotation ensures correct deserializationThe explicit
Tagtype annotation ensures the deserializer correctly handles the new Tag enum structure during sequence access.
151-152: LGTM: Consistent type annotations in map visitorThe explicit type annotations for both
tagandpseudonymvariables improve code clarity and ensure correct deserialization behavior.
157-157: LGTM: Fixed duplicate field error messageGood catch fixing the error message from
"tx_tag"to"tag"to match the actual field name.
295-295: LGTM: Consistent payload size usageThe replacement of the removed constant with
ApplicationData::PAYLOAD_SIZEmaintains consistency with the broader payload size unification effort.
487-491: LGTM: Buffer calculations correctly updatedThe buffer size calculations properly use
ApplicationData::PAYLOAD_SIZEfor chunking logic, maintaining the correct behavior while using the unified constant.
606-606: LGTM: Transfer function buffer size correctly updatedThe
transfer_sessionfunction correctly uses the unified payload size constant for buffer size calculations when segmentation capability is enabled.
668-668: LGTM: Test simplified with generic constructorThe test code correctly leverages the new generic
SessionId::newconstructor by passingu64directly instead of converting toTagfirst.transport/session/src/initiation.rs (4)
96-105: LGTM: Well-structured constants and trait implementationThe implementation correctly defines the fixed protocol tag and version constants. The
Fromtrait implementation properly returns the fixed tag for all variants, which aligns with the unified tagging approach.
117-140: LGTM: Encode method correctly implements versioned protocolThe encode method properly:
- Pre-allocates buffer with appropriate capacity
- Embeds version byte and discriminant for protocol versioning
- Maintains consistent serialization across all variants
- Returns the fixed tag for all messages
This design allows for future protocol evolution while maintaining a single tag identifier.
144-183: LGTM: Comprehensive validation in decode methodThe decode method implements proper validation:
- Checks minimum message length (3 bytes for version + discriminant + data)
- Validates the fixed protocol tag
- Verifies protocol version for backwards compatibility
- Handles unknown discriminants gracefully
- Uses appropriate error types with descriptive messages
The validation logic ensures protocol integrity and provides clear error reporting.
262-263: LGTM: Test assertions correctly updated for fixed tagAll test assertions have been consistently updated to expect the fixed
START_PROTOCOL_MESSAGE_TAGinstead of variant-specific tags. This validates that the unified tagging approach works correctly across all protocol message types.Also applies to: 298-299, 316-317, 331-332, 346-347
Teebor-Choka
commented
May 30, 2025
Teebor-Choka
commented
May 30, 2025
5 tasks
NumberFour8
approved these changes
May 30, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes to the packet structure in the tag section and reserved tag allocation: