Skip to content
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

draft: Demo for aries vcx #952

Closed
wants to merge 7 commits into from
Closed

draft: Demo for aries vcx #952

wants to merge 7 commits into from

Conversation

Patrik-Stas
Copy link
Contributor

@Patrik-Stas Patrik-Stas commented Aug 22, 2023

Partially addressing #870

  • Added Alice & Faber demo to establish connection
  • Modified simple_message_relay to enable its consumption also as library, rather than executable only
  • Extended simple_message_relay with mpsc channels which forward any incoming message to mpsc receiver. This is nice because in the demo, we don't need to poll HTTP API for new messages - but rather instantly get notified in-memory.

Having to interact with aries-vcx from scratch from new application, some improvements I've identified:

Note

This can be thought of as temporary solution to demonstrate the APIs and usage. And while far from ideal yet (todos left, issues created), it's a lot easier to understand than pointing someone to integration tests today. Main issue of integration tests is that they are all couploed with vcxagency-node mediator, but also other issues.
After integration tests are in better shape after:

I believe that integration tests could be in condition to demonstrate the usage (and surface outstanding API issues) equally good as this demo.
But for the time being, this demo is a lot simpler than the existing integration tests.

Demo

Running the demo produces following logs
image

@codecov-commenter
Copy link

codecov-commenter commented Aug 22, 2023

Codecov Report

Merging #952 (ab545cc) into main (4e65aec) will increase coverage by 30.32%.
The diff coverage is 0.00%.

❗ Current head ab545cc differs from pull request most recent head 58f03a7. Consider uploading reports for the commit 58f03a7 to get more accurate results

@@            Coverage Diff             @@
##            main     #952       +/-   ##
==========================================
+ Coverage   0.05%   30.38%   +30.32%     
==========================================
  Files        384      422       +38     
  Lines      21249    26230     +4981     
  Branches    3835     5084     +1249     
==========================================
+ Hits          12     7970     +7958     
+ Misses     21236    16041     -5195     
- Partials       1     2219     +2218     
Flag Coverage Δ
unittests-aries-vcx 30.38% <0.00%> (+30.32%) ⬆️

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

Files Coverage Δ
aries_vcx/src/protocols/connection/generic/mod.rs 9.16% <ø> (+9.16%) ⬆️
aries_vcx/src/protocols/connection/invitee/mod.rs 78.94% <ø> (+78.94%) ⬆️
aries_vcx/src/protocols/connection/mod.rs 7.89% <ø> (+7.89%) ⬆️
aries_vcx/src/handlers/out_of_band/sender.rs 39.74% <0.00%> (+39.74%) ⬆️

... and 349 files with indirect coverage changes

@Patrik-Stas
Copy link
Contributor Author

@gmulhearn as a creator of original issue, do you agree this being in a single file? I think your vision to have 2 files, 1 for alice, one for faber.
I am fine with that too. Just started in one file to make this simple for me, but actually might be a lot more readable for someone if we split the roles.

@mirgee
Copy link
Contributor

mirgee commented Aug 24, 2023

After integration tests are in better shape after:

I believe that integration tests could be in condition to demonstrate the usage (and surface outstanding API issues) equally good as this demo.
But for the time being, this demo is a lot simpler than the existing integration tests.

Will we remove the demo after the work mentioned is completed, i.e. integration tests are in such a condition that they fulfill the current purpose of the demo, i.e. to demonstrate aries-vcx usage, as discussed in speaking?

@Patrik-Stas
Copy link
Contributor Author

After integration tests are in better shape after:

I believe that integration tests could be in condition to demonstrate the usage (and surface outstanding API issues) equally good as this demo.
But for the time being, this demo is a lot simpler than the existing integration tests.

Will we remove the demo after the work mentioned is completed, i.e. integration tests are in such a condition that they fulfill the current purpose of the demo, i.e. to demonstrate aries-vcx usage, as discussed in speaking?

I'd be in favor, there surely will be a lot of duplication between this and integration tests after refactoring them

@Patrik-Stas Patrik-Stas force-pushed the demo/aries-vcx branch 2 times, most recently from 5dde0a7 to afcb250 Compare August 29, 2023 22:29
Base automatically changed from connection/ioless to main August 31, 2023 15:35
@Patrik-Stas Patrik-Stas force-pushed the demo/aries-vcx branch 2 times, most recently from f1a5366 to 7933912 Compare August 31, 2023 16:49
@Patrik-Stas Patrik-Stas marked this pull request as ready for review September 4, 2023 12:13
demo/src/http_client.rs Outdated Show resolved Hide resolved
demo/src/main.rs Outdated Show resolved Hide resolved
demo/src/main.rs Outdated Show resolved Hide resolved
@gmulhearn
Copy link
Contributor

nit; log colors are printing a little funky for me, is this expected?

(ran with RUST_LOG="info" cargo run)

image

}

pub async fn prepare_invitation(&self) -> (Invitation, PairwiseInfo) {
let (faber_pw_did, faber_invite_key) = self.wallet.create_and_store_my_did(None, None).await.unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit; not sure if you were looking for it , but you could also call PairwiseInfo::create(wallet) to avoid construction of PairwiseInfo at the end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like us to actually get rid of the whole PairwiseInfo thing, which why I am not using it, rather creating it as necessity at the point where it's needed.
The "did" portion of PairwiseInfo is generally useless

gmulhearn
gmulhearn previously approved these changes Sep 6, 2023
Copy link
Contributor

@gmulhearn gmulhearn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really awesome, nice work. preemptive approval from me; just some comments to consider. (assuming others are happy too ocourse)

@gmulhearn
Copy link
Contributor

send_message_2 bugs me a bit too.. maybe something like send_message_as_inviter send_message_as_invitee? not sure

demo/src/demo_agent.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@bobozaur bobozaur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly left remarks.

I think the message sending could be improved, but that would require quite a bit of a refactor. The duplicate methods should be easy to remove though, which is why I'd request some changes.

demo/src/demo_agent.rs Outdated Show resolved Hide resolved
tools/simple_message_relay/src/mpsc_registry.rs Outdated Show resolved Hide resolved
Comment on lines 5 to 9
#[derive(Clone)]
pub struct MpscRegistry<T: Debug> {
sending: Arc<RwLock<HashMap<String, mpsc::Sender<T>>>>,
receiving: Arc<RwLock<HashMap<String, mpsc::Receiver<T>>>>,
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't the previous implementation similar to this in terms of communication? Maybe I'm missing something but I don't get why the shared ownership and interior mutability is needed.

Each agent should be in charge of its own connections (senders to other agents and receivers from other agents) and practically own them exclusively. Or is there some reason why this can't be achieved?

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
@Patrik-Stas Patrik-Stas changed the title Demo for aries vcx Draft: Demo for aries vcx Nov 26, 2023
@Patrik-Stas Patrik-Stas marked this pull request as draft November 26, 2023 12:08
@Patrik-Stas Patrik-Stas changed the title Draft: Demo for aries vcx draft: Demo for aries vcx Nov 26, 2023
@Patrik-Stas
Copy link
Contributor Author

Archiving this. Instead, a new demo will be created soon on top of aries-vcx-agent crate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create Alice Faber CLI Demo for aries_vcx (Rust)
5 participants