Allow external signers to modify the payload

[Tbaut]

This is a tiny change, that has a lot of consequences.
I want to allow extensions and generally external signers, to change the payload. This way, wallets can have much more complex structure, and knowledge about accounts, e.g, it allows wallets to support proxies and multisigs, without the Dapp to know about it. Extensions can now wrap the payload with proxy.proxy, or multisig.asMulti

This seems to be a failsafe, and I'd argue that the feature to check the payload isn't actually that important:

• if the Dapp is compromised, the payload will be changed from the get go
• if the extension is compromised, then hot keys can be extracted and any website compromised

This also allows wallets to let users change the tip, nonce etc.. like in the Ethereum ecosystem.

[TarikGul]

Yea I agree with the general sentiment above. When I first wrote this it was to maintain some level of integrity around the original Call Data (so that the original validation before all the changes wouldn't be to drastic). That being said I think it's okay to move forward with this change, and accept that the control should really be on the wallet/extension.

This also allows wallets to let users change the tip, nonce etc.. like in the Ethereum ecosystem.

Side note: I believe this was already available.

[TarikGul]

I would like to note as well - this was discussed in the past with the PAPI team, and internal teammates. We did favor this general direction but in the future. Now that the future is here and the topic is put forth I am backing it.

That being said - should be NOTED that this is a BREAKING CHANGE.

[TarikGul]

Holding off on merging until there is a bit more discussion - I am in favor but believe I shouldn't be the only one who decides here.

[Tbaut]

Papi has this by default, this is something I discussed with the team, @josepot can confirm.
Also pinging some persons that have expressed the need to have the wallet do more: @kianenigma from Parity, @jonathanpdunne and @0xKheops from Talisman.

[0xKheops]

Definitely in favor of this ❤️
Another example would be allowing the user to change the fee asset on asset hub, from the wallet, if the dapp didn't provide the option to do so

[josepot]

I’ve always been vocal about the importance of enabling extensions to alter the payload of call-data. However, as @Tbaut rightly pointed out:

This is a tiny change, that has a lot of consequences.

Indeed, from the perspective of a PJS DApp developer, this change represents a significant shift in behavior, breaking the expectations that the library has established since its inception.

Here are my thoughts on the possible paths forward:

1. Release a minor version, introducing this behavior behind a flag on signAndSend (e.g., .signAndSend(alice, {allowChanges: true})). Alternatively, a new API could be introduced, but the key point is that this feature should be opt-in to avoid altering the current default behavior in a minor release.

2. Release a major version, making this new behavior the default, but keeping the old behavior accessible via a flag (e.g., .signAndSend(alice, {restrictChanges: true})).

3. Release a major version, removing the current check entirely, thus enforcing the new behavior without compromise.

Among these, my preferred approach is Option 2, while my least favorite is Option 3 (what this PR is currently doing).

Although I’m not a user of PJS myself, I believe it’s important to uphold the guarantees that the library has provided to its consumers from day one. Completely removing a guarantee without offering a fallback feels like a misstep in terms of DX and expectations.

[Tbaut]

Thanks for the feedback. Option 2 is the best IMHO so that nobody has to chase Dapps dev to add a flag.
I believe 99% of Dapp devs have no idea about this behavior, and I predict that the restrictChanges will not be used. But speculation shouldn't drive development, so I'm in favor of it.

I'll change the PR.

[josepot]

Thanks for the feedback. Option 2 is the best IMHO so that nobody has to chase Dapps dev to add a flag. I believe 99% of Dapp devs have no idea about this behavior, and I predict that the restrictChanges will not be used. But speculation shouldn't drive development, so I'm in favor of it.

I'll change the PR.

I have seen certain "DApps" (like the ones used for inscriptions and stuff like that), which included a tip in the form of a transfer inside a batch_all, for example. I think that those DApps would use the restrictChanges. As in, "if you want for my DApp to broadcast and handle this logic, then don't remove the tip"... It's just an example. I, personally, found those tips a bit abusive and I created the inscriptions myself with PAPI 🤷.

I don't know, maybe I'm reading too much into it. However, it feels right to leave that option somewhere, just in case.

[Tbaut]

I added an optional allowCallDataAlteration defaulting to true in the SignatureOptions.

[TarikGul]

Thanks 👍 - this will reflect as a Major Change in the next release.

[0xKheops]

I just realized that changing default behavior could lead to issues out there.
We're in a similar situation as when we introduced the withSignedTransaction flag.

if wallet changes anything to the payload before signing, dapps that didn't upgrade their pjs library to next release yet will break.
So wallet needs a way to know if it is allowed to modify the payload, before doing so.

Or did i miss something ?

[0xKheops]

Nevermind, read it again and we re good.
the property should not be in the json payload if the dapp didn't upgrade, so wallet will know that it can't modify the payload

[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.