Keycard signing for dapp interactions #21785
Conversation
clauxx
requested review from
alwx,
shivekkhurana,
flexsurfer,
smohamedjavid and
mohsen-ghafouri
Jenkins BuildsClick to see older builds (8)
|
|
this PR should wait for #21753 , because auth on keycard has little bit changed |
| @@ -8,7 +8,8 @@ | |||
| [:auth-button-label {:optional true} [:maybe string?]] | |||
| [:auth-button-icon-left {:optional true} [:maybe keyword?]] | |||
| [:blur? {:optional true} [:maybe boolean?]] | |||
| [:theme {:optional true} [:maybe :schema.common/theme]]]) | |||
| [:theme {:optional true} [:maybe :schema.common/theme]] | |||
| [:keycard-supported? {:optional true} [:maybe boolean?]]]) | |||
There was a problem hiding this comment.
we don't have this param anymore
| @@ -45,6 +45,7 @@ | |||
| [standard-authentication/slide-button | |||
| {:size :size-48 | |||
| :track-text slide-button-text | |||
| :keycard-supported? true | |||
There was a problem hiding this comment.
:on-complete (when sign-on-keycard? #(on-auth-success ""))
There was a problem hiding this comment.
@flexsurfer what's the difference between the :on-complete and :on-auth-success props here? Looking at the one used in send (status-im.contexts.wallet.send.transaction-confirmation.view) and seems like the changes are recent:
[standard-auth/slide-button {...
:on-complete (when sign-on-keycard?
#(rf/dispatch
[:wallet/prepare-signatures-for-transactions
:send
""]))
:on-auth-success
(fn [psw]
(rf/dispatch
[:wallet/prepare-signatures-for-transactions :send
psw]))}]There was a problem hiding this comment.
on-complete is used when you need to sign on keycard, on-auth-success used if you need password (for keycard also, it will work)
There was a problem hiding this comment.
it confusing yes, when we want to sign on keycard, we just need a signal from the slide button that it was activated, we don't need auth process
There was a problem hiding this comment.
thanks! yes we might need to rethink this, since now if we pass :on-complete, then :on-auth-success will never be called. It might make sense to add the keycard (or signing in general) into standard-auth.
There was a problem hiding this comment.
yes we might need to rethink this
100%, thank you
| (fn [{:keys [db]} [password]] | ||
| (let [prepared-hash (get-in db [:wallet-connect/current-request :prepared-hash]) | ||
| address (get-in db [:wallet-connect/current-request :address]) | ||
| keycard-sign? (-> (get-in db [:wallet :keypairs]) |
There was a problem hiding this comment.
you need to move this outside, because there are two options
- keypair is keycard we need to sign on keycard
- profile is keycard, means we need to get password from keycard and sign regular way with password
There was a problem hiding this comment.
how can we know whether we need to actually sign on the keycard? I see that there's (get-in transaction-for-signing [:signingDetails :signOnKeycard]), but that comes from the router, which we don't use for dapps.
There was a problem hiding this comment.
keycard-sign? is correct you can use it, to know that you need to sign on keycard, but you need to move it outside of this event, you need to move this code
(if keycard-sign?
{:fx [[:dispatch
[:standard-auth/authorize-with-keycard
to where :on-complete is used
There was a problem hiding this comment.
im not sure why you need check again, you already passed the empty password?
50% of end-end tests have passedFailed tests (4)Click to expandClass TestWalletMultipleDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Passed tests (4)Click to expandClass TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
|
clauxx
force-pushed
the
cl-21618-dapps-keycard-signing
branch
from
0983e41 to
c5e6a89
Compare
clauxx
force-pushed
the
cl-21618-dapps-keycard-signing
branch
from
4eaab72 to
7b40486
Compare
clauxx
force-pushed
the
cl-21618-dapps-keycard-signing
branch
from
7b40486 to
a2aa35c
Compare
| :hashes [prepared-hash] | ||
| :on-success (fn [signatures] | ||
| (rf/dispatch [:hide-bottom-sheet]) | ||
| (-> signatures first :signature on-success)) |
There was a problem hiding this comment.
Ambiguity in Intent
The use of -> in this context suggests that the operations are straightforward transformations of signatures. However, the final step (on-success) is a function call, not a pure transformation. This mix of data access (first and :signature) and side effects (calling on-success) makes the threading less intuitive.
Threading for Side Effects
The threading macro is typically used for chaining transformations, not invoking side-effectful functions. Here, on-success is likely a callback or effect, which introduces a conceptual break in the chain.
Reduced Readability
It’s not immediately clear that the result of :signature is being passed as an argument to on-success. This could be more explicit without using ->, especially since the threading macro is more suitable for operations where each step returns a value for the next.
Potential for Misleading Assumptions
Readers might mistakenly assume that on-success is part of a chain of transformations, rather than a separate effectful operation, which can lead to confusion about the purpose of the code.
(fn [signatures]
(rf/dispatch [:hide-bottom-sheet])
(on-success (:signature (first signatures))))There was a problem hiding this comment.
Wasn't aware -> was constrained to data transformations and that it shouldn't be mixed with other types of operations, but if it affects readability here, can remove/simplify it.
threading macro is more suitable for operations where each step returns a value for the next.
Well yeah, for threading to work they have to return a value, but the last op. doesn't really have to return.
Readers might mistakenly assume that on-success is part of a chain of transformations, rather than a separate effectful operation, which can lead to confusion about the purpose of the code.
I assume we're all familiar with what on-success does and that it's not a pure transformation, given how often it's used
p.s. this reads a bit like GPT 😄
| (fn [{:keys [db]} [password]] | ||
| (let [prepared-hash (get-in db [:wallet-connect/current-request :prepared-hash]) | ||
| address (get-in db [:wallet-connect/current-request :address]) | ||
| keycard-sign? (-> (get-in db [:wallet :keypairs]) |
There was a problem hiding this comment.
im not sure why you need check again, you already passed the empty password?
fixes #21618
Summary
eth_sign,eth_signTransaction(previously could only be tested on test dapps)Steps to test
status: ready