Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add Signer to Api #660

Closed
ianhe8x opened this issue Feb 1, 2019 · 3 comments
Closed

Add Signer to Api #660

ianhe8x opened this issue Feb 1, 2019 · 3 comments

Comments

@ianhe8x
Copy link
Contributor

ianhe8x commented Feb 1, 2019

As a dapp, it doesn't always have access to key pair. Key pairs could be managed in a browser extension or hardware wallet. So I propose to introduce signer to api and let it do the signing. This will allow our browser extension to work with the api by exposing a signer into browser's global.

export interface Signer {
  sign(extrinsic: Extrinsic, opt: {from: string} & SignatureOptions): Promise<void>;
}

signAndSend is the place to use the signer. I prefer to changing the signature to below which looks more neat

  signAndSend (options: SignAndSendOptions)
  signAndSend (options: SignAndSendOptions, statusCb: StatusCb)
  signAndSend (options: SignAndSendOptions, statusCb?: StatusCb)

interface SignAndSendOptions extends Partial<SignatureOptions> {
  from: string | KeyringPair
}

though to keep backward compatible, it could also be like:

signAndSend (account: KeyringPair | string, _options?: Partial<SignatureOptionsPartial> | StatusCb, statusCb?: StatusCb)

I would like to hear from you if it's the right direction before submitting my PR.

@jacogr
Copy link
Member

jacogr commented Feb 2, 2019

First going to comment on the signatures: they certainly a bit messy atm, we are not quite happy, but it is moving towards being more useful. We just always try to keep backwards compat, so happy with changes that cleans up the code as long as it does not affect downstream users.

Honestly, have not considered the signers up to now, which is certainly a gap. (The apps UI manages this by putting stuff on a queue when sent - which is not great) So happy to look at any suggestions in this area, it is certainly something that needs to be addressed.

We should probably keep the signature as you suggested as a last point - would just allow account: KeyringPair | string | AccountId | Address as well, the latter 2 can just be toString-ed. (Technically we need AccountIndex support as well, which is also part of Address - but for v1, can forgo that and the lookup conversion)

TL;DR good idea, we certainly need support sooner rather than later

EDIT: Re-wrote since some stuff (commented on via phone, was a bit less than sensical)

@jacogr jacogr added this to the 1.0 milestone Feb 4, 2019
@jacogr
Copy link
Member

jacogr commented Feb 9, 2019

Closed in #662

@polkadot-js-bot
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue if you think you have a related problem or query.

@polkadot-js polkadot-js locked as resolved and limited conversation to collaborators Jun 5, 2021
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants