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

Preview data and function #2847

Closed
fulldecent opened this issue Jan 3, 2018 · 32 comments
Closed

Preview data and function #2847

fulldecent opened this issue Jan 3, 2018 · 32 comments
Assignees
Labels
Projects
Milestone

Comments

@fulldecent
Copy link

screen shot 2018-01-03 at 1 22 31 am

I see "Data included: 36 bytes".

How can I inspect the 36 bytes that will be included?

@phiferd
Copy link

phiferd commented Jan 4, 2018

I also would like to see this functionality, especially for basic things like token transfers. At the very least, there should be an option to see the actual bytes -- it's not that useful for most people, but it's better than nothing.

At this point, users are forced to trust that the site they are using is going to send the transaction that they say they will. There's no reason for this -- you should only need to trust the signer and the contract you're sending to.

Also, as far as I know, there's no way for the site to demonstrate that it is legitimate. If there was an option in the API to send a more information about the transaction in addition to the actual bytes, MetaMask could verify the bytes separately. Please let me know if this is already possible in some other way.

Not sure if this would work, but maybe something like:

    var transactionObject = {
      "from": from,
      "gasPrice": "0x04e3b29200",
      "gasLimit": "0x7458",
      "to": contractAddress,
      "value": "0x0",
      "data": contract.someMethod.getData(param1, param2, {from: from}),

     // new optional field.  this can be check against the "data" field by metamask.  If it matches, then this 
     // data can be used to construct a more useful confirmation dialog to the user.  
     // If it doesn't match, abort -- either there was some bug or the site is trying to trick the user.
     // If it's not provided, show the bytes along with a warning that the there's no way to verify
     // the message is what the user thinks it should be.
      "signerData": {
            "methodAbi": {"name": "someMethod", "inputs": .... }
            "params": [ param1, param2 ]
       }
    };

    web3.eth.sendTransaction(transactionObject, ... );

I'm not sure what the web3 standard says about including extra fields, or if metamask could use it as I suggested. Just a thought...

@danfinlay
Copy link
Contributor

@cjeria data can look like this: https://www.4byte.directory/signatures/?page=2

@danfinlay
Copy link
Contributor

Data source as module: https://github.com/danfinlay/eth-method-registry

@phiferd
Copy link

phiferd commented Jan 15, 2018

Nice! Looks interesting @danfinlay , although I wonder if the client library should compute the hash of the returned method signature and verify it. It's a small extra step but removes the trust required between the signer (and therefore the end user) on the signature registry service. That way, you can safely use the fastest/most-reliable service and not worry about sticking with a "well-known" service that might be overloaded.

@danfinlay
Copy link
Contributor

@phiferd if you read the source of the used signature registry contract, you'll see that it already verifies hashes.

I guess we could always verify again, wouldn't hurt..

@fulldecent
Copy link
Author

It will be best if CryptoKitties gives all the information necessary to hash and sign into MetaMask, then MetaMask can show it to the user, hash and sign it.

This is preferable to CryptoKitties hashing and giving to MetaMask and then MetaMask reverse engineering the hash, showing the reverse engineered hash to the user and signing.

Reasons:

  1. The reverse engineering service may be offline
  2. The dApp's symbols may not be in the reverse engineering service
  3. Simplicity
  4. Hash collisions?

@phiferd
Copy link

phiferd commented Jan 16, 2018

@fulldecent I think you are suggesting something along the lines of my earlier post? Is that right? Where the application calling MetaMask would provide the method signature in a readable/parsable form.

I also prefer that approach because it puts the calling application in control and doesn't require registration with a 3rd party service. As you point out, and outage in that service could affect many apps. Also, the caller would typically have access to the ABI (or at least the portion they need) to construct the data field. However, I wasn't sure if that was feasible.

@danfinlay how do you feel about the caller-provides-signature approach? There's actually no reason both could not be used if there was a benefit:

  1. If the signature is provided by the caller, use it
  2. If it is not provided, try to look it up using whatever service you want
  3. In either case, check the signature hash against the data
  4. If they match, present approve option to the user, otherwise, show the user a failure message

@fulldecent
Copy link
Author

Here is my proposed work plan:

  • Keep the existing web3.eth.sendTransaction API as is.

    • When this API is called, MetaMask shall allow display of the raw bytes that are sent.
    • Also a warning will show that these bytes cannot be interpreted. This incentivizes developers to NOT use this method.
  • Add new api web3.eth.getDataAndSendTransaction.

    • This does not work for creating contracts.
    • This is the same as sendTransaction minus send parameter plus sendMethod and sendParams.
    • I assume sendMethod and sendParams are sufficient for MetaMask to perform getData and those types can be expressed in Javascript.

@phiferd
Copy link

phiferd commented Jan 16, 2018

MetaMask would need to know the method name as well as the order and the types for each parameter. I suggest if that data is included in the call, it is simply a segment of the contract ABI or the complete ABI (although that's a lot of wasted data).

e.g.

[ {
      "constant": true,
      "inputs": [
        {
          "name": "",
          "type": "address"
        },
        {
          "name": "",
          "type": "address"
        }
      ],
      "name": "authorizedUsers",
      "outputs": [
        {
          "name": "",
          "type": "address"
        }
      ],
      "payable": false,
      "stateMutability": "view",
      "type": "function"
    }
]

MetaMask can then use the built-in web3 classes/methods to construct the data parameter needed to make the call. Since an ABI is just an array of objects like the one listed above, MetaMask can use that input exactly as the ABI.

Apps calling metamask can the just use myABI.filter(m => m.name === "myMethod") to pick out the right data to send to MetaMask and not worry about assembling it themselves.

@danfinlay
Copy link
Contributor

I'm really glad to see so much enthusiasm coming out in support of better signing approval screens, I think it's a very important domain for us to grow in. It was actually the first topic I ever wrote about on joining the team (that blog isn't even up anymore).

I'm going to separate the scope of this proposal to distinguish it from #1142, which is basically what I've been proposing, ie using eth-method-registry. Regarding that:

The reverse engineering service may be offline

The current module I'm proposing using uses an on-chain registry, so if it's down, the network is unavailable, making it a pretty ideal source of this information, even without requesting it from the dapp interface.

Note that issue is an MVP of rendering transaction data, which should hold us over until one of various much-better solutions. I'll leave this open so you can explore improved sendTransaction methods.

One discussion towards a much more readable transaction approval screen is in EIP 719: Trustless Signing Protocol, and I'd love to see that issue develop further, some of these ideas seem to be approaching those.

@fulldecent
Copy link
Author

@danfinlay Your on-chain solution does not work on my test network.

@danfinlay
Copy link
Contributor

@fulldecent two ways it could (it hasn't been done yet! nothing is set in stone!):

  1. You could publish the registry contract to your test network and add it to the method-registry module.,
  2. We could cache known signatures client-side, so that signatures that have been seen on one network will also be shown on other networks.

Again, what I'm proposing is a first, quick, MVP way of adding metadata to the approval screen. Over 99% of our usage is on the main Ethereum network, so I'd like to not prematurely optimize for your development environment.

@cjeria
Copy link
Contributor

cjeria commented Jan 29, 2018

@danfinlay If we're only providing a preview of the data, does this mean it is only viewable from within the tx approval screen? Or can we link to external url to view it?
Is this data valuable enough for us to make it visible from the tx detail view in the tx history?

@danfinlay
Copy link
Contributor

If we're only providing a preview of the data, does this mean it is only viewable from within the tx approval screen?

Not sure what you mean by "only providing a preview". Ideally it's not just a preview, we're showing the actual tx info, like who the kitty is sending to.

Is this data valuable enough for us to make it visible from the tx detail view in the tx history?

If we could make it concise enough maybe it would be, but as it is, I think showing the receiving contract & time is already pretty good for a list view. We should have this data in the tx detail view, though.

@cjeria
Copy link
Contributor

cjeria commented Jan 30, 2018

@danfinlay how's this look?

tx metadata

@phiferd
Copy link

phiferd commented Jan 30, 2018

@cjeria looks great! One suggestion -- if a human-readable version of the signature is found, I think it should be shown in place of "36 bytes". The text could still be linked to show the popup, but I doubt end users actually care how many bytes are being sent.

The fact that method calls (or "commands", which might be a more meaningful term to non-technical users) are bundled together with the parameters and called the "data" is an implementation detail that is largely irrelevant to the approval process.

@danfinlay danfinlay added labeled and removed ready labels Feb 26, 2018
@2-am-zzz 2-am-zzz removed the labeled label Mar 13, 2018
@danfinlay danfinlay added this to the Sprint 04 milestone Mar 27, 2018
@danfinlay
Copy link
Contributor

Adding to the next sprint as a design task, we'd like at least one more design option, preferably that shows the method name on the main screen, not behind a tooltip.

@cjeria
Copy link
Contributor

cjeria commented Apr 9, 2018

@danfinlay did we ever get a list of the top transaction types from Infura?

@cjeria
Copy link
Contributor

cjeria commented Apr 11, 2018

@danfinlay will take another stab at this.

Do we know who the user type (average vs technical) is requesting this type of data exposed in the approval screen? We should design for that user type while keeping in mind the other type as well.

My concern with adding too much technical data front and center is that it may be overwhelming, even for some technical users.

@cjeria
Copy link
Contributor

cjeria commented Apr 11, 2018

The approach I think makes sense is the one that @phiferd is suggesting but placing this somewhere below the high-level tx data with an "expand details" button.

@cjeria
Copy link
Contributor

cjeria commented Apr 11, 2018

Also, has any progress been made on the list of human-readable signature/methods yet? Would be good to have some idea of how much area would need to be allocated for these methods strings.

@cjeria
Copy link
Contributor

cjeria commented Apr 25, 2018

@danfinlay here's another option for displaying method data in our confirm UI.

Collapsed and expanded views

image

@phiferd
Copy link

phiferd commented Apr 25, 2018

Looks clean. I like it.

I think the "Hex Data" section could even be pushed down into a "more details" section under the function drop down, but I get it if you don't want too much nesting. Alternately, it could be hidden unless the user has the plugin set to "developer mode". The main issue is just that most users will have no idea what the hex data is so at best they will ignore it but more likely it will just confuse them. Using a developer mode option is probably better.

Also, I'm assuming that's just dummy data in there -- otherwise I didn't understand why Function is "Transfer" and Text Signature is "convertibleTokenCount".

How will the individual parameters be shown? E.g. for a token transfer, there's an address and an amount. Will the amount be shown in hex, in units of grains, or in units of tokens? Ideally, it would be shown in units of tokens because that's the only representation that is meaningful to an end user.

@danfinlay
Copy link
Contributor

I think the "Hex Data" section could even be pushed down into a "more details" section under the function drop down

I was going to say the same thing. If we were reluctant about a function name in the first place, then this hex data should really be tucked further away.

I also think the function name should be higher up, maybe even above where the value ($20) is listed, because usually if there is a function called, ether isn't being sent!. This isn't a total rule, so we might want two mockups:

  • A function call when there is no ether transferred (maybe don't show the ether/$ rate!). Example: Transfer Ownership (cryptoKitties).
  • A function call with ether transferred. (Example: Deposit (Maker Dao)).

If the function is Deposit, it sure makes sense to have that word before the amount being deposited, so I think I it might make sense to put the function name above the amount when both are shown.

@danfinlay
Copy link
Contributor

Bonus: Maybe the word Function is redundant and scary! Why not just say > Deposit, and the detail section can label the fields more specifically?

@cjeria
Copy link
Contributor

cjeria commented Apr 25, 2018

Agreed, the HEX data (or transaction data) is addressing another issue we have open which is to add advanced developer mode settings which only shows hex data if user has turned it on in settings and also if the feature is used when sending a tx. See issue MetaMask/Design#2

I also think the function name should be higher up, maybe even above where the value ($20) is listed, because usually if there is a function called, ether isn't being sent!. This isn't a total rule, so we might want two mockups:

A few questions regaring function names:

  • How long can function names get? I'd like to design for edge cases like if function name is longer than one word and need to wrap around. We can use the top bar and replace "confirm" with function names.
  • Do we have a list of human readable function names? If not, please provide all types of anticipated function names. This will help me identify patterns in the presentation of function types and anticipate confirm screen changes (if any) per type.
  • For each function type, is there additional associated data per said function? In my mockup above, there's an expandable section with more associated function data. Can you provide a real example of what this data will look like?

@danfinlay
Copy link
Contributor

We can use the top bar and replace "confirm" with function names.

I agree with this. The top bar could definitely say the function name instead.

How long can function names get? I'd like to design for edge cases like if function name is longer than one word and need to wrap around. We can use the top bar and replace "confirm" with function names.

There isn't really a limit on function names, so we should allow wrapping, or cutting off at a certain point, but most function names should fit in a single line.

Do we have a list of human readable function names? If not, please provide all types of anticipated function names. This will help me identify patterns in the presentation of function types and anticipate confirm screen changes (if any) per type.

I've asked Infura if they can give us a list of the most frequent method names so we can consider them more specifically.

Beyond that, we can look at Etherscan and get a sense, but it's tedious.

We can also look at 4byte.directory for a list of possible method names.

For each function type, is there additional associated data per said function? In my mockup above, there's an expandable section with more associated function data. Can you provide a real example of what this data will look like?

Each method name is associated with some parameters, but we don't know their names, so the data we have to work with looks like this:

 {
    name :'Transfer From',
    methodSignature: 'transferFrom( address, uint256 )`,
    args: [
      { type: 'address', value: '0xabcdefg1234567890 },
      { type: 'uint256', value: '235' }
    ]
 }

@bdresser
Copy link
Contributor

this is now a feature in the new UI's updated Confirm screens

@fulldecent
Copy link
Author

Confirmed working

screen shot 2018-08-16 at 4 47 32 pm

@ArsenKulikov
Copy link

ArsenKulikov commented Aug 21, 2018

@bdresser
So, can you explain, how pass data in "FUNCTION TYPE" and in function name about transaction cost? Is it already possible?
I see that you closed many issues 5 days ago about this problem. But i can't find any solution :(

@bdresser
Copy link
Contributor

@ArsenKulikov we pull function names from parity's signature registry, here on Etherscan. If you add your function's signature it will appear when users call it via MetaMask.

@ArsenKulikov
Copy link

@bdresser
Thank you for the responce!
Sorry, but i'm beginner in eth :)
I can't understand data flow of this process.
I have function in my contract (someFunc(string _string)), i call function registry('someFunc(string _string)'), and it will added to mapping. But when i should to do this? With my func call or only call function 'registry' of SignatureReg, or maybe in constructor in my smart contract?
And i'm going to use Ropsten Testnet. Is it available?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Design
  
Awaiting triage
Development

No branches or pull requests

8 participants