prep for multichain release #477
Conversation
feat: solana send
…tworks without adapters, added namespace to useAccount hook
…ching transactions
fix: unsupported network popup + wagmi duplicated event subscription
chore: added react-native-compat import
chore: removed dev check from LogController
chore: wallet impression event
fix: added trycatch for safe provider access
feat: multichain support
🦋 Changeset detectedLatest commit: 08113ea The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 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 |
|
Claude finished @ignaciosantise's task —— View job Code Review CompleteReviewed PR #477: prep for multichain release (444 commits, 100 files changed) Critical Issues1. Race Condition in syncAccounts (AppKit.ts:479-490)adapters.forEach(async adapter => {
const namespace = adapter.getSupportedNamespace();
// ...async operations
});Issue: Using Fix: Use await Promise.all(adapters.map(async adapter => {
const namespace = adapter.getSupportedNamespace();
// ...
}));Files: packages/appkit/src/AppKit.ts:479 2. Async Operation Without Await in setConnection (AppKit.ts:598)adapters.forEach(async adapter => {
// async operations that are never awaited
});Issue: Same forEach + async issue - promises are created but never tracked or awaited. Fix: Change to synchronous forEach or properly await: adapters.forEach(adapter => {
const namespace = adapter.getSupportedNamespace();
// ...sync operations only
});Files: packages/appkit/src/AppKit.ts:598 3. Unsafe Type Assertion in BitcoinUtil (BitcoinUtil.ts:64-65)psbt.addOutput({
address: params.senderAddress,
value: BigInt(change) // 'change' is number, should validate
});Issue: Converting Fix: Add validation: if (!Number.isInteger(change)) {
throw new Error('Change amount must be an integer');
}
psbt.addOutput({
address: params.senderAddress,
value: BigInt(change)
});Files: apps/native/src/utils/BitcoinUtil.ts:64 4. Hardcoded API Endpoint Without Error Handling (SolanaActionsView.tsx:68,123)const connection = new Connection('https://api.mainnet-beta.solana.com', 'confirmed');Issue: Hardcoded mainnet endpoint used regardless of Fix: Derive endpoint from chainId: const endpoint = chainId === '5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp'
? 'https://api.mainnet-beta.solana.com'
: 'https://api.devnet.solana.com';
const connection = new Connection(endpoint, 'confirmed');Files: apps/native/src/views/SolanaActionsView.tsx:68, 123, 170 5. Security: Package Version Manipulation Without Rollback on Error (publish-initial-versions.js:130-133)parsedPackageJson.version = '0.0.1';
fs.writeFileSync(packageJsonPath, JSON.stringify(parsedPackageJson, null, 2));
runCommand('yarn', ['npm', 'publish', ...], { cwd: pkg.dir });Issue: If publish fails, the finally block restores package.json, but if the process is killed, package.json remains at 0.0.1. Fix: Use git-based restoration or atomic operations. Consider using High Priority Issues6. Memory Leak: Missing Cleanup for Event Subscriptions (AppKit.ts:632-686)private subscribeToAdapterEvents(adapter: BlockchainAdapter): void {
adapter.on('accountsChanged', ...);
adapter.on('chainChanged', ...);
// No cleanup/unsubscribe mechanism
}Issue: Event listeners are added but never removed. If adapters are recreated or connections change, old listeners persist. Fix: Store references and add cleanup: private unsubscribeFromAdapterEvents(adapter: BlockchainAdapter): void {
adapter.removeAllListeners();
}Files: packages/appkit/src/AppKit.ts:632-686 7. Unused Variable Warning (SolanaActionsView.tsx:16)const isConnected = true; // Hardcoded, never changesIssue: Variable Fix: Either remove or connect to actual state: const { isConnected } = useAccount();Files: apps/native/src/views/SolanaActionsView.tsx:16 8. Console.log Left in Production Code (BitcoinActionsView.tsx:101)// eslint-disable-next-line no-console
console.log('error', error);Issue: Debug console.log should use LogController for consistency. Fix: LogController.sendError(error, 'BitcoinActionsView', 'signPsbt');Files: apps/native/src/views/BitcoinActionsView.tsx:101 9. Error Handling Inconsistency - Silent Failures (AppKit.ts:556)} catch (error) {
// ignore
LogController.sendError(error, 'AppKit.ts', 'refreshBalance');
}Issue: Comment says "ignore" but error is logged. Inconsistent error handling pattern. Fix: Either handle the error meaningfully or document why it's safe to ignore. 10. Missing Dependency in useMemo (useProvider.ts:42)const returnValue = useMemo(() => {
if (!connection || !connection.adapter) {
return { provider: undefined, providerType: undefined };
}
// ...
}, [connection]);Issue: Fix: Access specific properties: const { connection } = useSnapshot(ConnectionsController.state);
const adapter = connection?.adapter;
const returnValue = useMemo(() => {
if (!adapter) return { provider: undefined, providerType: undefined };
// ...
}, [adapter]);Files: packages/appkit/src/hooks/useProvider.ts:42 Medium Priority Issues11. Commented Out Code Should Be Removed (w3m-router/index.tsx:34-51)// import { UiUtil } from '../../utils/UiUtil';
// import { useRouteTransition } from '../../hooks/useRouteTransition';
// const { animateTransition, getAnimatedStyle } = useRouteTransition();
// useEffect(() => {
// UiUtil.setRouteTransition(animateTransition);
// }, [animateTransition]);Issue: Large blocks of commented code reduce readability. If this is for future work, create a TODO or remove. Fix: Remove commented code or add clear TODO with issue reference. 12. Inefficient Array Operations (AllWalletsList.tsx:29-49)let combinedWallets = [...installed, ...featured, ...recommended, ...wallets];
const certifiedIndex = combinedWallets.findLastIndex(...);
if (certifiedIndex > -1) {
const nonCertifiedWallets = combinedWallets.splice(certifiedIndex + 1);
combinedWallets = combinedWallets.concat(customWallets ?? [], nonCertifiedWallets);
}
// Then deduplicate
const uniqueWallets = Array.from(
new Map(combinedWallets.map(wallet => [wallet?.id, wallet])).values()
);Issue: Creates temporary arrays multiple times, then deduplicates. Could deduplicate first. Fix: Use Set for deduplication during concatenation for O(n) instead of O(n²). 13. TypeScript: Missing Null Check (BitcoinUtil.ts:49)const payment = this.getPaymentByAddress(params.senderAddress, network);
if (!payment.output) {
throw new Error('Invalid payment output');
}Issue: Checks Fix: Add null check: const payment = this.getPaymentByAddress(params.senderAddress, network);
if (!payment || !payment.output) {
throw new Error('Invalid payment output');
}Files: apps/native/src/utils/BitcoinUtil.ts:49-55 14. Incomplete Error Context (BitcoinUtil.ts:129)} catch (e) {
// eslint-disable-next-line no-console
console.error('Error fetching fee rate', e);
}
return defaultFeeRate;Issue: Error is logged to console instead of LogController. No tracking or reporting. Fix: Use LogController for consistency with project patterns. 15. Missing Input Validation (WalletConnectConnector.ts:209)override switchNetwork(network: AppKitNetwork): Promise<void> {
if (!network) throw new Error('No network provided');
let caipNetworkId = network.caipNetworkId ?? `eip155:${network.id}`;
// ...
}Issue: Assumes Fix: if (!network || (!network.caipNetworkId && !network.id)) {
throw new Error('Invalid network: missing caipNetworkId or id');
}Files: packages/appkit/src/connectors/WalletConnectConnector.ts:209 Low Priority / Code Quality16. Magic Numbers Without Constants (BitcoinUtil.ts:138-143)const estimatedSize = 10 + 148 * utxos.length + 34 * 2;Issue: Magic numbers make code harder to understand and maintain. Fix: const FIXED_OVERHEAD = 10;
const INPUT_SIZE = 148;
const OUTPUT_SIZE = 34;
const estimatedSize = FIXED_OVERHEAD + INPUT_SIZE * utxos.length + OUTPUT_SIZE * 2;Files: apps/native/src/utils/BitcoinUtil.ts:138-143 17. Type Safety: any Type Usage (SolanaActionsView.tsx:106)} catch (error: any) {Issue: Using Fix: Use } catch (error: unknown) {
onSignError(error as Error, 'Sign Transaction failed');
}Files: apps/native/src/views/SolanaActionsView.tsx:106 18. Dead Code: Commented Tokens Config (App.tsx:76-83)// tokens: {
// 'eip155:1': {
// address: '0xdAC17F958D2ee523a2206206994597C13D831ec7'
// },
// ...
// }Issue: Commented configuration suggests incomplete feature or should be removed. Fix: Remove or document why it's commented. 19. Inconsistent Error Messages (BitcoinActionsView.tsx:23,29,61)ToastUtils.showErrorToast('Sign failed', 'No provider found');
ToastUtils.showErrorToast('Sign failed', 'No address found');
ToastUtils.showErrorToast('Sign failed', 'The selected chain is not bip122');Issue: Generic "Sign failed" title for different functions. Should be more specific. Fix: Use specific titles: "Sign Message Failed", "Sign PSBT Failed". Positive Observations✅ Well-structured multichain architecture with clean adapter pattern Recommendations
SummaryThis is a substantial PR adding multichain support (Bitcoin, Solana, EVM). The architecture is solid, but there are several issues that should be addressed before merging:
The code quality is generally good with strong typing and proper patterns, but needs cleanup of commented code and better error handling consistency. |
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| } | ||
|
|
||
| return selector + encodedParams; | ||
| } |
There was a problem hiding this comment.
Bug: Address Prefix Validation Error
In encodeERC20Function, addresses are processed with slice(2) to remove the '0x' prefix. This happens without validating if the address actually starts with '0x'. If an address lacks the prefix, slice(2) incorrectly removes the first two characters, resulting in malformed transaction data.
|
|
||
| if (!namespace || !chain || !address) { | ||
| throw new Error('Invalid address'); | ||
| } |
There was a problem hiding this comment.
Bug: Redundant Validation in fetchTransactions
The accountAddress?.split(':') ?? [] expression is overly defensive. The fetchTransactions function already validates accountAddress earlier, ensuring it's a defined string before this line. This makes the optional chaining and nullish coalescing redundant.
| this.checkOnRampBack(); | ||
| this.checkSocialLoginBack(); | ||
| this.checkOnRampBackLoading(); | ||
| }, |
There was a problem hiding this comment.
Bug: RouterUtil this Context Issue
The RouterUtil object's checkBack method calls other internal methods using this. As RouterUtil is an object literal, this will be undefined or incorrect, preventing these calls from executing.
| open: context.appKit.open.bind(context.appKit), | ||
| close: context.appKit.close.bind(context.appKit), | ||
| disconnect: (namespace?: ChainNamespace) => | ||
| context.appKit!.disconnect.bind(context.appKit!)(namespace), |
There was a problem hiding this comment.
Bug: Redundant Non-Null Assertions in disconnect Function
The disconnect function in useAppKit uses non-null assertions on context.appKit. These are redundant because a prior check within the useMemo callback already ensures appKit is available.
| sendToken() { | ||
| if (this.state.token?.address && this.state.sendTokenAmount && this.state.receiverAddress) { | ||
| state.loading = true; | ||
| async sendToken() { |
There was a problem hiding this comment.
Bug: Async Method Call Without Await
The sendToken method was changed to async. Callers that don't await this method may continue execution prematurely, which could lead to unexpected behavior.
| ? `${activeCaipNetworkId}:${ConstantsUtil.NATIVE_TOKEN_ADDRESS[activeNamespace]}` | ||
| : undefined; | ||
|
|
||
| const isNetworkToken = SwapController.state.sourceToken?.address === networkTokenAddress; |
There was a problem hiding this comment.
Bug: Token Address Comparison Fails
Comparing networkTokenAddress (CAIP-10 format) with sourceToken.address (plain hex) results in isNetworkToken always being false. This prevents the 'max' button from correctly deducting gas fees for native tokens.
Summary
Prep for multichain release
Note
Adds multichain capabilities (Bitcoin, Solana) with new adapters and connectors, updates AppKit/UI and wallet frameworks, and expands onramp/auth/connecting views across web, native, and scaffold.
packages/bitcoinandpackages/solanaadapters, providers, utils, and types.packages/common/src/types/**) and adapters (EvmAdapter,BitcoinBaseAdapter,SolanaBaseAdapter).AccountController,NetworkController,OnRampController,RouterController, etc.) and utilities (ApiUtil,NetworkUtil,RouterUtil,StorageUtil).useAccount,useProvider,useAppKit*).packages/wagmi,packages/ethers,packages/ethers5,packages/coinbase*with connectors/clients/configs.packages/wallet.wui-modal, buttons, lists, QR, OTP, tabs, loading) and assets.Written by Cursor Bugbot for commit 08113ea. This will update automatically on new commits. Configure here.