Skip to content
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

[Wallet] Sign transaction module #8026

Closed
6 tasks done
flexsurfer opened this issue Apr 24, 2019 · 2 comments · Fixed by #8361
Closed
6 tasks done

[Wallet] Sign transaction module #8026

flexsurfer opened this issue Apr 24, 2019 · 2 comments · Fixed by #8361
Labels
feature feature requests
Projects

Comments

@flexsurfer
Copy link
Member

flexsurfer commented Apr 24, 2019

Implement a new module, it should

  • get tx data map on input
  • show title (1)
  • show tx details on the bottom sheet (details should be different for sending eth or calling contract (2))
  • it's only possible to change one field - network fee in this module
  • sign with password
  • show sent popup

this issue doesn't include:

image

@flexsurfer flexsurfer self-assigned this Apr 24, 2019
@flexsurfer flexsurfer mentioned this issue Apr 24, 2019
12 tasks
@flexsurfer flexsurfer added this to To do in Core via automation May 2, 2019
@rachelhamlin
Copy link
Contributor

Notes from 8/5/2018 call:

  • Keeping signing phrase, not using emoji guard.
  • Overview screen can not be edited by the user in first iteration.
  • Cancel brings user back to the screen where Tx was initiated: from wallet, to the input fields. From DApp, to the screen that launched Tx. From chat, back to chat.
  • Confirmation pop-up appears over Tx launch screen (e.g. DApp).
  • Token amount can display as N/A if the Tx is only for gas.
  • New Data field will contain other information from the Tx, such as a TtT setting amount. For non-Status contracts, this data will not be human readable. Requires more effort. Power users appreciate seeing it. For Status contracts, we could make the data readable.
  • Not using Wallet field until we have multi-account.
  • Not using the DApp field in first iteration. In future, could pull the URL from the browser if Tx was launched there.
  • Not including conversion rates in first iteration. In future, must decide how many decimal points to show.
  • Passcode / pincode / Keycard sign are all required. First iteration can probably get away with passcode only, as pincode/Keycard are not in production yet.
  • QA is concerned about overflow on certain lines, e.g. if translated to different language or long network fee.

Design changes needed

  • A way to see full recipient address, either in a dedicated field or on a long press.
  • Error state in place of confirmation screen, to be used if Tx fails because user does not have enough ETH/token
  • Info button/tool tip with copy explaining what signing phrase does
  • Slight variation of overview screen required for signing messages that don't have on-chain Tx (e.g. logging in): "Signing message" vs. "Signing transaction" and only the recipient and signing phrase fields are necessary.

@flexsurfer flexsurfer added the feature feature requests label May 13, 2019
@flexsurfer
Copy link
Member Author

flexsurfer commented May 23, 2019

take this into account:
eth_signTypedData_v3 password is refused. #8241
web3.personal.sign() not displaying message to sign and refusing password #8209
Gas estimation on contract transfers for Eth transfers incorrect #8269

@flexsurfer flexsurfer moved this from To do (P0) to In progress in Core May 23, 2019
Core automation moved this from In progress to Done Jun 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature feature requests
Projects
Core
  
Done
Development

Successfully merging a pull request may close this issue.

2 participants