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: improve custom chains provisioning and eslint conflicts #1

Merged
merged 5 commits into from
Nov 19, 2024

Conversation

code-z2
Copy link

@code-z2 code-z2 commented Nov 19, 2024

  • update custom chains configuration
  • fix conflicting eslint-rules in next app

Summary by CodeRabbit

  • New Features

    • No new features introduced in this release.
  • Bug Fixes

    • No bug fixes included in this release.
  • Documentation

    • No documentation updates made.
  • Refactor

    • Adjusted ESLint configurations to enhance code quality and clarity.
    • Streamlined import statements for type safety across various components, including correcting type references from CosmsosChainType to CosmosChainType.
  • Style

    • Minor textual update in the NearPage component for improved readability.
  • Chores

    • Removed unused type imports to clean up the codebase.
    • Updated dependency management for @noble-assets/tangled-solana-react.

Copy link

coderabbitai bot commented Nov 19, 2024

Walkthrough

The changes in this pull request involve modifications to ESLint configurations, type imports, and minor textual updates across various files. The .eslintrc files have been adjusted to change the order of extended configurations and add new linting rules. Several components have updated import statements to clarify type usage. Additionally, some utility functions have been streamlined, particularly in handling chain configurations and supported chains, with a focus on simplifying logic and enhancing type safety. Type references have been corrected throughout the codebase to ensure consistency.

Changes

File Change Summary
.eslintrc Changed order of items in the "extends" array from ["react-app", "plugin:prettier/recommended", "./.eslintrc.base"] to ["react-app", "./.eslintrc.base", "plugin:prettier/recommended"].
example-next/.eslintrc.cjs Added rules: 'jsx-quotes': 'off' and 'import/no-default-export': 'off'.
example-next/src/app/wallets/mynearwallet/page.tsx Minor textual change in <h1> from "Oh, I see you're trying to connect to My Near Wallet." to "Oh, I see you are trying to connect to My Near Wallet."
example-next/src/components/ConnectedAccounts.tsx Changed import of ConnectedAccount to a type import: import { type ConnectedAccount }.
example-next/src/components/Example.tsx Changed import of ChainId to a type import: import { type ChainId }.
example-next/src/components/Providers.tsx Updated imports for CosmsosChainType and ReactNode to type imports: import { type CosmosChainType } and import { type ReactNode }.
example-next/src/components/Tokens.tsx Updated import of TokenMetadata to a type import: import { type TokenMetadata }.
packages/react/src/utils/createChainConfigs.ts Removed imports of CHAIN_DATA and CHAIN_NAME, changed overrides parameter type, and simplified logic in overrideChainConfig.
packages/react/src/utils/getDefaultSupportedChains.ts Removed type assertions for arrays in supportedChains and updated imports to remove unused types.
packages/react/src/actions/cosmos/getCosmosToken.ts Corrected type from CosmsosChainType to CosmosChainType in the getCosmosTokenMetadata function.
packages/react/src/chains/injective.ts Corrected type from CosmsosChainType to CosmosChainType for the injective constant.
packages/react/src/chains/noble.ts Corrected type from CosmsosChainType to CosmosChainType for the noble constant.
packages/react/src/chains/osmosis.ts Corrected type from CosmsosChainType to CosmosChainType for the osmosis constant.
packages/react/src/providers/CosmosProvider.tsx Updated type for chains from CosmsosChainType to CosmosChainType and enhanced error handling in the connection logic.
packages/react/src/store/Cosmos.ts Corrected import and type casting from CosmsosChainType to CosmosChainType.
packages/react/src/types/index.ts Renamed CosmsosChainType to CosmosChainType and updated references in ChainData.
packages/react/package.json Removed and re-added dependency on @noble-assets/tangled-solana-react.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant App
    participant Wallet
    User->>App: Request to connect wallet
    App->>Wallet: Initiate connection
    Wallet-->>App: Connection status
    App-->>User: Display connection status
Loading

🐇 "In a world where types are clear,
We hop along without a fear.
With linting rules and imports bright,
Our code now shines with pure delight!
So let’s connect and dance with glee,
For changes made, oh joy, oh me!" 🐇✨

Warning

There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure.

🔧 eslint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

example-next/src/components/Providers.tsx

Oops! Something went wrong! :(

ESLint: 8.57.1

ESLint couldn't find the config "next/core-web-vitals" to extend from. Please check that the name of the config is correct.

The config "next/core-web-vitals" was referenced from the config file in "/example-next/.eslintrc.cjs".

If you still have problems, please stop by https://eslint.org/chat/help to chat with the team.


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between a03aa39 and bbdd93a.

📒 Files selected for processing (1)
  • example-next/src/components/Providers.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • example-next/src/components/Providers.tsx

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@code-z2 code-z2 changed the title Fix/custom chains and eslint fix: improve custom chains provisioning and eslint conflicts Nov 19, 2024
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (9)
example-next/.eslintrc.cjs (1)

29-29: Consider using a specific quote style instead of disabling jsx-quotes.

While disabling the rule resolves conflicts, it might lead to inconsistent JSX quote styles across the codebase. Consider enforcing either single or double quotes instead.

-    'jsx-quotes': 'off',
+    'jsx-quotes': ['error', 'prefer-double']
packages/react/src/utils/getDefaultSupportedChains.ts (1)

37-42: LGTM! Improved type inference usage

The removal of explicit type assertions (like as EVMChain[]) is a good practice here because:

  1. TypeScript can correctly infer the types from the initial SupportedChainsByType declaration
  2. The code is more maintainable and less verbose
  3. The type safety is maintained through structural typing

Consider adding a JSDoc comment to document the purpose of this utility function and its return type for better developer experience:

+/**
+ * Returns the default supported chains for each blockchain ecosystem
+ * @returns {SupportedChainsByType} Object containing arrays of supported chains by type
+ */
const getDefaultSupportedChains = (): SupportedChainsByType => {
example-next/src/components/Providers.tsx (1)

Line range hint 7-28: Consider enhancing chain configuration robustness

A few suggestions to improve the chain configuration:

  1. Consider adding multiple RPC endpoints for failover
  2. The chain ID type assertion could be more strictly typed
  3. Consider adding runtime validation for the chain configuration

Here's a suggested improvement:

 const dydx: CosmsosChainType = {
-  id: 'dydx-mainnet-1' as `${string}-${number}`,
+  id: 'dydx-mainnet-1' as const,
   chainName: 'dydx',
   name: 'dYdX',
   type: 'cosmos',
   nativeCurrency: {
     name: 'dydx',
     symbol: 'dydx',
     decimals: 18,
   },
   rpcUrls: {
     default: {
-      http: ['https://dydx-dao-rpc.polkachu.com'],
+      http: [
+        'https://dydx-dao-rpc.polkachu.com',
+        'https://dydx-mainnet-full-rpc.public.blastapi.io',
+        'https://dydx-rpc.publicnode.com',
+      ],
     },
   },
   blockExplorers: {
     default: {
       name: 'DyDx Explorer',
       url: 'https://www.mintscan.io/dydx',
     },
   },
 } as const;
example-next/src/components/ConnectedAccounts.tsx (2)

Line range hint 41-54: Consider extracting repeated patterns into reusable utilities.

Several patterns in the code could be extracted for better maintainability:

  1. Address truncation logic
  2. Chain ID formatting

Consider applying these improvements:

+const ADDR_PREFIX_LENGTH = 6;
+const ADDR_SUFFIX_LENGTH = 4;
+
+const formatAddress = (address: string) => 
+  `${address.slice(0, ADDR_PREFIX_LENGTH)}...${address.slice(-ADDR_SUFFIX_LENGTH)}`;
+
+const formatChainInfo = (chain: { name?: string } | null, chainId: string) =>
+  `${chain?.name ?? 'Unknown'} [${chainId}]`;
+
 const ConnectedAccountItem = ({ account }: { account: ConnectedAccount }) => {
   const { disconnect } = useDisconnect();
   const { connect } = useConnect();
   const wallet = useWallet(account.chainType, account.wallet);
   const chain = useChain(account.chainId);

   return (
     <tr className='border-b border-gray-700'>
       {/* ... */}
       <td className='px-4 py-2'>
-        {account.address.slice(0, 6)}...{account.address.slice(-4)}
+        {formatAddress(account.address)}
       </td>
       <td className='px-4 py-2'>
-        {chain?.name ?? 'Unknown'} [{account.chainId}]
+        {formatChainInfo(chain, account.chainId)}
       </td>

Add error handling and user protection for wallet operations

Based on the code analysis:

  1. The useDisconnect hook in packages/solana-react/src/providers/WalletProvider.tsx shows error handling for Solana disconnections, but the component doesn't utilize this.
  2. The connect/disconnect operations in ConnectedAccounts.tsx are direct calls without try-catch blocks or user confirmations.
  3. No loading states are implemented during these async operations.

Specific locations requiring changes:

  • example-next/src/components/ConnectedAccounts.tsx:
    • Add try-catch blocks around connect/disconnect operations
    • Implement a confirmation dialog for disconnect action
    • Add loading states during wallet operations
🔗 Analysis chain

Line range hint 71-85: Verify error handling and user protection for wallet operations.

The disconnect and connect buttons could benefit from additional safety measures:

  1. Error handling for failed operations
  2. Confirmation dialogs for disconnect actions
  3. Loading states during operations

Please verify the implementation of error handling in the tangled-react hooks:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check error handling implementation in useConnect and useDisconnect hooks
ast-grep --pattern 'export function use(Connect|Disconnect)() {
  $$$
  try {
    $$$
  } catch {
    $$$
  }
  $$$
}'

Length of output: 120


Script:

#!/bin/bash
# Let's try a broader search for connect/disconnect hooks
rg -A 10 "export (const|function) use(Connect|Disconnect)" --type typescript

# Also search for any error handling patterns in the component
rg -A 5 "catch|error|Error" "example-next/src/components/ConnectedAccounts.tsx"

# Look for the actual implementation of connect/disconnect functions
ast-grep --pattern 'const $_ = () => {
  connect($$$)
}'

# Search for any existing confirmation dialogs
rg -i "confirm|dialog|modal" "example-next/src/components/ConnectedAccounts.tsx"

Length of output: 333


Script:

#!/bin/bash
# Try searching without file type specification
rg -A 10 "export (const|function) use(Connect|Disconnect)"

# Look for any error handling in the component file
cat "example-next/src/components/ConnectedAccounts.tsx"

# Search for connect/disconnect implementations in all files
rg -A 5 "connect\(.*chainType.*wallet"
rg -A 5 "disconnect\("

Length of output: 12349

example-next/src/components/Tokens.tsx (1)

Line range hint 37-58: Consider enhancing error handling and type safety

The component implementation could benefit from some improvements:

  1. Add more specific error handling for network issues
  2. Add type guard for chain?.type access
  3. Consider using a proper loading spinner component

Consider this enhancement:

 export const Token = (token: TokenMetadata) => {
   const {
     data: fetchedToken,
     error,
     isLoading,
   } = useToken({
     chainId: token.chainId,
     token: token.address,
   });
   const chain = useChain(token.chainId);
   const dataOK = fetchedToken?.symbol === token.symbol && fetchedToken.decimals === token.decimals;
+  const isNetworkError = error instanceof Error && error.message.includes('network');
+  const chainType = chain?.type ?? 'unknown';
   return (
     <tr className='border-b border-gray-700'>
       <td className='px-4 py-2 max-w-[10ch] overflow-hidden text-ellipsis'>{token.address}</td>
       <td className='px-4 py-2'>{chain?.name}</td>
-      <td className='px-4 py-2'>{chain?.type}</td>
+      <td className='px-4 py-2'>{chainType}</td>
       <td className='px-4 py-2'>{fetchedToken?.name}</td>
       <td className='px-4 py-2'>{fetchedToken?.symbol}</td>
       <td className='px-4 py-2'>{fetchedToken?.decimals}</td>
-      <td className='px-4 py-2'>{isLoading ? 'Loading...' : error ? error.message : 'Fetched'}</td>
+      <td className='px-4 py-2'>
+        {isLoading ? <LoadingSpinner /> : 
+         isNetworkError ? 'Network Error' :
+         error ? error.message : 'Fetched'}
+      </td>
       <td className='px-4 py-2'>{isLoading ? '' : dataOK && !error ? '✅' : '❌'}</td>
     </tr>
   );
 };
example-next/src/components/Example.tsx (3)

Line range hint 43-55: Consider adding a loading state and debounce for chain switching.

While the optimistic updates and error handling are well implemented, rapid chain switching could lead to race conditions. Consider these improvements:

  1. Add a debounce to prevent rapid switching
  2. Show a loading indicator during the switch
  3. Disable the select during the pending state (already implemented, good!)

Here's a suggested implementation:

+ import { useCallback } from 'react';
+ import debounce from 'lodash/debounce';

  const CurrentAccountAndWallet = () => {
+   const handleChainChange = useCallback(
+     debounce((chainId: ChainId) => {
        setSelectedChain(chainId);
        switchNetworkAsync(chainId)
          .then(() => {
            // ui remains unchanged
          })
          .catch(() => {
            setSelectedChain(currentAccount?.chainId);
          });
+     }, 500),
+     [switchNetworkAsync, currentAccount?.chainId]
+   );

Line range hint 89-93: Enhance type safety for chain selection.

The current type assertion e.target.value as ChainId might be unsafe. Consider adding runtime validation to ensure type safety.

Here's a suggested improvement:

+ const isValidChainId = (value: string): value is ChainId => {
+   return chains.some(chain => chain.id === value);
+ };

- onChange={(e) => handleChainChange(e.target.value as ChainId)}
+ onChange={(e) => {
+   if (isValidChainId(e.target.value)) {
+     handleChainChange(e.target.value);
+   } else {
+     console.error('Invalid chain ID:', e.target.value);
+   }
+ }}

Line range hint 89-102: Enhance accessibility for chain selection.

The select element could benefit from improved accessibility features to better support screen readers and keyboard navigation.

Consider these improvements:

  <select
    id='chain'
+   aria-label="Select blockchain network"
+   aria-busy={isPending}
+   aria-invalid={false}
    className='h-12 border border-gray-300 text-gray-600 text-base rounded-lg block w-full focus:outline-none'
    onChange={(e) => handleChainChange(e.target.value as ChainId)}
    disabled={isPending}
    value={selectedChain}
  >
+   <option value="" disabled>Select a network</option>
    {chains.map((chain) => (
      <option
        key={chain.id}
        value={chain.id}
+       aria-selected={chain.id === selectedChain}
      >
        {chain.name}
      </option>
    ))}
  </select>
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between c782f6a and e60180b.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (9)
  • .eslintrc (1 hunks)
  • example-next/.eslintrc.cjs (1 hunks)
  • example-next/src/app/wallets/mynearwallet/page.tsx (1 hunks)
  • example-next/src/components/ConnectedAccounts.tsx (1 hunks)
  • example-next/src/components/Example.tsx (1 hunks)
  • example-next/src/components/Providers.tsx (1 hunks)
  • example-next/src/components/Tokens.tsx (1 hunks)
  • packages/react/src/utils/createChainConfigs.ts (2 hunks)
  • packages/react/src/utils/getDefaultSupportedChains.ts (2 hunks)
✅ Files skipped from review due to trivial changes (2)
  • .eslintrc
  • example-next/src/app/wallets/mynearwallet/page.tsx
🔇 Additional comments (9)
packages/react/src/utils/createChainConfigs.ts (1)

17-24: Verify chain type compatibility across the codebase

The changes to chain type handling might affect other parts of the codebase that expect specific chain types.

✅ Verification successful

Let me try a different approach to gather the necessary information.


Based on the comprehensive search results, I can now provide a final response:

Chain type handling is properly integrated across the codebase

The changes to chain type handling in createChainConfigs are consistent with the type system used throughout the codebase. The code shows:

  • Proper type definitions in types/index.ts with SupportedChainsByType interface
  • Consistent usage across hooks, actions, and providers
  • Type-safe implementations for different chain types (EVM, Sui, Cosmos, etc.)
  • Correct chain type validation in dependent components
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for usages of createChainConfigs and chain type references
echo "Searching for createChainConfigs usage patterns..."
rg -t typescript -t javascript "createChainConfigs\(" -A 3

echo "Searching for Chain type references..."
ast-grep --pattern 'type $name = $$$Chain$$$'

Length of output: 341


Script:

#!/bin/bash
# Search for chain type references and usages
echo "Searching for createChainConfigs usage..."
rg "createChainConfigs"

echo -e "\nSearching for chain type definitions and usages..."
rg "type.*Chain"

echo -e "\nSearching for files containing chain configurations..."
fd -e ts -e js | xargs rg "chain.*type" -l

echo -e "\nLooking for chain type imports and exports..."
rg "(import|export).*Chain.*from"

Length of output: 14454

example-next/.eslintrc.cjs (1)

30-30: LGTM! Disabling no-default-export aligns with Next.js patterns.

This change is appropriate as Next.js relies heavily on default exports for pages and components.

Let's verify the usage of default exports in the Next.js app:

✅ Verification successful

Disabling no-default-export rule is appropriate for this Next.js project

The verification confirms this is the right decision as default exports are extensively used throughout the codebase:

  • Next.js specific files use default exports (app/page.tsx, app/layout.tsx)
  • React components follow the pattern (WalletList.tsx, Tokens.tsx, Example.tsx)
  • Configuration files also use default exports (next.config.js, tailwind.config.ts)

The codebase shows consistent usage of default exports which aligns with Next.js conventions and patterns, validating the decision to disable the 'import/no-default-export' rule.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for default exports in pages/components to confirm this rule is necessary

# Look for default exports in pages and components
echo "Checking for default exports in pages and components..."
rg "export default" -t ts -t tsx

Length of output: 183


Script:

#!/bin/bash
# Let's try a different approach to find default exports
# Using ripgrep without file type filters first
echo "Checking for default exports in all files..."
rg "export default"

# Also let's check what file extensions exist in the repository
echo -e "\nChecking file extensions in the repository..."
fd . -t f -x basename {} | grep -o '\.[^.]*$' | sort | uniq -c

Length of output: 2188

packages/react/src/utils/getDefaultSupportedChains.ts (1)

7-7: LGTM! Clean up of type imports

The removal of unused type imports (CosmsosChainType, EVMChain, OtherChainData, SuiChainType) while keeping only the necessary SupportedChainsByType improves code clarity and reduces bundle size.

example-next/src/components/Providers.tsx (2)

2-2: LGTM! Type-only imports are properly annotated

The addition of the type keyword for type imports follows TypeScript best practices and helps with tree-shaking optimization.

Also applies to: 5-5


Line range hint 32-57: Security and reliability improvements needed

Several concerns in the provider configuration:

  1. The project ID should not be hardcoded in the source code
  2. Single Solana RPC endpoint creates a single point of failure
  3. Consider adding error boundaries around providers

Here's a suggested improvement:

 <QueryClientProvider client={queryClient}>
+  <ErrorBoundary fallback={<div>Something went wrong</div>}>
     <TangledContextProvider
       config={{
         projectName: 'Tangled Example',
         chains: {
           cosmos: [dydx],
         },
         chainConfigs: {
           solana: {
             ...solana,
             rpcUrls: {
               default: {
-                http: ['https://api.mainnet-beta.solana.com'],
+                http: [
+                  process.env.NEXT_PUBLIC_SOLANA_RPC_URL,
+                  'https://api.mainnet-beta.solana.com',
+                  'https://solana-mainnet.rpc.extrnode.com',
+                ],
               },
             },
           },
         },
-        projectId: '41980758771052df3f01be0a46f172a5',
+        projectId: process.env.NEXT_PUBLIC_TANGLED_PROJECT_ID,
       }}
     >
       {children}
     </TangledContextProvider>
+  </ErrorBoundary>
 </QueryClientProvider>

Let's verify if the project ID is used elsewhere in the codebase:

example-next/src/components/ConnectedAccounts.tsx (1)

Line range hint 1-7: LGTM! Good improvement in type imports.

The explicit type import for ConnectedAccount is a good practice as it:

  • Clearly indicates the import is used only for type checking
  • Helps with tree-shaking during compilation
  • Follows modern TypeScript conventions
example-next/src/components/Tokens.tsx (2)

1-1: LGTM: Type import clarification

The explicit type annotation for TokenMetadata follows TypeScript best practices and improves code clarity.


Line range hint 89-143: Verify token configurations for potential issues

There are several concerns in the token configurations:

  1. Empty address for MATIC token on Polygon
  2. Inconsistent chain ID formats (some numeric, some string)
  3. Suspicious reuse of ETH_ADDRESS for BTC and NEAR tokens

Let's verify these configurations:

example-next/src/components/Example.tsx (1)

2-2: LGTM! Good TypeScript practice.

The explicit type import for ChainId follows TypeScript best practices by clearly distinguishing between value imports and type imports. This change improves code clarity without affecting runtime behavior.

packages/react/src/utils/createChainConfigs.ts Outdated Show resolved Hide resolved
Copy link

@xanderjl xanderjl left a comment

Choose a reason for hiding this comment

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

Lots of really great cleanup in here. Take type-fest for a spin. If it's not working the way we'd expect, feel free to tag me for re-review and I'll approve it 🤝

packages/react/src/utils/createChainConfigs.ts Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (5)
packages/react/src/chains/noble.ts (1)

Line range hint 3-24: Consider enhancing chain configuration robustness.

While the current configuration is functional, consider these improvements for better reliability and integration:

  1. Add multiple RPC endpoints for redundancy
  2. Include REST API endpoints
  3. Consider adding testnet configuration for development purposes

Example enhancement:

 export const noble: CosmosChainType = {
   id: 'noble-1',
   chainName: 'noble',
   name: 'Noble',
   type: 'cosmos',
   nativeCurrency: {
     name: 'usdc',
     symbol: 'uusdc',
     decimals: 6,
   },
   rpcUrls: {
     default: {
-      http: ['https://noble-rpc.polkachu.com/'],
+      http: [
+        'https://noble-rpc.polkachu.com/',
+        'https://rpc.noble.strange.love/',
+        'https://noble-rpc.lavenderfive.com/'
+      ],
     },
   },
+  apiUrls: {
+    default: {
+      http: [
+        'https://noble-api.polkachu.com/',
+        'https://api-noble.strange.love/',
+        'https://noble-api.lavenderfive.com/'
+      ],
+    },
+  },
   blockExplorers: {
     default: {
       name: 'Noble Explorer',
       url: 'https://www.mintscan.io/noble',
     },
   },
 } as const;
packages/react/src/chains/osmosis.ts (1)

Line range hint 3-24: Consider enhancing chain configuration robustness

While the current configuration is functional, consider these improvements for better reliability and user experience:

  1. Add fallback RPC endpoints for better reliability
  2. Include additional popular block explorers (e.g., Mintscan)
  3. Add IBC configuration for cross-chain functionality

Here's a suggested enhancement:

 export const osmosis: CosmosChainType = {
   id: 'osmosis-1',
   chainName: 'osmosis',
   name: 'Osmosis',
   type: 'cosmos',
   nativeCurrency: {
     name: 'osmo',
     symbol: 'osmo',
     decimals: 18,
   },
   rpcUrls: {
     default: {
-      http: ['https://rpc.osmosis.zone'],
+      http: [
+        'https://rpc.osmosis.zone',
+        'https://osmosis-rpc.quickapi.com',
+        'https://osmosis-rpc.polkachu.com'
+      ],
     },
   },
   blockExplorers: {
     default: {
       name: 'Cosmos Explorer',
       url: 'https://celatone.osmosis.zone',
     },
+    mintscan: {
+      name: 'Mintscan',
+      url: 'https://www.mintscan.io/osmosis',
+    },
   },
+  // Optional: Add IBC configuration if needed
+  ibc: {
+    enabled: true,
+    channels: {
+      'osmosis-1': '0',
+    },
+  },
 } as const;
packages/react/src/actions/cosmos/getCosmosToken.ts (1)

Line range hint 6-77: Consider enhancing error handling and input validation

While the implementation is functional, consider the following improvements:

  1. Add specific error types for different failure scenarios (token not found, network issues, invalid responses).
  2. Add input validation for token addresses and account addresses.
  3. Wrap the balance and allowance queries in try-catch blocks with proper error handling.

Example implementation for enhanced error handling:

class CosmosTokenError extends Error {
  constructor(message: string, public readonly code: string) {
    super(message);
    this.name = 'CosmosTokenError';
  }
}

// In getCosmosTokenMetadata
if (!asset) {
  throw new CosmosTokenError(
    `Token ${token} not found in chain ${chain.chainName}`,
    'TOKEN_NOT_FOUND'
  );
}

// In getCosmosTokenBalanceAndAllowance
try {
  const [balanceResult, allowanceResult] = await Promise.all([...]);
} catch (error) {
  throw new CosmosTokenError(
    `Failed to fetch token data: ${error.message}`,
    'QUERY_FAILED'
  );
}
packages/react/package.json (2)

Line range hint 47-47: Avoid using 'latest' tag for @near-js/providers

Using the 'latest' tag can lead to unexpected breaking changes when new versions are released. Consider pinning to a specific version or version range.

-    "@near-js/providers": "latest",
+    "@near-js/providers": "^1.0.0",

Dependencies review reveals minor version mismatches and missing types

  • The cosmos-kit packages have version mismatches that could lead to compatibility issues:

    • @cosmos-kit/core: ^2.15.0
    • @cosmos-kit/keplr: ^2.12.2
    • @cosmos-kit/leap: ^2.14.1
    • @cosmos-kit/xdefi: ^2.10.2
  • Several blockchain SDK dependencies are missing TypeScript type definitions, which may affect type safety and development experience.

Recommendations:

  1. Align cosmos-kit package versions to the latest compatible version (2.15.0)
  2. Consider bundled types or verify if types are included in the packages themselves for:
    • @chain-registry/client
    • @cosmjs/* packages
    • @cosmos-kit/core
    • @mysten/sui
    • @solana/web3.js
    • @near-js/providers
🔗 Analysis chain

Line range hint 1-92: Verify package dependencies structure

The package has a complex dependency tree with multiple blockchain integrations. Let's verify the dependency structure to ensure there are no conflicts or missing peer dependencies.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for potential version conflicts in peer dependencies
jq -r '.peerDependencies | to_entries | .[] | select(.key | IN(.["@tanstack/react-query"], "@wagmi/core", "wagmi", "viem")) | "\(.key): \(.value)"' package.json

# Verify that all blockchain SDK dependencies have corresponding types
for pkg in $(jq -r '.dependencies | keys[]' package.json); do
  if [[ $pkg == @*/* ]]; then
    types_pkg="@types/${pkg#@*/}"
    jq --arg pkg "$types_pkg" '.devDependencies[$pkg] // empty' package.json
  fi
done

Length of output: 321


Script:

#!/bin/bash
# Let's try a simpler approach to analyze dependencies
cat packages/react/package.json | jq '{
  dependencies: .dependencies,
  peerDependencies: .peerDependencies,
  devDependencies: .devDependencies
}'

# Check if the same package versions exist in both dependencies and peerDependencies
for pkg in "@tanstack/react-query" "@wagmi/core" "wagmi" "viem"; do
  echo "Checking $pkg:"
  cat packages/react/package.json | jq --arg pkg "$pkg" \
    'if .dependencies[$pkg] and .peerDependencies[$pkg] then
      {
        "package": $pkg,
        "in_dependencies": .dependencies[$pkg],
        "in_peerDependencies": .peerDependencies[$pkg]
      }
    else
      empty
    end'
done

Length of output: 3939


Script:

#!/bin/bash
# Let's check for any missing type definitions for blockchain SDKs
echo "Checking for potentially missing type definitions:"
for pkg in "@chain-registry/client" "@cosmjs/cosmwasm-stargate" "@cosmjs/proto-signing" "@cosmjs/stargate" "@cosmos-kit/core" "@mysten/sui" "@solana/web3.js" "@near-js/providers"; do
  echo "Package: $pkg"
  # Check if types package exists in dependencies or devDependencies
  cat packages/react/package.json | jq --arg pkg "@types/${pkg#@*/}" \
    'if (.dependencies[$pkg] or .devDependencies[$pkg]) then
      "✓ Types found"
    else
      "⚠️ No types package found"
    end'
done

# Check for potential version mismatches between related packages
echo -e "\nChecking version alignment for related packages:"
cat packages/react/package.json | jq '{
  "cosmjs": [.dependencies["@cosmjs/cosmwasm-stargate"], .dependencies["@cosmjs/proto-signing"], .dependencies["@cosmjs/stargate"]],
  "cosmos-kit": [.dependencies["@cosmos-kit/core"], .dependencies["@cosmos-kit/keplr"], .dependencies["@cosmos-kit/leap"], .dependencies["@cosmos-kit/xdefi"]],
  "near-wallet-selector": [.dependencies["@near-wallet-selector/core"], .dependencies["@near-wallet-selector/ethereum-wallets"], .dependencies["@near-wallet-selector/my-near-wallet"], .dependencies["@near-wallet-selector/near-mobile-wallet"], .dependencies["@near-wallet-selector/wallet-connect"]]
}'

Length of output: 4877

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between e60180b and a03aa39.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (9)
  • packages/react/package.json (1 hunks)
  • packages/react/src/actions/cosmos/getCosmosToken.ts (2 hunks)
  • packages/react/src/chains/injective.ts (1 hunks)
  • packages/react/src/chains/noble.ts (1 hunks)
  • packages/react/src/chains/osmosis.ts (1 hunks)
  • packages/react/src/providers/CosmosProvider.tsx (2 hunks)
  • packages/react/src/store/Cosmos.ts (2 hunks)
  • packages/react/src/types/index.ts (2 hunks)
  • packages/react/src/utils/createChainConfigs.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/react/src/utils/createChainConfigs.ts
🔇 Additional comments (11)
packages/react/src/chains/noble.ts (1)

1-1: LGTM! Type correction looks good.

The type correction from CosmsosChainType to CosmosChainType has been properly implemented both in the import statement and the type annotation.

Also applies to: 3-3

packages/react/src/chains/osmosis.ts (1)

1-1: LGTM! Type correction improves type safety

The type correction from CosmsosChainType to CosmosChainType properly aligns with the type system and fixes the previous typo.

Also applies to: 3-3

packages/react/src/chains/injective.ts (2)

Line range hint 3-25: LGTM! Configuration matches Injective mainnet specifications

The chain configuration is well-structured and includes all necessary parameters. The type correction has been properly implemented.

✅ Verification successful

Chain configuration is consistent and properly implemented

The verification results show:

  • Chain ID 'injective-1' is consistently used across the codebase
  • RPC endpoint is uniquely referenced in the chain configuration
  • CosmosChainType is consistently used across other chain configurations (Noble, Osmosis) with similar structure
  • All configurations follow the same pattern with proper type assertions
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify chain configuration consistency across the codebase
# Test 1: Check for any other references to Injective chain ID
echo "Checking references to Injective chain ID..."
rg "injective-1"

# Test 2: Check for any hardcoded Injective RPC URLs to ensure consistency
echo "Checking Injective RPC URL references..."
rg "sentry.tm.injective.network"

# Test 3: Check if the CosmosChainType is used consistently for other chains
echo "Checking CosmosChainType usage..."
ast-grep --pattern 'export const $_: CosmosChainType = $_'

Length of output: 4508


1-1: Verify the .js extension in TypeScript imports

The import statement uses a .js extension while importing from a TypeScript file. While this might be intentional for ESM compatibility, let's verify this is consistent with the project's configuration.

✅ Verification successful

Based on the shell script results, I can now make a definitive assessment about the .js extension usage in TypeScript imports:

Using .js extension in TypeScript imports is the correct approach in this codebase

The TypeScript configuration files reveal that this project:

  • Uses ES modules ("module": "NodeNext"/"ESNext")
  • Uses modern module resolution ("moduleResolution": "NodeNext"/"Bundler")
  • The pattern is consistently used across the entire codebase as shown by the ripgrep results

This is actually the recommended approach for ES modules in TypeScript, as the .js extension is required for proper ESM resolution, even though the source files are .ts. The TypeScript compiler will handle this correctly during compilation.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check TypeScript configuration and import patterns
# Test 1: Check if this is a consistent pattern in the codebase
echo "Checking import patterns with .js extensions in TypeScript files..."
rg -t ts "from.*\.js[\"']" 

# Test 2: Check TypeScript configuration
echo "Checking TypeScript configuration..."
fd tsconfig.json --exec cat {}

Length of output: 34558

packages/react/src/actions/cosmos/getCosmosToken.ts (1)

3-3: LGTM! Import statement is well-structured

The import statement correctly includes the necessary types and follows ESM compatibility guidelines with the .js extension.

packages/react/package.json (1)

53-53: Verify potential circular dependency with tangled-solana-react

The re-addition of @noble-assets/tangled-solana-react could create a circular dependency if that package also depends on this one. This might cause issues during installation and builds.

✅ Verification successful

No circular dependency detected - dependency structure is correct

After examining the package.json of @noble-assets/tangled-solana-react, there is no circular dependency as it does not depend on @noble-assets/tangled-react. The package has appropriate Solana-specific dependencies and React as its main dependencies, with proper peer dependency declarations.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if tangled-solana-react depends on tangled-react
fd -t f "package.json" -x jq -r 'select(.name == "@noble-assets/tangled-solana-react") | .dependencies["@noble-assets/tangled-react"] // empty'

Length of output: 200


Script:

#!/bin/bash
# First, let's find the package.json file for tangled-solana-react
fd "package.json" | grep -i "solana-react"

# Then, let's examine its content if found
fd "package.json" | grep -i "solana-react" | xargs cat

Length of output: 1949

packages/react/src/store/Cosmos.ts (1)

8-8: LGTM: Import statement correction

The import statement now correctly references CosmosChainType instead of the misspelled CosmsosChainType.

packages/react/src/types/index.ts (2)

55-58: LGTM! Interface rename fixes the typo.

The interface definition is well-structured with proper type constraints and maintains backward compatibility.


68-68: Verify the interface rename across the codebase.

The type union looks correct. Let's verify that all references to the old interface name have been updated.

packages/react/src/providers/CosmosProvider.tsx (2)

21-21: LGTM! Import statement correction.

The import statement correctly imports the renamed type CosmosChainType, fixing the previous typo in the type name.


Line range hint 192-218: LGTM! Robust error handling implementation.

The connect mutation implementation includes:

  • Clear error messages for wallet not found scenarios
  • Proper fallback mechanism for adding chains when enable fails
  • Strong type safety throughout the implementation

packages/react/src/store/Cosmos.ts Show resolved Hide resolved
@code-z2 code-z2 merged commit 5a17360 into main Nov 19, 2024
1 check passed
@code-z2 code-z2 deleted the fix/custom-chains-and-eslint branch November 19, 2024 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants