-
Notifications
You must be signed in to change notification settings - Fork 20
Feat: Integrate Reown AppKit; replace Web3 Onboard; add wallet priorities and Google socials #604
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
base: master
Are you sure you want to change the base?
Conversation
…ority (GoodWallet, Valora), Google socials, MiniPay labeling; migrate connect flows to AppKit
|
Hi @L03TJ3 , can you have a look of my draft pr for this task at your earliest convenience, so I can know if am on the right track and what's remaining for me to do, thanks alot 🙂 |
package.json
Outdated
| "@babel/runtime": "^7.18.9", | ||
| "@gooddollar/good-design": "^0.4.24", | ||
| "@gooddollar/goodprotocol": "2.0.34-beta.1", | ||
| "@gooddollar/good-design": "^0.4.21", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be updated/merged with master to reflect latest changes
| return ( | ||
| <TransactionWrapper> | ||
| <TransactionState url={getExplorerLink(chainId, hash, 'transaction')} dataAttr="external_explorer"> | ||
| <TransactionState url={getExplorerLink(+(chainId ?? 1), hash, 'transaction')} dataAttr="external_explorer"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <TransactionState url={getExplorerLink(+(chainId ?? 1), hash, 'transaction')} dataAttr="external_explorer"> | |
| <TransactionState url={getExplorerLink(+(chainId ?? 42220), hash, 'transaction')} dataAttr="external_explorer"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We default to celo, not ethereum mainnet
| {i18n._(t`Disconnect`)} | ||
| </WalletAction> | ||
| )} | ||
| {(walletInfo?.name || isMiniPay()) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is minipay should be variable, outside of render
| //eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| const [{ wallet, connecting }, connect, disconnect] = useConnectWallet() | ||
|
|
||
| const { address } = useAppKitAccount() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create a wrapper hook, something like 'useConnectionInfo' that returns things like wallet-info/chainId/account
replace everywhere we use more then one appkit hook
| text={buttonText} | ||
| web3Action={noop} | ||
| supportedChains={[SupportedChains.CELO, SupportedChains.MAINNET, SupportedChains.FUSE, SupportedChains.XDC]} | ||
| supportedChains={[SupportedChains.CELO, SupportedChains.MAINNET, SupportedChains.FUSE]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One merge with master has incorrect resolved conflicts.
How to resolve conflicts:
- master branch changes are almost always leading.
- when not sure, don't assume what should be resolution, ask the team please!
XDC is an added/new supported chain, and this should not be removed by your PR
| const { chainId, error, active } = useActiveWeb3React() | ||
| const { initialized } = useAppKitState() | ||
| const { chainId } = useAppKitNetwork() | ||
| // TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this?
| return ( | ||
| <View style={{ ...styles }}> | ||
| <NavLink key={route} to={route} onPress={handleInternal}> | ||
| <NavLink key={index} to={route} onPress={handleInternal}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
never use index as key
src/components/WalletModal/index.tsx
Outdated
|
|
||
| const { account, error } = useActiveWeb3React() | ||
| const { address } = useAppKitAccount() | ||
| // TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the todo?
| @@ -0,0 +1,11 @@ | |||
| export const WalletLabels: Readonly<string[]> = [ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have the different wallets been tested?
The main ones that should work are: minipay, goodwallet (using wallet-connect), valora.
metamask.
| ) | ||
| }, [address, open]) | ||
|
|
||
| const handleError = useCallback(async (e) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain this change?
|
Also, wallet-connect does not load when I run it from localhost. it works for you? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We failed to fetch the diff for pull request #604
You can try again by commenting this pull request with @sourcery-ai review, or contact us for help.
… refactor, useConnectionInfo, restore XDC, clean TODOs, fix keys and ESLint warnings
…eError, add wallet QA checklist
…Chat, AccountDetails, Web3ReactManager, Web3Network)
works well actually: https://www.loom.com/share/992e55bd5efc410aa76e3702737aad4b?sid=a14f2393-1071-40eb-90a7-1f9268ff41be |
Resolved conflicts and API compatibility: - AppBar.tsx: Integrated with new AppKit hooks - NetworkModal/index.tsx: Combined AppKit with activeNetworksByFeature - ClaimBalance.tsx: Updated to use new SDK API - OldClaim.tsx: Kept AppKit connection handling - WalletBalance/index.tsx: Fixed chainId type conversion - All locale files: Accepted master's updated translations - yarn.lock: Regenerated with updated dependencies Note: New SDK version - useG no longer accepts chainId parameter
|
|
||
| // walletInfo provided by useConnectionInfo | ||
|
|
||
| const miniPay = React.useMemo(() => isMiniPay(), []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't see a need for useMemo, also, following style across the dapp, we import useMemo, not using from React namespace
|
|
||
| const { G$ } = useG$Balance(5, chainId) | ||
| const reserveEnabled = isFeatureActive('reserveEnabled', Number(chainId)) | ||
| const { G$ } = useG$Balance(5, Number(chainId)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your merge with master went wrong, this should not have any changes. master is the code that is expected
| }/${currencyB === ETHER ? WETH[chainId].address : currencyId(currencyB)}`} | ||
| > | ||
| Receive W{Currency.getNativeCurrencySymbol(chainId)} | ||
| Receive W{Currency.getNativeCurrencySymbol(getSafeChainId(chainId))} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could use assigning 'safeChainId' as const, and re-use that. no sense in repeating the same method throughout this component
| const [toAddNetwork, setToAddNetwork] = useState<SupportedChains | undefined>() | ||
|
|
||
| const networkLabel: string | null = error ? null : (NETWORK_LABEL as any)[chainId] | ||
| const networkLabel: string | null = error ? null : (NETWORK_LABEL as any)[+(chainId ?? 1)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default fallback should be 42220
| borderBottomRightRadius={12} | ||
| > | ||
| <WalletBalance balances={balances} chainId={chainId} /> | ||
| <WalletBalance balances={balances} chainId={+(chainId ?? 1)} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fallback default is 42220
| dispatch( | ||
| addTransaction({ | ||
| chainId: chainId!, | ||
| chainId: +(chainId ?? 1)!, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fallback 42220, everywhere
| } | ||
|
|
||
| // Supported chain IDs for validation | ||
| const SUPPORTED_CHAINS = [1, 122, 42220] // MAINNET, FUSE, CELO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be based on SupportedChains (import from web3sdk-v2)
| // params[0].gasPrice = gasPriceSettings[chainId].maxFeePerGas | ||
| delete params[0].gasPrice | ||
| params[0] = { ...params[0], ...gasPriceSettings[chainId] } | ||
| params[0] = { ...params[0], ...gasSettings } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure how third-party provider handle nested/chain gasPriceSettings
for safety, please revert back to only passing down the current connected chain gas settings ...gasSettings[Number(chainId)]
| XDC: | ||
| process.env.REACT_APP_GOODID_XDC_EXPLORER ?? | ||
| `https://api.etherscan.io/v2/api?chainid=50&apikey=${process.env.REACT_APP_ETHERSCAN_KEY}&`, | ||
| XDC: process.env.REACT_APP_GOODID_XDC_EXPLORER ?? 'https://xdc.blocksscan.io/api?&', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was the explorer endpoint changed exactly?
| } | ||
| return false | ||
| }, [connect, activeChainFeatures]) | ||
| // const handleConnect = useCallback(async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this commented out and not fixed?
The user-flow is when clicking on the claim-button when not connected, it should trigger the connect flow. What happens now is that when I click, all the button say is 'connecting..' but I am not actually connecting
|
@vortex-hue What also needs to be fixed is the 'disconnect' flow when using wallet-connect. |
Description
Summary: Complete migration from Web3 Onboard to Reown AppKit with prioritized wallet support, social login, and MiniPay compatibility.
Issue Fixed: #593 - Integrate with reown appkit
Motivation: The existing Web3 Onboard integration was becoming outdated and lacked the flexibility needed for prioritized wallet display and social authentication. Reown AppKit provides better wallet management, analytics, and modern React patterns that align with current best practices.
Context: This migration enables:
How Has This Been Tested?
Local Development Testing:
yarn install- All dependencies installed successfullyyarn build- Project compiles without errorsCode Quality:
@web3-onboarddependenciesChecklist:
Description by Korbit AI
What change is being made?
Integrate Reown AppKit across the app, replace Web3 Onboard with AppKit-based wallet and network handling, and introduce wallet priorities and Google socials; update multiple components to consume AppKit hooks (account, network, label, etc.), and adjust explorer links and token network handling accordingly.
Why are these changes being made?
Converge to AppKit for consistent wallet/connect flows and network state, replacing the old Onboard integration; this simplifies state management, improves compatibility with new wallet providers, and enables wallet prioritization and Google social login support.