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

Fix: default wallet when none are enabled #1597

Merged
merged 1 commit into from
Jan 26, 2023
Merged

Fix: default wallet when none are enabled #1597

merged 1 commit into from
Jan 26, 2023

Conversation

katspaugh
Copy link
Member

What it solves

Resolves #1596

How this PR fixes it

When all wallets are disabled, we pass an empty array to onboard, but it needs at least one wallet. So I made it return the "injected" wallet as a default.

How to test it

Cello has currently 0 wallets enabled in the staging config.

@github-actions
Copy link

github-actions bot commented Jan 26, 2023

Branch preview

✅ Deploy successful!

https://fix_chain_wallets--webcore.review-web-core.5afe.dev

@github-actions
Copy link

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

const enabledWallets = Object.entries(WALLET_MODULES).filter(([key]) => isWalletSupported(chain.disabledWallets, key))

if (enabledWallets.length === 0) {
return [WALLET_MODULES.INJECTED()]
Copy link
Member

Choose a reason for hiding this comment

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

MetaMask can be disabled:

image

We should probably return an empty array here, as well as filter the "recommended injected wallets" in getRecommendedInjectedWallets. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you're thinking too much into it. In practice, no chain will have all wallets disabled, and defaulting to the injected wallet is a quick and pragmatic solution w/o adding too many conditions IMHO.

Copy link
Member

Choose a reason for hiding this comment

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

We should remove MetaMask from the list of wallets that can be disabled when we eventually remove the "legacy" wallet names then.

@liliya-soroka
Copy link
Member

verified
can be merged

@katspaugh katspaugh merged commit e84003f into dev Jan 26, 2023
@katspaugh katspaugh deleted the fix-chain-wallets branch January 26, 2023 11:11
@github-actions github-actions bot locked and limited conversation to collaborators Jan 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 wallets are displayed by default when no selected wallets in the config service
3 participants