Sign with sargon #1277

micbakos-rdx · 2024-12-10T13:36:06Z

Description

This PR uses sargon to sign transactions, subintents, and rola. This is the first time that sargon can dictate and orchestrate what UI should be shown in host, and expect a response.

Instead of using AccessFactorSourcesProxy directly we use sargon os.
Then sargon os decides on the order of factor sources, factor instances and transactions, interacts with android with WalletInteractor
Then WalletInteractor is asking for signatures with AccessFactorSourcesProxy and returns back the result
- Since we plug in sargon os components, the exceptions should always be of CommonException kind.
- The only error the host should expect from sargon os when signing is the CommonException.SigningRejected.
- The rest of the errors during signing are presented inside the bottom sheet. The user can either retry or dismiss. Such errors can be related to missing mnemonics, ledger communication or errors with the biometrics system.
GetSignaturesViewModel is now responsible of returning the signatures to sargon os. The only logic that it handles is that it groups into a single biometrics request, multiple mnemonics. There is no ordering of the factor sources as it was. The ordering is handled by signatures collector in sargon.
This view model can sign transactions, subintents and rola. In order to minimize the footprint of the view model, I created a Signable interface which works similar to a Signable in sargon.

~~### This PR is still WIP. Remaining tasks:~~

GetSignaturesViewModel needs to use BiometricsHandler for obvious reasons, but also to handle the BiometricsException gracefully.
Non fatal errors need to be shown in UI. One example is failing to sign with ledger due to blind signing being disabled. The user can fix it in their ledger device and retry without even having to reopen the bottom sheet.
Unfortunately sign auth, needs to use the signatures collector in sargon os which finds the path of least resistance when signing with multiple factors. One example is when we need to authorise multiple accounts. Some of those accounts might be configured with a ledger and some with device factor sources. Current AuthenticationSigner does not do that. So, sargonOs.signAuth needs to be integrated with signatures collector. Fortunately the android host will not change much in terms of implementation since the interactor is the sole point of friction.

giannis-rdx

excellent work!

I’m still reviewing and testing, but I’ve left some initial comments.

Additionally to the comments, you can go ahead and delete the ResolveNotaryAndSignersUseCase.

giannis-rdx · 2025-01-02T08:26:21Z

app/src/main/java/com/babylon/wallet/android/presentation/interactor/WalletInteractor.kt

+import kotlin.time.Duration.Companion.milliseconds
+
+@Singleton
+class WalletInteractor @Inject constructor(


maybe this class name should include the "sign" word, since it's all about signing?
E.g. "WalletSignerInteractor", "SignerInteractor"?
unless this WalletInteractor will implement other interactors later?

Another point to consider is that if this is solely about signing, perhaps we could move it under the accessfactorsources package?

Also you do not need the @Inject constructor here since you have already the @Provides module that does exactly this thing.

Yes it is intended to be used for other interactions as well. Although this is something that can change.

giannis-rdx · 2025-01-02T09:34:31Z

app/src/main/java/com/babylon/wallet/android/MainActivity.kt

@@ -67,6 +68,9 @@ class MainActivity : FragmentActivity() {
    @Inject
    lateinit var biometricsHandler: BiometricsHandler

+    @Inject
+    lateinit var getProfileUseCase: GetProfileUseCase


I think this is not needed, right?

I removed it (locally in my branch) and signed a couple of transactions without issues.

giannis-rdx · 2025-01-02T09:43:19Z

app/src/main/java/com/babylon/wallet/android/domain/usecases/signing/ROLAClient.kt

giannis-rdx · 2025-01-02T09:48:20Z

core/src/main/java/rdx/works/core/sargon/ProfileExtensions.kt

@@ -195,45 +194,6 @@ fun Profile.addAccounts(
    return updatedProfile.withUpdatedContentHint()
 }

-@Suppress("UnusedParameter")
-fun Profile.addAuthSigningFactorInstanceForEntity(


sergiupuhalschi-rdx

Looks nice and clean!

sergiupuhalschi-rdx · 2024-12-12T09:41:56Z

app/src/main/java/com/babylon/wallet/android/MainActivity.kt

@@ -67,6 +68,9 @@ class MainActivity : FragmentActivity() {
    @Inject
    lateinit var biometricsHandler: BiometricsHandler

+    @Inject
+    lateinit var getProfileUseCase: GetProfileUseCase


This is not invoked anywhere, is it still needed here?

sergiupuhalschi-rdx · 2025-01-02T12:03:40Z

app/src/main/java/com/babylon/wallet/android/presentation/interactor/WalletInteractor.kt

+                )
+            ).perFactorSource
+        } else {
+            request.perFactorSource.map { perFactorSource ->


I'd like to understand the behavior here. Why do we have different logic if the factorSourceKind is FactorSourceKind.DEVICE? Is it because we should only request the biometric prompt once? I assume a request with factorSourceKind == FactorSourceKind .DEVICE can have multiple perFactorSource but we need to ask for biometrics once to fulfill all of them?

Exactly as it currently stands, when FactorSourceKind.DEVICE is requested we perform poly sign. Meaning that we sign with multiple device factor sources with just one biometric. This is tbd though. One reason somewhat against this is what happens if one of those many devices misses the mnemonic. Do we fail the whole operation or we just return a signature of each device that can sign and skip the rest? How should that be depicted in the ui. I think we are going to discuss this tomorrow.

giannis-rdx · 2025-01-02T13:04:38Z

...n/java/com/babylon/wallet/android/presentation/accessfactorsources/signatures/InputOutput.kt

+                }
+            }
+
+        fun List<OutputPerFactorSource<Signable.ID.Transaction>>.into() = SignWithFactorsOutcomeOfTransactionIntentHash.Signed(


Perhaps a clearer name would be better, such as signed or toSigned?

sonarqubecloud · 2025-01-09T10:50:38Z

Quality Gate failed

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

See analysis details on SonarQube Cloud

giannis-rdx

Amazing work! 🙇🏽

micbakos-rdx self-assigned this Dec 10, 2024

micbakos-rdx force-pushed the feature/sign-with-sargon branch 2 times, most recently from 1fb76b6 to 7098d4d Compare December 24, 2024 08:24

micbakos-rdx marked this pull request as ready for review December 24, 2024 11:11

micbakos-rdx requested review from giannis-rdx and sergiupuhalschi-rdx December 24, 2024 11:11

giannis-rdx reviewed Jan 2, 2025

View reviewed changes

sergiupuhalschi-rdx approved these changes Jan 2, 2025

View reviewed changes

giannis-rdx reviewed Jan 2, 2025

View reviewed changes

micbakos-rdx added 21 commits January 9, 2025 10:16

WIP Impl of host interactor

8f14a8e

Make proxy singleton

0be3d62

Remove usage of profile use case from host interactor

5930690

Derive keys

24ac8f6

Add purpose to deriving keys

8556dfb

Add tests

eaea0f0

Check errors

37a3783

Throw specific errors back to sargon

4c435d9

Remove result api

a056998

Create auth signing use case

09a55dd

Avoid using signAuth from sargon os at the moment

536e528

WIP

67fc30e

WIP

7d2fdc3

WIP

511bb98

Use latest sargon

ad29003

WIP

905ed43

WIP

c6fc963

Error messages during signing

2ad56ce

Add delay per factor source

cf9b271

Run detekt

793e04a

Fix errors

5304354

micbakos-rdx added 6 commits January 9, 2025 10:16

WIP

fcbbf09

WIP

a554e37

Pass origin string instead of converting url to string

6a4d51b

Fix detekt issues

4c1a380

WIP

3e23833

Bump sargon version

9028a72

micbakos-rdx force-pushed the feature/sign-with-sargon branch from 6a87fad to 9028a72 Compare January 9, 2025 09:58

micbakos-rdx added 2 commits January 9, 2025 12:08

Remove unnecessary injections

9371eea

Remove import

dd871a6

giannis-rdx approved these changes Jan 9, 2025

View reviewed changes

sergiupuhalschi-rdx approved these changes Jan 9, 2025

View reviewed changes

micbakos-rdx merged commit e26ad0d into main Jan 9, 2025
10 of 11 checks passed

micbakos-rdx deleted the feature/sign-with-sargon branch January 9, 2025 15:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sign with sargon #1277

Sign with sargon #1277

micbakos-rdx commented Dec 10, 2024 •

edited

Loading

giannis-rdx left a comment

giannis-rdx Jan 2, 2025

giannis-rdx Jan 2, 2025

micbakos-rdx Jan 6, 2025

giannis-rdx Jan 2, 2025

giannis-rdx Jan 2, 2025

giannis-rdx Jan 2, 2025

sergiupuhalschi-rdx left a comment

sergiupuhalschi-rdx Dec 12, 2024

sergiupuhalschi-rdx Jan 2, 2025

micbakos-rdx Jan 6, 2025

giannis-rdx Jan 2, 2025

sonarqubecloud bot commented Jan 9, 2025

giannis-rdx left a comment

Sign with sargon #1277

Sign with sargon #1277

Conversation

micbakos-rdx commented Dec 10, 2024 • edited Loading

Description

giannis-rdx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergiupuhalschi-rdx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 9, 2025

Quality Gate failed

giannis-rdx left a comment

Choose a reason for hiding this comment

micbakos-rdx commented Dec 10, 2024 •

edited

Loading