Skip to content
This repository has been archived by the owner on Mar 27, 2024. It is now read-only.

feat: Adding sqlite to mobile agent #3306

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

afrancoc2000
Copy link
Contributor

@afrancoc2000 afrancoc2000 commented Jul 28, 2022

Title:
Adding sqlite to mobile agent

Description:
I created a component in the mobile agent that allows a user to use a storage provider that connects with SQLite.

Summary:

Using https://github.com/hyperledger/aries-framework-go-ext/blob/main/component/storage/mysql/store.go as reference I created a store provider that connects with SQLite.
Also, I implemented the TotalItems() method inside the storage wrapper.

@DRK3, @sudeshrshetty, What do you think?

@codecov
Copy link

codecov bot commented Jul 28, 2022

Codecov Report

Merging #3306 (402bdce) into main (368f53b) will increase coverage by 0.48%.
The diff coverage is n/a.

❗ Current head 402bdce differs from pull request most recent head e3021a9. Consider uploading reports for the commit e3021a9 to get more accurate results

@@            Coverage Diff             @@
##             main    #3306      +/-   ##
==========================================
+ Coverage   87.57%   88.05%   +0.48%     
==========================================
  Files         343      318      -25     
  Lines       46809    43156    -3653     
==========================================
- Hits        40994    38002    -2992     
+ Misses       4318     3799     -519     
+ Partials     1497     1355     -142     
Impacted Files Coverage Δ
pkg/doc/signature/proof/proof.go 91.72% <0.00%> (-5.33%) ⬇️
pkg/doc/jwt/jwt_support.go 60.37% <0.00%> (-2.48%) ⬇️
pkg/framework/aries/framework.go 81.88% <0.00%> (-0.78%) ⬇️
pkg/doc/verifiable/credential_jwt.go 86.04% <0.00%> (-0.77%) ⬇️
pkg/vdr/peer/creator.go 69.79% <0.00%> (-0.62%) ⬇️
pkg/doc/presexch/definition.go 86.33% <0.00%> (-0.60%) ⬇️
pkg/framework/aries/default.go 77.38% <0.00%> (-0.56%) ⬇️
pkg/doc/util/kmsdidkey/kmsdidkey.go 89.86% <0.00%> (-0.46%) ⬇️
pkg/doc/cm/resolve.go 90.96% <0.00%> (-0.42%) ⬇️
pkg/doc/verifiable/presentation_jwt.go 88.88% <0.00%> (-0.40%) ⬇️
... and 83 more

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

@afrancoc2000 afrancoc2000 force-pushed the feature/add_sqlite_mobile_impl branch 2 times, most recently from 4d5c3f3 to ca7c46d Compare August 5, 2022 11:28
@DRK3
Copy link
Contributor

DRK3 commented Aug 9, 2022

Hi @afrancoc2000! Thanks again for the contribution.

I think this new storage provider implementation should be in https://github.com/hyperledger/aries-framework-go-ext/tree/main/component/storage alongside the other SQL storage provider implementations, as I think it could be useful to non-mobile applications as well. Let me know what you think.

@DRK3
Copy link
Contributor

DRK3 commented Aug 9, 2022

Hi again @afrancoc2000, after I wrote that comment, I realized that your storage provider implementation here returns the interface types defined in https://github.com/hyperledger/aries-framework-go/blob/main/cmd/aries-agent-mobile/pkg/api/storage.go rather than the ones in https://github.com/hyperledger/aries-framework-go/blob/main/spi/storage/storage.go, which is why I'm assuming you didn't put your implementation in aries-framework-go-ext.

I think what you can do to allow your storage provider implementation to be usable with both mobile and non-mobile bindings is this:

  1. Move your implementation to aries-framework-go-ext and change the interface types to the ones in https://github.com/hyperledger/aries-framework-go/blob/main/spi/storage/storage.go
  2. Create a wrapper type that "converts" between the interface types, similar but opposite to the wrapper in https://github.com/hyperledger/aries-framework-go/blob/main/cmd/aries-agent-mobile/pkg/wrappers/storage/storage.go.

Now your new SQLite Aries storage implementation is usable across non-mobile and mobile (using the wrapper) implementations.

Let me know what you think or if you have any questions!

@afrancoc2000
Copy link
Contributor Author

Hi, @DRK3 great idea! I agree, I will do the change, I have a question, what do you think about adding a getter with the wrapper that imports the https://github.com/hyperledger/aries-framework-go-ext/tree/main/component/storage classes for the mobile agent? this, because when I tried importing 2 different aar files generated using go mobile, they conflicted with each other as both have one library that is the same for both inside, and gradle doesn't know how to solve this conflict. What do you think?

Thanks!

@DRK3
Copy link
Contributor

DRK3 commented Aug 15, 2022

Hi, @DRK3 great idea! I agree, I will do the change, I have a question, what do you think about adding a getter with the wrapper that imports the https://github.com/hyperledger/aries-framework-go-ext/tree/main/component/storage classes for the mobile agent? this, because when I tried importing 2 different aar files generated using go mobile, they conflicted with each other as both have one library that is the same for both inside, and gradle doesn't know how to solve this conflict. What do you think?

Thanks!

Hi @afrancoc2000!

For your getter idea, what would that look like? One thing we would want to avoid is importing aries-framework-go-ext into aries-framework-go as it creates a circular dependency.

Also, what is conflicting when you try to generate the Go mobile bindings? Is it a naming conflict?

@afrancoc2000
Copy link
Contributor Author

Hi @DRK3, The issue is that if I create an aar using go mobile for 2 different projects they will both generate an aar project with this folder structure:

> classes > go
          > META-INF
          > org/hyperledger....

And the go class gets duplicated. That wouldn't be an issue, if gradle could ignore one of the packages, but there's an issue inside gradle trying to exclude packages from an aar, as shown here, we couldn't figure out how to exclude packages when they come from an aar. Also I'm not completely sure how would the classes from the aries-framework-go-ext get exported, hopefuly they would be ok, but I had to create an extra getter in the code I created for the Provider constructor to appear in the aar.

@DRK3
Copy link
Contributor

DRK3 commented Aug 22, 2022

Hi @afrancoc2000, unfortunately my knowledge of how the mobile bindings work is somewhat limited, but is the issue basically that you don't have a way to create the mobile bindings with a storage provider from a different project/module (like aries-framework-go-ext?)

@DRK3
Copy link
Contributor

DRK3 commented Aug 22, 2022

I can also ask around and see if someone else who knows the mobile binding might be able to help...

@DRK3
Copy link
Contributor

DRK3 commented Sep 15, 2022

Hi @afrancoc2000, just wanted to check in on this. My apologies - I meant to send an update earlier, but unfortunately I couldn't find someone who knows how the mobile bindings work. Are you still stuck on this? If so, I think perhaps our best option is to just merge the sqlite implementation you have in as-is where you've originally put it and not worry about adding it to aries-framework-go-ext for now, since I realize that this might be blocking your progress.

@afrancoc2000
Copy link
Contributor Author

Hi @DRK3, sorry for the late response, I also was a little busy. For my project I generated the aar file and used it as it was, and also I created the sqlite component to the aries-framework-go-ext here it would be great, if you can give it a look.
I started working on the wrapper idea here but I haven't had the time to finish it, I want to review the code again, but the issue with not beeing able to import two aar files from gomobile into an android app remains, that's why I wanted to import the aries-framework-go-ext library from aries-framework-go only for the mobile app.
So let me give it an extra look to the wrapper, but for now I think we can merge the sqlite implementation for the aries-framework-go-ext repo. What do you think?

@afrancoc2000 afrancoc2000 force-pushed the feature/add_sqlite_mobile_impl branch from 402bdce to 595840f Compare September 29, 2022 15:53
@DRK3
Copy link
Contributor

DRK3 commented Sep 29, 2022

Hi @afrancoc2000, I think what we can do is is merge this PR in as-is, and I'll create a follow-up issue for someone to investigate if it's possible to use the aries-framework-go-ext implementation. I have a minor comment to make on the aries-framework-go-ext implementation, and then I think that one will be good to merge too. Do you have any other changes you want to make to this PR? If not, I can get some of the maintainers to take a look (need their approval to merge). Also, can you take a look at that unit test failure?

@afrancoc2000 afrancoc2000 force-pushed the feature/add_sqlite_mobile_impl branch 3 times, most recently from 43dce59 to 923f222 Compare September 30, 2022 14:02
@afrancoc2000
Copy link
Contributor Author

Hi @DRK3, I don't want to make other changes to the PR. I checked the test and it looks like it is failing because of a timeout, It looks ok on mine here. I will try to run it again changing comments

@afrancoc2000 afrancoc2000 force-pushed the feature/add_sqlite_mobile_impl branch from 923f222 to 4398fcb Compare October 1, 2022 13:06
@DRK3
Copy link
Contributor

DRK3 commented Oct 4, 2022

@afrancoc2000 I'm also seeing errors in Ursa/CL unit tests in my open PR, which doesn't make any changes to related code, so I suspect there's an unrelated issue going on... @sudeshrshetty @rolsonquadras Can we merge this PR?

@afrancoc2000 afrancoc2000 force-pushed the feature/add_sqlite_mobile_impl branch from 4398fcb to 29caf31 Compare October 14, 2022 16:08
@DRK3
Copy link
Contributor

DRK3 commented Oct 14, 2022

@sudeshrshetty @rolsonquadras @fqutishat If you're ok with this PR, can we merge?

@afrancoc2000 afrancoc2000 force-pushed the feature/add_sqlite_mobile_impl branch 5 times, most recently from 4d6c8b6 to 67e5f8f Compare October 21, 2022 20:07
Signed-off-by: Ana Maria Franco <[email protected]>
@afrancoc2000 afrancoc2000 force-pushed the feature/add_sqlite_mobile_impl branch from 67e5f8f to e3021a9 Compare October 25, 2022 14:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants