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

anchor integration #517

Closed
wants to merge 9 commits into from
Closed

anchor integration #517

wants to merge 9 commits into from

Conversation

7flash
Copy link
Collaborator

@7flash 7flash commented Feb 18, 2021

I created a ticket for this

#675

[Nik here] Purpose to enable Seeds light wallet to sign transactions on the DHO or any website that uses the Anchor protocol

Prior, we were able to sign QR codes in the ESR format

But Anchor is an open source protocol to implement a secure channel between a website an an app.

https://github.com/greymass/anchor

Basically, this pull request enables correct handling of identity request with callbacks, and in other PRs later we can implement long-living sessions and etc

Peek 2021-02-19 02-26

lib/screens/app/app.dart Outdated Show resolved Hide resolved
lib/screens/app/app.dart Outdated Show resolved Hide resolved
lib/screens/app/app.dart Outdated Show resolved Hide resolved
lib/screens/app/app.dart Outdated Show resolved Hide resolved
lib/screens/app/app.dart Outdated Show resolved Hide resolved
lib/screens/app/app.dart Outdated Show resolved Hide resolved
@gguijarro-c-chwy
Copy link
Collaborator

Thank you for the video, but this PR has no issue attached to it and no explanations of whats the purpose of the PR.

Copy link
Member

@n13 n13 left a comment

Choose a reason for hiding this comment

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

Some potentially unused files, some unused code, please fix

lib/providers/services/links_service.dart Outdated Show resolved Hide resolved
lib/screens/app/app.dart Outdated Show resolved Hide resolved
lib/screens/app/app.dart Outdated Show resolved Hide resolved
lib/screens/app/temp.json Outdated Show resolved Hide resolved
Copy link
Member

@n13 n13 left a comment

Choose a reason for hiding this comment

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

move auth functions to other file - not sure if that means separating UX and logic as well - may have to at that point

lib/screens/app/app.dart Outdated Show resolved Hide resolved
lib/screens/app/app.dart Outdated Show resolved Hide resolved
@n13
Copy link
Member

n13 commented Mar 16, 2021

@7flash I tested this on the hypha website and it doesn't work for login.

I don't know why it doesn't work

The anchor login seems to be ok (get a success) but there's a backend protocol where it sets up a secure channel between the phone and the website and that doesn't seem to work

I would be OK with setting up a whole different website integration -

Example - we could just sign a policy transaction that connects a session ID with an account, proving login

Then sign all transactions with QR Code / link when on mobile

That would be easy to do for ut

Or we implement the anchor protocol but if we do that then we also need to handle the secure channel and requests coming from the website through anchor to our wallet.

I don't see any code that would do that in this PR - and I also don't know how to do that. Socket connection? IDK.

@7flash
Copy link
Collaborator Author

7flash commented Mar 17, 2021

@7flash I tested this on the hypha website and it doesn't work for login.

I don't know why it doesn't work

The anchor login seems to be ok (get a success) but there's a backend protocol where it sets up a secure channel between the phone and the website and that doesn't seem to work

I would be OK with setting up a whole different website integration -

Example - we could just sign a policy transaction that connects a session ID with an account, proving login

Then sign all transactions with QR Code / link when on mobile

That would be easy to do for ut

Or we implement the anchor protocol but if we do that then we also need to handle the secure channel and requests coming from the website through anchor to our wallet.

I don't see any code that would do that in this PR - and I also don't know how to do that. Socket connection? IDK.

Yes, websocket connection via anchor-link protocol, I think it's already implemented on website, and I can implement long sessions on the app too

@7flash
Copy link
Collaborator Author

7flash commented Mar 24, 2021

Tried with DHO - working too, can you please merge @n13

@7flash
Copy link
Collaborator Author

7flash commented Mar 24, 2021

Peek 2021-03-24 17-20

@7flash
Copy link
Collaborator Author

7flash commented Mar 29, 2021

@gguij004 can you merge please

@n13
Copy link
Member

n13 commented Mar 29, 2021

@gguij004 can you merge please

I think you mean Gery @gguijarro-c-chwy

Needs to approve ;)

@gguijarro-c-chwy
Copy link
Collaborator

I see merge conflicts and lint issues.

This so also targeting master and i think we should target the current release build 1.4.4 i think?

We can take care adding this feature to the master v2 build once it passes by design and product manager that creates a story to track the work and acceptance criteria.

@JGDoh @7flash @7flash

@n13
Copy link
Member

n13 commented Mar 29, 2021 via email

@gguijarro-c-chwy
Copy link
Collaborator

conflicts.
Please reopen when fixed

@n13
Copy link
Member

n13 commented Apr 15, 2021

I see merge conflicts and lint issues.

This so also targeting master and i think we should target the current release build 1.4.4 i think?

We can take care adding this feature to the master v2 build once it passes by design and product manager that creates a story to track the work and acceptance criteria.

@JGDoh @7flash @7flash

We really need this - important to integrate with many of our projects, seeds swap etc.

I see no harm merging it and testing it.

Although lint needs to pass obviously @7flash

@n13 n13 reopened this Apr 15, 2021
@n13
Copy link
Member

n13 commented Apr 15, 2021

@gguijarro-c-chwy this one targets v2 / master, this is correct

@n13
Copy link
Member

n13 commented Apr 15, 2021

Ticket is here #675

@gguijarro-c-chwy

@gguijarro-c-chwy
Copy link
Collaborator

Conflicts

@@ -30,7 +30,7 @@ class ProductModel extends HiveObject {
currency: data.data()[PRODUCT_CURRENCY_KEY],
position: data.data()[PRODUCT_POSITION_KEY] ?? 0,
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any actual changes on this file or only formatting ?
It helps when the actual changes are pointed out. Saves the reviewers times.

@@ -0,0 +1,76 @@
import 'dart:typed_data';

import 'package:eosdart_ecc/eosdart_ecc.dart';
Copy link
Collaborator

Choose a reason for hiding this comment

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

This code is pointing to master v2
All new code for v2 should be inside the V2 folder.

This is a service, should be inside the v2 services folder. remote

import 'package:eosdart/src/serialize.dart' as ser;
import 'package:dart_esr/dart_esr.dart';

class IdentityResponse {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a model, should not live in the service, but in a model class

import 'package:dart_esr/dart_esr.dart';

class IdentityResponse {
final String callback;
Copy link
Collaborator

Choose a reason for hiding this comment

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

callback?
a string?

When i think of a callback i think of a callback i think of a function.

Can we add comments to these params, explain what they represent.

Copy link
Member

Choose a reason for hiding this comment

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

Callback is a callback URL provided by the identity request

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah i see, perhaps name it
callbackUrl

}

class EsrService {
Future<IdentityResponse> getIdentityResponse({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Services return "Result" values

import 'package:seeds/widgets/pending_notification.dart';

import 'process_dynamic_links.dart';
Copy link
Collaborator

Choose a reason for hiding this comment

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

this import is not importing the package like the rest of imports?

}

void processTransactionRequest(SeedsESR request) async {
await request.resolve(
Copy link
Collaborator

Choose a reason for hiding this comment

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

we no longer use SettingsNotifier to get the accountName
we use the storageService i think its the name. Its a singleton

import 'package:url_launcher/url_launcher.dart' as UrlLauncher;
import 'package:convert/convert.dart';

class ProcessDynamicLinks extends StatefulWidget {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This entire class is a view,

There should be no network requests here
No Logic
No mappings
Etc

This class should have a bloc that handles all these things.

https://pub.dev/packages/flutter_bloc#:~:text=BlocProvider%20is%20a%20Flutter%20widget,multiple%20widgets%20within%20a%20subtree.

We have many examples how to integrate blocs into our views in V2. I am also here to help with any questions.

@@ -1,7 +1,9 @@
import 'package:flutter/material.dart';
import 'package:http/http.dart';
Copy link
Collaborator

Choose a reason for hiding this comment

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

this class is already obsolete. we moved all the scan code over to v2

return esr.actions.first.account.isNotEmpty && esr.actions.first.name.isNotEmpty;
void processIdentityRequest(SeedsESR esr) async {
try {
var response = await EsrService().getIdentityResponse(
Copy link
Collaborator

Choose a reason for hiding this comment

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

But fyi, no network calls are made from UI. Those are moved into services, blocs, useCases, mappers.

@n13
Copy link
Member

n13 commented May 5, 2021

using anchor_v2 PR instead

@n13 n13 closed this May 5, 2021
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.

None yet

3 participants