feat(connectors): filter mipd by connector rdns #4343
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: f490398 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Size Change: 0 B Total Size: 796 kB ℹ️ View Unchanged
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4343 +/- ##
===========================================
- Coverage 93.51% 70.37% -23.14%
===========================================
Files 278 259 -19
Lines 7091 6606 -485
Branches 807 545 -262
===========================================
- Hits 6631 4649 -1982
- Misses 448 1934 +1486
- Partials 12 23 +11 ☔ View full report in Codecov by Sentry. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Open thoughts:
|
tmm
force-pushed
the
tmm/6963-rdns-filter
branch
from
c2f1c6d
to
618be64
Compare
I totally get the concern about setting an invalid rdns. To reduce that risk, we can limit the use of rdns to default built-in connectors... A simple validation check could ensure that only valid or registered rdns values are used, which would prevent any issues with third-party connectors while keeping things stable for the built-ins. As for hardcoded values, the rdns for well-known connectors like MetaMask and Coinbase is stable and specific to those connectors, so it’s not really arbitrary or prone to errors. For third-party connectors, developers could handle these identifiers however they want. Wagmi could also expose the mechanism used for the default connectors so developers have the option to extend it if needed, without messing with the core code. |
|
hey @tmm, thanks for drafting this up. To clarify, we are specifically addressing scenarios where developers are integrating the MetaMask SDK[it's opt-in], aiming to offer a more feature-rich experience, including mobile wallet support. The core package is focused on supporting the most widely used and popular connectors, including MetaMask. As such, hardcoding the reverse DNS (RDNS) for these specific connectors is a logical approach. This would not disrupt downstream libraries but would instead enhance the core functionality by not adding extra filtering logic there where you have already made a decision on the wagmi/connector level. |
tmm
commented
Oct 21, 2024
| @@ -257,7 +263,7 @@ export function metaMask(parameters: MetaMaskParameters = {}) { | |||
| parameters.dappMetadata ?? | |||
| (typeof window !== 'undefined' | |||
| ? { url: window.location.origin } | |||
| : { name: 'wagmi' }), | |||
| : { name: 'wagmi', url: 'https://wagmi.sh' }), | |||
There was a problem hiding this comment.
error spamming SSR (e.g. Next.js) without a URL set
EdouardBougon
added a commit
to EdouardBougon/carbon-app
that referenced
this pull request
Oct 22, 2024
Wagmi, with its latest version, has directly implemented connector deduplication. If a custom provider (MetaMask or Coinbase Wallet) is added to the configuration, it will automatically replace the equivalent EIP-6963 connector if detected. Therefore, there is no longer a need to detect and filter them beforehand. Link: wevm/wagmi#4343
EdouardBougon
added a commit
to EdouardBougon/carbon-app
that referenced
this pull request
Oct 23, 2024
Wagmi, with its latest version, has directly implemented connector deduplication. If a custom provider (MetaMask or Coinbase Wallet) is added to the configuration, it will automatically replace the equivalent EIP-6963 connector if detected. Therefore, there is no longer a need to detect and filter them beforehand. Link: wevm/wagmi#4343
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Allows connectors to specify
rdnsproperty and filters out matching EIP 6963 connectors from being created.Related #4328