-
Notifications
You must be signed in to change notification settings - Fork 81
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
Make thread id optional #378
Conversation
Signed-off-by: Miroslav Kovar <miroslavkovar@protonmail.com>
Signed-off-by: Miroslav Kovar <miroslavkovar@protonmail.com>
Signed-off-by: Miroslav Kovar <miroslavkovar@protonmail.com>
d04aeaf
to
a4fd531
Compare
Codecov Report
@@ Coverage Diff @@
## main #378 +/- ##
=======================================
Coverage 50.45% 50.46%
=======================================
Files 153 153
Lines 11954 11950 -4
Branches 2345 2343 -2
=======================================
- Hits 6031 6030 -1
+ Misses 4252 4250 -2
+ Partials 1671 1670 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Signed-off-by: Miroslav Kovar <miroslavkovar@protonmail.com>
aadc26d
to
02222cd
Compare
@@ -369,13 +368,13 @@ impl SmConnectionInvitee { | |||
Ok(Self { state, ..self }) | |||
} | |||
|
|||
pub fn handle_problem_report(self, problem_report: ProblemReport) -> VcxResult<Self> { | |||
pub fn handle_problem_report(self, _problem_report: ProblemReport) -> VcxResult<Self> { |
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.
I was about to say I strongly oppose not storing problem report after it's received - only to find out we didn't do it previously either and just ignored it in impl From<(InvitedState, ProblemReport)> for InitialState {
:-)
So it's good that this modification pointed that out - I think we should store it, what do you think?
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.
I am not so sure. Maybe we can start storing it when we have a use case for it?
let new_recipient_keys = vec!(new_pairwise_info.pw_vk.clone()); | ||
Response::create() | ||
.set_did(new_pairwise_info.pw_did.to_string()) | ||
.set_service_endpoint(new_service_endpoint) | ||
.set_keys(new_recipient_keys, new_routing_keys) | ||
.ask_for_ack() | ||
.set_thread_id(&thread_id) | ||
.set_thread_id(&request.get_thread_id()) |
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.
Would feel safer to always copy from self.thread_id
, though after verification those values should be the same (except when the verification of the message thread id is forgotten or incorrect like happened above)
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.
Actually, self.thread_id
can't be used here because it is undefined in the scenario where the inviter is creating a connection from received request (the thread id is set after the response is built). There are ways to satisfy your demand to use self.thread_id
, but I don't see the upside of that.
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.
few comments
Signed-off-by: Miroslav Kovar <miroslavkovar@protonmail.com>
902ba85
to
dbf8ce1
Compare
Signed-off-by: Miroslav Kovar miroslavkovar@protonmail.com