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

Feature/cred migrator #867

Merged
merged 59 commits into from
Jul 4, 2023
Merged

Feature/cred migrator #867

merged 59 commits into from
Jul 4, 2023

Conversation

bobozaur
Copy link
Contributor

@bobozaur bobozaur commented Jun 1, 2023

This PR introduces the wallet_migrator library crate, currently just for the purpose of migrating anoncreds related objects from their libvdrtools implementation to indy-credx. This is accomplished by creating a new Indy wallet, so the old one remains as a backup.

The migrator is flexible enough though to support migrating anything in terms of an Indy wallet.

In the future the crate could be extended to assist with migrating from an Indy wallet to a different one too.

The PR also adds a testing feature (unfortunately necessary) to accommodate current tests in aries-vcx so that wallets get migrated in the middle of the tests to ensure things keep working post migration.

Stubs are already in place for the indy-credx to anoncreds-rs migration, but that will be fully implemented in a future PR.

@codecov-commenter
Copy link

codecov-commenter commented Jun 1, 2023

Codecov Report

Merging #867 (036747d) into main (c49c1ab) will decrease coverage by 41.78%.
The diff coverage is 0.64%.

@@            Coverage Diff             @@
##             main    #867       +/-   ##
==========================================
- Coverage   49.89%   8.12%   -41.78%     
==========================================
  Files         432     434        +2     
  Lines       35058   34783      -275     
  Branches     7617    7538       -79     
==========================================
- Hits        17493    2826    -14667     
- Misses      12287   31156    +18869     
+ Partials     5278     801     -4477     
Flag Coverage Δ
unittests-aries-vcx 8.12% <0.64%> (-41.74%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
agency_client/src/agency_client.rs 0.00% <0.00%> (-76.70%) ⬇️
aries_vcx/src/core/profile/vdrtools_profile.rs 35.00% <0.00%> (-37.50%) ⬇️
aries_vcx/src/utils/devsetup.rs 33.18% <0.00%> (-31.70%) ⬇️
aries_vcx/tests/test_creds_proofs.rs 0.08% <0.00%> (-88.00%) ⬇️
aries_vcx/tests/test_creds_proofs_revocations.rs 0.13% <0.00%> (-87.73%) ⬇️
aries_vcx/tests/test_mysql_wallet.rs 3.03% <0.00%> (-78.61%) ⬇️
aries_vcx/tests/utils/devsetup_agent.rs 0.00% <0.00%> (-76.62%) ⬇️
aries_vcx/tests/utils/scenarios.rs 0.00% <0.00%> (-83.88%) ⬇️
aries_vcx_core/src/anoncreds/credx_anoncreds.rs 0.00% <0.00%> (-56.49%) ⬇️
aries_vcx_core/src/wallet/indy_wallet.rs 2.85% <0.00%> (-52.39%) ⬇️
... and 21 more

... and 297 files with indirect coverage changes

@bobozaur bobozaur force-pushed the feature/cred_migrator branch 2 times, most recently from 606a990 to 268b4a2 Compare June 15, 2023 15:45
@bobozaur bobozaur marked this pull request as ready for review June 22, 2023 10:25
Copy link
Contributor

@Patrik-Stas Patrik-Stas left a comment

Choose a reason for hiding this comment

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

left few comments

I'd suggest to work a little bit more on testing - the current approach includes migration to existing tests, but usually the migration happens at the same spot, after issuing credentials.
We could probably take some tests and tweak around the position of migration, but I think following scenario could deserve a dedicated test:

Scenario 1:

  • create creddef, revocation registry
  • issue credential A, revoke locally
  • issue credential B
  • migrate
  • revoke locally B
  • issue another credential C
  • revoke locally C
  • publish revocation

Mainly to check that revocation registries and accumulated deltas properly survive across the migration

libvdrtools/indy-wallet/src/lib.rs Show resolved Hide resolved
libvdrtools/indy-wallet/src/wallet.rs Show resolved Hide resolved
wallet_migrator/src/lib.rs Show resolved Hide resolved
wallet_migrator/src/lib.rs Outdated Show resolved Hide resolved
@bobozaur
Copy link
Contributor Author

@Patrik-Stas The requested test scenario was added. I'll be monitoring the CI to check on the latest changes, but apart from that I think the points you mentioned in your review are covered now.

@bobozaur bobozaur force-pushed the feature/cred_migrator branch 5 times, most recently from b3c048d to 89be025 Compare June 29, 2023 15:01
Copy link
Contributor

@Patrik-Stas Patrik-Stas left a comment

Choose a reason for hiding this comment

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

left some comments

aries_vcx/migrations/20210903123945_migration0.sql Outdated Show resolved Hide resolved
wallet_migrator/src/lib.rs Outdated Show resolved Hide resolved
wallet_migrator/src/vdrtools2credx/conv.rs Outdated Show resolved Hide resolved
wallet_migrator/src/lib.rs Outdated Show resolved Hide resolved
wallet_migrator/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@Patrik-Stas Patrik-Stas left a comment

Choose a reason for hiding this comment

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

left further comment on logging

Signed-off-by: Bogdan Mircea <[email protected]>
Signed-off-by: Bogdan Mircea <[email protected]>
Signed-off-by: Bogdan Mircea <[email protected]>
Signed-off-by: Bogdan Mircea <[email protected]>
Signed-off-by: Bogdan Mircea <[email protected]>
Signed-off-by: Bogdan Mircea <[email protected]>
Signed-off-by: Bogdan Mircea <[email protected]>
Signed-off-by: Bogdan Mircea <[email protected]>
Signed-off-by: Bogdan Mircea <[email protected]>
Signed-off-by: Bogdan Mircea <[email protected]>
@mirgee mirgee merged commit 57c57e0 into main Jul 4, 2023
44 checks passed
@mirgee mirgee deleted the feature/cred_migrator branch July 4, 2023 09:32
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.

5 participants