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

[BUG] Support Incompatible Wallets #1141

Closed
kneerose opened this issue Jun 3, 2024 · 4 comments
Closed

[BUG] Support Incompatible Wallets #1141

kneerose opened this issue Jun 3, 2024 · 4 comments
Assignees
Labels
🐛 Bug Something isn't working

Comments

@kneerose
Copy link
Contributor

kneerose commented Jun 3, 2024

Context & versions

Steps to reproduce

  • Install a wallet extension that does not support CIP-95 or is not listed on the compatibility list .
  • Click the "connect wallet" button.
  • Observe the available wallet options.
  • Attempt to connect with the incompatible wallet.

Actual behaviour

  • The popup modal displays both compatible and incompatible wallets, as shown in the attached images:

image

  • Attempting to connect with an incompatible wallet results in the 'undefined reading getExtensions' error.

image (1)

Expected behaviour

  • Should verify CIP-95 compatibility before displaying any wallets.

For more details, refer to this report

@NabinKawan NabinKawan added the 🐛 Bug Something isn't working label Jun 3, 2024
@MSzalowski MSzalowski removed their assignment Jun 13, 2024
@MSzalowski
Copy link
Contributor

MSzalowski commented Jun 14, 2024

@Ryun1 What do you think of filtering out CIP-95 incompatible wallets?

Edit:
It seems like we have the wrong filtering in GovTool and based on the code the intention indeed was to filter out these wallets. So that, I'll cover that issue

@MSzalowski
Copy link
Contributor

After a more in-depth analysis, the issue is rather on the wallet side.
We display on a list every wallet that includes:

supportedExtensions: [{ cip: 95 }]

and the wallet you provided on a picture, indeed includes this entry in its declaration:
Zrzut ekranu z 2024-06-14 13-13-50

And that makes, that we, based on CIP-95 Extension API, provide the extensions: [{ cip: 95 }] argument while enabling the wallet. And at this step there is the problem described in the issue - priority wallet even though it declares support for CIP-95, it doesn't really have such support that is why such error on modal occurs.

However, I made some improvement to the listing of the wallets and displaying an error on a modal informing the user, that the wallet that has been listed doesn't really have the CIP-95 support or even CIP-30 support due to missing getExtensions described here.

@Ryun1 Should we depend only on the list of compatible wallets and display only these? Or on the choose wallet modal
should we display every wallet that has the cip-95 and cip-30 supported extension or the wallets that has only the cip-95 extension as we do currently?

So far within this PR I introduced following solution:
Nagranie ekranu z 2024-06-14 13-38-24

@Ryun1
Copy link
Member

Ryun1 commented Jun 14, 2024

@MSzalowski thanks for digging into this

This error is being caused by a faulty wallet implementation (intentional or not), as it is missing the .getExtensions() endpoint in the Full CIP-30 API.

@Ryun1 Should we depend only on the list of compatible wallets and display only these?

No
for context; unfortunelty there is a precedent in Cardano to whitelist wallets for access to dApps, which has bad knock-on effects like wallets spoofing their whitelisted competitors AND general confusion for users.
Furthermore, the point of standards is that we don't need to explicitly whitelist, if a wallet says that it is compatible with the standard (at connection time) we should believe it.
And if it turns out that that wallet did not implement the standard as it stated, then that is the wallet's fault and the user should contact the wallet's support.

should we display every wallet that has the cip-95 and cip-30 supported extension or the wallets that has only the cip-95 extension as we do currently?

I much prefer only showing users the CIP95 compatible wallets that they have installed.
Mainly this forces people to dig into why their CIP30 wallet does not support governance and thus will also encourage wallets to implement CIP95.

@bosko-m
Copy link
Contributor

bosko-m commented Jul 3, 2024

Closing this with the discussion above.

@bosko-m bosko-m closed this as completed Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 Bug Something isn't working
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants