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

Contract interaction and Signature request transaction types #8909

Closed
andmironov opened this issue Sep 4, 2019 · 14 comments
Closed

Contract interaction and Signature request transaction types #8909

andmironov opened this issue Sep 4, 2019 · 14 comments

Comments

@andmironov
Copy link

andmironov commented Sep 4, 2019

Working on some updates on the transaction overview modal, I found out that there are some use-cases we are missing.

Contract interaction

At this point, we have just regular transactions and message signing with a password. What is missing is the contract interaction type. It's different because sometimes it does not require sending any funds, but just the network fee (found that use-case in our Teller Network DApp)

Screen Shot 2019-09-04 at 16 02 20

Signature request

It's important to have a special type of overview for signature requests, because they behave very similar to contract interactions, but do not require any funds to be spent (even no network fee, which is explicitly mentioned in the modal sub-header). It's important to show the account name and the network name if it's not Mainnet as well.

Also, it's important to separate the overview from signing not only for consistency reasons, but also to make it possible to sign messages with not only the password but with all the signing options we will provide soon.

Screen Shot 2019-09-04 at 16 00 28

~FIGMA https://www.figma.com/file/XUehMnhyD1FGcWzvGz6SXqvh/Mobile-wallet?node-id=3864%3A33253

@3esmit
Copy link
Member

3esmit commented Sep 6, 2019

Related EIPs

EIP712

https://eips.ethereum.org/EIPS/eip-712
ethereum/EIPs#712

EIP191

ethereum/EIPs#191

MetaMask UI:

For reference, Ill add what MetaMask does.

Unsafe Signing (unprefixed data, can sign an ETH transaction):

Safe Signing (prefixed data, typed, cant sign an ETH transaction)

@3esmit
Copy link
Member

3esmit commented Sep 6, 2019

Test MetaMask with this: https://danfinlay.github.io/js-eth-personal-sign-examples/

Basically we want to support these, for now :)

@andmironov
Copy link
Author

andmironov commented Sep 6, 2019

Thanks a lot, @3esmit ! Here's a design for a dangerous signature request, based on the Metamask one you've referenced above.

Screen Shot 2019-09-06 at 14 44 53

@3esmit
Copy link
Member

3esmit commented Sep 6, 2019

Very nice @andmironov , we can use this one for now for any signature (just for completing the full eth support of wallet and launching Status V2), and in a new issue discuss the support of ERC712, which seems to be the better alternative, then wouldn't need this scary warning, for Status V2.1 or something.

ERC191 was mostly mentioned for historical/learning purposes.

@3esmit
Copy link
Member

3esmit commented Sep 6, 2019

BTW, Status V1 merged support to EIP-712 in #7680

@3esmit
Copy link
Member

3esmit commented Sep 6, 2019

Regarding Contract Iteration, would be nice if it had somewhere to display the transaction data, many users will want to verify it before signing. It shouldn't allow edit the data field, just display it. A Show Details button could popup a display only transaction details (to, value, data, gasPrice, gasLimit, nonce), which is the information being signed.

@andmironov
Copy link
Author

andmironov commented Sep 6, 2019

This made me think that some of the fields are for advanced users only (such as the data field) and to make it look simpler and cleaner and not to overwhelm users with the information they might be confused about or scared of, it makes sense to hide some of the fields.

Also, I suggest showing the full contract data in a separate scrollable modal

Screen Shot 2019-09-06 at 17 18 37

@yenda
Copy link
Contributor

yenda commented Sep 9, 2019

So I think it is important here to specify that we are not talking about transaction types but signature types:

  • transaction signature: user is signing something that is going on chain and will cost a fee
  • data signature: user is signing some data but it is not going to go on chain and the signed data is going to be returned i.e to the dapp. In some cases the user might actually sign data that is going to be put on chain later on by the 3rd party it was sent to, so the user better understand what he's signing for, is that right @3esmit ?

Then we are talking about signing method:

  • keycard: the key is on the card, the card is required to sign
  • password: the key used to sign is on the device, the password is required to unlock the key to sign

So I think there is two separate issues here:

  • support other signing methods in the signing flow (keycard and not only password)
  • better distinguish the type of signature, show the user what he's signing correctly so that he doesn't sign a blank check

@andmironov
Copy link
Author

andmironov commented Sep 9, 2019

Thanks for your input @yenda!

support other signing methods in the signing flow (keycard and not only password)

Let me provide some context for this one. For regular transactions at the moment the flow consists of 3 steps: Overview -> Signing process -> Transaction status.

Signing with password:

1

Signing with Keycard. Here the signing process has two steps of its own:

2

So as you can see, the transaction overview is separated from transaction signing in the UI to make it possible to have any transaction signing method work, for example, this is how sending 10 SNT and signing that transaction with Ledger could look like:

3

The problem with signature requests is that there was no "overview" step so the overview has not been separated from the signing process. This is how the current design of a signature request looks like:

Screen Shot 2019-09-09 at 17 52 39

As you can see, there's no "overview". Besides the fact, that it does not show the user's account name, it's pretty hard to make additional signing options work here since the flow is different.

What I suggest signing a message with a password should look like is:

4

As you can see, here the overview is separated from the signing process, which makes it work and behave just like a regular transaction. This allows us to add any signing options with ease. And it's also a better UX since the structure is the same: Overview -> Signing -> Status.

@3esmit
Copy link
Member

3esmit commented Sep 9, 2019

I agree that this issue is contains multiple elements: transaction signing, message signing, multiple keystore types.
I think that this are both "two interaction types" with dapps, because some Dapps use only contract calls, and others uses also ethereum signed messages.

There are two types of interactions from "keystore":

  1. ethereum signed transactions: contract interactions (which would be a transaction with destination and data) or even sending ether to other externally owned account (which would be a transaction with destination and no data)
  2. ethereum signed messages: are very like the above, but can sign any message (not only transactions).

For 1. I really liked the proposed design by andmirnov with feature for displaying the data being signed by user (important to security).
For 2. we need the support of EIP191 (and the warning when using unsafe/legacy method)

The keystore being used could be any supported, I'd expect an abstraction layer handling the different types of keystores (password,fingerprint,keycard,nano ledger,etc), such as a plugin system, and then each keystore need to implement its own UI for 1. and 2.

Regarding your question, @yenda, yes, the data being signed must be meaningful for the user, and depending in the contract can lead to consequences. How the user can know what they are signing in the ethereum signed message? There should be some better way, that's EIP191.

In current v1 we don't even display transaction data, which has the same problem (fixed by the new UI in this issue). For transaction data, we can also discuss how the method will be "interpreted" by the UI, but seems like there is shameful solution of using a server from 4byte.directory, which is great as temporary solution, but later try using swarm to lookup the contract metadata, which contains the ABI of contract.

@rachelhamlin
Copy link
Contributor

Nice work on these @andmironov :)

I'll have to check against our current implementations but it looks like we're missing a total of 3 transaction types: contract interaction (+ advanced view), signature request and dangerous signature request (a build on the regular signature request).

@3esmit do you happen to know if the dangerous signature request is meant to convey the same problem as discussed in this issue? Or is it different?

@3esmit
Copy link
Member

3esmit commented Sep 10, 2019

@rachelhamlin No, it's part of ethereum signed transaction. It's an error of no balance, this check should happen for any ethereum signed transaction (regardless of being a contract interaction) but not for ethereum signed message (regardless of EIP191).

I think this would be more specific, Rachel:

  1. ethereum signed transaction
    a. ETH value transfer
    b. contract call
    1. ERC20 value transfer
    2. DApp transaction request
  2. ethereum signed messages
    a. DApp signature request
    1. Safe Signing https://eips.ethereum.org/EIPS/eip-191
      a. version 0x00 Validated Data rpc: eth_signTypedData (? not really sure)
      b. version 0x01 https://eips.ethereum.org/EIPS/eip-712 rpc: eth_signTypedData_v3 (? not really sure)
      c. version 0x45 (E) Legacy safe sign ("\x19Ethereum Signed Message" prefixed) rpc: personal_sign
    2. Unsafe signing rpc: eth_sign

@andmironov
Copy link
Author

cc @hesterbruikman

@flexsurfer flexsurfer removed their assignment Sep 30, 2019
@guylouis guylouis removed the keycard label Feb 7, 2020
@StatusWrike StatusWrike changed the title Contract interaction and Signature request transaction types Contract interaction and Signature request transaction types Feb 14, 2020
@cammellos
Copy link
Contributor

This is stale given the new wallet redesign

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants