-
-
Notifications
You must be signed in to change notification settings - Fork 35
Add documentation for deep links #158
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
base: main
Are you sure you want to change the base?
Conversation
davidmurdoch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a quick pass. I'll need to read more in depth later.
docs/deep-links.md
Outdated
|
|
||
| ```mermaid | ||
| --- | ||
| title: Examole user flow diagram for deep linking to swaps page |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| title: Examole user flow diagram for deep linking to swaps page | |
| title: Example user flow diagram for deep linking to swaps page |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
| title: Examole user flow diagram for deep linking to swaps page | ||
| displayMode: compact | ||
| --- | ||
| graph TD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
graph text is cut off in many areas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to fix few things. Let me know if it helps or what exactly you think should be better.
docs/deep-links.md
Outdated
| B -->|Yes| C{Is the Link Signed?} | ||
| B -->|No| D[Take User to metamask.io/download] | ||
|
|
||
| C -->|Yes| E[Skip Interstitial Warning Page\n and show link info page once] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is an interstitial screen for signed links as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated this description now.
docs/deep-links.md
Outdated
| Branch service is configured and automatically capable of linking any path to the extension route, if the route is registered. | ||
| So, adding `/home` or `/swap` at the end of `link.metamask.io` would be enough, and there is no need for additional configurations on the Branch side. | ||
|
|
||
| Internal deep links are supposed to be signed and skip interstitial (warning page). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no such think as "internal" deep links. And there is an interstitial shown for both signed and unsigned links.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the terminology and description around this. Let me know if it helps.
docs/deep-links.md
Outdated
| Branch service is configured and automatically capable of linking any path to the extension route, if the route is registered. | ||
| So, adding `/home` or `/swap` at the end of `link.metamask.io` would be enough, and there is no need for additional configurations on the Branch side. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should clarify this. The wording here on the first sentence makes it seems like the link MUST be registered in branch. I know what you mean, but it can be easily be read to mean the opposite of what you intended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to explain this in a better way. Let me know if it's good enough or any other advice if you have.
docs/deep-links.md
Outdated
|
|
||
| export default new Route({ | ||
| pathname: '/home', | ||
| getTitle: (_: URLSearchParams) => 'deepLink_theHomePage', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mention that this is a translation key (in messages.json)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added comment.
docs/deep-links.md
Outdated
|
|
||
| - Deep link route definition consists of: | ||
| - `pathname` - Route path identifier (e.g., `/home`, `/swaps`, `/notifications`) | ||
| - `getTitle` - Callback function that should return title of the deep link page. This is usually represented by the translation constant (e.g., `deepLink_theHomePage`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not usually though. Always.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
| - `path` - Exact path of the route used in extension. Taken from the existing routes definitions (e.g., `DEFAULT_ROUTE`, `NOTIFICATIONS_ROUTE`). | ||
| - `query` - URL query params. Constructed using `URLSearchParams` constructor function. | ||
| - Make sure that the route exists and return it from a handler under `path` key. | ||
| - Handler function can transform query parameters if necessary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its worth noting that
a) it must be synchronous.
b) it must NEVER change anything. Deep links are read only and all actions/changes must be confirmed by the user in the UI layer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. I added what you suggested.
docs/deep-links.md
Outdated
| RouteRegistry --> Route : registers and stores | ||
| ``` | ||
|
|
||
| Deep Link Router is instantiated in service worker ([background.js](https://github.com/MetaMask/metamask-extension/blob/main/app/scripts/background.js#L757)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not if its in Firefox where there are now service workers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote this in a little bit different way now. Let me know if calling it just background script is better or we should clarify something more?
docs/deep-links.md
Outdated
|
|
||
| ## Security | ||
|
|
||
| Internal deep links are supposed to be signed and signed links will skip interstitial (warning page). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use "internal". All links are public.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated this now to explain it better.
| Branch service is configured and automatically capable of linking to any path registered within the extension as a deep link route. | ||
| So, adding `/home` or `/swap` at the end of `link.metamask.io` would be enough, and there is no need for additional configurations on the Branch side. | ||
|
|
||
| Signed deep links will skip the interstitial warning page and show the link information interstitial instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to link the original ADR here with more explanation of trusted/untrusted flow:
https://github.com/MetaMask/decisions/blob/dcb42a5395507928e87c183dd1809c83d9cb408d/decisions/core/0007-deep-linking-into-wallet.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added, as you suggested.
This PR adds contributor documentation for deep links.
Click here for rendered markdown version.
Note
Adds comprehensive Deep Links guidelines in
docs/deep-links.mdand links it fromREADME.md.docs/deep-links.mddetailing deep links overview, example behavior, user flow (mermaid), creation/signing with Branch, route registration in the extension (with code samples), architecture (class diagram), and security recommendations.docs/deep-links.mdin the table of contents.Written by Cursor Bugbot for commit 9569b69. This will update automatically on new commits. Configure here.