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

Pr/aopp #4572

Merged
merged 9 commits into from Jan 11, 2022
Merged

Pr/aopp #4572

merged 9 commits into from Jan 11, 2022

Conversation

marekrjpolak
Copy link
Contributor

@marekrjpolak marekrjpolak commented Nov 29, 2021

First version of AOPP protocol support (#4512), without any product input for now. It is an alternative for PR #4468, which is WIP and probably stalled.

  • you can give it a try e.g. here
  • aopp: protocol link works only in desktop app, you must copy URI/scan QR code in web app instead
  • sending ownership proof to the example page above doesn't work from web app because of CORS configuration on their side

Copy link
Member

@tomasklim tomasklim left a comment

Choose a reason for hiding this comment

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

Works great!

  1. I can open n number of aopp notifications but only the info from last one is filled.

Screenshot 2021-11-29 at 14 47 52

  1. Maybe message about old fw should be available in prior to signing?

  2. There is incorrect text in Paste URI. (But maybe it is waiting for product right?)

Screenshot 2021-11-29 at 14 57 19

packages/suite/src/utils/suite/env.ts Outdated Show resolved Hide resolved
packages/suite/src/reducers/suite/protocolReducer.ts Outdated Show resolved Hide resolved
packages/suite/src/utils/suite/parseUri.ts Outdated Show resolved Hide resolved
packages/suite/src/utils/suite/parseUri.ts Outdated Show resolved Hide resolved
packages/suite/src/utils/suite/parseUri.ts Outdated Show resolved Hide resolved
@marekrjpolak
Copy link
Contributor Author

  1. I can open n number of aopp notifications but only the info from last one is filled.

But it should be exactly the same with bitcoin: protocol, shouldn't it? I'm unfortunately not able to test it in desktop app on NixOS and web app opens each link in another tab.

  1. Maybe message about old fw should be available in prior to signing?

It totally should, I'll add it.

  1. There is incorrect text in Paste URI. (But maybe it is waiting for product right?)

Yes, it's waiting for product.

@dspicher
Copy link

Thanks a lot for taking this on @marekrjpolak. Works great, I was able to test everything except native URI support.

I'll look into the CORS issue on our testing backend. Please let me know if I can help out with anything else.

@tomasklim
Copy link
Member

  1. I can open n number of aopp notifications but only the info from last one is filled.

But it should be exactly the same with bitcoin: protocol, shouldn't it? I'm unfortunately not able to test it in desktop app on NixOS and web app opens each link in another tab.

Check protocolMiddleware.ts

@marekrjpolak
Copy link
Contributor Author

Thanks a lot for taking this on @marekrjpolak. Works great, I was able to test everything except native URI support.

Thank you for the feedback, @dspicher. Could you please elaborate on what you mean by native URI support? There are currently two ways how to initiate AOPP in Suite:

  • by clicking the aopp:xxxxxxxx link, which works only if you have desktop Suite app registered as a default handler for aopp: scheme, and doesn't work in web Suite at all, because the scheme is not permitted in browsers, or
  • by scanning QR/pasting URI by clicking Import AOPP button in Sign&Verify form, but you have to have the latest (yet unreleased) firmware installed or the button is hidden.

Which of these is causing trouble?

@szymonlesisz
Copy link
Contributor

paste URI it doesnt work for me either. I've used url from the test fixture:

aopp:?v=0&msg=MESSAGE&asset=btc&format=any&callback=https%3A%2F%2Ftesting.21analytics.ch%2Fproofs%2Fc220a28e-0e99-4be6-8578-a886f628ee20

@lclc
Copy link

lclc commented Nov 29, 2021

paste URI it doesnt work for me either. I've used url from the test fixture:

aopp:?v=0&msg=MESSAGE&asset=btc&format=any&callback=https%3A%2F%2Ftesting.21analytics.ch%2Fproofs%2Fc220a28e-0e99-4be6-8578-a886f628ee20

The URI is only valid one-time. When the specific proof has been registered it's not valid anymore (this is specific to our backend implementation of AOPP). @dspicher maybe we should add one proof that is always valid for automated testing purposes?

@dspicher
Copy link

dspicher commented Nov 29, 2021

paste URI it doesnt work for me either. I've used url from the test fixture:

aopp:?v=0&msg=MESSAGE&asset=btc&format=any&callback=https%3A%2F%2Ftesting.21analytics.ch%2Fproofs%2Fc220a28e-0e99-4be6-8578-a886f628ee20

The URI is only valid one-time. When the specific proof has been registered it's not valid anymore (this is specific to our backend implementation of AOPP). @dspicher maybe we should add one proof that is always valid for automated testing purposes?

If you guys want to have automated tests that use our backend we can consider that. For manual testing, you can just get a new one under

https://testing.aopp.group/ for BTC
https://testing.aopp.group/?asset=eth&format=standard for ETH

and the big button will link to a fresh URI where a proof can be submitted. After submission, you should see the registered address on the page. Worked flawlessly the first time for me.

@dspicher
Copy link

Thanks a lot for taking this on @marekrjpolak. Works great, I was able to test everything except native URI support.

Thank you for the feedback, @dspicher. Could you please elaborate on what you mean by native URI support? There are currently two ways how to initiate AOPP in Suite:

  • by clicking the aopp:xxxxxxxx link, which works only if you have desktop Suite app registered as a default handler for aopp: scheme, and doesn't work in web Suite at all, because the scheme is not permitted in browsers, or
  • by scanning QR/pasting URI by clicking Import AOPP button in Sign&Verify form, but you have to have the latest (yet unreleased) firmware installed or the button is hidden.

Which of these is causing trouble?

Hey @marekrjpolak, yeah, this is just specific to my setup. For some reason, I can't even get the bitcoin: URI scheme to link to the Desktop app (neither with a released AppImage, nor with a development build). That's why I was not able to test the process from the beginning of an AOPP link. But as I said, starting from scanning the QR code or pasting the URI, it worked flawlessly.

@lclc
Copy link

lclc commented Nov 29, 2021

First version of AOPP protocol support (#4512), without any product input for now. It is an alternative for PR #4468, which is WIP and probably stalled.

* you can give it a try e.g. [here](https://demo.aopp.group)

* `aopp:` protocol link works only in desktop app, you must copy URI/scan QR code in web app instead

* sending ownership proof to the example page above doesn't work from web app because of CORS configuration on their side

We fixed the CORS on our testing/AOPP integration playground (https://testing.aopp.group).

Please always use https://testing.aopp.group and not demo.aopp.group.

The AOPP proofs delivered at https://testing.aopp.group are accessible here: https://testing.21analytics.ch/aopp (admin / admin).

@dspicher
Copy link

First version of AOPP protocol support (#4512), without any product input for now. It is an alternative for PR #4468, which is WIP and probably stalled.

* you can give it a try e.g. [here](https://demo.aopp.group)

* `aopp:` protocol link works only in desktop app, you must copy URI/scan QR code in web app instead

* sending ownership proof to the example page above doesn't work from web app because of CORS configuration on their side

We fixed the CORS on our testing/AOPP integration playground (https://testing.aopp.group).

Please always use https://testing.aopp.group and not demo.aopp.group.

The AOPP proofs delivered at https://testing.aopp.group are accessible here: https://testing.21analytics.ch/aopp (admin / admin).

You will find them under "Private Wallets" in the top nav bar.

@marekrjpolak
Copy link
Contributor Author

@tomasklim

  1. I can open n number of aopp notifications but only the info from last one is filled.

Please test it on Mac, I'm not able to do that on NixOS. 682b743#diff-9d71296f39030b17aaa9f186a7caea8f9f70916fac55e5ed8384d5cabb5026cb

  1. Maybe message about old fw should be available in prior to signing?

I'll ask product what to do with this. But as I prefer to show this message in the autofill notification, it would be much easier to do it in the following PR #4589.

tomasklim
tomasklim previously approved these changes Jan 10, 2022
@tomasklim tomasklim self-requested a review January 10, 2022 14:57
@matejkriz matejkriz added READY FOR REVIEW Developed pull request ready to be reviewed by another developer release Will be included in the upcoming release. Needs to be backported to the release branch. labels Jan 11, 2022
Copy link
Member

@tomasklim tomasklim left a comment

Choose a reason for hiding this comment

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

🥳 amazing work

@tomasklim tomasklim merged commit eafbaae into develop Jan 11, 2022
@tomasklim tomasklim deleted the pr/aopp branch January 11, 2022 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
READY FOR REVIEW Developed pull request ready to be reviewed by another developer release Will be included in the upcoming release. Needs to be backported to the release branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AOPP support
6 participants