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: 0.4.0 migration script #1392

Merged

Conversation

TimoGlastra
Copy link
Contributor

@TimoGlastra TimoGlastra commented Mar 19, 2023

Adds migration script for 0.4.0 covering most of the changes between 0.3.1 and 0.4.0. There's some issues to resolve with how we are going to handle legacy identifiers vs qualified identifiers in AnonCreds (see #1391). did:sov dids are all transformed to did:indy dids already as we store the qualified indy did. Same for AnonCreds schema and cred def records, so it's mainly about anoncreds credential records that need some work.

Need to extend the migration docs a bit more, to say you need to add the anoncreds module to update the anoncreds related records, and that any dids created using agent.ledger. should be imported manually into the agent as we don't have the DidRecord for them.

This doesn't yet solve #998, but I left some comments there on my current thoughts

@TimoGlastra TimoGlastra requested a review from a team as a code owner March 19, 2023 15:58
@codecov-commenter
Copy link

codecov-commenter commented Mar 19, 2023

Codecov Report

Merging #1392 (fa9974d) into main (996c08f) will increase coverage by 0.06%.
The diff coverage is 90.32%.

@@            Coverage Diff             @@
##             main    #1392      +/-   ##
==========================================
+ Coverage   84.90%   84.96%   +0.06%     
==========================================
  Files         788      795       +7     
  Lines       19445    19606     +161     
  Branches     3162     3172      +10     
==========================================
+ Hits        16509    16658     +149     
- Misses       2929     2941      +12     
  Partials        7        7              
Impacted Files Coverage Δ
.../repository/AnonCredsCredentialDefinitionRecord.ts 100.00% <ø> (ø)
.../anoncreds/src/repository/AnonCredsSchemaRecord.ts 100.00% <ø> (ø)
packages/core/src/error/index.ts 100.00% <ø> (ø)
packages/indy-sdk/src/wallet/IndySdkWallet.ts 46.69% <0.00%> (-0.39%) ⬇️
...kages/indy-vdr/src/dids/IndyVdrIndyDidRegistrar.ts 94.50% <ø> (ø)
packages/askar/src/wallet/AskarWallet.ts 47.61% <33.33%> (-0.14%) ⬇️
...re/src/wallet/error/WalletExportPathExistsError.ts 50.00% <50.00%> (ø)
...ages/core/src/storage/migration/UpdateAssistant.ts 86.32% <60.00%> (-3.20%) ⬇️
...e/src/storage/migration/updates/0.3.1-0.4/cache.ts 87.50% <87.50%> (ø)
...ages/anoncreds/src/updates/0.3.1-0.4/linkSecret.ts 89.47% <89.47%> (ø)
... and 12 more

... and 1 file with indirect coverage changes

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

Copy link
Contributor

@berendsliedrecht berendsliedrecht left a comment

Choose a reason for hiding this comment

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

Awesome! Looks extensively tested, very nice!

Do you think we should create some tests that do all the migrations in different order? Maybe they change some state in a weird way which breaks it (and seemingly unrevertable).


const indyCredentialRequestMetadata = credentialRecord.metadata.get(INDY_CREDENTIAL_REQUEST_METADATA)
if (indyCredentialRequestMetadata) {
// TODO: we if we choose to rename master secret to link secret in anoncreds-rs we should also rename it in the request
Copy link
Contributor

Choose a reason for hiding this comment

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

which specific fields do you mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you call create credential request in indy-sdk / anoncreds rs) it will return the following metadata:

{
          "master_secret_blinding_data": {
            "v_prime": "6088566065720309491695644944398283228337587174153857313170975821102428665682789111613194763354086540665993822078019981371868225077833338619179176775427438467982451441607103798898879602785159234518625137830139620180247716943526165654371269235270542103763086097868993123576876140373079243750364373248313759006451117374448224809216784667062369066076812328680472952148248732117690061334364498707450807760707599232005951883007442927332478453073050250159545354197772368724822531644722135760544102661829321297308144745035201971564171469931191452967102169235498946760810509797149446495254099095221645804379785022515460071863075055785600423275733199",
            "vr_prime": null
          },
          "nonce": "131502096406868204437821",
          "master_secret_name": "walletId28c602347-3f6e-429f-93cd-d5aa7856ef3f"
        }

If we move to using link secret in AnonCreds RS, we should update all metadata and transform the master_secret keys to link_secret keys in the Indy SDK holder service in AFJ.

Did you also update these fields in your link secret pr in anoncreds rs?

@TimoGlastra
Copy link
Contributor Author

Do you think we should create some tests that do all the migrations in different order? Maybe they change some state in a weird way which breaks it (and seemingly unrevertable).

Do you mean the indy sdk to askar update script and the normal update assistant? If so, yes that'd probably be a good idea!

@TimoGlastra TimoGlastra enabled auto-merge (squash) March 21, 2023 13:08
auto-merge was automatically disabled March 23, 2023 17:02

Merge queue setting changed

@TimoGlastra TimoGlastra enabled auto-merge March 23, 2023 17:05
auto-merge was automatically disabled March 27, 2023 09:28

Merge queue setting changed

@TimoGlastra TimoGlastra merged commit bc5455f into openwallet-foundation:main Mar 29, 2023
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.

Write migration script for breaking changes between 0.3.x and 0.4.0 in AFJ
3 participants