paritytech / substrate-connect

Run Wasm Light Clients of any Substrate based chain directly in your browser.
https://paritytech.github.io/substrate-connect/
GNU General Public License v3.0
246 stars 78 forks source link

Add and stabilize a basic interface for signing transactions #2468

Open jsdw opened 2 months ago

jsdw commented 2 months ago

We currently have a smoldot-v1 interface for communicating with a Smoldot in an extension, and a substrate-connect-unstable interface for talking with extensions like Substrate Connect (this hasn't been given much love I think).

What we want is to be able to stabilise some interface that libraries like PJS can use to have transactions signed. If we have this, we can migrate things like PJS to using this new interface so that they aren't stuck relying on the PJS based one which has various issues including no standard way to handle new signed extensions.

How about an interface like this?:

/** The interface we'll expose */
type Interface = {
    /** Subscribe to acocunts. The callback fires whenever the list of acocunts changes */
    subscribeAccounts: (callback: (accounts: Account[]) => void) => void
    /** Sign a transaction, handing back the signed bytes ready to submit */
    signTransaction: (transaction: Transaction) => Promise<SignedTransaction>
}

/** Details about an available account */
type Account = {
    /** Often this will be a MultiAddress, or an AccountID20 on eth chains */
    address: Bytes
    /** Name to present with the account. */
    name: string
}

/** 
 * The information required to construct a transaction. We can ask to 
 * construct either a V4 or a V5 transaction.
 */
type Transaction = {
    output: 'v4-signed',
    address: Bytes,
    transactionExtensions: Bytes,
    callData: Bytes
} | {
    output: 'v5-signed',
    address: Bytes,
    transactionExtensionsVersion: number,
    transactionExtensions: Bytes,
    callData: Bytes
} | {
    output: 'v5-general',
    transactionExtensionsVersion: number,
    transactionExtensions: Bytes,
    callData: Bytes
}

/** Bytes representing the signed transaction, ready to submit */
type SignedTransaction = ArrayBuffer

/** Some binary data, in the form of a hex encoded string (0x prefixed) or raw bytes */
type Bytes = string | Uint8Array | ArrayBuffer;

The aim here is to be fairly minimal, but allow getting accounts (so that we know what the extension is able to sign) and signing transactions (which here allows you to ask for a V4 or V5 transaction and then provide the relevant details required to construct it.

\cc @josepot

jsdw commented 2 months ago

@ryanleecode asked about V5 general extensions, and this raises a good point.

To construct a V5 General extrinsic, the signature will live inside one of the extensions rather than as its own field. This means that a wallet would need to be able to insert the bytes for this extension into whatever extension bytes are given in order to sign it.

Given this, perhaps instead of providing the transaction extensions as one set of bytes, we should provide a key/value map from extension name to extension bytes like so:

wallet.signTransaction({
    output: 'v5-general',
    transactionExtensionsVersion: 0,
    transactionExtensions: {
        'CheckMortality': { encoded: '0xaabbcc', signerPayload: '0x1122' },
        'CheckNonce': '0x112233',
        // ...
    },
    callData: '0x1122334455aacc'
});

If a signed extension has "additional" data that makes it into the signer payload as well as data of its own, we can specify the data here with 'encoded' and 'signerPayload'. If one hex value is given we could default to assuming that there is no signerPayload, or we could deny this and just be explicit.

Then, a wallet can append these bytes together as dictated by the metadata (complaining if any extension bytes are required but not provided) and also add in the signature bytes as needed in the process in order to sign the thing.

What do you guys reckon?

josepot commented 1 month ago

I would like to propose that we first focus on defining the basic and most important interfaces before tackling the more complex ones. Specifically, I suggest we start by defining a new signature method to replace the problematic PJS signPayload.

In an ideal scenario, the extension would not only handle the creation of transactions but also manage the broadcasting process, while keeping the consumer informed about the transaction's status. This approach would allow us to have a single source of truth for the nonce, which is currently a significant challenge. However, this is beyond the scope of the interface I'm proposing here. For now, my focus is on making the current PJS extension interface library-agnostic.

Before proceeding, we need to agree on a couple of premises:

  1. The extension can access the latest version of the chain's metadata for the transaction. We don't need to standardize how this is achieved at this point. For example, the PJS extension could implement an interface for registering a callback that the extension invokes whenever it needs to request the latest metadata from the dApp. Alternatively, adding light-client support to the PJS extension could allow it to obtain the latest metadata trustlessly.

  2. The extension can access block information about the current tip of the transaction's chain. Again, it's not crucial to define how this is accomplished right now; what's important is that we agree that this premise can be fulfilled by all extensions. This is important because, to display the transaction's mortality properly to the user, the extension must have access to this information. This requirement is the main reason why the block number is passed as part of the payload in the problematic signPayload method.

Assuming we agree on these premises, I propose the following interface:

// Indicates that the string content must be a valid hexadecimal value
type HexString = string;

type BinaryData = Uint8Array | HexString;

type CreateTransaction = (
  from: BinaryData, // public key
  callData: BinaryData,
  signedExtensions: Record<string, BinaryData>
) => Promise<HexString>;

A few important notes:

  1. The keys of the signedExtensions parameter must match the identifier property found in the extrinsic.signedExtensions entry of the metadata. The values are a SCALE encoded tuple containing (value, additionalSigned).

  2. The extension should be able to identify the chain for which this transaction is for based on the values of the signedExtensions record (i.e thanks to the value of the CheckGenesis signed-extension, however this is something that could change in the future and it would be the responsibility of the extension to map that transaction to the correct chain based on the signedExtensions)

  3. The consumer doesn't need to specify the type of transaction they want back; the extension can decide whether to assemble a version 5 or version 4 transaction. If the consumer wants to know the specifics of the transaction, they can decode it and examine it themselves.

  4. The extension has the right to ask the user for additional input, allowing the user to modify some parameters (wrapping the tx in a multisig, changing the tip, changing the mortality, etc). When the dApp receives the final transaction, it can decide whether to broadcast it or not.

By starting with this fundamental interface, we can create a more flexible and library-agnostic foundation for transaction creation and signing, paving the way for future enhancements and more complex functionalities.

I want to re-iterate the fact that this suggestion is just for replacing the problematic PJS signPayload interface.

jsdw commented 1 month ago

Ok that makes sense. I had assumed that we'd also want access to account information in order that we know what "from" address to use (else we'll need to rely on the old interface or whatever for that stuff), but I also understand the reaosning behind just fixing this specific interface and not worrying about the rest yet so sounds good to me!

jsdw commented 1 month ago

@josepot / @carlosala, the thing I was asking in the call was:

If I have some extensions which expose this createTransaction interface, and I have a set of addresses which I've obtained via the "old" interfaces, then how do I know which extension(s) are able to sign a given address?

In the "old" APIs (and please correct me if wrong; I haven't looked much at this code) I'd likely use this method to get a list of available addresses: https://github.com/polkadot-js/extension/blob/master/packages/extension-dapp/src/bundle.ts#L164. Then, for a given address, I can use the source field, and call https://github.com/polkadot-js/extension/blob/master/packages/extension-dapp/src/bundle.ts#L242 to get the associated extension and then .signer.signPayload(sillyJsonPayload).

I guess since every provider in the new interface hands back some info like this:

export type ProviderInfo = {
  uuid: string
  name: string
  icon: string
  rdns: string
}

We can map that source that we get back in the old interface to one of the fields here? Or perhaps we can just add a source field to this new interface for now (acknowledging that this is a bit crappy) so that it's obvious how to associate addresses from old interface with the extensions that provided them from the new interface?

josepot commented 1 month ago

If I have some extensions which expose this createTransaction interface, and I have a set of addresses which I've obtained via the "old" interfaces, then how do I know which extension(s) are able to sign a given address?

If an extension has previously provided an address via its API, you can infer that it might be able to sign a transaction for that address. However, you can never be completely certain that the extension can actually sign a transaction for a given address. This is true regardless of the API design.

An extension could give you an address for which it doesn’t control the private key, and simply reject the signing request when you try to sign a transaction. For example, the extension might provide an address linked to a Ledger device because the user imported it. But if the Ledger device isn't connected at the time, the extension won’t be able to sign anything.

Additionally, advanced users might create accounts without controlling the private key and still have the extension expose those accounts. In every scenario, the most you can know is that the extension might be able to sign a transaction. There's no guarantee beyond that.

In the "old" APIs (and please correct me if wrong; I haven't looked much at this code) I'd likely use this method to get a list of available addresses: https://github.com/polkadot-js/extension/blob/master/packages/extension-dapp/src/bundle.ts#L164. Then, for a given address, I can use the source field, and call https://github.com/polkadot-js/extension/blob/master/packages/extension-dapp/src/bundle.ts#L242 to get the associated extension and then .signer.signPayload(sillyJsonPayload).

I guess since every provider in the new interface hands back some info like this:

export type ProviderInfo = {
  uuid: string
  name: string
  icon: string
  rdns: string
}

We can map that source that we get back in the old interface to one of the fields here? Or perhaps we can just add a source field to this new interface for now (acknowledging that this is a bit crappy) so that it's obvious how to associate addresses from old interface with the extensions that provided them from the new interface?

I don’t see how this point is relevant to the current discussion. Could we please concentrate on finding a clear and effective replacement for the problematic PJS signPayload API? 🙏

The urgent priority is to revise the public interface that PJS extensions use, so they can be decoupled from the arbitrary implementation details of PJS ASAP. Once we’ve addressed that, we’d be happy to engage in discussions around any new standard interface for a "Polkadot Provider" that Parity may propose.

jsdw commented 1 month ago

If an extension has previously provided an address via its API, you can infer that it might be able to sign a transaction for that address. However, you can never be completely certain that the extension can actually sign a transaction for a given address. This is true regardless of the API design.

Yup, I get all of this.

All I'm saying is that, using purely the old interface, an app is able pass an address to the extension that it learned about it from, and have a reasonable chance that it would then be signable by it.

When addresses come from an old interface and signing goes via the new one, an app literally has to guess which of the extensions providing this new createTransaction interface to use to try and sign an address? This is what I want to avoid. I think it's reasonable to have a solution for this before we commit to this new interface.

Or to phrease it another way: what steps would you expect an app to take now?

In the old interface:

  1. Get addresses
  2. App user picks an address they want to use
  3. Look at address.source, and retrieve the associated extension to then signTransaction with it.

With this new interface, the best I can think of is:

  1. Get addresses via old interface
  2. App puser picks an address they want to use
  3. Get things that can createTransaction via new interface
  4. Randomly pick one and hope for the best?

I hope I'm just missing something obvious here and we can already do something sensible for (4) and I guess I'm just trying to clarify what that is? @ryanleecode perhaps you know better too about this?

josepot commented 1 month ago

All I'm saying is that, using purely the old interface, an app is able pass an address to the extension that it learned about it from, and have a reasonable chance that it would then be signable by it.

The exact same thing can be said about the interface that I'm suggesting. Why wouldn't that be the case?

When addresses come from an old interface and signing goes via the new one, an app literally has to guess which of the extensions providing this new createTransaction interface to use to try and sign an address?

What? Why? This makes no sense whatsoever. In the old interface DApps provided the address as one of the properties of the SignerPayloadJson object. In the interface that I'm suggesting they provide the address as the first argument of the createTransaction call... Why are you creating an imaginary problem?

I'm just trying to improve the signPayload method of the PJS Signer. That's all! This change can not in any way, shape or form alter how a DApp knows which extensions can sign a transaction for a given address.

In the old interface:

  1. Get addresses
  2. App user picks an address they want to use
  3. Look at address.source, and retrieve the associated extension to then signTransaction with it.

For starters, this is not even true. In the current PJS interface, the accounts do not have a source property. You are just making this up. Please try it yourself, run the following code in the terminal of a browser where the PJS extension is installed:

const pjsExtension = await injectedWeb3['polkadot-js'].enable();
const accounts = await pjsExtension.accounts.get();
accounts.forEach(account => {
    console.log('address', account.address);
    console.log(account.source);
});

However, let's pretend that this was true, let's pretend that the accounts had this made up source property, ok? I suppose that this made up source property would have the PJS Signer, inside, right?

Well, since the only change that I'm proposing is on changing the signature of one of the functions inside the signer, then the accounts would still have access to the new method from the account. Once again: the change that I'm proposing doesn't have anything to do with how the extension access the accounts. It's just improving the crappy PJS signPayload API.

Now, could we please, please, please stop creating imaginary problems and focus on improving the PJS signer?

jsdw commented 1 month ago

However, let's pretend that this was true, let's pretend that the accounts had this made up source property, ok?

Ah, I meant name here by the looks of it (ie https://github.com/polkadot-js/extension/blame/master/packages/extension-dapp/src/bundle.ts#L248). source is just the input arg name.

Well, since the only change that I'm proposing is on changing the signature of one of the functions inside the signer

Oh hold on; I thought that we were talking about exposing this createTransaction interface via the new discovery protocol (hence the issue being raised here and why I was going on about needing addresses from the same interface). This is where my confusion was coming from and hence my raising this issue, so thank you for clarifying!

If I'm not understadning right, then how about we move/recreate this issue with suggested interface in the PJS-apps repo and ask there for it to be added? :)

josepot commented 1 month ago

Ah, I meant name here by the looks of it (ie https://github.com/polkadot-js/extension/blame/master/packages/extension-dapp/src/bundle.ts#L248). source is just the input arg name.

In the PJS Account interface the name property is just the name that the user has given to an account for display purposes. It can not be used for determining which extensions can sign a transaction for an account. Although, once again, I fail to see how this has anything to do with the interface that we are trying to propose.


Oh hold on; I thought that we were talking about exposing this createTransaction interface via the new discovery protocol

Who cares about the protocol by which the interface is being discovered?

I have stated in multiple occasions that I'm only trying to propose a better interface for the problematic PJS signPayload API. I am not interested in discussing anything else until we agree on this basic signature. I have also stated that the ideal "extension interface" probably wouldn't need for a signature like this one. The very first thing that I said in this discussion was:

I would like to propose that we first focus on defining the basic and most important interfaces before tackling the more complex ones. Specifically, I suggest we start by defining a new signature method to replace the problematic PJS signPayload.

In an ideal scenario, the extension would not only handle the creation of transactions but also manage the broadcasting process, while keeping the consumer informed about the transaction's status. This approach would allow us to have a single source of truth for the nonce, which is currently a significant challenge. However, this is beyond the scope of the interface I'm proposing here. For now, my focus is on making the current PJS extension interface library-agnostic.

And then, that very same comment ended with:

I want to re-iterate the fact that this suggestion is just for replacing the problematic PJS signPayload interface.

In my second comment I insisted that:

I don’t see how this point is relevant to the current discussion. Could we please concentrate on finding a clear and effective replacement for the problematic PJS signPayload API? 🙏

The urgent priority is to revise the public interface that PJS extensions use, so they can be decoupled from the arbitrary implementation details of PJS ASAP. Once we’ve addressed that, we’d be happy to engage in discussions around any new standard interface for a "Polkadot Provider" that Parity may propose.

And in my third comment I said the same thing another gazillion times.

I'm starting to fear that I'm being trolled at this point.


If I'm not understadning right, then how about we move/recreate this issue with suggested interface in the PJS-apps repo and ask there for it to be added? :)

I really don't care where this discussion is being held, the only thing that I care is that we find a way to improve that horrendous interface. One step at a time. The very first step should be to replace the horrendous signPayload function with another one that can accomplish the same thing without having to leak all those horrendous PJS implementation details, while also establishing some important premises (like the 2 premises that I stated in my initial comment).

jsdw commented 1 month ago

I'm sorry that this is frustrating; I'm not trying to troll and genuinely want to help!

Looking at the "old" interface, I believe the flow to get a transaction signed is as follows (but pelase correct if I'm wrong; the PJS code isn't an area I'm by any means an expert in):

// 1. Ask for accounts info so the dApp can pick an address to use to sign the TX:
// (source: https://github.com/polkadot-js/extension/blob/master/packages/extension-dapp/src/bundle.ts#L164)
async function web3Accounts (opts: Web3AccountsOptions): Promise<InjectedAccountWithMeta[]> { /* impl */ }

// Where the opts are all optional, and the returned InjectedAccountWithMeta type is:
export interface InjectedAccountWithMeta {
    address: string;
    meta: {
        genesisHash?: string | null;
        name?: string;
        source: string;
    };
    type?: KeypairType;
}

// 2. To then sign some tx, I pick one of the addresses above. Now I need to pick an extension
// to actually do the signing. I want to pick the one(s) that know about the address I just chose,
// so to that end it looks like I can either call:

// - This function, which takes some `source` (I've been assuming this is the `source` field
// handed back above with each account) and finds the extension whose name matches this.
// (source: https://github.com/polkadot-js/extension/blob/master/packages/extension-dapp/src/bundle.ts#L242)
async function web3FromSource (source: string): Promise<InjectedExtension> { /* */ }

// - This function, which takes the actual address string by the looks of it, and returns the
// first extension it finds that knows about this address.
// (source: https://github.com/polkadot-js/extension/blob/master/packages/extension-dapp/src/bundle.ts#L263)
async function web3FromAddress (address: string): Promise<InjectedExtension> { /* */ }

// Where InjectedExtension expands into the following, including a means to sign via signPayload.
// (source: https://github.com/polkadot-js/extension/blob/master/packages/extension-inject/src/types.ts#L113)
type InjectedExtension = {
    name: string;
    version: string;
    accounts: InjectedAccounts;
    metadata?: InjectedMetadata;
    provider?: InjectedProvider;
    signer: {
        // TODO?: We could add the new interface being proposed above here somewhere,
        // so that apps have the opportunity to use something sane instead of the current
        // `signPayload` call (at least, when extensions implement the new interface).

        signPayload?: (payload: SignerPayloadJSON) => Promise<SignerResult>;
        signRaw?: (raw: SignerPayloadRaw) => Promise<SignerResult>;
        update?: (id: number, status: H256 | ISubmittableResult) => void;
    };
}

So, next to signPayload, we could add the newly proposed interface and everything would work great. (Yes, I acknowledge that the extension returned from web3FromSource or web3FromAddress isn't tightly coupled to the address and may not actually be able to sign a tx using it, but it'll have a decent chance of working at least.)

If we want to take that route, then IMO that's all great and perfectly doable, and let's raise an issue in the PJS repo and go from there so that somebody on that side with PJS experience can look to get it implemented!

The alternate route (which I initially thought was what we wanted, hence raising the issue in this substrate-connect repo) is to expose a new interface that would be discovered via the new discovery protocol (ie via https://github.com/paritytech/substrate-connect/blob/main/packages/discovery/src/index.ts#L18). But if we go this route, then we are in a position where we have to discover accounts via the "old" interface still (eg web3Accounts above), and then pick a discovered extension via the "new" discovery protocol to sign the tx (via `createTransaction(fromAddress, callData, signedExtensions)). But which extension do we pick? We want one which actually knows about the address, right? So either we could expose a list of accounts via the new interface too, so it's fairly obvious, or we could relate the source for an "old interface" address with some value given back in the new interface? Or we could just iterate all of the extensions and try them one by one until we get a positive response back and not an error?

I have stated in multiple occasions that I'm only trying to propose a better interface for the problematic PJS signPayload API. I am not interested in discussing anything else until we agree on this basic signature.

I want to re-iterate the fact that this suggestion is just for replacing the problematic PJS signPayload interface.

Could we please concentrate on finding a clear and effective replacement for the problematic PJS signPayload API?

I totally get this and am with you 100%. The old interface sucks, and purely as a replacement for signPayload I think your proposed signature looks good to me! That's not the issue here at all.

I'm just trying to convey that, if we take add a new interface via the discovery protocol (which I had assumed we wanted to do), but fetch addresses via the "old" interface, then we do need to at least have an answer for how to relate the two together (or simply acknowledge that iterating through extensions till we hit one that works with the address is all good).

Really not trying to be annoying about this; I just want to make sure we've covered our bases and add something that can be added in to the existing workflows or whatever.

josepot commented 1 month ago

The alternate route (which I initially thought was what we wanted, hence raising the issue in this substrate-connect repo) is to expose a new interface that would be discovered via the new discovery protocol

This is not an alternative route. That is a different and complementary route. That would be a completely new interface, it's a different conversation. It would be accessed through different means, it shouldn't inherit the constrains that PJS extensions have today, etc. That is a different conversation. Also, it's a conversation that I'm not interested in getting involved with until the mess with the PJS interface has been addressed.

Since the current PJS extension interface won't die any time soon, we must first find ways to first decouple it from PJS internals. This is also in the best interest of PJS Dapp users, because once that messy interface is fixed, then they won't have all the issues that they have every time that the signed-extensions change.

Also, I am not proposing to add this function besides signPayload. What I'm proposing is to remove signPayload in favour of this new function. They could both exist together for a rather short span of time to facilitate the migration, but the idea is to find a way to decouple the current interface from the PJS internal details. So, it should have to be removed once all the browser extensions implement the new method.

I urge Parity to please put all the focus on first decoupling the current interface from PJS internals, and only once that's done we can start a discussion for a proper spec for a new "Polkadot Provider".