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

feat: connection type #994

Merged
merged 22 commits into from
Sep 29, 2022

Conversation

KolbyRKunz
Copy link
Contributor

Added a connection type enum to the connection record so that you can filter the connections based on type.

@KolbyRKunz KolbyRKunz requested a review from a team as a code owner August 26, 2022 18:03
@KolbyRKunz
Copy link
Contributor Author

We went with putting the connection type in as a tag so that you can query a specific connection type, as was discussed in a previous working group call.

@KolbyRKunz KolbyRKunz changed the title Feat/connection type Feat:connectionType Aug 26, 2022
@codecov-commenter
Copy link

codecov-commenter commented Aug 26, 2022

Codecov Report

Merging #994 (e91aa01) into main (8efade5) will increase coverage by 0.00%.
The diff coverage is 84.61%.

@@           Coverage Diff           @@
##             main     #994   +/-   ##
=======================================
  Coverage   88.70%   88.71%           
=======================================
  Files         521      522    +1     
  Lines       12130    12156   +26     
  Branches     2003     1913   -90     
=======================================
+ Hits        10760    10784   +24     
- Misses       1308     1368   +60     
+ Partials       62        4   -58     
Impacted Files Coverage Δ
...modules/connections/repository/ConnectionRecord.ts 95.65% <ø> (ø)
.../modules/connections/services/ConnectionService.ts 87.09% <0.00%> (-0.71%) ⬇️
.../core/src/modules/connections/ConnectionsModule.ts 71.03% <88.88%> (+2.53%) ⬆️
...e/src/modules/connections/models/ConnectionType.ts 100.00% <100.00%> (ø)
...kages/core/src/modules/connections/models/index.ts 100.00% <100.00%> (ø)
...ules/routing/services/MediationRecipientService.ts 85.63% <100.00%> (+0.24%) ⬆️
packages/node/src/NodeFileSystem.ts 37.14% <0.00%> (ø)
packages/core/src/crypto/JwsService.ts 90.90% <0.00%> (ø)
packages/core/src/agent/MessageSender.ts 85.88% <0.00%> (ø)
packages/core/src/agent/MessageReceiver.ts 80.76% <0.00%> (ø)
... and 26 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@KolbyRKunz KolbyRKunz changed the title Feat:connectionType feat: connection type Aug 26, 2022
Signed-off-by: KolbyRKunz <[email protected]>
Copy link
Contributor

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

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

I'm not sure if we should add this, but open to be convinced otherwise.

There is already a way to know if a connection is a mediator connection. You can check if there is a mediaton record assoaciated with the connection id.

Adding the tag means it can get out of sync. What if we remove the mediation record? Then the tag should also be removed. What if the connection is also used for other purposes than just mediation? To me this feels like something that should be handled on the application level.

@genaris
Copy link
Contributor

genaris commented Aug 29, 2022

I agree with @TimoGlastra in that this seems a bit application-specific. We are also using in our app this concept of 'connection type' but use other roles than the 'Mediator' used in this case, and I guess others might use it differently as well.

That being said, I think it will be nice to improve in AFJ the access to setting and querying tags from outside the framework. Currently, in order to set a tag to a record we have to set the new tag in the record, then resolve the associated service or repository and call update(), etc. And also a similar procedure to find by tag (using findByQuery or findSingleByQuery).

I won't say that this is impossible or very hard currently, but tags are a good feature to classify and efficiently search for records and it would be nice to leverage its usage in a general-purpose manner.

@TimoGlastra
Copy link
Contributor

Agreed @genaris, we should probably expose the findByQuery and update methods on the public api of all modules.

@KolbyRKunz
Copy link
Contributor Author

What we are thinking we are wanting to do is go and change the connectionType tag to be of [ConnectionType | string] so that the end application developer can add their own custom tags while supporting our own defined tags. The other goal we have is to create methods within the module that will allow for the getting and setting of these tags from the application level along with methods to generically handle the accessing of tags at the application level. The Idea behind making a specific method for updating the connectionType is to make is obvious for the developer using AFJ to see that it is a value for them to use instead of having every developer have to learn what every tag on a connection record is, while at the same time still allowing those who know what is going on internally in AFJ to have access to the other tags on the record. In the future if more tags are added then we can create more specific methods for those tags at that time or for other existing tags. @TimoGlastra @genaris

@genaris
Copy link
Contributor

genaris commented Sep 8, 2022

Well, if this solves the problem for you I think it's fine to add this convenient connectionType tag, as long as it's not limited to a single or couple of possible values (as today is for ConnectionType.Mediator) so it allows each developer to define their own types besides the default ones. It would be certainly useful to have something to easily categorize connections and efficiently search for a bunch of them.

@TimoGlastra suggested today to use string array for this tag, which might be even more useful as we could be more specific when searching for connections (e.g. a connection could be a Mediator and also an Issuer or a Verifier or whatever the application uses). I have not used arrays for tags so I don't know about the complexity of using it, but I think it's worth to take a look at this possibility.

To make this really usable besides this particular case, I think it will be important to add a module-level method in Connections that allows to search for connection type. For instance something like findAllByConnectionType() so we can search for particular types without the need of retrieving all records and then filtering in JS (this could become significant when the wallet has lots of connections and records).

@KolbyRKunz
Copy link
Contributor Author

@TimoGlastra @genaris Made some updates to this, the main thing being that connectionType is now an array accepting strings or values from the connectionType enum. I also added some methods to the connectionModule that allows you to give a connectionId and view the Tags on that record along with methods to modify the connectionType array from the module. Currently I figured the only Tag we want to expose to direct modification is the connectionType tag, the other two methods added are purely for viewing the tags on the record by either getting all of them or just one specified. Let me know if there is any feedback you have.

Copy link
Contributor

@genaris genaris left a comment

Choose a reason for hiding this comment

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

Looks nice and useful. I left a few comments regarding getTagFromRecord and getAllTagsFromRecord that IMHO are not necessary.

If possible, I would like to add the request of adding a method called findAllByConnectionType that allow us to query all connections that match the specified type/s. This would be useful for classifying the connections and querying in an efficient way. For instance, suppose that we want to retrieve only the mediators or the connections that the user tagged as 'friends': this query will do a direct request on the wallet backend instead of retrieving all the records and then filtering in the frontend.

packages/core/src/modules/connections/ConnectionsModule.ts Outdated Show resolved Hide resolved
packages/core/src/modules/connections/ConnectionsModule.ts Outdated Show resolved Hide resolved
packages/core/src/modules/connections/ConnectionsModule.ts Outdated Show resolved Hide resolved
packages/core/src/modules/connections/ConnectionsModule.ts Outdated Show resolved Hide resolved
packages/core/tests/oob-mediation.test.ts Outdated Show resolved Hide resolved
Signed-off-by: KolbyRKunz <[email protected]>
@JamesKEbert
Copy link
Contributor

Hey @TimoGlastra, the CI appears to be failing on this PR, in particular though the Postgres wallet is failing to get setup, so it appears to be unrelated to changes made in this PR. Is this related to the same CI issues w/the BBS signatures branch(es)?

@genaris
Copy link
Contributor

genaris commented Sep 23, 2022

Hey @TimoGlastra, the CI appears to be failing on this PR, in particular though the Postgres wallet is failing to get setup, so it appears to be unrelated to changes made in this PR. Is this related to the same CI issues w/the BBS signatures branch(es)?

Seems to be something different, as this is on the main branch and the issue with BBS signatures was in 0.3.0-pre branch. I noticed that yesterday there were other weird issues in CI with other PR as well.

Looking at the logs I see that it is getting a new version of rust:

info: latest update on 2022-09-22, rust version 1.64.0 (a55dd71d5 2022-09-19)

Probably it has to do with that? There is a related issue here which apparently has to do with some new dependencies. Dependency hell not only present in JavaScript 😄

   Compiling socket2 v0.3.11
error[E0512]: cannot transmute between types of different sizes, or dependently-sized types
   --> /home/runner/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/socket2-0.3.11/src/sockaddr.rs:156:9
    |
156 |         mem::transmute::<SocketAddrV4, sockaddr_in>(v4);
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: source type: `SocketAddrV4` (48 bits)
    = note: target type: `sockaddr_in` (128 bits)

@sairanjit
Copy link
Contributor

sairanjit commented Sep 23, 2022

@JamesKEbert @genaris @TimoGlastra this is the issue with the rust dependencies in 1.64.0 but it works fine with 1.63.0 Can I create a PR to by default use rust version 1.63.0

I ran the tests here https://github.com/sairanjit/aries-framework-javascript/actions/runs/3115296783/jobs/5052043298 it is working fine changes are here #1036

@genaris
Copy link
Contributor

genaris commented Sep 26, 2022

@KolbyRKunz could you update the branch so CI is relaunched? It should work now with the fix done by @sairanjit.

Also I have my last suggestion: to add findAllByConnectionId at module level (ConnectionsModule.ts) in addition to the method in ConnectionService you've already added (i.e. the module-level method internally calls the service-level method, like it happens with findById).

@genaris genaris merged commit 0d14a71 into openwallet-foundation:main Sep 29, 2022
TimoGlastra added a commit that referenced this pull request Oct 11, 2022
* feat: OOB public did (#930)

Signed-off-by: Pavel Zarecky <[email protected]>

* feat(routing): manual mediator pickup lifecycle management (#989)

Signed-off-by: Ariel Gentile <[email protected]>

* docs(demo): faber creates invitation (#995)

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

* chore(release): v0.2.3 (#999)

Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* fix(question-answer): question answer protocol state/role check (#1001)

Signed-off-by: Ariel Gentile <[email protected]>

* feat: Action Menu protocol (Aries RFC 0509) implementation (#974)

Signed-off-by: Ariel Gentile <[email protected]>

* fix(ledger): remove poolConnected on pool close (#1011)

Signed-off-by: Niall Shaw <[email protected]>

* fix(ledger): check taa version instad of aml version (#1013)

Signed-off-by: Jakub Koci <[email protected]>

* chore: add @janrtvld to maintainers (#1016)

Signed-off-by: Timo Glastra <[email protected]>

* feat(routing): add settings to control back off strategy on mediator reconnection (#1017)

Signed-off-by: Sergi Garreta <[email protected]>

* fix: avoid crash when an unexpected message arrives (#1019)

Signed-off-by: Pavel Zarecky <[email protected]>

* chore(release): v0.2.4 (#1024)

Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* style: fix some lint errors

Signed-off-by: Ariel Gentile <[email protected]>

* feat: use did:key flag (#1029)

Signed-off-by: Ariel Gentile <[email protected]>

* ci: set default rust version (#1036)

Signed-off-by: Sai Ranjit Tummalapalli <[email protected]>

* fix(oob): allow encoding in content type header (#1037)

Signed-off-by: Timo Glastra <[email protected]>

* feat: connection type (#994)

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

* chore(module-tenants): match package versions

Signed-off-by: Ariel Gentile <[email protected]>

* feat: improve sending error handling (#1045)

Signed-off-by: Ariel Gentile <[email protected]>

* feat: expose findAllByQuery method in modules and services (#1044)

Signed-off-by: Jim Ezesinachi <[email protected]>

* feat: possibility to set masterSecretId inside of WalletConfig (#1043)

Signed-off-by: Andrii Uhryn <[email protected]>

* fix(oob): set connection alias when creating invitation (#1047)

Signed-off-by: Jakub Koci <[email protected]>

* build: fix missing parameter

Signed-off-by: Ariel Gentile <[email protected]>

Signed-off-by: Pavel Zarecky <[email protected]>
Signed-off-by: Ariel Gentile <[email protected]>
Signed-off-by: conanoc <[email protected]>
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Signed-off-by: Niall Shaw <[email protected]>
Signed-off-by: Jakub Koci <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Sergi Garreta <[email protected]>
Signed-off-by: Sai Ranjit Tummalapalli <[email protected]>
Signed-off-by: KolbyRKunz <[email protected]>
Signed-off-by: Jim Ezesinachi <[email protected]>
Signed-off-by: Andrii Uhryn <[email protected]>
Co-authored-by: Iskander508 <[email protected]>
Co-authored-by: conanoc <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Niall Shaw <[email protected]>
Co-authored-by: jakubkoci <[email protected]>
Co-authored-by: Timo Glastra <[email protected]>
Co-authored-by: Sergi Garreta Serra <[email protected]>
Co-authored-by: Sai Ranjit Tummalapalli <[email protected]>
Co-authored-by: KolbyRKunz <[email protected]>
Co-authored-by: Jim Ezesinachi <[email protected]>
Co-authored-by: an-uhryn <[email protected]>
genaris added a commit to 2060-io/aries-framework-javascript that referenced this pull request Oct 13, 2022
* feat: OOB public did (openwallet-foundation#930)

Signed-off-by: Pavel Zarecky <[email protected]>

* feat(routing): manual mediator pickup lifecycle management (openwallet-foundation#989)

Signed-off-by: Ariel Gentile <[email protected]>

* docs(demo): faber creates invitation (openwallet-foundation#995)

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

* chore(release): v0.2.3 (openwallet-foundation#999)

Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* fix(question-answer): question answer protocol state/role check (openwallet-foundation#1001)

Signed-off-by: Ariel Gentile <[email protected]>

* feat: Action Menu protocol (Aries RFC 0509) implementation (openwallet-foundation#974)

Signed-off-by: Ariel Gentile <[email protected]>

* fix(ledger): remove poolConnected on pool close (openwallet-foundation#1011)

Signed-off-by: Niall Shaw <[email protected]>

* fix(ledger): check taa version instad of aml version (openwallet-foundation#1013)

Signed-off-by: Jakub Koci <[email protected]>

* chore: add @janrtvld to maintainers (openwallet-foundation#1016)

Signed-off-by: Timo Glastra <[email protected]>

* feat(routing): add settings to control back off strategy on mediator reconnection (openwallet-foundation#1017)

Signed-off-by: Sergi Garreta <[email protected]>

* fix: avoid crash when an unexpected message arrives (openwallet-foundation#1019)

Signed-off-by: Pavel Zarecky <[email protected]>

* chore(release): v0.2.4 (openwallet-foundation#1024)

Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* style: fix some lint errors

Signed-off-by: Ariel Gentile <[email protected]>

* feat: use did:key flag (openwallet-foundation#1029)

Signed-off-by: Ariel Gentile <[email protected]>

* ci: set default rust version (openwallet-foundation#1036)

Signed-off-by: Sai Ranjit Tummalapalli <[email protected]>

* fix(oob): allow encoding in content type header (openwallet-foundation#1037)

Signed-off-by: Timo Glastra <[email protected]>

* feat: connection type (openwallet-foundation#994)

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

* chore(module-tenants): match package versions

Signed-off-by: Ariel Gentile <[email protected]>

* feat: improve sending error handling (openwallet-foundation#1045)

Signed-off-by: Ariel Gentile <[email protected]>

* feat: expose findAllByQuery method in modules and services (openwallet-foundation#1044)

Signed-off-by: Jim Ezesinachi <[email protected]>

* feat: possibility to set masterSecretId inside of WalletConfig (openwallet-foundation#1043)

Signed-off-by: Andrii Uhryn <[email protected]>

* fix(oob): set connection alias when creating invitation (openwallet-foundation#1047)

Signed-off-by: Jakub Koci <[email protected]>

* build: fix missing parameter

Signed-off-by: Ariel Gentile <[email protected]>

Signed-off-by: Pavel Zarecky <[email protected]>
Signed-off-by: Ariel Gentile <[email protected]>
Signed-off-by: conanoc <[email protected]>
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Signed-off-by: Niall Shaw <[email protected]>
Signed-off-by: Jakub Koci <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Sergi Garreta <[email protected]>
Signed-off-by: Sai Ranjit Tummalapalli <[email protected]>
Signed-off-by: KolbyRKunz <[email protected]>
Signed-off-by: Jim Ezesinachi <[email protected]>
Signed-off-by: Andrii Uhryn <[email protected]>
Co-authored-by: Iskander508 <[email protected]>
Co-authored-by: conanoc <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Niall Shaw <[email protected]>
Co-authored-by: jakubkoci <[email protected]>
Co-authored-by: Timo Glastra <[email protected]>
Co-authored-by: Sergi Garreta Serra <[email protected]>
Co-authored-by: Sai Ranjit Tummalapalli <[email protected]>
Co-authored-by: KolbyRKunz <[email protected]>
Co-authored-by: Jim Ezesinachi <[email protected]>
Co-authored-by: an-uhryn <[email protected]>
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.

6 participants