Page MenuHomePhabricator

[Tunnelbroker] add MessageToDeviceRequestStatus message
ClosedPublic

Authored by kamil on Oct 12 2023, 5:10 AM.
Tags
None
Referenced Files
F3521327: D9463.diff
Mon, Dec 23, 3:00 AM
Unknown Object (File)
Sat, Dec 14, 6:44 PM
Unknown Object (File)
Sat, Dec 14, 6:44 PM
Unknown Object (File)
Sat, Dec 14, 6:44 PM
Unknown Object (File)
Sat, Dec 14, 6:44 PM
Unknown Object (File)
Sat, Dec 14, 6:38 PM
Unknown Object (File)
Fri, Dec 6, 12:21 PM
Unknown Object (File)
Fri, Nov 29, 3:08 PM
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)