Implement signRaw

[c410-f3r]

Fixes #115

With the following interface:

export interface RequestSign {
  inner: SignerPayloadJSON | SignerPayloadRaw;
  sign(pair: KeyringPair): { signature: string };
}

extension package abstracts the signing procedure using the sign method and because extension-ui needs to be aware if it is dealing with an extrinsic or a bunch of bytes, inner returns the inner payload content of its implementations.

[jacogr]

Thanks for doing this, this is a great addition. I will take a look in the next couple of hours.

[jacogr]

Generally looks good, thank you very much for this. I only have some really small nitpick-y comments.

[c410-f3r]

Code updated. Thanks for reviewing

[c410-f3r]

@jacogr I tested the implementation manually and fixed some issues

[jacogr]

Thanks for that, it is the only thing I was still missing to merge - I have it top of the list for the morning. (I am truly sorry for the long delay between the PR and merge here, not quite cool seeing thing stuck, so my humble apologies)

[jacogr]

Via this one, polkadot-js/apps#1936, the signature comes back as bytes instead of hex -

It should be hex. (Just need to pass the response through u8aToHex)

[c410-f3r]

Updated

[jacogr]

Thank you. Will do the final play-through this morning, really excited to get this in.

[jacogr]

A couple of issues -

1. When I sign text, e.g. hello it works. When I sign hex data e.g. 0x123456 the signature does not match. I believe it is due to the fact that we pass the data through the TextEncoder - we should just pass the raw input data

2. When I try and send an extrinsic, pub(extrinsic.sign) passed, it still brings up the bytes signing dialog.This is a screenshot of the 2, the first being the current extrinsic, the second being actual bytes

3. The text styling for from & bytes above does not match up with the actual old extinsic display and looks out of place. Compare the above against this -

[jacogr]

I have merged support for this on the apps UI, so this feature should be easier to test and explore now.

[c410-f3r]

WTF! So much problems. Sorry for the noise.
The code has been updated to reflect the issues

[jacogr]

This one is tough to test properly without having some way of well, testing :) So no reason to apologize, I also looked at the code and well, that looks good - however, you were playing in the depths of midnight, so those excursions are never easy.

[c410-f3r]

This one is tough to test properly without having some way of well, testing :) So no reason to apologize, I also looked at the code and well, that looks good - however, you were playing in the depths of midnight, so those excursions are never easy.

Indeed! These are good examples of a programmer's life

[c410-f3r]

According to the SignerPayloadRawBase contract,

export interface SignerPayloadRawBase {
    /**
     * @description The hex-encoded data for this request
     */
    data: string;
    /**
     * @description The type of the contained data
     */
    type?: 'bytes' | 'payload';
}

data will always be hex, so it is up to the caller to pass valid data.

Also, type?: 'bytes' | 'payload'; doesn't make sense. One can simply const bytes = extrinsic.toU8a(true) and signRaw(bytes). Can we remove it?

[jacogr]

The payload was added specifically for other extensions, such as the centrality one - where they don't care about de-serliazing the payload, but rather just want to sign it as a raw blob. From an implementation perspective, both can just be treated as bytes here, it does not need to be treated separately for the case where signRaw is called.

The reason behind it - in case of transactions, the API will first try to use the normal payload sign, if that is not available will fallback to signRaw. Since the normal sign path is catered for, we should not receive a payload in normal circumstances.

Either way, it it ever does receive a type: payload it actually cannot quite decode it, since it is in a signable form, not a decodable form, so it should be treated it as a "piece of data". (There is a small trick here which came in with v4 where the signature needs to have type information attached in the case of a payload, but we don't need to cater for it here in signRaw)

TL;DR Ignore the type specifier on signRaw, we can just treat all data as bytes.

[c410-f3r]

Very interesting, thank you for writing this in an explanatory way

[jacogr]

I'll need to take a peek again at the code here, since something is going wrong, either in the extension or somewhere along the way, i.e. possibly the UI doing something weird now that it has signRaw?

When submitting a transaction (verified against current, it doesn't happen), the transactions come through as "immortal", not "mortal with era", while the API def submits mortals. ... Update, after checking some more - the decoding is def. completely wrong, since here we have chain with version 195 and 194 displayed on the extension (in addition to the mortal being diaplyed as immortal being wrong)

Not sure, will take a peek though both sides again.

Additionally, the hex signing works, but I mucked-up the apps UI side for signing, i.e. when non-hex data inputted there, it doesn't send hex through. That needs a fix on that side, fix incoming.

[jacogr]

So the issue with transactions is twofold -

• the request is wrongly displayed, i.e. the mortal information is missing, which indicated the decoding was not properly done
• the generated signature fails, i.e. the transaction cannot be submitted

(Both these are verified on an older version as working on the same chain)

I believe is is due to how inner is constructed. So rather,

• the message that is sent, instead of doing an "inner" with a specific type, just keep it as-is for the data received, this will always be hex. We don't need to convert via type to anything, when signing we can just . keep the logic as-is. This means -

• for bytes, pass it as e are currently doing here

• for the normal extrinsic sign, keep this in-place https://github.com/polkadot-js/extension/pull/196/files#diff-dcc23ab59637c1964f03628674b307f8L163

With the above change reverted, at least the signing portion should still work, I'm trying to figure . out wht the .inner and cast makes the decoding not work in . the way it was.

[jacogr]

signRaw portion seems 100% now, however some small issues with transaction signing snuck in.

[c410-f3r]

My recent sickness plus some other responsibilities temporally took out my motivation to work on this PR, thus the delay.

Well, back on track. Here is a screenshot of a correctly displayed substrate transaction info.

And another showing the same transaction successfully registered.

[jacogr]

Thank you !

Hope you are doing ok.

[jacogr]

Sorry, I'm very aware I need a play-through and review here ASAP. Just "a bit" snowed under after the Kusama CC3 launch. So need to, want to, I just have not had a gap.

My humble apologies for dragging my feet, not out of choice.

[c410-f3r]

@jacogr

Don't worry about it, we all have stuff to do. Personally, you are one of the most attentive reviewers I have ever met

[jacogr]

I will take a play through today, thanks for the patience on this :(

[jacogr]

Absolutely brilliant! Thanks a million for this, it does exactly what it says on the tin. Tested against CC3 with transfers, signing (bytes & strings), all 100% ok.

[c410-f3r]

This is awesome! Thanks!

[c410-f3r]

@jacogr Do you know when will be next marketplace deployment?

[jacogr]

I will do it soon - I am just stuck with a nasty flu atm, so don’t quite trust a release process. So just need an update to latest Kusama CC3 and a full test through.

I am hoping in the next couple of days (hopefully latest on Monday), I just need a clear head. ;)

[c410-f3r]

Flu catches us all. Hope you get better
Thanks for the information!

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