feat: add wallet migration to askar #1144
Conversation
xprazak2
force-pushed
the
mig-cp
branch
4 times, most recently
from
cca98d5
to
23fe486
Compare
xprazak2
force-pushed
the
mig-cp
branch
18 times, most recently
from
ef0638a
to
c4a8772
Compare
There was a problem hiding this comment.
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"] | |||
There was a problem hiding this comment.
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"] | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -123,6 +133,8 @@ mod tests { | |||
|
|
|||
| assert_eq!(new_key.key().len(), 32); | |||
|
|
|||
| println!("first did data: {:?}", first_data); | |||
| askar_wallet = ["aries_vcx/askar_wallet"] | ||
| vdrtools_wallet = ["aries_vcx/vdrtools_wallet"] | ||
| main_vdrtools_wallet = ["vdrtools_wallet"] |
There was a problem hiding this comment.
Seems like feature alias? I'd rather keep it simple, just keep the original feature flag names.
|
@xprazak2 needs some rebasing now, should be prolly easier now to simplify feature flags? |
Signed-off-by: Ondrej Prazak <ondrej.prazak@absa.africa>
|
Rebased, looks much cleaner now. |
|
well done 👍 |
No description provided.