feat: Default Wallets Module #379
Conversation
|
Hi @kujtimprenkuSQA , I'm curious what the benefit of this package is? Consumers of the wallet-selector package can already select which wallets they'd like to implement using the individual packages. I feel like only putting a subset of wallets in this "recommended package" kinda goes against the ethos of the Near ecosystem being "decentralized" and "open", this seems to give a rather biased pathway for folks to implement external wallets. In the end- the amount of effort required to import and use the individual wallets is almost exactly the same as importing them all from this "recommended-wallets" package. Why not just create a list of "recommended wallets" in the Readme to import, if you'd really like to only recommend the tried and tested wallets? This would be something a lot easier to change later on down the line as you can recommend more wallets. And if this is the direction you guys are going to head, as a developer in a wallet team myself- what would it take for us to get on this "recommended package" ? |
|
@lostpebble Thanks for your comment. The developer ultimately has complete control over which wallets to show on their dapp. The fact that we are asking all dapp developers to move away from hardcoding a single wallet url and use wallet-selector as well as onboarding wallets to wallet-selector are very much in the spirit of decentralization. However, as we open wallet-selector to new wallets, there is a fine balancing act of ensuring the user funds are safe and that the wallets provided by third party are compliant to wallet standards and security standards while we decentralize the wallet space. The list that you see included here (sender is not part of the list) may change and consists our current understanding of wallet codebases what we have good understanding of wallet security implications. We will continuously evaluating this list as the space grows. I believe I shared on the PR you had raised. Here's a criteria for wallets https://github.com/near/wallet-selector/blob/main/CONTRIBUTING.md and we will also be listening closely to community on the feedback for the wallets listed. Thanks. |
|
This PR negates the original intent as well, we want DApp developers not to have to re-deploy everytime a new list of recommended wallets is introduced. This pushes the complexity of DApp developers to understand that a new list has been authored and will need to re-deploy their entire web app just to update wallet selector with a new list of wallets. Ideally, DApp developers can:
hope this makes sense. |
@agileurbanite Which wallets were you suggesting to go on the recommended default list in this ticket? As long a there is a list, it is either a) the entire list, or b) a subset. Since the original ticket asked for a list of wallets as recommended defaults, I see that taking a conservative approach by only listing wallets we understand from a risk perspective is better than listing all the wallets. I don't think that the original ask of importing wallets during runtime has its security risks flushed out. |
There was a problem hiding this comment.
@kujtimprenkuSQA Removing the magic strings and 'whitelist' argument is a good change, but we do still want the recommended-wallets package to expose a single function like setupRecommendedWallets that will return the value that should be passed in as the modules argument to setupWalletSelector(). This function is the 'sane defaults' that we want to encourage developers to use; without it, as @lostpebble pointed out, there is really no difference between the implementation here and just importing the individual packages themselves and manually passing them in as modules. We still want the interface to look like the example code you had in #376:
const selector = await setupWalletSelector({
network: "testnet",
modules: await setupRecommendedWallets(),
});If someone wanted default wallets plus one of their own, they could do something like:
const myExampleWallet = {...};
const selector = await setupWalletSelector({
network: "testnet",
modules: [...(await setupRecommendedWallets()), myExampleWallet],
});
kujtimprenkuSQA
changed the title
github-actions
bot
changed the title
This API feels a little convoluted when you want to add extra wallets and opens the door for weirdness (example, adding a wallet twice because it happens to be in the "recommended" package and a separate package- its not very obvious what exactly is in the recommended wallets package). I still don't really understand why this is necessary- "Recommended Wallets" could easily be a section in the readme with an example of how to import them. Also we're unsure why MyNearWallet is included by default- especially when they are now not an "official" wallet anymore and since they are a carbon copy of the previous Near Wallet, I expect they still have a known (and quite serious in my eyes) security issue (https://twitter.com/smsunarto/status/1555382740228247554). We've done research in this area in our own wallet, and it seems a purely "web wallet" will never be completely secure because of these storage issues. As a final point, I feel like this whole extra package is going to discourage DApps from adding other "non-recommended" wallets. I feel like a lot of people would just implement this package and call it a day- without taking the time to do research on other wallets, since they're not "recommended" anyway. |
kujtimprenkuSQA
changed the title
There was a problem hiding this comment.
Just two questions here
Also, I am curious about the inclusion of WalletConnect as a default; I was under the impression there are not yet any NEAR wallets supporting WC in the wild cc: @janewang
| if (modules.some((x) => x.id === module.id)) { | ||
| throw new Error("Duplicate module id detected: " + module.id); | ||
| continue; |
There was a problem hiding this comment.
Curious as to why this was removed - should we not still be alerting someone if they configure the selector with multiple instances of the same module -- unique id values are important because they are used as keys on an object
There was a problem hiding this comment.
If we add 2 wallets with the same id the last one this way will be skipped.
If we don't add continue here the initialization of the wallet selector in setupWalletSelector will fail with the above error.
This was done to prevent failing of setupWalletSelector when we want to override one of the default wallets.
There was a problem hiding this comment.
Developers may want to override one of the default wallets in the list to add custom options in the setup function of that specific wallet.
| setupMyNearWallet(), | ||
| setupLedger(), | ||
| setupWalletConnect({ | ||
| projectId: "c8cb6204543639c31aef44ea4837a554", |
There was a problem hiding this comment.
hmm -- where did this projectId come from?
There was a problem hiding this comment.
This is coming from here: https://cloud.walletconnect.com/sign-in .
I have mentioned it a few times on Slack that if we add WalletConnect to the default wallets it will require some params ( This is a hardcoded - default projectId otherwise the pairing does not work without it).
Update:
WalletConnect is one of those wallet options that would require a projectId which is different for each dApp.
These options should be more dApp specific than Wallet Selector specific:
{
projectId: "c8cb6204543639c31aef44ea4837a554",
metadata: {
name: "NEAR Wallet Selector",
description: "Example dApp used by NEAR Wallet Selector",
url: "https://github.com/near/wallet-selector",
icons: ["https://avatars.githubusercontent.com/u/37784886"],
},
}
@MaximusHaximus , that's correct there's no official wallet that supports NEAR + WalletConnect. |
… to prior states Tweaked wording of customization example to use a customized ledger icon rather than WalletConnect
|
@kujtimprenkuSQA I had a chat with @janewang and she confirmed that we won't be shipping with WalletConnect in the initial default list. I pushed a commit to remove it in the interest of efficiency/time. I think this PR is ready to ship. |
|
@MaximusHaximus, thank you very much for the help and the review in this PR. |
Description
This package includes these default wallets:
This PR groups the setup of the mentioned wallets above with default options.
Created new package
@near-wallet-selector/default-walletsExposed
setupDefaultWallets().Follow the docs here to learn more on how to use this package: Readme
Closes # (issue)
Checklist:
Type of change. This type of change is the main reason for the PR.
Breaking changes