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.

Sign up for GitHub

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

Jump to bottom

Login per site onboarding #7602

Merged
merged 7 commits into from
Dec 20, 2019
Merged

Login per site onboarding #7602

merged 7 commits into from
Dec 20, 2019

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Nov 29, 2019 •
edited
Loading

This PR restores the functionality required for one-click onboarding that was removed in LoginPerSite due to the provider refactor. Instead of restoring a similar "experimental" function that used an independent onboarding stream, the onboarding registration is now implemented as custom RPC message: wallet_registerOnboarding. This message is captured by middleware, which registers the sender's origin and tabId as being the onboarding initiator.

This PR depends upon this forwarder PR and this onboarding PR being merged and published first. (Update: those have both been merged and published! 🎉 Thanks @whymarrh )

  • Remove unused onboarding stream
  • Pass sender through to setupProviderEngine
    The Port sender has been passed down a few more layers. This allows us to get more information from the sender deeper in the stack, but also simplifies things a bit as well. For example, now the "fake" URL object with the metamask hostname is no longer needed.
  • Create onboarding middleware
    This middleware intercepts wallet_registerOnboarding RPC messages. It will register the sender as an oboarding initiator if possible, and otherwise ignores the message.
  • Update versions of forwarder and onboarding library

@Gudahtt Gudahtt force-pushed the LoginPerSite-onboarding branch from 5ea1378 to 6d253ce Compare November 29, 2019 01:58
@Gudahtt Gudahtt mentioned this pull request Nov 29, 2019
Merged
@Gudahtt Gudahtt force-pushed the LoginPerSite-onboarding branch 4 times, most recently from 6bbaba4 to 516ace2 Compare December 2, 2019 20:54
@metamaskbot
Copy link
Collaborator

Builds ready [516ace2]

@Gudahtt Gudahtt force-pushed the LoginPerSite-onboarding branch from 516ace2 to 06dd852 Compare December 3, 2019 18:22
@Gudahtt Gudahtt requested a review from frankiebee as a code owner December 3, 2019 18:22
@Gudahtt Gudahtt changed the base branch from LoginPerSite to develop December 3, 2019 18:23
@Gudahtt Gudahtt removed the request for review from frankiebee December 3, 2019 18:24
@metamaskbot
Copy link
Collaborator

Builds ready [06dd852]

@Gudahtt Gudahtt requested review from danfinlay and rekmarks December 4, 2019 03:21
@danfinlay
Copy link
Contributor

I think I may be missing something conceptually here:

Marking a user as onboarding is something we would do for a user who doesn't have MetaMask yet, but is going to, and will want to return to the invoking site after being onboarded.

If the user doesn't have MetaMask installed yet, how would MetaMask catch an rpc message like wallet_registerOnboarding?

@Gudahtt
Copy link
Member Author

Gudahtt commented Dec 6, 2019 •
edited
Loading

The @metamask/onboarding library injects the forwarder which listens for when MetaMask is installed, which then triggers the site (which is using @metamask/onboarding) to refresh so the provider can be injected. Then the message is sent via the provider while the user is still in the onboarding flow.

It's the same flow as in this diagram (created for #7017 ):

The difference is that we're using this RPC message instead of the _metamask.registerOnboarding() function.

@danfinlay
Copy link
Contributor

Oh awesome, that's a wonderful diagram for demonstrating the flow!

@whymarrh whymarrh requested a review from tmashuang December 9, 2019 14:20
@whymarrh
Copy link
Contributor

whymarrh commented Dec 9, 2019

I'll need to test this out locally. @tmashuang would you mind playing around with it as well?

danjm
danjm previously approved these changes Dec 10, 2019
View reviewed changes
Copy link
Contributor

@danjm danjm left a comment

Choose a reason for hiding this comment

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

Read through all the code. 👍 for the solution for passing the sender and its data through to metamask-controller setup functions.

rekmarks
rekmarks previously approved these changes Dec 11, 2019
View reviewed changes
Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

I had one, really nitty nit. I love it!

app/scripts/metamask-controller.js Outdated Show resolved Hide resolved
@Gudahtt
Remove unused onboarding stream
dced981
@Gudahtt
Pass sender through to setupProviderEngine
096c482 
The Port `sender` has been passed down a few more layers. This allows
us to get more information from the sender deeper in the stack, but
also simplifies things a bit as well. For example, now the "fake"
URL object with the `metamask` hostname is no longer needed.
@Gudahtt
Create onboarding middleware
7b55971 
This middleware intercepts `wallet_registerOnboarding` RPC messages. It
will register the sender as an oboarding initiator if possible, and
otherwise ignores the message.
@Gudahtt
update types
d900fe7
@Gudahtt
Update versions of forwarder and onboarding library
9fa0075
@Gudahtt
Ensure TAB_ID_NONE is not mistaken for a real tab id
98c8485
@Gudahtt Gudahtt dismissed stale reviews from rekmarks and danjm via c31e5cc December 11, 2019 17:31
@Gudahtt Gudahtt force-pushed the LoginPerSite-onboarding branch from 06dd852 to c31e5cc Compare December 11, 2019 17:31
@metamaskbot
Copy link
Collaborator

Builds ready [c31e5cc]

@danjm danjm mentioned this pull request Dec 12, 2019
Merged
whymarrh
whymarrh reviewed Dec 20, 2019
View reviewed changes
app/scripts/lib/createOnboardingMiddleware.js
* @param {{ location: string, tabId: number, registerOnboarding: Function }} opts - The middleware options
* @returns {(req: any, res: any, next: Function, end: Function) => void}
*/
function createOnboardingMiddleware ({ location, tabId, registerOnboarding }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This could pull the tabId from the request in a world where #7695 is merged first

Copy link
Member Author

@Gudahtt Gudahtt Dec 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

That PR relies upon many things in this PR, but yes I agree that it might be cleaner to do it that way. That can be a follow-up PR.

whymarrh
whymarrh approved these changes Dec 20, 2019
View reviewed changes
Copy link
Contributor

@whymarrh whymarrh left a comment

Choose a reason for hiding this comment

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

LGTM, this is a clean implementation

@Gudahtt Gudahtt merged commit 69d418a into develop Dec 20, 2019
@Gudahtt Gudahtt deleted the LoginPerSite-onboarding branch January 13, 2020 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@whymarrh whymarrh whymarrh approved these changes

@rekmarks rekmarks rekmarks left review comments

@danjm danjm danjm left review comments

@danfinlay danfinlay Awaiting requested review from danfinlay

@tmashuang tmashuang Awaiting requested review from tmashuang

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Gudahtt @metamaskbot @danfinlay @whymarrh @danjm @rekmarks