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

Add tx qrcode #2657 #3454

Merged
merged 11 commits into from
Apr 18, 2022
Merged

Add tx qrcode #2657 #3454

merged 11 commits into from
Apr 18, 2022

Conversation

xiangxn
Copy link
Contributor

@xiangxn xiangxn commented Feb 21, 2022

add tx qrcode in to TranscationConfirm

#2657

Copy link
Member

@abitmore abitmore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. The commit history is a bit messy, it would be better if rebase to develop branch and force-push.

@xiangxn
Copy link
Contributor Author

xiangxn commented Feb 21, 2022

OK, I'll attention to this next time.

@sschiessl-bcp
Copy link
Contributor

sschiessl-bcp commented Feb 23, 2022

Current QR code is giving this now

{"ref_block_num":24281,"ref_block_prefix":3991233336,"expiration":"2022-02-23T21:08:39","operations":[[0,{"fee":{"amount":"86869","asset_id":"1.3.0"},"from":"1.2.23812","to":"1.2.479","amount":{"amount":"100000","asset_id":"1.3.0"},"extensions":[]}]],"extensions":[],"signatures":[]}

Is that consumable by the app @syalon ? Should it maybe only contain the ops? (what you see in the json popup)

@abitmore
Copy link
Member

Is that consumable by the app @syalon ? Should it maybe only contain the ops? (what you see in the json popup)

As mentioned in the issue, I think the ref_block_* and expiration fields are good to be there since it enables offline signing. The app can always replace the fields if not needed.

@sschiessl-bcp
Copy link
Contributor

sschiessl-bcp commented Feb 23, 2022

image
I played around with a QR code icon, thoughts?

On another note: This will not be usable for now in the way intendend, the user will need to unlock his wallet before TransactionConfirm dialog pops up. One way possible would be to login with private memo key.

Here is a more extensive QR from updating vote
image

@abitmore
Copy link
Member

The icon looks good to me. Thanks.

What if the "propose" option is switched on?

By the way, can you add a text box so that I can copy the JSON text easily? Although I can extract it from the QR code with a tool, it is not that convenient.

@xiangxn
Copy link
Contributor Author

xiangxn commented Feb 24, 2022

It might be better to have a copy button on top of the QRcode modal box than the text box.

@xiangxn
Copy link
Contributor Author

xiangxn commented Feb 24, 2022

To be available without unlocking the wallet, the entry needs to be placed on the unlock mode box.

@abitmore
Copy link
Member

It might be better to have a copy button on top of the QRcode modal box than the text box.

How about a text box / area with a copy button? I think it might be confusing if there is only a copy button -- copy an image?

To be available without unlocking the wallet, the entry needs to be placed on the unlock mode box.

As Stefan mentioned, for a transfer or another operation which contains a (non-empty) memo, the memo private key is needed to encrypt the memo when constructing the transaction. That's why Stenfan said "login with the memo key". Another scenario is that the memo key is on the device to scan / read the QR code but can not be exported, so the user can not login anyway. I think we need to adjust the UX in this case. For now, perhaps show more info on the UI about why need to login and login with which key or password? A new user probably does not know the difference between logging in with a private key or a master password, an advanced user may know. On the other hand, many people like to see a stupid simple UI - sometimes one click for all. There has to be a compromise.

Generally, we can defer the "has logged in" check. Or say, only check whether the user has logged in when the "confirm" button is clicked, remove the check from the send modal (also see #1285).

@abitmore
Copy link
Member

I just had a discussion with @syalon . To deal with the scenario that a user wants to encrypt memo on the QR code reader side, we can add some flags in the JSON, E.G. (specification to be defined)

{
  "tx": { "operations":[ [ 0, { /* a transfer */ } ],
                         [ 0, { /* another transfer */ } ],
                         ...
                       ], ... },
  "op_flags": [ { /* no additional process */ },
                { "memo_encrypted": false },
                ...
              ],
  ...
}

Note: it can be more complicated if an operation in the transaction is a proposal_create_operation.

In addition, for future expansion, maybe it's better to add a version tag to the JSON, or just add it in later versions. I think it's worth drafting an informational BSIP document.

@abitmore
Copy link
Member

This reminds me of earlier discussions about QR code scanning, E.G.

Perhaps I have made it too complicated already. If @syalon confirms the current implementation fits his needs, we can release it first and improve in later versions.

@xiangxn
Copy link
Contributor Author

xiangxn commented Feb 25, 2022

Yes, there should be a complete solution with the mobile App.

@abitmore
Copy link
Member

If it is only the memo issue, we can go with a simpler struct, E.G.

{
  "tx": { "operations":[ [ 0, { /* a transfer */ } ],
                         [ 0, { /* another transfer */ } ],
                         [ 22, { /* a proposal with transfers */ } ],
                         ...
                       ], ... },
  "unencrypted_memos": [ 0, 2 ]
}

Or even

{
  "operations":[ [ 0, { /* a transfer */ } ],
                 [ 0, { /* another transfer */ } ],
                 [ 22, { /* a proposal with transfers */ } ],
                 ...
               ],
  "unencrypted_memos": [ 0, 2 ],
  ...
}

With one of these approaches the length of the JSON string won't increase much.

Thoughts?

@syalon
Copy link
Member

syalon commented Feb 27, 2022

is the unencrypted_memos field just an index? or unencrypted message data?

@sschiessl-bcp
Copy link
Contributor

sschiessl-bcp commented Feb 27, 2022

The solution in #775 asks for a URI scheme that can be used in places that do not allow or do not want to build transactions themselves, I see this as a separate use case, the holistic approach is out of scope for this PR. This one only adds the QRcode to the Confirm dialog, and that's it, even changing the current login routine is out of scope.

To stay as close as possible to the use case at hand (integration of beet or any other external signing app would require the same change), my suggestion would be an implicit approach without changing the current format since it reflects an unsigned json transaction.

An encrypted memo looks like this

{
   "ref_block_num":51097,
   "ref_block_prefix":1063508082,
   "expiration":"2022-02-27T10:08:29",
   "operations":[
      [
         0,
         {
            "fee":{"amount":"91204","asset_id":"1.3.0"},
            "from":"1.2.23812",
            "to":"1.2.479",
            "amount":{"amount":"100000","asset_id":"1.3.0"},
            "memo":{
               "from":"TEST7EN1WbTPfpwYcU3cZ5uRN8bYXqPNmoWcrVAyBu5UGPyjWfcLEy",
               "to":"TEST6wyTNWB9zJf8b8m2MZWpZBrh32ESug3kLYwznpN56guetmMbvT",
               "nonce":"421364849689661",
               "message":"be523de3a33b080e82443968226f614c"
            },
            "extensions":[]
         }
      ]
   ],
   "extensions":[],
   "signatures":[]
}

Let's assume there is a way in the UI to get to the transaction confirm dialog before logging in, i.e. memo is only available as plain string. A representation could then look like this

{
  "ref_block_num":51097,
  "ref_block_prefix":1063508082,
  "expiration":"2022-02-27T10:08:29",
  "operations":[
     [
        0,
        {
           "fee":{"amount":"91204","asset_id":"1.3.0"},
           "from":"1.2.23812",
           "to":"1.2.479",
           "amount":{"amount":"100000","asset_id":"1.3.0"},
           "memo":{
              "from":"TEST7EN1WbTPfpwYcU3cZ5uRN8bYXqPNmoWcrVAyBu5UGPyjWfcLEy",
              "to":"TEST6wyTNWB9zJf8b8m2MZWpZBrh32ESug3kLYwznpN56guetmMbvT",
              "message_to_encrypt":"this is some text that should be encrypted"
           },
           "extensions":[]
        }
     ]
  ],
  "extensions":[],
  "signatures":[]
}

The above payload could then be sent to the mobile app via QRcode, or to Beet. Both applications recognize there is need to encrypt and do that by themselves.

@xiangxn xiangxn closed this Mar 1, 2022
@xiangxn xiangxn deleted the add-tx-qrcode branch March 1, 2022 12:57
@sschiessl-bcp
Copy link
Contributor

Why close this?

@xiangxn
Copy link
Contributor Author

xiangxn commented Mar 1, 2022

Sorry, I deleted it by mistake while organizing my library, it has been restored

@xiangxn xiangxn restored the add-tx-qrcode branch March 1, 2022 13:55
@xiangxn xiangxn reopened this Mar 1, 2022
@syalon
Copy link
Member

syalon commented Apr 12, 2022

is qrcode available on develop.bitshares.org?

@sschiessl-bcp
Copy link
Contributor

I will merge this for now, let's iterate on this to fit your and anyone elses needs @syalon

@sschiessl-bcp sschiessl-bcp merged commit f31401a into bitshares:develop Apr 18, 2022
@syalon
Copy link
Member

syalon commented Apr 18, 2022

Thanks, found it. @sschiessl-bcp

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

Successfully merging this pull request may close these issues.

4 participants