Page MenuHomePhabricator

[Tunnelbroker] add MessageToDeviceRequestStatus message
ClosedPublic

Authored by kamil on Oct 12 2023, 5:10 AM.
Tags
None
Referenced Files
F2903174: D9463.diff
Sat, Oct 5, 9:21 PM
Unknown Object (File)
Wed, Oct 2, 1:06 PM
Unknown Object (File)
Wed, Oct 2, 1:06 PM
Unknown Object (File)
Wed, Oct 2, 1:06 PM
Unknown Object (File)
Wed, Oct 2, 6:01 AM
Unknown Object (File)
Fri, Sep 13, 5:35 AM
Unknown Object (File)
Sat, Sep 7, 1:50 AM
Unknown Object (File)
Aug 30 2024, 1:56 AM
Subscribers

Details

Summary

Issue: ENG-5149.

Protocol diagram: notion doc.

Response which client should receive after sending MessageToDeviceRequest.

Depends on D9462

Test Plan

Run tests

Diff Detail

Repository
rCOMM Comm
Branch
tb-work-3
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

kamil held this revision as a draft.
kamil published this revision for review.Oct 12 2023, 8:15 AM
shared/tunnelbroker_messages/src/messages/send_confirmation.rs
1–2 ↗(On Diff #31961)

You can use this syntax to apply doc comments to the item they are in. So in this case this would be applied to the send_confirmation module. This is useful for module-level documentation.

6–14 ↗(On Diff #31961)

This should probably be moved to be above MessageSentStatus.
You could also move the variant specific comments above the variant definition e.g.

enum MessageSentStatus {
  /// Message with id was processed by Tunnelbroker
  Success(String)
  ...
}
45 ↗(On Diff #31961)

Shouldn't this be SendConfirmation?

shared/tunnelbroker_messages/src/messages/send_confirmation.rs
33–36 ↗(On Diff #31961)

Nit: "confirmation" sounds like the message already succeeded but this also contains message send failures (not entirely happy with my name suggestion too tho)

address review

shared/tunnelbroker_messages/src/messages/send_confirmation.rs
1–2 ↗(On Diff #31961)

I'll do it in separate diff and apply this comment to all files with messages

45 ↗(On Diff #31961)

oh right, I probably messed something up while rebasing, thanks

kamil retitled this revision from [Tunnelbroker] add SendConfirmation message to [Tunnelbroker] add MessageToDeviceRequestStatus message.Oct 18 2023, 1:39 AM
This revision is now accepted and ready to land.Oct 18 2023, 2:56 AM
shared/tunnelbroker_messages/src/messages/send_confirmation.rs
1–2 ↗(On Diff #31961)