feat(mediator): support for pickup protocol processing #1029

nain-F49FF806 · 2023-10-26T12:24:25Z

No description provided.

codecov-commenter · 2023-10-26T13:29:05Z

Codecov Report

Merging #1029 (2470758) into main (63590d1) will not change coverage.
The diff coverage is n/a.

❗ Current head 2470758 differs from pull request most recent head 4b50d89. Consider uploading reports for the commit 4b50d89 to get more accurate results

@@          Coverage Diff          @@
##            main   #1029   +/-   ##
=====================================
  Coverage   0.05%   0.05%           
=====================================
  Files        384     384           
  Lines      21249   21249           
  Branches    3835    3835           
=====================================
  Hits          12      12           
  Misses     21236   21236           
  Partials       1       1

Flag	Coverage Δ
unittests-aries-vcx	`0.05% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
messages/src/msg_fields/protocols/pickup/mod.rs	`0.00% <ø> (ø)`

nain-F49FF806 · 2023-10-26T13:32:35Z

todo: tests

Done.

agents/rust/mediator/mediation/src/router.rs

agents/rust/mediator/mediation/src/routes/pickup.rs

nain-F49FF806 · 2023-10-27T12:34:00Z

Note: This PR depends on #1028, hence the WIP tag.
Clearer diff can be found at #1031

agents/rust/mediator/mediation/src/didcomm_types.rs

Patrik-Stas · 2023-10-31T12:48:55Z

agents/rust/mediator/mediation/src/routes/pickup.rs

            StatusCode::OK,
-            handle_pickup_status_req(status_request, storage).await,
+            handle_pickup_status_req(&status_request.content, storage, auth_pubkey).await,
        ),
        // Why is client sending us status? That's server's job.


Rather than writing a comment, I think we could improve the code to be bit more self documenting - we could return a message string to the return type, which could in this case be something like "Unexpected message of type <the message type>"

Patrik-Stas · 2023-10-31T13:00:32Z

agents/rust/mediator/mediation/src/routes/pickup.rs

            StatusCode::BAD_REQUEST,
-            handle_pickup_type_not_implemented().await,
+            handle_pickup_default_status(storage, auth_pubkey).await,


With respect to your comment above, what is this handling we call here? We should probably really just return BAD_REQUEST http response, but I don't see what any handle_* function is actually called upon receiving invalid request.

On slightly other note, from mediator client perspective, even more graceful response than getting 400 HTTP error, would be to actually return 200 with properly encrypted ProblemReport reporting that a message they submitted was unexpected.

Currently, I am responding with a simple Pickup Status message on receipt of any non-request type pickup message that the client sends.

Here for example, if client sends mediator a status message (they should be sending status-request), then we respond with the correct status message, assuming that is what the client actually, wants.

Agreed, problem report feels more elegant. For the time being, I was having the handler functions map from Pickup (receipt) to Pickup (response). To keep everything within the same protocol at protocol handler level.

Ideally, these protocol handler functions should return a Result so the error case can be converted to Problem Report messages, like you mentioned elsewhere. In combination with your MediatorError suggestion, for example.

Is it okay if I do this as part of another PR, since this would be a useful change for all protocols and not just pickup?

Here for example, if client sends mediator a status message (they should be sending status-request), then we respond with the correct status message, assuming that is what the client actually, wants.

You are too kind to these buggy clients 😆

do this as part of another PR

Fair enough, let's leave this for later

Patrik-Stas · 2023-10-31T13:02:33Z

agents/rust/mediator/mediation/src/routes/pickup.rs

-) -> (StatusCode, Json<PickupMsgEnum>) {
+    Json(pickup_message): Json<Pickup>,
+    auth_pubkey: &str,
+) -> (StatusCode, Json<Pickup>) {


I could imagine that this function could return custom Result type (using custom MediatorError type).
In that model you would typically return Ok(response) or Error(<some_err_info_struct/enum>)

Patrik-Stas · 2023-10-31T13:48:43Z

agents/rust/mediator/src/didcomm_handlers/mod.rs

@@ -35,7 +34,7 @@ pub async fn handle_aries<T: BaseWallet + 'static, P: MediatorPersistence>(
    log::info!("processing message {:?}", &didcomm_msg);
    let unpacked = agent.unpack_didcomm(&didcomm_msg).await.unwrap();
    let aries_message: GeneralAriesMessage =
-        serde_json::from_str(&unpacked.message).expect("Decoding unpacked message as AriesMessage");
+        serde_json::from_str(&unpacked.message).map_err(|e| e.to_string())?;


As mentioned elsewhere, consider introducing custom MediatorError error type, along with something like MediatorResult<T> as alias to Result<T, MediatorError>. See how we do the same in many of our other crates.

Adding custom errors is sounds like a separate PR though :-)

Patrik-Stas · 2023-10-31T13:52:02Z

agents/rust/mediator/src/didcomm_handlers/mod.rs

@@ -47,13 +46,21 @@ pub async fn handle_aries<T: BaseWallet + 'static, P: MediatorPersistence>(
            handle_routing_forward(agent.clone(), forward).await?;
            return Ok(Json(json!({})));
        } else {
-            let (account_name, our_signing_key, their_diddoc) =
+            // Auth known VerKey then process account related messages


this seems like a "todo", consider prefixing the comment text with // todo: , it tends to have different highlighting in IDEs

This one is just a comment. The auth_and_get_details method does the auth.
Just had the comment to explain the latter flow.

Thanks for tip about // todo:

agents/rust/mediator/src/didcomm_handlers/mod.rs

agents/rust/mediator/tests/mediator-protocol-pickup.rs

Signed-off-by: Naian <[email protected]>

Messages can be serialized after wrapping in surrounding AriesMessage enum Signed-off-by: Naian <[email protected]>

and other .expect -> errors Signed-off-by: Naian <[email protected]>

Signed-off-by: Naian <[email protected]>

Patrik-Stas

Some stuff has been addressed, some improvements we agreed to do in separate PRs, some of my comments you acknowledged, but I am not sure if you want to address them in this PR or separately. We can merge this but please keep track of the things I've noted. You already have stuff planned out so I don't want to hold you back on a PR when your other upcoming work is already built on this one.

Let me know if we shall merge

nain-F49FF806 changed the title ~~feat(mediator): support for pickup protocol~~ feat(mediator): support for pickup protocol processing Oct 26, 2023

nain-F49FF806 changed the title ~~feat(mediator): support for pickup protocol processing~~ WIP: feat(mediator): support for pickup protocol processing Oct 26, 2023

Patrik-Stas reviewed Oct 26, 2023

View reviewed changes

agents/rust/mediator/mediation/src/router.rs Show resolved Hide resolved

agents/rust/mediator/mediation/src/routes/pickup.rs Show resolved Hide resolved

agents/rust/mediator/mediation/src/routes/pickup.rs Show resolved Hide resolved

nain-F49FF806 force-pushed the nain/mediator/feat/pickup branch from 5ed2f8c to 55c259e Compare October 27, 2023 11:16

nain-F49FF806 force-pushed the nain/mediator/feat/pickup branch 2 times, most recently from b3a07df to cd73500 Compare October 30, 2023 12:48

nain-F49FF806 changed the title ~~WIP: feat(mediator): support for pickup protocol processing~~ feat(mediator): support for pickup protocol processing Oct 30, 2023

nain-F49FF806 mentioned this pull request Oct 30, 2023

Refactor and strip down Aries transport abstraction and further code trimming #1040

Merged

nain-F49FF806 force-pushed the nain/mediator/feat/pickup branch 2 times, most recently from 2470758 to 4b50d89 Compare October 31, 2023 13:33

Patrik-Stas requested changes Oct 31, 2023

View reviewed changes

nain-F49FF806 force-pushed the nain/mediator/feat/pickup branch from 4b50d89 to b26a101 Compare November 1, 2023 12:55

nain-F49FF806 added 5 commits November 1, 2023 14:02

feat(mediator): support for pickup protocol

b351769

Signed-off-by: Naian <[email protected]>

feat(mediator): test for pickup

8abc3e2

Signed-off-by: Naian <[email protected]>

refactor: remove unnecessary change

9ab570c

Messages can be serialized after wrapping in surrounding AriesMessage enum Signed-off-by: Naian <[email protected]>

fix: do not revalidate sender key after auth.

ef78c3a

and other .expect -> errors Signed-off-by: Naian <[email protected]>

refactor: remove dead pickup code : structs

b26a101

Signed-off-by: Naian <[email protected]>

Patrik-Stas approved these changes Nov 1, 2023

View reviewed changes

Patrik-Stas merged commit ad26ecf into main Nov 2, 2023
28 checks passed

Patrik-Stas deleted the nain/mediator/feat/pickup branch November 2, 2023 09:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(mediator): support for pickup protocol processing #1029

feat(mediator): support for pickup protocol processing #1029

nain-F49FF806 commented Oct 26, 2023

codecov-commenter commented Oct 26, 2023 •

edited

Loading

nain-F49FF806 commented Oct 26, 2023 •

edited

Loading

nain-F49FF806 commented Oct 27, 2023 •

edited

Loading

Patrik-Stas Oct 31, 2023

Patrik-Stas Oct 31, 2023

nain-F49FF806 Nov 1, 2023 •

edited

Loading

Patrik-Stas Nov 1, 2023 •

edited

Loading

Patrik-Stas Oct 31, 2023

Patrik-Stas Oct 31, 2023

Patrik-Stas Nov 1, 2023

Patrik-Stas Oct 31, 2023

nain-F49FF806 Nov 1, 2023

Patrik-Stas left a comment •

edited

Loading

feat(mediator): support for pickup protocol processing #1029

feat(mediator): support for pickup protocol processing #1029

Conversation

nain-F49FF806 commented Oct 26, 2023

codecov-commenter commented Oct 26, 2023 • edited Loading

Codecov Report

nain-F49FF806 commented Oct 26, 2023 • edited Loading

nain-F49FF806 commented Oct 27, 2023 • edited Loading

Patrik-Stas Oct 31, 2023

Choose a reason for hiding this comment

Patrik-Stas Oct 31, 2023

Choose a reason for hiding this comment

nain-F49FF806 Nov 1, 2023 • edited Loading

Choose a reason for hiding this comment

Patrik-Stas Nov 1, 2023 • edited Loading

Choose a reason for hiding this comment

Patrik-Stas Oct 31, 2023

Choose a reason for hiding this comment

Patrik-Stas Oct 31, 2023

Choose a reason for hiding this comment

Patrik-Stas Nov 1, 2023

Choose a reason for hiding this comment

Patrik-Stas Oct 31, 2023

Choose a reason for hiding this comment

nain-F49FF806 Nov 1, 2023

Choose a reason for hiding this comment

Patrik-Stas left a comment • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Oct 26, 2023 •

edited

Loading

nain-F49FF806 commented Oct 26, 2023 •

edited

Loading

nain-F49FF806 commented Oct 27, 2023 •

edited

Loading

nain-F49FF806 Nov 1, 2023 •

edited

Loading

Patrik-Stas Nov 1, 2023 •

edited

Loading

Patrik-Stas left a comment •

edited

Loading