-
Notifications
You must be signed in to change notification settings - Fork 91
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
DEPRECATE in 3.0 Add backwards compatible API to 2.1.x #6600
base: master
Are you sure you want to change the base?
Conversation
This reverts commit b70b4a9.
📝 WalkthroughWalkthroughThe pull request introduces significant changes to the REST API request structures in three files: Changes
Sequence DiagramsequenceDiagram
participant Client
participant RestAPI
participant HoprNode
Client->>RestAPI: Send request with mandatory destination
RestAPI->>HoprNode: Process request with validated destination
HoprNode-->>RestAPI: Perform requested operation
RestAPI-->>Client: Return response
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
hoprd/rest-api/src/alias.rs
(2 hunks)hoprd/rest-api/src/channels.rs
(3 hunks)hoprd/rest-api/src/messages.rs
(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: hoprd / docker
- GitHub Check: hopli / docker
- GitHub Check: Docs / Rust docs
- GitHub Check: tests-smoke-websocket
- GitHub Check: tests-unit-nightly
- GitHub Check: tests-unit
- GitHub Check: tests-smart-contracts
- GitHub Check: Linter
🔇 Additional comments (9)
hoprd/rest-api/src/alias.rs (2)
105-105
: LGTM! Code is now more straightforward.The simplified error handling through direct use of
args.destination
improves code readability while maintaining proper error handling.
39-41
: LGTM! Verify API deprecation notice is documented.The change to make
destination
mandatory and remove the redundantpeer_id
field simplifies the API. This is a good change that reduces complexity and potential errors.Let's verify the API deprecation notice is properly documented:
✅ Verification successful
API deprecation notice is properly documented ✅
All alias endpoints are consistently marked with deprecation notices indicating removal in v3.0.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the API deprecation notice is consistently documented # Test: Search for deprecation notices in API documentation rg -A 2 "deprecated.*v3\.0" hoprd/rest-api/src/alias.rsLength of output: 662
hoprd/rest-api/src/channels.rs (3)
183-183
: LGTM! Improved error handling.The use of
and_then
instead ofmap
provides better error handling in the async context.
306-306
: LGTM! Consistent with API simplification pattern.The simplified error handling through direct use of
open_req.destination
maintains consistency with changes across the codebase.
252-254
: LGTM! Verify API deprecation notice is documented.The change to make
destination
mandatory and remove the redundantpeer_address
field aligns with the API simplification across the codebase.Let's verify the API deprecation notice is properly documented:
hoprd/rest-api/src/messages.rs (4)
149-149
: LGTM! Consistent error handling improvement.The simplified error handling through direct use of
args.destination
maintains consistency with changes across the codebase.
362-362
: LGTM! Consistent with websocket handling.The simplified error handling through direct use of
msg.destination
maintains consistency in the websocket message handling.
731-731
: LGTM! Test updated to match API changes.The test has been properly updated to reflect the non-optional destination field.
758-758
: LGTM! Test updated to match API changes.The test has been properly updated to reflect the non-optional destination field.
#[serde_as(as = "DisplayFromStr")] | ||
#[schema(value_type = String)] | ||
peer_id: Option<hopr_lib::PeerId>, | ||
destination: PeerOrAddress, |
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.
💡 Codebase verification
Breaking changes need documentation
The changes to SendMessageBodyRequest
struct are breaking changes that need to be properly documented:
- Removing the
Option
wrapper fromdestination
field makes it mandatory - Removing the
peer_id
field affects existing API consumers
Please add:
- Deprecation notices in the API documentation
- Version upgrade notes in README.md
- Migration guide for API consumers
🔗 Analysis chain
LGTM! Verify API deprecation notice is documented.
The change to make destination
mandatory and remove the redundant peer_id
field completes the consistent API simplification across all files.
Let's verify the API deprecation notice is properly documented:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the API deprecation notice is consistently documented
# Test: Search for deprecation notices in API documentation
rg -A 2 "deprecated.*v3\.0" hoprd/rest-api/src/messages.rs
Length of output: 60
Script:
#!/bin/bash
# Search for any deprecation notices or version annotations
rg -i "(deprecated|breaking|version|v3\.0)" hoprd/rest-api/src/messages.rs
# Check for API documentation comments
rg "^///|^//!" hoprd/rest-api/src/messages.rs
# Look for README or documentation files in rest-api directory
fd -e md -e txt . hoprd/rest-api/
Length of output: 1874
Script:
#!/bin/bash
# Check README for version information and breaking changes
cat hoprd/rest-api/README.md
# Look for version-related changes in git history
git log -n 5 --oneline hoprd/rest-api/src/messages.rs
Length of output: 33
This reverts commit b70b4a9.