fix: Add social login feature toggle from config service #2635
Conversation
Branch preview✅ Deploy successful! https://feature_toggle--walletweb.review-wallet-web.5afe.dev |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
| {supportedChains.map((chain, idx) => ( | ||
| <> | ||
| {chain} | ||
| {idx < supportedChains.length - 1 && ', '} | ||
| </> | ||
| ))} |
There was a problem hiding this comment.
| {supportedChains.map((chain, idx) => ( | |
| <> | |
| {chain} | |
| {idx < supportedChains.length - 1 && ', '} | |
| </> | |
| ))} | |
| {supportedChains.join(', ')} |
| return chains.reduce((result: string[], currentChain) => { | ||
| if (CGW_NAMES.SOCIAL_LOGIN && !currentChain.disabledWallets.includes(CGW_NAMES.SOCIAL_LOGIN)) { | ||
| result.push(currentChain.chainName) | ||
| } | ||
| return result | ||
| }, []) | ||
| } |
There was a problem hiding this comment.
Nit: I feel like this is a bit simpler to read
| return chains.reduce((result: string[], currentChain) => { | |
| if (CGW_NAMES.SOCIAL_LOGIN && !currentChain.disabledWallets.includes(CGW_NAMES.SOCIAL_LOGIN)) { | |
| result.push(currentChain.chainName) | |
| } | |
| return result | |
| }, []) | |
| } | |
| const filteredChains = chains.filter((chain) => CGW_NAMES.SOCIAL_LOGIN && !currentChain.disabledWallets.includes(CGW_NAMES.SOCIAL_LOGIN)) | |
| return filteredChains.map((chainConfig) => chainConfig.chainName) |
| const useGetSupportedChains = () => { | ||
| const chains = useChains() | ||
|
|
||
| return _getSupportedChains(chains.configs) |
There was a problem hiding this comment.
Should we memoize this to reduce re-renders?
| const MPCLogin = ({ onLogin }: { onLogin?: () => void }) => { | ||
| export const _getSupportedChains = (chains: ChainInfo[]) => { | ||
| return chains.reduce((result: string[], currentChain) => { | ||
| if (CGW_NAMES.SOCIAL_LOGIN && !currentChain.disabledWallets.includes(CGW_NAMES.SOCIAL_LOGIN)) { |
There was a problem hiding this comment.
Do you know why we type the mapped type of CGW_NAMES as string | undefined?
Can it really be undefined?
Should we refactor it as
export const CGW_NAMES = {
[WALLET_KEYS.INJECTED]: 'detectedwallet',
[WALLET_KEYS.WALLETCONNECT_V2]: 'walletConnect_v2',
[WALLET_KEYS.COINBASE]: 'coinbase',
[WALLET_KEYS.PAIRING]: 'safeMobile',
[WALLET_KEYS.SOCIAL]: 'socialLogin',
[WALLET_KEYS.LEDGER]: 'ledger',
[WALLET_KEYS.TREZOR]: 'trezor',
[WALLET_KEYS.KEYSTONE]: 'keystone',
} as constOr am I missing something about this mapping?
There was a problem hiding this comment.
I was also wondering about that. The only reason I can see is that the WALLET_MODULES and CGW_NAMES should be coupled but currently are not so it could happen that we try to access a key in CGW_NAMES that doesn't exist. I will add a TODO to tackle this as a separate refactor.
| it('returns chain names where social login is enabled', () => { | ||
| const mockEthereumChain = { chainId: '1', chainName: 'Ethereum', disabledWallets: ['socialLogin'] } as ChainInfo | ||
| const mockGnosisChain = { chainId: '100', chainName: 'Gnosis Chain', disabledWallets: ['Coinbase'] } as ChainInfo | ||
| const mockGoerliChain = { chainId: '5', chainName: 'Goerli', disabledWallets: ['TallyHo'] } as ChainInfo |
There was a problem hiding this comment.
Nit: If we leave it empty we test that it also works if no wallets are disabled.
| const mockGoerliChain = { chainId: '5', chainName: 'Goerli', disabledWallets: ['TallyHo'] } as ChainInfo | |
| const mockGoerliChain = { chainId: '5', chainName: 'Goerli', disabledWallets: [] } as ChainInfo |
Coverage report
Show new covered files 🐣
Test suite run success988 tests passing in 139 suites. Report generated by 🧪jest coverage report action from 8fa8f9d |
What it solves
Resolves #2458
How this PR fixes it
How to test it
Screenshots
Checklist