request_id attribute for generic response
#42
Conversation
This reverts commit 4d6f489.
kyle-tennison
requested review from
Tjemmmic and
tbraun96
and removed request for
Tjemmmic
kyle-tennison
changed the title
ResponseId trait for responsesResponseId attribute for generic response
kyle-tennison
changed the title
ResponseId attribute for generic responseresponse_id attribute for generic response
kyle-tennison
changed the title
response_id attribute for generic responserequest_id attribute for generic response
Tjemmmic
left a comment
Tjemmmic
left a comment
There was a problem hiding this comment.
Looks good! Being able to get the request_id for all enum variants is a great feature
| } | ||
| } | ||
|
|
||
| impl InternalServiceResponse { |
There was a problem hiding this comment.
I think you could use a proc macro to simplify this. Take a look at the answer on this.
| pub cid: u64, | ||
| pub peer_cid: u64, | ||
| pub metadata: VirtualObjectMetadata, | ||
| pub request_id: Option<Uuid>, |
There was a problem hiding this comment.
It looks like there are a few occurrences in tests that didn't get updated to reflect the addition of request_id
| pub cid: u64, | ||
| pub peer_cid: Option<u64>, | ||
| pub status: ObjectTransferStatus, | ||
| pub request_id: Option<Uuid>, |
There was a problem hiding this comment.
Potentially the same as with the other struct. Take a look at the clippy workflow to see the test it shows an error for.
| cid: implicated_cid, | ||
| peer_cid, | ||
| status: status_message, | ||
| request_id: None, |
There was a problem hiding this comment.
We would ideally want to make this the same as the request_id as the initial file transfer request that initiated this.
| cid: implicated_cid, | ||
| peer_cid, | ||
| metadata, | ||
| request_id: None, |
| let response = | ||
| InternalServiceResponse::ServiceConnectionAccepted(ServiceConnectionAccepted); | ||
| InternalServiceResponse::ServiceConnectionAccepted(ServiceConnectionAccepted { | ||
| request_id: None, |
There was a problem hiding this comment.
Ideally, this request_id matches that of the initiated connection so we know which request this belongs to
|
This commit already has this attribute for responses and requests: f6cc31b Ignore the diff for I just posted today and didn't see this PR, though, I'd just copy and paste the diff into your PR here, add some tests like usual inside the |
Depends on Avarok-Cybersecurity/Citadel-Protocol#218
This allows us to use
.request_idon any incoming response, which makes it much easier (and quicker) to access response ids from incoming messages.Let me know if there is any way I can restructure this.