-
Notifications
You must be signed in to change notification settings - Fork 3
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
Make human readable #1
Comments
Hi @caffeinum, thanks for taking your time to check it. My primary concern is that currently the bigger (full screen ) QR can only handle 2900 numeric chars. That gets 1.6 times worse if it is alphanumeric in QR. What I'm afraid of with visible QR codes is that I fight to save each byte. The whole QR code is encoded into numeric string to preserve precious space. The main question here is:
I'm afraid that users care less for readablity, than for comfort. The reason I used |
I am not sure about 2900 chars' limit. This (outdated) source states it's 4000+ alphanumeric chars. https://qrcode.meetheed.com/technical.html For a typical transaction, maximum you need to hold, is an address of a receiving contract, extra variables and arguments. This QR Code, for example, encodes a
It's 192 symbols, which is way less even than 2900 limit. I am not sure then that we need compactification at all. P.S. Probably I didn't understand your use case? |
The 2900 char limit was actually for numeric only and it was a measurement on the implemented code in Metamask, and not numbers from the standard. Yes it is possible that the standard allows for greater numbers but with the QR screen size I used, and with qr-code react element that is available, this is the max possible. (Which does not mean that we could not make the QR code even bigger to allow for more data) Which means that according to your table (using the proportions of the encoding schemes) the max alphanumerics will be: 2900 * 4296 / 7089 = 1757, and for byte (if we use your recommendation the majority will be bytes: 2900 * 2953 / 7089 = 1208 bytes. And of course it is not in the standard that the QR code readers will not be able to read very dense codes, because they make mistakes, or can't read the code at all, or you have to try multiple times. That is not in the standard, but this was reality when I tried to transmit QR code. "I am not sure then that we need compactification at all." - We do need it I believe, because you or I are not the ones that create the data, but Metamask does. And Metamask does not create a code like yours, but for tx s it uses the standard: And dont get me wrong: I am not against standards. We can do this in a way like: I think that data in ethereum tx-es will grow in the future. Augur has now quite big data field in everyday trades more than 500 chars. And Im afraid tx es going to get much bigger than this. So a compression is needed. To sum up:
I say: we use encoding and compression of data, and use the EIP67, but extend it with encoded data field, and signature field
I think for users it is enough to see the decoded tx on their signers, which is perfectly possible with the option I presented, this makes their lives easier, since no multiple QR reading is necessary in most cases. |
BTW: even if you make the code human readable, you can't change it, since Metamask will not accept the signature unless it belongs to exactly the same tx data as was in the QR code originally. EIP67 is not a strict standard. It only defines that the QR data should be in URL format, and gives an example scheme. We can easily extend it with new schemes. |
I have two more points:
JSON became popular even though it's not very compact, but because of how easy you can check the data you're sending. P.S. All in all, I am here only to critique. As long as you're coding and I am talking, it's on you to make a final decision. But there's a chance many people, including me, will be using your protocol, so it makes sense to make their life easier. For example, I shall code Swift iOS implementation for that at Flightwallet. |
Copied from: MetaMask/metamask-extension#5640
I have few objections to your current version, sorry, haven't had time earlier to list them.
ethereum:
URI scheme. I think it won't be bad to use the same protocol. ERC: Standard URI scheme with metadata, value and byte code ethereum/EIPs#67As I understand, URI scheme should say, which app you should launch, and not what's inside the link (e.g. Microsoft Office links, https://docs.microsoft.com/en-us/office/client-developer/office-uri-schemes). If it's so,
ethereum:
is a good match.What about backwards compatibility with existing
ethereum:
QR code reader apps? It defines the link which can contain a destination address, amount and even contracts' ABI.As argued in the EIP67 linked thread, it makes sense to make the link human-readable. What if we use
ethereum:0xDE571NA710NADD5E55?nonce=145&amount=0.5
and then build the transaction on the device? The only change from EIP67 then would be the additional nonce parameter.Remove error check
(del) okay, that makes sense, but worth discussing again. Probably, better use case-checksum, like in ethereum addresses?
References: ethereum/EIPs#831, ethereum/EIPs#67, ethereum/EIPs#681
The text was updated successfully, but these errors were encountered: