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

feat: Update HERE Wallet and add topLevelInjected #1077

Merged
merged 3 commits into from
Mar 29, 2024

Conversation

AZbang
Copy link
Contributor

@AZbang AZbang commented Feb 13, 2024

It was necessary to add the ability to associate an application inside an iframe with a parent window and allow seamless initialization and method calls. This will help integrate applications inside wallets. For example Telegram HERE Wallet:

RPReplay_Final1707785907.mp4

topLevelInjected method is needed because wallet-selector is designed in such a way that only modal-ui knows about the contract that the application uses.

@AZbang AZbang changed the base branch from main to dev February 13, 2024 01:20
@ewiner
Copy link
Contributor

ewiner commented Feb 13, 2024

@AZbang can you help me understand why topLevelInjected is different from a normal injected wallet? (Also if we go forward with this PR, you'll have to document that option anyway.)

@AZbang
Copy link
Contributor Author

AZbang commented Feb 13, 2024

@ewiner Added a description to the metadata parameters

@AZbang
Copy link
Contributor Author

AZbang commented Feb 13, 2024

@ewiner The video in the PR description shows that when opening a site inside an iframe, here-wallet transmits wallet data inside the site through here-wallet/core using an iframe postMessage. However, in order for the wallet to be immediately initialized inside wallet-selector, it is necessary to call initialization through modal-ui. The only way to tell modal-ui whether to signIn immediately is to pass a special parameter to metadata.

TopLevel means that the wallet is injected from the parent window or, in the case of native mobile applications, the WebView controller.

@ewiner
Copy link
Contributor

ewiner commented Feb 13, 2024

So the idea is for an injected wallet to force automatic sign-in, so the user doesn't have to click "Connect Wallet" or "Sign In".

I'm new to this codebase, but two questions/comments come to mind:

  1. Shouldn't that be a decision for the app developer to make instead of the wallet provider? Are there apps that would break if the signIn happens upon initialization instead of when the user asks?
  2. The wallet selector supports multiple injected wallets - it seems to me like with this change one of those wallets could force sign-in and prevent you from using others.

@AZbang
Copy link
Contributor Author

AZbang commented Feb 14, 2024

@ewiner

  1. wallet-selector remembers the last authorized wallet between sessions and automatically logs in upon initialization; the application should not care how wallet-selector received the active wallet in own reactive state. By design of the library, the application must subscribe to changes in the selector state.
    Adding the same code to every application seems less sensible than simply updating a tool that is designed to provide access to wallets through a unified API.

  2. In general, yes, if there are two topLevelInjected wallets, then wallet-selector will consider the last one in the list to be the active wallet.
    However, in practice, topLevelInjected should only be used if the wallet is confident that it runs in its native environment and will not conflict. For example, native WebView controllers or embedded widgets on websites.

@trechriron
Copy link
Collaborator

This PR will be eligible for merge into main after review by community and Pagoda. Thanks!

@trechriron
Copy link
Collaborator

@kujtimprenkuSQA is no longer working on this project. If he wants to volunteer his personal time, it would be welcome, however I don't want any SQA folks added as reviewers. I am trying to encourage the community to review this PR. This is still in progress. Thanks!

@hcho112
Copy link
Contributor

hcho112 commented Mar 26, 2024

Agree with @ewiner. This PR change is tailored for specific problem that Here wallet is experiencing. Not disagree with the direction of the PR, but ideally this should have been proposed on NEP and approved by the community and users.

Or at least, I would prefer to get reviewed/feedback by other builder group community, specially around the maintainers of other injected wallets.

@pvolnov
Copy link

pvolnov commented Mar 26, 2024

The idea of wallet-selector is to help projects have access to users. Now we have a Telegram Wallet where large group of all active users are on NEAR. At the same time, no project on NEAR can provide services to such users. By delaying, we are isolating the ecosystem from users.

I believe that taking into account the real state of affairs in the ecosystem, this PR is for the public good and should be implemented in the near future.

@AZbang
Copy link
Contributor Author

AZbang commented Mar 26, 2024

@ewiner @trechriron @hcho112
I really don't understand why you consider topLevelInjected a specific feature for HERE Wallet. Any other wallets will be able to use this to integrate web dApps inside their mobile wallets. This seems very useful. Just because we're the only ones who want to use it doesn't mean it's a feature only for us.

Copy link

@Patrick1904 Patrick1904 left a comment

Choose a reason for hiding this comment

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

LGTM

@bowenwang1996 bowenwang1996 merged commit 6e3c9d1 into near:dev Mar 29, 2024
5 checks passed
@vikpande
Copy link

thank you @Patrick1904

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

8 participants