PKG -- [sdk] Use keyID from signatures

[scottphc]

The reason why this is crucial for us is that for our non-custodial users, each device they login with have a different key. When a transaction request appears, it is pushed to all of their devices and we can't know in advance which device they will use to sign the transaction.

cc @boczeratul

[changeset-bot]

🦋 Changeset detected

Latest commit: a2dfa14

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@onflow/sdk	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[JeffreyDoyle]

Hey @scottphc!

Thank you for submitting this PR! I'm wondering if FCL's existing pre-authz mechanics will work for this use case ? During pre-authz, a wallet can specify which key(s) on which account(s) will act for the various roles of the transaction. If the wallet can detect which account/key combination(s) the user wishes to use for the transaction during pre-authz, and return that information, this should allow the existing mechanics of FCL to work.

Also i'm thinking that changing the account/keys combinations(s) after authz might cause issues for the multi-signer/multi-authorizer scenario, where potentially multiple signers may wish to know exactly which keys on which other accounts will be used in a transaction.

[scottphc]

Hi @JeffreyDoyle!

Unfortunately, pre-authz can't resolve this issue. During pre-authz, we can't know the non-custodial user want to use which key to sign. We send out push notifications to the user's all devices when authz, and then the user choose one of devices to check transaction details and confirm the transaction. That's why we need to update it after authz.

I'm thinking that authorizers in payload are only addresses and the keyId may not affect other signers/authorizers?

[jribbink]

Hey @scottphc!

The FCL team has had a lot of discussions surrounding this PR. The consensus we have ultimately landed on here is that this is something that should be able to be done in the pre-authz service if the necessary adjustments to your existing architecture are implemented.

The suggestion here would be to do a lot of what you traditionally do in the authz service in pre-authz step instead. This would look something like the following:

1. Pre-authz service is executed, and supplied with the transaction payload.
2. Here, your backend would push notifications to sign this payload to each of the user's devices.
3. The user would go to their device, choose to sign the transaction, and this signature and keyId could be sent back to your backend. This could be store on your backend for later use in the authz service.
4. Now that the keyId the user wishes to use is known, your pre-authz service would resolve with an authz service for the authorizer corresponding to the keyId the user has chosen to use. I believe this is similar to what Blocto already does for transaction payers.

   {
       ...
       type: "authz",
       method: "HTTP/POST",
       endpoint: MY_ENDPOINT_TO_SIGNATURE,
       "identity": {
           "address": ADDRESS_HERE,
           "keyId": KEY_ID_HERE
       },
       "params": {
           "sessionId": SOME_SESSION_ID_TO_IDENTIFY_SIGNATURE
       }
   }

5. After pre-authz has resolved, these authz endpoints will be called by FCL, resulting in the user having the appropriate signatures.

I understand the inconvenience associated with implementing these changes, so I just wanted to give some context on why this PR won't be merged. The reasoning boils down to what the authz service is intended for and how merging this PR could break existing design patterns. The authz service intends to retrieve a signature for a particular keyId and having the authz function redefine the key the transaction is being signed would undermine this pattern. We want to maintain this breaking away from this pattern could have further consequences in FCL.

Do you see any reason that this approach may not work?

[bluesign]

I think PR here is the correct approach. I don't understand why it is not merged?

KeyId is part of signature, am I missing something here? I think this is an FCL bug that you need to provide keyId in advance.

It shouldn't matter to dapp which keys I choose to provide, as long as I provide valid keys and have total key weight. I think @tarakby can explain better than I do.

The authz service intends to retrieve a signature for a particular keyId

I think this is bad design, building on this assumption is the problem here. FCL should not make any assumptions on its own like this. If I can build a transaction with cli, send people to sign (with arbitrary keyId), and send to network, and network says this is valid transaction, SDK putting arbitrary limits on this flow is fault of SDK.

We want to maintain this breaking away from this pattern could have further consequences in FCL.

I think we can assure backwards compatibility here easily, by overriding keyId only if provided in the signature. ( which is already done in the PR )

[tarakby]

Thanks for the tag!

It shouldn't matter to dapp which keys I choose to provide, as long as I provide valid keys and have total key weight.

This sentence sounds valid to me, as long as the signers provide the keys they used for each signature.

Unfortunately I am not familiar with FCL use cases and not sure I understood the arguments above correctly (I work more on the protocol side). I'll ping the FCL guys to have a look again at this issue.

[bymi15]

Hi, are there any updates on this?
Looks like Blocto have released their own branched version of FCL @blocto/fcl@^1.4.0 with this fix, so we can use that version of FCL to resolve this issue for non-custodial Blocto wallets. But this doesn't seem to be a great idea in terms of maintainability.

[bjartek]

Blocto is now sunsetting their v1 wallet and this PR has not been resolved yet?
https://docs.blocto.app/web-wallet-v1-sunset-notice-and-migration-guide

They recommend that dapps use their fork of fcl?

[jribbink]

Hey everyone. I've just opened a FLIP with proposed changes to the FCL specification in order to solve this issue. onflow/flips#215

We should try to move discussion of this issue here :)