Add signedTransaction to SignerResult for injected Signers in signAndSend

[TarikGul]

Summary

Disclaimer: (NO BREAKING CHANGES) The following additions do not change any existing logic unless a wallet opts in to using signedTransaction. More information below.

The following allows signers to modify fields in the payload passed into signPayload. This results in the return type of SignerResult to now be:

export interface SignerResult {
  /**
   * @description The id for this request
   */
  id: number;

  /**
   * @description The resulting signature in hex
   */
  signature: HexString;

  /**
   * @description The payload constructed by the signer.
   */
  signedTransaction?: HexString | Uint8Array;
}

signedTransaction field. (Optional)

If no signedTransaction is present in SignerResult everything will continue as it always has. When signedTransaction is present it will broadcast the transaction that is inputted.

Additional Changes

mode - I added the ability to decode mode from the output of an ExtrinsicBase

TODO:

• Live testing via https://github.com/TarikGul/signer-test
• Internal Docs

Edit: I have edited out the last summary and iteration which has been agreed upon as a bad iteration. It used SignerPayloadJSON as the response in the SignerResult which makes it hard for other library implementations. Moving forward there will be a standardization for the external facing API to make it better for wallets and lib maintainers.

[josepot]

Using this incredibly leaky interface as an output only makes the problem worse for other library authors.

The solution should not be to make these interfaces leakier, but rather: to start using a non-leaky interface.

For instance, a solution could be to add a new method like the signTransaction that I described 4 months ago. All what PJS would have to do then is to check whether the signTransaction API is available... If it isn't, then it just keeps doing the same stuff that it's currently doing, and if it is available then it uses the non-leaky API (which is properly decoupled from PJS).

This would also be a "non breaking change" that wallets could adhere to if they want to. However, with the approach that I'm suggesting we could provide a deadline for deprecating these leaky methods that only make sense to PJS.

It is incredibly frustrating and unfair for other library authors to have to witness how PJS keeps making the situation worse.

[TarikGul]

@josepot I have changed the whole structure to outline the decisions we have discussed and agreed upon.

[josepot]

@josepot I have changed the whole structure to outline the decisions we have discussed and agreed upon.

Thank you very much, @TarikGul, for making this change more library-agnostic.

However, there's an important point to note: the new API allows the signer/extension to modify the original call-data and/or the values of other signed extensions.

I believe this is actually beneficial. To clarify, the DApp should merely suggest what it wants the user to sign, but the final decision lies with the user. Additionally, the extension must handle certain aspects no matter what, such as the nonce and the value of the new signed extension.

From the perspective of PJS DApp developers, there used to be a "guarantee" that if the extension signed data different from what was instructed (altered call-data, tip, nonce, mortality, etc.), PJS would ensure the transaction wasn't broadcasted from their DApp. The extension could still broadcast the transaction on its own, so the old API wasn't inherently more secure.

Some might argue that this PR alters the signAndSend (et al) behavior because the signed and sent data can now differ. Therefore, it might be wise for PJS to maintain consistency with the "old" behavior in signAndSend.

A practical approach could be to encode the extra-data from the SignerPayloadJSON excluding the "mode," then compare these bytes with those from the signedTransaction extra's field while removing the CheckMetadataHash bytes.

We could consider releasing this change as a new major version of PJS, ensuring backward compatibility. This version could introduce a new API like signAndSubmitStrict, which enforces the "old" behavior, while signAndSubmit would broadcast whatever the extension returns. Alternatively, we could maintain the current restrictive behavior of signAndSubmit... Another option could be to just release it as a major version with a change in behaviour on signAndSend et al. I think that all these options are actually reasonable.

[TarikGul]

Thank you very much, @TarikGul, for making this change more library-agnostic.

You're very welcome, and thank you for the write up!

Another option could be to just release it as a major version with a change in behaviour on signAndSend et al. I think that all these options are actually reasonable.

I like all the options outlined, but I would align with this current one the most. As mentioned all have their trade offs and while I do think decoupling the stricter ("old") signAndSend from the new unrestricted one would be nice, internally it would feel like a nightmare IMO. Being that our main goal for the future is to create a more standardized signTransaction api that is well documented and is more generalized for all library authors making the current api (and the new signedTransaction field) as simple as possible to integrate with would be a big plus which I think the current implementation does.

Some might argue that this PR alters the signAndSend (et al) behavior because the signed and sent data can now differ. Therefore, it might be wise for PJS to maintain consistency with the "old" behavior in signAndSend.

Agreed, technically speaking yes some might argue that technically this is a "breaking change" since the signer can bypass the original payload being signed so it would be wise for us to do a major increase of the version.

[TarikGul]

A practical approach could be to encode the extra-data from the SignerPayloadJSON excluding the "mode," then compare these bytes with those from the signedTransaction extra's field while removing the CheckMetadataHash bytes.

This is also obviously up for discussion, but I added validation to the Extra Data {nonce, era, tip}, In addition to the Call data. The Call being what's left for discussion.

Mode - and Metadata hash were left out for obvious reasons.

Edit: As mentioned and discussed validating the call data doesn't really improve security as the Signer can just broadcast the transaction without the need of the current API.

I just added it in to start discussion with others:

cc @IkerAlus

[IkerAlus]

Note that this validation logic only covers the current case of Polkadot / Kusama relay chains. Other chains (and future relay chain upgrades) may have other signed extensions which would be skipped if we go for this function (for example, the asset ID for fee payment in the Asset Hubs).

Other potential approaches I see would be to:

1. Do no validation at all before submission, and clearly document the behaviour of thesignedTransaction mode,
2. Only validate the payload method, also documenting the implications, or
3. do a byte-level comparison of the included_in_extrinsic kind of signed extensions given by the relevant chain, this is, the "extra-data" present in the signedTransaction.

My suggestion is to go for 1 or 2 as discussed beforehand.

[SBalaguer]

I agree with what @IkerAlus proposes, however as discussed, I'm more of a proponent of option (2): I would validate that payload.method remains the same.

Even though adding this check does not inherently improves the security of the system as the signer can broadcast any transaction it wants, I believe it does help to take a more conservative approach until the ecosystem can move to a bigger, different structure.

[TarikGul]

Sounds good, I'll just ensure that the method is the same and that's it!

[bee344]

Amazing work

[polkadot-js-bot]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.