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

AOPP support #4512

Closed
tsusanka opened this issue Apr 30, 2021 · 15 comments · Fixed by #4572
Closed

AOPP support #4512

tsusanka opened this issue Apr 30, 2021 · 15 comments · Fixed by #4572
Assignees

Comments

@tsusanka
Copy link
Contributor

Website: https://aopp.group
Short demo video: https://www.youtube.com/watch?v=heP-ufUke-g

@andrewkozlik
Copy link
Contributor

andrewkozlik commented Apr 30, 2021

Trezor actually already supports the SLIP-19 proof-of-ownership format, but as far as I know it's the only wallet to do so at the moment. SLIP-19's most important feature is that it can be used to prove to a wallet that it does not own a scriptPubKey. SLIP-19 has a commitmentData field which can be used the same way as the message field in AOPP. So my first though was that we should contact the AOPP group if they wouldn't rather go with SLIP-19. But now that I look at their spec, all they are doing is SignMessage and the spec is more about the communication format between the exchange service, the user's browser and Suite. Since SignMessage is already widely implemented and they don't need the proof-of-non-ownership feature, it makes sense to go with SignMessage rather than SLIP-19.

The spec specifies an aopp: URL format. Clicking the URL should open Suite, which will send a SignMessage command to Trezor. Suite will then post the signature returned by Trezor to a specified URL.

The message being signed can be up to 1024 ASCII characters. Apparently there is no special format. So we don't need to do any modifications in trezor-firmware except for ensuring that SignMessage can be paginated to show 1024 characters. In core we already support that, see trezor/trezor-firmware#1384. I am not sure about legacy though.

@prusnak
Copy link
Member

prusnak commented Apr 30, 2021

Apparently there is no special format.

I wonder how they handle signing of segwit and native segwit addresses. The constants we used in here (+4 and +8) were not adopted by Bitcoin Core, but there are the logical way how to extend the original signing protocol. Did AOPP use the same method?

@andrewkozlik
Copy link
Contributor

Apparently there is no special format.

I wonder how they handle signing of segwit and native segwit addresses. The constants we used in here (+4 and +8) were not adopted by Bitcoin Core, but there are the logical way how to extend the original signing protocol. Did AOPP use the same method?

Good catch! AOPP adopted the Bitcoin Core solution as stated here. So we either need to add a new bool parameter to SignMessage called no_witness_info, no_script_type or maybe no_script_info, or Suite would need to remove the script type information from the recovery byte of the signature, which seems like a messy solution. So I'd vote for adding a new parameter to SignMessage.

@prusnak
Copy link
Member

prusnak commented Apr 30, 2021

So I'd vote for adding a new parameter to SignMessage.

Agreed. Option no_script_type sounds good.

@prusnak
Copy link
Member

prusnak commented May 10, 2021

It seems we are not showing the address being used for signing during the confirmation step. This needs to be done for both T1 and TT while implementing this feature.

@andrewkozlik
Copy link
Contributor

andrewkozlik commented May 26, 2021

  • Add a new bool parameter to SignMessage called no_script_type, which will be false by default and when set to true SignMessage will not include script type information in the recovery byte of the signature, same as in Bitcoin Core.
  • Implement pagination in SignMessage on T1 so that up to 1024 characters can be displayed.
  • Show the address being used for signing in SignMessage on T1 and TT.
  • Show the address being used for signing in EthereumSignMessage and LiskSignMessage on T1 and TT. (This may be deferred to a separate issue, because it's unrelated to the present one, but it makes sense to do this sooner or later for consistency.)
  • Possibly tweak message color, see below. (Again may be deferred to a separate issue since it's unrelated to the present one, but might make sense to look into if we are modifying the UI.) @matejzak

Currently the TT shows:

image

I suggest adding the following screen at the beginning of the workflow:

image

I also suggest changing the color of the message so that externally provided text is distinguished from internally generated information. For example:

image

@lclc
Copy link

lclc commented May 27, 2021

That's great to hear. Thanks for moving swiftly!

Can we put the Trezor logo here https://aopp.group/#wallets ?

If you have any questions: We're watching this issue and also have a Telegram group with other wallet developers and VASPs that have implemented AOPP: https://t.me/joinchat/HjHCr6atTZqVE8Wf

@slush0
Copy link

slush0 commented Jun 30, 2021

What's the payload of AOPP signing message? Is that human readable so we don't need to care and only reuse plain signMessage? I don't see any discussion here about how to particularly render AOPP screen.

@haarts
Copy link

haarts commented Jun 30, 2021

The payload can be decided by the exchange/VASP/what have you. I think a short, intelligible message makes sense since the end user is going to sign it. See bullet point 2 here: https://gitlab.com/aopp/address-ownership-proof-protocol/-/blob/master/README.md#specification
An other example can be found on https://demo.aopp.group. Just inspect the link under 'register bitcoin address' (or decode the QR code).

@andrewkozlik
Copy link
Contributor

What's the payload of AOPP signing message? Is that human readable so we don't need to care and only reuse plain signMessage? I don't see any discussion here about how to particularly render AOPP screen.

The payload is required to be ASCII, thus human readable, so we intend to reuse SignMessage. It can be up to 1024 ASCII characters, but it is recommended to choose a small message to improve UX using hardware wallets (small screens). The specification will also contain a recommended message style.

@21isenough
Copy link

Hey, dear Trezor Team

We are Pocket Bitcoin. A growing Bitcoin DCA-platform from Switzerland 🇨🇭

Many of our clients sign up to Pocket Bitcoin with their Trezors and receive their bitcoin straight to their cold storage. In order to sign up, the users needs to sign a message with their Trezor and for this particular case, AOPP support would be super convenient.

Very happy to find this open issue here on the topic. Is there a timeline or an eta for AOPP support?

@dspicher
Copy link

dspicher commented Nov 8, 2021

@andrewkozlik see the PR linked above for an initial attempt. Feedback would be greatly appreciated.

@andrewkozlik
Copy link
Contributor

Firmware PR ready: trezor/trezor-firmware#1903

@qa Once the PR has been merged, please test that the following features work correctly:

  • T1 support of pagination in SignMessage and Verify message for Bitcoin and Ethereum.
  • T1 and TT show the address in SignMessage and VerifyMessage for Bitcoin and Ethereum.

@alex-jerechinsky alex-jerechinsky transferred this issue from trezor/trezor-firmware Nov 15, 2021
@github-actions github-actions bot added this to 📥 Inbox in Backlog 🗂 Nov 15, 2021
@alex-jerechinsky alex-jerechinsky moved this from 📥 Inbox to 📽 Product in Backlog 🗂 Nov 15, 2021
@andrewkozlik andrewkozlik removed their assignment Nov 15, 2021
@andrewkozlik
Copy link
Contributor

The AOPP-related changes in firmware have been merged into the master branch and will be released in the next firmware update, which is planned in December.

The rest of the work is in trezor-suite. A PR is already open: #4468

@matejzak matejzak added the HIGH label Nov 18, 2021
@marekrjpolak marekrjpolak self-assigned this Nov 19, 2021
@alex-jerechinsky alex-jerechinsky removed this from 📽 Product in Backlog 🗂 Nov 25, 2021
@alex-jerechinsky alex-jerechinsky added this to 🎯 To do in December 8 🎄 via automation Nov 25, 2021
@alex-jerechinsky alex-jerechinsky moved this from 🎯 To do to 🏃‍♀️ In progress in December 8 🎄 Nov 25, 2021
@marekrjpolak marekrjpolak mentioned this issue Nov 29, 2021
marekrjpolak added a commit that referenced this issue Dec 6, 2021
marekrjpolak added a commit that referenced this issue Jan 6, 2022
marekrjpolak added a commit that referenced this issue Jan 6, 2022
@hynek-jina hynek-jina moved this from 🏃‍♀️ In progress to 🔎 Needs review in January 19 ☕️ Jan 11, 2022
January 19 ☕️ automation moved this from 🔎 Needs review to 🤝 Needs QA Jan 11, 2022
tomasklim pushed a commit that referenced this issue Jan 11, 2022
tomasklim pushed a commit that referenced this issue Jan 11, 2022
@bosomt
Copy link
Contributor

bosomt commented Jan 17, 2022

QA OK

Info:

  • Suite version: desktop 22.1.1 (75797e8)
  • Browser: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) TrezorSuite/22.1.1 Chrome/94.0.4606.81 Electron/15.3.0 Safari/537.36
  • OS: MacIntel
  • Screen: 1680x1050
  • Device: model 1 1.10.5 regular

@bosomt bosomt moved this from 🤝 Needs QA to ✅ Approved in January 19 ☕️ Jan 17, 2022
@trezor trezor deleted a comment Jan 31, 2022
@trezor trezor deleted a comment Jan 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
January 19 ☕️
✅ Approved
Development

Successfully merging a pull request may close this issue.