Contract interaction and Signature request transaction types

andmironov commented 4 years ago

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)

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.

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

3esmit commented 4 years ago

Related EIPs

EIP712

https://eips.ethereum.org/EIPS/eip-712 https://github.com/ethereum/EIPs/pull/712

EIP191

https://github.com/ethereum/EIPs/issues/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 commented 4 years ago

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

Basically we want to support these, for now :)

andmironov commented 4 years ago

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

3esmit commented 4 years ago

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 commented 4 years ago

BTW, Status V1 merged support to EIP-712 in https://github.com/status-im/status-react/pull/7680

3esmit commented 4 years ago

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 commented 4 years ago

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

yenda commented 4 years ago

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 commented 4 years ago

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:

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

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:

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:

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:

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 commented 4 years ago

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":

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)
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 commented 4 years ago

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 commented 4 years ago

@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:

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

andmironov commented 4 years ago

cc @hesterbruikman

cammellos commented 3 years ago

This is stale given the new wallet redesign

status-im / status-mobile