-
Notifications
You must be signed in to change notification settings - Fork 78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add and stabilize a basic interface for signing transactions #2468
Comments
@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? |
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 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:
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:
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 |
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! |
@josepot / @carlosala, the thing I was asking in the call was: If I have some extensions which expose this 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 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 |
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.
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 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. |
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 Or to phrease it another way: what steps would you expect an app to take now? In the old interface:
With this new interface, the best I can think of is:
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? |
The exact same thing can be said about the interface that I'm suggesting. Why wouldn't that be the case?
What? Why? This makes no sense whatsoever. In the old interface DApps provided the address as one of the properties of the I'm just trying to improve the
For starters, this is not even true. In the current PJS interface, the accounts do not have a 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 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? |
Ah, I meant
Oh hold on; I thought that we were talking about exposing this 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? :) |
In the PJS Account interface the
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:
And then, that very same comment ended with:
In my second comment I insisted that:
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.
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 |
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 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
I totally get this and am with you 100%. The old interface sucks, and purely as a replacement for 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. |
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 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". |
We currently have a
smoldot-v1
interface for communicating with a Smoldot in an extension, and asubstrate-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 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.
keypair: type KeypairType = 'ed25519' | 'sr25519' | 'ecdsa' | 'ethereum'
to the account info, or is it enough to rely on the address type + metadata to decode and understand. I think we don't need the extra info.\cc @josepot
The text was updated successfully, but these errors were encountered: