feat: possibility to set masterSecretId inside of WalletConfig

[an-uhryn]

No description provided.

[jakubkoci]

Thanks, @an-uhryn. Could you please fix the linter error? I think we can disable @typescript-eslint/no-non-null-assertion for the whole Wallet.test.ts file.

[genaris]

Thanks, @an-uhryn. Could you please fix the linter error? I think we can disable @typescript-eslint/no-non-null-assertion for the whole Wallet.test.ts file.

Yes, and don't forget the DCO issue following the instructions from here.

[codecov-commenter]

Codecov Report

Merging #1043 (08a9440) into main (a230841) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1043      +/-   ##
==========================================
+ Coverage   88.72%   88.77%   +0.04%     
==========================================
  Files         523      523              
  Lines       12173    12173              
  Branches     1913     1914       +1     
==========================================
+ Hits        10801    10806       +5     
+ Misses       1368     1363       -5     
  Partials        4        4              

Impacted Files	Coverage Δ
packages/core/src/types.ts	100.00% <ø> (ø)
packages/core/src/wallet/IndyWallet.ts	71.18% <100.00%> (+0.56%)	⬆️
packages/core/src/storage/IndyStorageService.ts	95.08% <0.00%> (+0.81%)	⬆️
packages/core/src/error/IndySdkError.ts	100.00% <0.00%> (+60.00%)	⬆️

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

[genaris]

Thank you @an-uhryn!

I'm not an expert on this topic so consider some of my comments more as suggestions than changes requested.

I guess this PR tackles mainly scenarios where wallet has been created externally, but the resulting code left me a doubt regarding usage of this masterSecretId when it is created within the framework: in the method createAndOpen from IndyWallet.ts I see:

      await this.createMasterSecret(this.handle, walletConfig.id)

Shouldn't be walletConfig.masterSecretId used in this case unless not specified (in which case walletConfig.id would be used instead)?

[genaris]

Maybe I'm missing something but I think it should be string without quotes

Suggested change

	masterSecretId?: 'string'
	masterSecretId?: string

[berendsliedrecht]

Yes, this only allows the string literal string. Any other string is not allowed.

[genaris]

Considering the checks that are done in line 63, for consistency with other conditions, maybe we could avoid this if sentence and simply return this.walletConfig.masterSecretId ?? this.walletConfig.id

[jakubkoci]

That's a good point. We found out that !this.walletConfig?.id is not needed if we're not mistaken. It should not get into a state where the walletConfig.id is missing but isInitialized is true.

[an-uhryn]

@jakubkoci @genaris @blu3beri Thanks for your review and suggestions. I made all changes.

[TimoGlastra]

As the masterSecret is still created when you create the wallet, what is the use case of this feature?

Fine to merge, just curious as to why you'd want this

[TimoGlastra]

This can lead to undefined being returned, which wasn't possible before. We should probably check if either have a value and throw an error otherwise

[an-uhryn]

It's good question. I'm not sure but is it possible case that isInitialized is true but we don't have this.walletConfig.id?
It seems like we have to open wallet to make isInitialized to be true, but we can't open wallet without wallet id.

[genaris]

I think this is the reason why CI is failing (a few lines in IndyHolderService.ts are expecting it to be always defined, something that makes sense because otherwise it cannot work).

[an-uhryn]

I returned back this check and also aded for masterSecretId. Hope now everything will be fine 🙂

[TimoGlastra]

Maybe you can extend the getAgentConfig method and pass the masterSecretId to that method?

[an-uhryn]

Yeah, sure!

[an-uhryn]

Done with extending of getAgentConfig

[an-uhryn]

As the masterSecret is still created when you create the wallet, what is the use case of this feature?

Fine to merge, just curious as to why you'd want this

Hey @TimoGlastra. We are migrating from VCX Library to Aries Framework JavaScript. We have some credentials which were created with VCX Library (with masterSecretId) and after credentials migrating we can't accept proof request without setting masterSecretId.

[TimoGlastra]

Makes sense! 👍

[TimoGlastra]

Hmm extraConfig can already take the wallet config, so not so sure on adding it as a separate property. I understand that if we take it from extraConfig, we will also have to provide the walletConfig object in whole, but I don't think that is necesarily bad.

So call getBaseConfig like this:

getBaseConfig({
    walletConfig: {
      id: `Wallet: Name of the test`,
      key: `Key: Name of the test`,
      masterSecretId: 'the key'
    }
})

[an-uhryn]

Yeah thanks, I missed that.

[an-uhryn]

Did you mean getAgentConfig?

getAgentConfig('WalletTestWithMasterSecretId', {
    walletConfig: {
        id: `Wallet: WalletTestWithMasterSecretId`,
        key: `Key: WalletTestWithMasterSecretId`,
        masterSecretId: 'customMasterSecretId',
    },
})

[TimoGlastra]

There's some prettier errors, can you fix those?