[ABW-2744] Access factor sources refactoring - create accounts #761

giannis-rdx · 2024-01-23T09:52:41Z

Description

This PR:

introduces a new CreateAccountUseCase that accepts only factor sources that are able to create account, see FactorSource.CreatingEntity interface. Removed CreateAccountWithBabylonDeviceFactorSourceUseCase and CreateAccountWithLedgerFactorSourceUseCase
refactors creating account extensions/functions in the profile module to support the above use case
adds a AccessFactorSourceProvider in the profile module that access the MnemonicRepository and the ProfileRepository in order to calculate the derivation path and to derive the public key
adds a AccessFactorSourceProxy that works as a mediator between the UI (bottomsheet), and the "clients" who request access to factor sources, e.g. CreateAccountViewModel requires a public key of the factor source to create an account
refactors the ChooseLedgerScreen to return the selected ledger device and to not derive the public key

This PR is a chained PR of the #750

How to test

Please test it not only by creating accounts with device and ledger but also by switching networks and creating accounts during the flows that creation is possible (e.g. when login to a dapp)

Video

Screen.Recording.2024-01-23.at.11.38.59.AM.mov

PR submission checklist

I have tested account creation in different flows
I have written one unit test

giannis-rdx · 2024-01-24T09:13:29Z

...java/com/babylon/wallet/android/presentation/account/createaccount/CreateAccountViewModel.kt

+            // if main babylon factor source is not present, it will be created during the public key derivation
+            val publicKeyAndDerivationPath = accessFactorSourceProxy.getPublicKeyAndDerivationPathForFactorSource(
+                accessFactorSourceInput = AccessFactorSourceInput.ToCreateAccount(
+                    forNetworkId = onNetworkId,
+                    factorSource = if (isWithLedger && selectedFactorSource != null) {
+                        selectedFactorSource
+                    } else {
+                        null
+                    }
+                )
+            )


actually here is the part that I don't like and we discussed about it in the chapter meeting.
What it should be ideal here is to pass an already created babylon device factor source and not to pass the null.

app/src/main/java/com/babylon/wallet/android/di/ViewModule.kt

...va/com/babylon/wallet/android/presentation/accessfactorsource/AccessFactorSourceProxyImpl.kt

micbakos-rdx · 2024-01-24T16:11:51Z

profile/src/main/java/rdx/works/profile/data/repository/AccessFactorSourceProvider.kt

+import rdx.works.profile.derivation.model.NetworkId
+import javax.inject.Inject
+
+class AccessFactorSourceProvider @Inject constructor(


I am a bit confused with the purpose of this class. It does not access any factor sources, it looks like to me a wrapper of methods that will get increasingly bigger in the future like a utils class. Both methods below have no connection together.

my initial idea is to push all the related to access factor sources (either derive public key or signing) logic in the profile module and keep the viewmodel of the bottomsheet with the minimum logic. And then this "provider" would return the result (public key, signers) to the UI.
However, that's not really possible because some factor sources e.g. ledger are off device.

But I still think to proceed with this way and see how this will escalate. Maybe this provider can be a "AccessOnDeviceFactorSourcesWhatever".

This refactoring is definitely not in the final version( in this PR) and it's in my mind that design can change later when implementing the rest of factor sources.

also keep in mind that this class would go away with the shared lib 😄 and this is one more reason to keep the view with the minimum access factor sources logic 😉

Understood, I get your point

micbakos-rdx · 2024-01-24T16:13:22Z

.../com/babylon/wallet/android/presentation/accessfactorsource/AccessFactorSourceBottomSheet.kt

+    isAccessingFactorSourceInProgress: Boolean,
+    isAccessingFactorSourceCompleted: Boolean,
+    showContentForFactorSource: AccessFactorSourceUiState.ShowContentFor,


Currently this content is only for deriving public keys, correct? If so we should separate each case on its own composable or have different dialogs for different cases. Like

Instead of naming this AccessFactorSourceBottomSheet we could name it like DerivePublicKeysDialog. Then we can create other dialogs that do other things with factor sources and let the app event decide what dialog to open.

Yeah it seems that one dialog might be too much to handle all the factor sources., and that's also in my mind for a future change. But my answer here is similar to the above. Let's see how it will really escalate and proceed step by step.

Btw I am not gonna merge this PR, it needs testing from us and Umair, and if all looks fine then I will merge.

jakub-rdx

Overall looks good and works fine. Nice work. One thing that might be a bug is scenario when you create first time account with ledger. Please see attached video and let me know if doing biometrics twice is expected here:

screen-20240129-124527.mp4

giannis-rdx · 2024-01-30T13:13:39Z

Overall looks good and works fine. Nice work. One thing that might be a bug is scenario when you create first time account with ledger. Please see attached video and let me know if doing biometrics twice is expected here:

yeah right, I also spotted this last week and am aware of it. Thanks for clarifying it!
Once I am back to access factor sources refactoring I will investigate.

create accounts with an interface

the profile module and update ProfileGenerationTest

and its unit test

not to derive public key from it

the new CreateAccountUseCase

when purpose is to derive public key

AccessFactorSourcesViewModel

raf-rdx

Tested works fine 👍

sonarcloud · 2024-02-13T15:39:53Z

Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 40%)

See analysis details on SonarCloud

giannis-rdx requested review from raf-rdx, jakub-rdx and micbakos-rdx January 23, 2024 09:52

giannis-rdx self-assigned this Jan 23, 2024

giannis-rdx commented Jan 24, 2024

View reviewed changes

micbakos-rdx reviewed Jan 24, 2024

View reviewed changes

giannis-rdx force-pushed the feature/ABW-2743-create-basic-ui-for-bottom-sheet branch from af736ef to 688feb7 Compare January 24, 2024 17:42

giannis-rdx force-pushed the feature/ABW-2744-access-factor-sources-create-accounts branch from 3eee9f8 to fe9fcec Compare January 24, 2024 17:58

micbakos-rdx approved these changes Jan 25, 2024

View reviewed changes

jakub-rdx suggested changes Jan 29, 2024

View reviewed changes

giannis-rdx force-pushed the feature/ABW-2743-create-basic-ui-for-bottom-sheet branch from 688feb7 to 4825e52 Compare February 5, 2024 08:42

giannis-rdx force-pushed the feature/ABW-2744-access-factor-sources-create-accounts branch from e3a2419 to 3431b7c Compare February 5, 2024 10:02

giannis-rdx force-pushed the feature/ABW-2743-create-basic-ui-for-bottom-sheet branch from 5376eb4 to e525b75 Compare February 6, 2024 13:32

giannis-rdx force-pushed the feature/ABW-2744-access-factor-sources-create-accounts branch from b9c754d to 2110b4e Compare February 6, 2024 13:36

giannis-rdx requested a review from jakub-rdx February 6, 2024 14:20

giannis-rdx force-pushed the feature/ABW-2743-create-basic-ui-for-bottom-sheet branch from e525b75 to feed700 Compare February 9, 2024 16:24

giannis-rdx added 13 commits February 9, 2024 18:24

make LedgerHardwareWalletFactorSource and DeviceFactorSource able to

96af5cc

create accounts with an interface

added CreateAccountExtensions and replaced them in use cases of

849f61d

the profile module and update ProfileGenerationTest

added AccessFactorSourceProvider that provides access to factor sources

28ba9f3

added new CreateAccountUseCase which takes a CreateEntity factor source

c2cb59d

and its unit test

updated ChooseLedgerScreen to only return the selected ledger and

53bf3c8

not to derive public key from it

added AccessFactorSourceProxyContract

db5d211

added implementation of AccessFactorSourceProxy

46d8378

refactored CreateAccount screen to use the AccessFactorSourceProxy and

cb41e6e

the new CreateAccountUseCase

implemented logic for AccessFactorSourceBottomSheet and its viewmodel

e44f280

removed old use cases to create account

3867ecb

removed wrong interface from AccessFactorSourceProxyImpl

248935a

renamed ViewModule to UiModule

820d84e

renamed every AccessFactorSource to AccessFactorSources

94a4293

giannis-rdx added 11 commits February 9, 2024 18:24

fixed detekt

167c2d6

fixed unit test ChooseLedgerViewModelTest

86d2010

renamed app event ToCreateAccount to DeriveAccountPublicKeys

1e6eab7

refactored AccessFactorSourcesProxyContract to return failure

f26f3f3

renamed LedgerSelectionPurpose CreateAccount to DerivePublicKey

725dfa0

do not ensureBabylonFactorSourceExistUseCase in ChooseLedgerViewModel

8df6a48

when purpose is to derive public key

ensureBabylonFactorSourceExistUseCase when derive public key in

41783dd

AccessFactorSourcesViewModel

one off event for biometric prompt in AccessFactorSourcesViewModel

ff2236e

next line

373fb44

better naming

253abe0

fixed naming

fb12bc4

giannis-rdx force-pushed the feature/ABW-2744-access-factor-sources-create-accounts branch from ff8318e to fb12bc4 Compare February 9, 2024 16:25

giannis-rdx added 2 commits February 12, 2024 11:27

removed unnecessary val ledger in DeriveAccountPublicKey app event

e03ef9d

better names for AccessFactorSources functions

c7dbac7

raf-rdx approved these changes Feb 12, 2024

View reviewed changes

giannis-rdx added 3 commits February 12, 2024 15:23

minor

73924e5

fixed detekt

7fc3927

removed unnecessary public reset

70b53d7

giannis-rdx merged commit 115fc02 into feature/ABW-2743-create-basic-ui-for-bottom-sheet Feb 13, 2024
8 of 9 checks passed

giannis-rdx deleted the feature/ABW-2744-access-factor-sources-create-accounts branch February 13, 2024 18:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ABW-2744] Access factor sources refactoring - create accounts #761

[ABW-2744] Access factor sources refactoring - create accounts #761

giannis-rdx commented Jan 23, 2024

giannis-rdx Jan 24, 2024

micbakos-rdx Jan 24, 2024

giannis-rdx Jan 24, 2024

giannis-rdx Jan 24, 2024 •

edited

Loading

micbakos-rdx Jan 25, 2024

micbakos-rdx Jan 24, 2024

giannis-rdx Jan 24, 2024 •

edited

Loading

micbakos-rdx Jan 25, 2024

jakub-rdx left a comment •

edited

Loading

giannis-rdx commented Jan 30, 2024

raf-rdx left a comment

sonarcloud bot commented Feb 13, 2024

[ABW-2744] Access factor sources refactoring - create accounts #761

[ABW-2744] Access factor sources refactoring - create accounts #761

Conversation

giannis-rdx commented Jan 23, 2024

Description

How to test

Video

PR submission checklist

giannis-rdx Jan 24, 2024

Choose a reason for hiding this comment

micbakos-rdx Jan 24, 2024

Choose a reason for hiding this comment

giannis-rdx Jan 24, 2024

Choose a reason for hiding this comment

giannis-rdx Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

micbakos-rdx Jan 25, 2024

Choose a reason for hiding this comment

micbakos-rdx Jan 24, 2024

Choose a reason for hiding this comment

giannis-rdx Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

micbakos-rdx Jan 25, 2024

Choose a reason for hiding this comment

jakub-rdx left a comment • edited Loading

Choose a reason for hiding this comment

giannis-rdx commented Jan 30, 2024

raf-rdx left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Feb 13, 2024

Quality Gate failed

giannis-rdx Jan 24, 2024 •

edited

Loading

giannis-rdx Jan 24, 2024 •

edited

Loading

jakub-rdx left a comment •

edited

Loading