[ABW-2746] Refactor ROLA signing #1076
Conversation
giannis-rdx
force-pushed
the
task/ABW-2746-rola-signing
branch
from
032ada1
to
2906cf8
Compare
giannis-rdx
requested review from
jakub-rdx,
micbakos-rdx and
sergiupuhalschi-rdx
There was a problem hiding this comment.
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.
this is a place that needs improvement. Maybe during MFA work
giannis-rdx
force-pushed
the
task/ABW-2746-rola-signing
branch
from
87b4fa3
to
0a48694
Compare
There was a problem hiding this comment.
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.
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.
you can return response declared above.
There was a problem hiding this comment.
not really because this return returns a type of WalletToDappInteractionResponse
There was a problem hiding this comment.
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 😄 |
giannis-rdx
force-pushed
the
task/ABW-2746-rola-signing
branch
from
cb0224f
to
e2785ec
Compare
…UseCase and SignWithLedgerFactorSourceUseCase
signing related classes there because it's better
… all entities and then build the responses
giannis-rdx
force-pushed
the
task/ABW-2746-rola-signing
branch
from
e2785ec
to
b50e679
Compare
|
Description
This PR refactors the proving ownership with ROLA (again signatures are required). In short:
When connect to a dApp wallet receives an
AuthorizedRequestthat can require persona loginWithoutChallengeand loginWithChallengeand 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):
AuthorizedRequestis received and the wallet starts building the response by iterating through requested entities (accounts + personas) and asking for signature for each entity.After refactor:
AuthorizedRequestis 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