fix: #426 (Payload Details When Signing Hash), #427 (Multipart Missed Frames) #428

pmespresso · 2019-10-21T13:02:59Z

-- on Signer

- show the payload prehash image so user knows what's being signed (fixes setCode is showed as a Message signing #427)
- detect when a frame is missed and alert user what frame number it is. (fixes Enhance Multipart scanning UX #426)
-- on @polkadot/react-qr
on the generation end, make it possible to skip to a certain frame
Proposal: UX enhancement for when Multipart frame cannot be scanned in time polkadot-js/ui#233

Tbaut

Signing multipart works well.. as long as the QR codes are in order/can be read.
I found 2 problems (the second is not new):

I have 3 QR codes that seem to be the 3/13, the 10/13 and the 13/13. I will cycle though them manually.

Scanning the 3/13 is fine
then the 10/13 it shows that I missed 4,5,6,7,8,9 fine
then scanning 13/13 shows that I missed 4,5,6,7,8,9,11,12 fine
then back to the 3/13 it erroneously removed the 4 from the missed list
then back the 10/13 it erroneously removed the 11 from the missed list
So the missed frame count is off by one after one round.
video (real device mirrored): https://youtu.be/kEPZe1uR7FM

I've been able to leave the Signer in a weird state so that clicking on "scan" from the account list directly shows me a multipart advancement.

let's start from scratch, I click on "start over".. and keep pointing at this 3/13 QR code
scan the 3/13 fine, move on
scanning the 10/13, because I move too much it fires an error "Unable to parse transaction", fine
click on "start over" and keep pointing at the 10/13
it shows erroneously directly that I've scanned 8/13 frames.
video (real device mirrored): https://youtu.be/Hm3KDVyXJFc

My 3 QR codes:

3
10
13

pmespresso · 2019-10-23T14:49:38Z

hey @Tbaut , could you please review again? I can't reproduce the second issue, but I suspect it might have been a side effect of the off by one error, which is fixed now

pmespresso · 2019-10-23T15:10:34Z

pmespresso · 2019-10-29T15:34:40Z

Addressed some final grumbles, tested sudo.system.remark for single and multiframe on Kusama, extrinsic goes through, @hanwencheng @Tbaut, if you are happy with this then let's merge and get a patch release out.

hanwencheng · 2019-10-29T16:26:30Z

It seems that the missing part is not shown

Tested functionality with 4 random appeared frame on iOS with sudo system.setRemark

hanwencheng · 2019-10-29T18:12:03Z

Thanks for the PR! Tested well on iOS.

With discussed with YJ, I was misunderstand the meaning of missedFrames, I thought it is the frames to be scanned, but it actually means the frames was skipped.

I think currently the 1/4 count could perfectly shows the Scanned frames count and Total frames count. It would be nice we could show the the list of frame numbers has not been scanned since gives the information both the missed frames and and the following frames.

I have two following suggestion to be discussed for the further improvement:

Move setState(multipartComplete) to the end of the setPartData function, so there is no 10/10 screen and directly jump to the TxDetails screen.
Do the if(!multipartComplete && parsedData.isMultipart) cleanup() in componentWillUnmount in the QrScanner, so that when user click cancel the scanning and come back they do not need to click Start Over again.

pmespresso · 2019-10-29T19:36:41Z

Thanks for the review, I will log those as UX/UI enhancement requests, to be added on top of this so that this PR can remain in bulk about the core issues. I've addressed the first grumble, the second one is going to be more hairy and require a lot more testing again, so will log that for a near future PR.

Tbaut

After a yarn clean; yarn; yarn start as well as a cargo clean

Substrate Dev

transfer: I get that error Staking.bond instead of balance.transfer on Substrate #438 and entering my pin tells me "wrong pin", although this pin works to backup the seed ⚔️
system.remark, shows the right method -> says I've the wrong pin ⚔️

Kusama dev

transfer shows the right method -> says I've the wrong pin ⚔️
system.remark, shows the right method -> says I've the wrong pin ⚔️

Ethereum Kovan

Transfer ETH ✔️

I've checked twice each account (Substrate and Kusama, the pin is ok to view the recovery, not for signing anything.)

pmespresso · 2019-10-30T11:06:51Z

@Tbaut I really could not reproduce the "wrong pin" issue, I'm able to sign and send on Kusama and Substrate Dev both no problem. Did you build the Rust code again after cleaning?

src/stores/ScannerStore.js

Tbaut

So after many checks and clean:
✔️ I am able to sign multipart QR codes
✖️ I am not able to sign a single part QR code (bond, remark, transfer were tested), neither is Hanwen on Android

Tbaut

Can't believe this dead code could cause so much troubles.. we're almost there!

Tested e2e on Kusama
✔️ Tx
✔️ multipart sudo remark
✔️ phragmentElection
✔️ bond Kusama

on ETH
✔️ ETH transfer
✖️ ETH signging with MyCrypto

which leads to this weird screen (it's eth message signing)

pmespresso · 2019-10-30T16:43:17Z

on another note, eslint should be catching and pointing out the dead code (of which there is quite a lot from the previous owner).

Tbaut

Thank you so much :)

… Frames) (#428) * fix: show prehash image along with hash * fix: lint * fix: display missed frames by index * fix: remove from missed frames list once scanned * fix: edge case for multiple loops * fix: multiple loops * fix: off by one error * fix: asyn cscanner state clear * chore: bump @polkadot-js deps * fix: make tests pass * fix: qrcodehash * fix: blake2s -> blake2b * fix: signing hash * fix: hash the correct thing ffs * fix: stray print * fix: show payload details along with hash * fix: stray logs * fix: minor grumble * make android able to sign single part * fix: ethereum sign msg * fix: ethereum sign msg * fix: undeifned * fix: prehash optional

fix: show prehash image along with hash

82d5ea8

pmespresso added the A3-inprogress label Oct 21, 2019

pmespresso added this to the v3.2 Add network / metadata with QR signing milestone Oct 21, 2019

pmespresso added 4 commits October 21, 2019 15:15

fix: lint

1148b03

fix: display missed frames by index

5c88f87

fix: remove from missed frames list once scanned

f622245

fix: edge case for multiple loops

fcb3420

pmespresso marked this pull request as ready for review October 21, 2019 15:38

pmespresso added A0-pleasereview and removed A3-inprogress labels Oct 21, 2019

pmespresso changed the title ~~Multipart part 2 (mostly UX, non-core functionality)~~ Fix: #426 (Payload Details When Signing Hash), #427 (Multipart Missed Frames) Oct 21, 2019

pmespresso added A5-grumble and removed A0-pleasereview labels Oct 21, 2019

fix: multiple loops

241bb29

Tbaut changed the title ~~Fix: #426 (Payload Details When Signing Hash), #427 (Multipart Missed Frames)~~ fix: #426 (Payload Details When Signing Hash), #427 (Multipart Missed Frames) Oct 22, 2019

Tbaut reviewed Oct 22, 2019

View reviewed changes

pmespresso added A5-grumble and removed A0-pleasereview labels Oct 22, 2019

fix: off by one error

074a4e7

pmespresso added A0-pleasereview and removed A5-grumble labels Oct 23, 2019

pmespresso requested review from Tbaut and hanwencheng October 23, 2019 15:12

fix: asyn cscanner state clear

682ca33

pmespresso added 3 commits October 29, 2019 15:46

fix: stray print

8bb1c24

fix: show payload details along with hash

52a4291

fix: stray logs

cb246f9

hanwencheng approved these changes Oct 29, 2019

View reviewed changes

fix: minor grumble

2ef1eec

Tbaut reviewed Oct 29, 2019

View reviewed changes

hanwencheng reviewed Oct 30, 2019

View reviewed changes

src/stores/ScannerStore.js Show resolved Hide resolved

Tbaut reviewed Oct 30, 2019

View reviewed changes

make android able to sign single part

910324f

Tbaut suggested changes Oct 30, 2019

View reviewed changes

pmespresso added 2 commits October 30, 2019 17:41

fix: ethereum sign msg

9f44f3a

fix: ethereum sign msg

275ae62

pmespresso added 2 commits October 30, 2019 17:47

fix: undeifned

93545b1

fix: prehash optional

cdcd8ca

Tbaut approved these changes Oct 30, 2019

View reviewed changes

Tbaut added A8-looksgood and removed A0-pleasereview labels Oct 30, 2019

pmespresso merged commit 5c587fd into master Oct 30, 2019

pmespresso deleted the yj-multipart2 branch October 30, 2019 16:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: #426 (Payload Details When Signing Hash), #427 (Multipart Missed Frames) #428

fix: #426 (Payload Details When Signing Hash), #427 (Multipart Missed Frames) #428

pmespresso commented Oct 21, 2019 •

edited

Loading

Tbaut left a comment •

edited

Loading

pmespresso commented Oct 23, 2019

pmespresso commented Oct 23, 2019

pmespresso commented Oct 29, 2019

hanwencheng commented Oct 29, 2019 •

edited

Loading

hanwencheng commented Oct 29, 2019 •

edited

Loading

pmespresso commented Oct 29, 2019 •

edited

Loading

Tbaut left a comment •

edited

Loading

pmespresso commented Oct 30, 2019

Tbaut left a comment •

edited

Loading

Tbaut left a comment •

edited

Loading

pmespresso commented Oct 30, 2019

Tbaut left a comment

fix: #426 (Payload Details When Signing Hash), #427 (Multipart Missed Frames) #428

fix: #426 (Payload Details When Signing Hash), #427 (Multipart Missed Frames) #428

Conversation

pmespresso commented Oct 21, 2019 • edited Loading

Tbaut left a comment • edited Loading

Choose a reason for hiding this comment

pmespresso commented Oct 23, 2019

pmespresso commented Oct 23, 2019

pmespresso commented Oct 29, 2019

hanwencheng commented Oct 29, 2019 • edited Loading

hanwencheng commented Oct 29, 2019 • edited Loading

pmespresso commented Oct 29, 2019 • edited Loading

Tbaut left a comment • edited Loading

Choose a reason for hiding this comment

pmespresso commented Oct 30, 2019

Tbaut left a comment • edited Loading

Choose a reason for hiding this comment

Tbaut left a comment • edited Loading

Choose a reason for hiding this comment

pmespresso commented Oct 30, 2019

Tbaut left a comment

Choose a reason for hiding this comment

pmespresso commented Oct 21, 2019 •

edited

Loading

Tbaut left a comment •

edited

Loading

hanwencheng commented Oct 29, 2019 •

edited

Loading

hanwencheng commented Oct 29, 2019 •

edited

Loading

pmespresso commented Oct 29, 2019 •

edited

Loading

Tbaut left a comment •

edited

Loading

Tbaut left a comment •

edited

Loading

Tbaut left a comment •

edited

Loading