search

hyperledger / aries-framework-swift

A Swift framework for Aries.
Apache License 2.0
17 stars 6 forks source link

Exclude walletKey from AgentConfig encode/decode. #32

Closed kukgini closed 1 year ago

kukgini commented 1 year ago

Checklist

Description

Exclude walletKey from AgentConfig encode/decode. It should be stored separately in a safer place.

conanoc commented 1 year ago

The wallet key should not be optional because it is a mandatory field. You could replace it with a dummy value before saving the config to a file.

I still do not understand fully what's the use case of saving/reading the config from files. Is it related to supporting multiple ledgers?

kukgini commented 1 year ago

Sorry. I accidentally hit close with the comment button by mistake.

kukgini commented 1 year ago

This PR is a separate commit that I think is individually meaningful in the process of Wallet Per Pool.

Although it is not a well-organized source code yet, I am trying to create a mobile wallet for proof of concept in this way. (see: https://github.com/kukgini/AliceWallet2) My thought is that it will be useful scenario where ledger pool is switched and used before a system is built that can be interoperable between multiple indy networks.

That's why I'm trying to save the agent config by pool, but I thought that, even though, the wallet key should be stored separately in a safer place separate from the agent config.

kukgini commented 1 year ago

Let me explain more. Of course walletKey is mandatory. However, it only needs to be set until the agent is initialized. I defined the process of saving the AgentConfig in UserDefaults as a task of the client side (users of this framework). And my intension is to prevent client side does not accidentally store the walletKey in UserDefault together.

kukgini commented 1 year ago

Is it necessary to prevent try! (force try) in UnitTest ?

kukgini commented 1 year ago

I can't figure it out what's the problem with RunTests. @conanoc could you help me?

conanoc commented 1 year ago

I'm not sure why this check fails. Let me check this on Monday.

conanoc commented 1 year ago

@kukgini I think #35 will fix the build error. As for using try! in the unit tests, you can disable the check like this. But, it would be better avoiding using force-try.

Here's my opinion about supporting multiple wallets (or supporting multiple ledgers):

One thing I don't understand is why you want different wallets per pool. Can you elaborate?

kukgini commented 1 year ago

From your comment: I think it's better to save the whole config in the keychain

That's why I separate this from #34. Because I think it needs some more discussion to see if this PR is acceptable.

I think it is good when whole config is saved in keychain. however, on a second thought, saving something into keychain would be a decision of device owner. So, I'm not sure that it would be a good design about saving whole config than wallet key.

About your comment: save it somewhere else after deleting the key

The framework user has a burden to understand the usage or intention of this framework. It seems easy to overlook the need to remove certain fields before saving the structure.

On the other hand, it seems ok that all other settings except wallet key can be treated as one set. Moreover, these do not have to be secret information.

To answer your question: why I want different wallet(s) per pool

I'm not sure if this would be answer for your question. I will answer this question assuming you understand the situation where I need to switch multiple ledgers.

The reason I want wallet per ledger (pool) is that VCs issued by issuers are dependent on a specific indy ledger's schema, credential definition and revocation registries. It means, issued VCs are only meaningful among participants sharing the same ledger. So, If user changed ledger in the app, I thought that the wallet should be changed either.

conanoc commented 1 year ago

So, If user changed ledger in the app, I thought that the wallet should be changed either.

Using the same wallet for different ledgers will be OK. If we don't have to change the wallet for different ledgers, why should we change the wallet? In case we use a mediator for the agent, changing the wallet will make trouble.

Besides that, I don't have any scenario where we use multiple ledgers. Are there any aries RFCs or scenarios utilizing multiple ledgers?

kukgini commented 1 year ago

from your comment: Using the same wallet for different ledgers will be OK.

I didn't know this was possible. Consider that if a VC issued by issuer based on the information of ledger A, then presented to verifier based on another ledger B, it would not be verifiable. And if the name is the same between the two, there will be confusion.

and In case we use a mediator for the agent, changing the wallet will make trouble.

The mediator won't be a big deal. because it dosen't participate in the issuance and verification process, It only deliver informations.

kukgini commented 1 year ago

I was wrong about mediator. when wallet changed, then connection to mediator will be lost. it can be troublesome.

kukgini commented 1 year ago

As far as i know, effort to enable multiple ledgers is discussed in indy-did-method channel. and i'm not sure it is implemented. In aca-py side, release 1.0 will adopt universal resolver concept which is needed for multiple ledgers.

If all of these are implemented, a single wallet will be sufficient because the information in different ledgers is distinguished by the did method. However, I would like my app to be able to issue, store and present VCs while switching between sovrin, bcovrin, and indicio mainnets, for example, in a transitional stage before they are complete.

kukgini commented 1 year ago

This is a case where the specifics cannot be revealed. In case our client want to operate two different indy networks for political reasons. At this point, the customer may request to create separate apps for each network, but may also want a single app to be used across networks. Either way, we must be able to give a solution.

conanoc commented 1 year ago

I see. Let's assume, for some reason, the app needs to use two different indy networks. The app should control which network to use in receiving credentials and in presenting proofs. If the app controls the network id properly, then there will be no problem with a single wallet.

So, for now, I think we don't need this change(#32) and #34. What do you think?

kukgini commented 1 year ago

I'm not quite sure how to control with a single wallet with different ledgers. could you give me some tips or examples?

On the other hand. This PR is only a suggestion, exchange of opinions is sufficient. I have a slightly different opinion on #34, I will put my opinion on there.

Finally, And even if it's not absolutely necessary, wouldn't it be the direction as a framework to give developers a little more freedom when they want to? Feel free to close this PR with some thoughts.

conanoc commented 1 year ago

Alright. I'll close this and let's discuss how to use multiple ledgers on a new issue. You could create a new issue and let me know what is your concern.