-
Notifications
You must be signed in to change notification settings - Fork 6
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
[ABW-2746] Refactor ROLA signing #1076
Conversation
032ada1
to
2906cf8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this usecase can be improved but I didn't have time to do it, because it's not easy at all!
The auth requests can be very complicated and can have several combinations (requested entities to auth).
var allEntitiesWithSignatures: Map<ProfileEntity, SignatureWithPublicKey> = entitiesWithSignatures | ||
if (challenge != null && entitiesWithSignatures.isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a place that needs improvement. Maybe during MFA work
87b4fa3
to
0a48694
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job!
A minor UI issue to note:
On 'Proving Ownership' bottom sheet there is a 'Retry' button which opens the biometric prompt. After a successful biometric authentication, there are a few seconds until the bottom sheet dismisses and the transaction result is shown when you can still press "Retry", but it doesn't make sense because in the background the transaction is being processed. Can we do something about it? Like replacing the Retry button with a loading or disabling it while the transaction is in progress?
@@ -71,6 +71,7 @@ import kotlinx.collections.immutable.ImmutableList | |||
import kotlinx.coroutines.flow.Flow | |||
import rdx.works.core.qr.QRCodeGenerator | |||
|
|||
// TODO rename status package to dialogs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just do it! :))
@sergiupuhalschi-rdx your argument is valid 👌🏽 I also noticed the same thing. I pushed a fix to disable the Retry button when wallet collects the signature for device (right after the biometric prompt auth) |
entitiesWithSignatures = entitiesWithSignaturesResult | ||
|
||
val signatureForPersona = entitiesWithSignatures[selectedPersona.asProfileEntity()] | ||
?: return Result.failure(RadixWalletException.DappRequestException.FailedToSignAuthChallenge()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can return response
declared above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not really because this return
returns a type of WalletToDappInteractionResponse
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One general question about collecting signatures in GetSignaturesViewModel, each device factor source that appear in results of getSigningEntitiesByFactorSource
is translated into one DeviceRequest
? I see in proceedToNextSigners
that biometrics are displayed for every DeviceRequest. Does it mean that for single transaction that will have multiple device factor sources we will display biometrics many times?
Actually that was a bug that I had in the previous version of this viewmodel, and actually you can try now to see that you won't get multiple time the biometric prompt by restoring an existing profile (either from cloud or file) and skip the main seed phrase during restoring.
Yes and no 😄 |
cb0224f
to
e2785ec
Compare
…UseCase and SignWithLedgerFactorSourceUseCase
signing related classes there because it's better
… all entities and then build the responses
e2785ec
to
b50e679
Compare
Quality Gate failedFailed conditions |
Description
This PR refactors the proving ownership with ROLA (again signatures are required). In short:
When connect to a dApp wallet receives an
AuthorizedRequest
that can require persona loginWithoutChallenge
and loginWithChallenge
and at the latter is where ROLA takes place. Plus the request can contain other entities (accounts) that require proof of ownership.Previously the implementation was like this (very simplified description):
AuthorizedRequest
is received and the wallet starts building the response by iterating through requested entities (accounts + personas) and asking for signature for each entity.After refactor:
AuthorizedRequest
is received, wallet gathers all requested entities and ask for signatures. Once the signatures are collected wallet proceeds with the building of the response.❗ This PR doesn't change the UI flow, the way/order of connect flow, but it adds a bottom sheet dialog when requiring proof of ownership. So for example, if previously wallet asked biometric prompt at the end of the connect flow, the same happens after refactor.
The only flow that changed is to not close the request screen when user rejects/cancel signing of an One Time Request.
How to test
Test several connect flows with Radix Sandbox (Data Requests and One Time Requests)
Test connect with any other dapp.
Video
check slack
PR submission checklist