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.

Sign up for GitHub

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

Jump to bottom

feat: add wallet migration to askar #1144

Merged
merged 1 commit into from
Mar 15, 2024
Merged

feat: add wallet migration to askar #1144

merged 1 commit into from
Mar 15, 2024

Conversation

xprazak2
Copy link
Contributor

No description provided.

@xprazak2 xprazak2 force-pushed the mig-cp branch 4 times, most recently from cca98d5 to 23fe486 Compare March 12, 2024 09:06
@codecov-commenter
Copy link

codecov-commenter commented Mar 12, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 156 lines in your changes are missing coverage. Please review.

Project coverage is 0.05%. Comparing base (a3c5a99) to head (0cb5fc3).

Files Patch % Lines
...s/aries_vcx_core/src/wallet/base_wallet/migrate.rs 0.00% 28 Missing ⚠️
...ries/misc/wallet_migrator/src/migrate2askar/mod.rs 0.00% 25 Missing ⚠️
...s_vcx_core/src/wallet/base_wallet/base64_string.rs 0.00% 18 Missing ⚠️
...s_vcx_core/src/wallet/base_wallet/base58_string.rs 0.00% 17 Missing ⚠️
...ies_vcx_core/src/wallet/indy/indy_wallet_config.rs 0.00% 15 Missing ⚠️
aries/aries_vcx_core/src/wallet/indy/mod.rs 0.00% 15 Missing ⚠️
...aries_vcx_core/src/wallet/base_wallet/key_value.rs 0.00% 10 Missing ⚠️
aries/aries_vcx_core/src/wallet/utils.rs 0.00% 10 Missing ⚠️
...es/aries_vcx_core/src/wallet/base_wallet/record.rs 0.00% 6 Missing ⚠️
aries/misc/test_utils/src/mock_wallet.rs 0.00% 6 Missing ⚠️
... and 2 more
Additional details and impacted files 
@@          Coverage Diff          @@
##            main   #1144   +/-   ##
=====================================
  Coverage   0.05%   0.05%           
=====================================
  Files        476     479    +3     
  Lines      24134   24100   -34     
  Branches    4500    4482   -18     
=====================================
  Hits          13      13           
+ Misses     24121   24087   -34
Flag Coverage Δ
unittests-aries-vcx 0.05% <0.00%> (+<0.01%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@xprazak2 xprazak2 force-pushed the mig-cp branch 18 times, most recently from ef0638a to c4a8772 Compare March 13, 2024 07:59
Patrik-Stas
Patrik-Stas requested changes Mar 14, 2024
View reviewed changes
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.

Some changes requested. Also we are now going to remove libvcx_core and vcx_napi_rs, so you can disable the CI for those components and that should simplify your life here.
In fact feel free to open PRs to delete libvcx_core and vcx_napi_rs (+all nodejs stuff) so you won't have to deal with it at all.

.github/workflows/main.yml Outdated
@@ -100,7 +100,8 @@ jobs:
runs-on: ubuntu-20.04
strategy:
matrix:
wallet: ["vdrtools_wallet", "askar_wallet"]
main_wallet: ["main_vdrtools_wallet,vdrtools_wallet", "main_askar_wallet,askar_wallet"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's have a separate job for migration clippy check. This seems like slippery slope if we were to start combining with even more feature flag going forward.

In fact, let's keep just ["vdrtools_wallet", "askar_wallet"] here when clippy-ing workspace holistically.

.github/workflows/main.yml Outdated
@@ -399,7 +417,7 @@ jobs:
runs-on: ubuntu-20.04
strategy:
matrix:
wallet: ["vdrtools_wallet", "askar_wallet"]
wallet: ["main_vdrtools_wallet,target_askar_wallet", "main_askar_wallet,target_askar_wallet"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Integration test do not know about migrations, do they? I am sure there was technical reason which required you introduce these feature flags here, but conceptually when running normal integration tests, there should be no notion of migrations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, libvcx knows about migrations and exposes the migration as a part of its public API.

The reason for the new feature flags is that a migration has source and destination wallet and we need to decide which wallet is to be considered 'main' so that the appropriate global variable is included. Anyway, this seems to be irrelevant now, removing the vcx_napi_rs and libvcx_core will definitely make things easier.

aries/aries_vcx_core/src/wallet/base_wallet/mod.rs Outdated
@@ -123,6 +133,8 @@ mod tests {

assert_eq!(new_key.key().len(), 32);

println!("first did data: {:?}", first_data);
Copy link
Contributor

Choose a reason for hiding this comment

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

forgotten println

aries/wrappers/uniffi-aries-vcx/core/Cargo.toml Outdated
askar_wallet = ["aries_vcx/askar_wallet"]
vdrtools_wallet = ["aries_vcx/vdrtools_wallet"]
main_vdrtools_wallet = ["vdrtools_wallet"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like feature alias? I'd rather keep it simple, just keep the original feature flag names.

@Patrik-Stas
Copy link
Contributor

@xprazak2 needs some rebasing now, should be prolly easier now to simplify feature flags?

@xprazak2
Copy link
Contributor Author

Rebased, looks much cleaner now.

@Patrik-Stas
Copy link
Contributor

well done 👍

@Patrik-Stas Patrik-Stas merged commit f16be59 into main Mar 15, 2024
26 checks passed
@Patrik-Stas Patrik-Stas deleted the mig-cp branch March 15, 2024 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Patrik-Stas Patrik-Stas Patrik-Stas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@xprazak2 @codecov-commenter @Patrik-Stas