chalabi/cross chain x #194
Conversation
WalkthroughMultiple components have been updated to integrate dynamic chain configurations and Osmosis-specific data. The changes include revised props and state management in cross-chain components; new hooks and utilities for fetching and formatting token and IBC data; updates to configuration files with additional chain and environment variables; enhancements in fee estimation, wallet connection, and transaction handling; modifications to tests and scripts; and improvements in asset display and denomination formatting. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant SB as SendBox Component
participant EF as useEffect Hook
participant GF as getAvailableToChains
participant IF as IbcSendForm
U->>SB: Initiate cross-chain transfer
SB->>EF: Monitor selectedFromChain & selectedToChain
EF->>GF: Request available destination chains
GF-->>EF: Return filtered chain list
EF->>SB: Update selectedToChain if needed
SB->>IF: Pass updated chain selections
IF->>U: Render updated transfer UI
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (17)
components/bank/forms/sendForm.tsx (2)
228-230: Enhance token display name handling logicThe current implementation makes assumptions about token prefixes that might not always hold true:
- The 'u' prefix is assumed to always represent micro units
- There's no validation for empty strings after removing prefixes
Consider this safer implementation:
- tokenDisplayName.startsWith('u') - ? tokenDisplayName.slice(1).toUpperCase() - : truncateString(tokenDisplayName, 10).toUpperCase(); + tokenDisplayName.startsWith('u') + ? (tokenDisplayName.length > 1 + ? tokenDisplayName.slice(1).toUpperCase() + : tokenDisplayName.toUpperCase()) + : truncateString(tokenDisplayName, 10).toUpperCase();Also, consider maintaining a mapping of known token prefixes to their display formats for better maintainability.
Line range hint
41-51: Consider enhancing cross-chain support documentationWhile the component is chain-agnostic, it would be beneficial to document how it handles different chain-specific requirements, especially:
- Fee estimation variations
- Chain-specific token display rules
- Balance type differences
Consider adding JSDoc comments to clarify:
/** * Form component for sending tokens across supported chains. * @param balances - Combined balance information from supported chains * @param selectedDenom - Selected denomination, should be compatible across chains * ... */utils/ibc.ts (1)
181-212: ReplacemockIbcDatawith actual IBC dataThe
mockIbcDataconstant is currently used to provide IBC connection and channel information. To ensure accurate and up-to-date data, replace this mock data with dynamic data retrieved from a reliable source, such as the chain registry or IBC module queries.pages/bank.tsx (1)
229-231: Include Osmosis loading states inisLoadingThe
isLoadingstate currently considersisBalancesLoading,resolvedLoading, andisMetadatasLoading. Since you've introduced Osmosis balances and metadata, consider includingisOsmosisBalancesLoadingandresolvedOsmosisLoadingto accurately reflect the loading state when Osmosis data is being fetched.components/bank/forms/ibcSendForm.tsx (1)
158-167: Review fee buffer logic for different tokensIn the validation schema, the
leave-for-feestest only applies whenselectedToken.denom === 'umfx'. If users are sending tokens from Osmosis or other chains, consider whether the fee buffer logic should also apply to those tokens to prevent failed transactions due to insufficient fees.config/env.ts (1)
5-15: LGTM! Consider documenting the new environment variables.The new Osmosis testnet environment variables follow consistent patterns and have appropriate fallbacks. Consider updating the project's documentation to include these new variables and their expected values.
hooks/useLcdQueryClient.ts (1)
23-37: Consider refactoring to reduce code duplication.While the implementation is correct, there's significant code duplication between
useOsmosisLcdQueryClientanduseLcdQueryClient. Consider creating a shared factory function:-export const useOsmosisLcdQueryClient = () => { - const lcdQueryClient = useQuery({ - queryKey: ['lcdQueryClientOsmosis', env.osmosisTestnetApiUrl], - queryFn: () => - createLcdQueryClient({ - restEndpoint: env.osmosisTestnetApiUrl, - }), - enabled: !!env.osmosisTestnetApiUrl, - staleTime: Infinity, - }); - - return { - lcdQueryClient: lcdQueryClient.data, - }; -}; +const createLcdQueryHook = (chainType: 'Cosmos' | 'Osmosis') => () => { + const apiUrl = chainType === 'Cosmos' ? env.apiUrl : env.osmosisTestnetApiUrl; + const lcdQueryClient = useQuery({ + queryKey: [`lcdQueryClient${chainType}`, apiUrl], + queryFn: () => + createLcdQueryClient({ + restEndpoint: apiUrl, + }), + enabled: !!apiUrl, + staleTime: Infinity, + }); + + return { + lcdQueryClient: lcdQueryClient.data, + }; +}; + +export const useLcdQueryClient = createLcdQueryHook('Cosmos'); +export const useOsmosisLcdQueryClient = createLcdQueryHook('Osmosis');components/factory/components/DenomImage.tsx (1)
101-101: Update alt text for Osmosis tokenThe alt text "MFX Token Icon" is incorrect when displaying Osmosis tokens. This affects accessibility.
- alt="MFX Token Icon" + alt={denom?.base?.includes('umfx') ? 'MFX Token Icon' : 'Osmosis Token Icon'}Also applies to: 109-111
utils/yupExtensions.ts (1)
92-92: Consider moving chain prefixes to configurationThe list of valid prefixes could be moved to configuration to improve maintainability and align with the PR's objective of removing hardcoded values.
- const validPrefixes = ['manifest', 'manifestvaloper', 'manifestvalcons', 'osmo']; + const validPrefixes = env.VALID_BECH32_PREFIXES;This would require adding the configuration to your environment setup:
// config/env.ts export default { // ... other config VALID_BECH32_PREFIXES: ['manifest', 'manifestvaloper', 'manifestvalcons', 'osmo'] as const, }components/bank/components/sendBox.tsx (1)
44-45: Consider memoizing chain selection handlersThe chain selection handlers could be memoized to prevent unnecessary re-renders.
+ const handleFromChainChange = useCallback((chain: string) => { + setSelectedFromChain(chain); + }, []); + + const handleToChainChange = useCallback((chain: string) => { + setSelectedToChain(chain); + }, []); // In JSX - setSelectedFromChain={setSelectedFromChain} - setSelectedToChain={setSelectedToChain} + setSelectedFromChain={handleFromChainChange} + setSelectedToChain={handleToChainChange}Also applies to: 100-103
components/bank/components/tokenList.tsx (1)
118-127: Enhance denomination display logic.The current implementation could be simplified and made more maintainable.
Consider extracting the denomination display logic into a separate utility function:
-<p className="text-sm text-[#00000099] dark:text-[#FFFFFF99]"> - {balance.metadata?.denom_units[0]?.denom.startsWith('ibc') - ? balance.metadata?.denom_units[0]?.aliases[0] - : balance.metadata?.denom_units[0]?.denom.split('/').pop()} -</p> +<p className="text-sm text-[#00000099] dark:text-[#FFFFFF99]"> + {getDenominationDisplay(balance.metadata)} +</p>Add this utility function:
function getDenominationDisplay(metadata: DenomMetadata | null): string { if (!metadata?.denom_units?.[0]?.denom) return ''; const { denom } = metadata.denom_units[0]; return denom.startsWith('ibc') ? metadata.denom_units[0].aliases?.[0] ?? '' : denom.split('/').pop() ?? ''; }components/bank/components/historyBox.tsx (2)
220-238: Consider caching asset information.The
denomToAssetfunction is called multiple times for the same denomination, which could be optimized.Consider using useMemo to cache the asset information:
const assetInfo = useMemo(() => denomToAsset(env.chain, amt.denom), [env.chain, amt.denom]);
Line range hint
251-309: Simplify denomination handling logic.The code duplicates the IBC denomination handling logic in multiple places.
Consider extracting the common logic into a utility function:
function getBaseDenom(denom: string, assetInfo: AssetInfo | null): string { if (denom.startsWith('ibc/') && assetInfo?.traces?.[0]?.counterparty?.base_denom) { return assetInfo.traces[0].counterparty.base_denom.slice(1); } return formatDenom(denom); }hooks/useQueries.ts (2)
689-715: Consider adding error type information.While the implementation is correct, consider adding type information for the error in the return object to improve error handling downstream.
return { balances: balancesQuery.data?.balances, isBalancesLoading: balancesQuery.isLoading, isBalancesError: balancesQuery.isError, + error: balancesQuery.error as Error | null, refetchBalances: balancesQuery.refetch, };
745-771: Consider consolidating duplicate code.The Osmosis hooks share significant code with their non-Osmosis counterparts. Consider extracting the common logic into a shared utility function.
// Example shared utility function const createBalanceHook = (useClient: () => { lcdQueryClient: any }, queryKeyPrefix: string) => { return (address: string) => { const { lcdQueryClient } = useClient(); const fetchBalances = async () => { if (!lcdQueryClient) { throw new Error('LCD Client not ready'); } return await lcdQueryClient.cosmos.bank.v1beta1.allBalances({ address, resolveDenom: true, }); }; const balancesQuery = useQuery({ queryKey: [`${queryKeyPrefix}-resolved`, address], queryFn: fetchBalances, enabled: !!lcdQueryClient && !!address, staleTime: Infinity, }); return { balances: balancesQuery.data?.balances, isBalancesLoading: balancesQuery.isLoading, isBalancesError: balancesQuery.isError, refetchBalances: balancesQuery.refetch, }; }; }; // Usage export const useTokenBalancesResolved = createBalanceHook(useLcdQueryClient, 'balances'); export const useOsmosisTokenBalancesResolved = createBalanceHook(useOsmosisLcdQueryClient, 'osmosisBalances');hooks/useFeeEstimation.ts (1)
28-28: Consider using an environment variable for chain comparison.Instead of hardcoding 'manifesttestnet', consider using an environment variable for better maintainability.
- GasPrice.fromString(`${gasPrice}${chainName === 'manifesttestnet' ? 'umfx' : 'uosmo'}`) + GasPrice.fromString(`${gasPrice}${chainName === env.chain ? 'umfx' : 'uosmo'}`)components/groups/components/myGroups.tsx (1)
264-289: LGTM! Robust IBC token metadata handling.The implementation properly handles IBC token metadata with appropriate fallbacks for missing data.
Consider adding error handling for malformed IBC denoms.
While the implementation is solid, consider adding error handling for malformed IBC denoms.
if (coreBalance.denom.startsWith('ibc/')) { + try { const assetInfo = denomToAsset(env.chain, coreBalance.denom); + if (!assetInfo) { + throw new Error('Asset info not found'); + } // ... rest of the code + } catch (error) { + console.warn(`Failed to process IBC token ${coreBalance.denom}:`, error); + return { + denom: coreBalance.denom, + coreDenom: coreBalance.denom, + amount: coreBalance.amount, + metadata: null, + }; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
public/osmosis.pngis excluded by!**/*.pngpublic/osmosis.svgis excluded by!**/*.svg
📒 Files selected for processing (26)
components/bank/components/historyBox.tsx(4 hunks)components/bank/components/sendBox.tsx(4 hunks)components/bank/components/tokenList.tsx(4 hunks)components/bank/forms/ibcSendForm.tsx(11 hunks)components/bank/forms/sendForm.tsx(1 hunks)components/bank/modals/sendModal.tsx(3 hunks)components/bank/modals/txInfo.tsx(2 hunks)components/factory/components/DenomImage.tsx(1 hunks)components/factory/forms/TokenDetailsForm.tsx(1 hunks)components/groups/components/groupControls.tsx(2 hunks)components/groups/components/myGroups.tsx(3 hunks)components/groups/forms/groups/ConfirmationForm.tsx(2 hunks)components/groups/modals/voteModal.tsx(2 hunks)config/defaults.ts(0 hunks)config/env.ts(1 hunks)config/index.ts(0 hunks)hooks/useFeeEstimation.ts(1 hunks)hooks/useLcdQueryClient.ts(1 hunks)hooks/useQueries.ts(4 hunks)hooks/useTx.tsx(2 hunks)package.json(1 hunks)pages/_app.tsx(5 hunks)pages/bank.tsx(7 hunks)utils/constants.ts(1 hunks)utils/ibc.ts(5 hunks)utils/yupExtensions.ts(1 hunks)
💤 Files with no reviewable changes (2)
- config/index.ts
- config/defaults.ts
🧰 Additional context used
🪛 GitHub Check: build
components/bank/forms/ibcSendForm.tsx
[failure] 95-95: TypeError: null is not an object (evaluating 'searchParams.get')
at IbcSendForm (/home/runner/work/manifest-app/manifest-app/components/bank/forms/ibcSendForm.tsx:95:25)
at renderWithHooks (/home/runner/work/manifest-app/manifest-app/node_modules/react-dom/cjs/react-dom.development.js:16305:18)
at mountIndeterminateComponent (/home/runner/work/manifest-app/manifest-app/node_modules/react-dom/cjs/react-dom.development.js:20074:13)
at beginWork$1 (/home/runner/work/manifest-app/manifest-app/node_modules/react-dom/cjs/react-dom.development.js:27426:14)
at performUnitOfWork (/home/runner/work/manifest-app/manifest-app/node_modules/react-dom/cjs/react-dom.development.js:26560:12)
at workLoopSync (/home/runner/work/manifest-app/manifest-app/node_modules/react-dom/cjs/react-dom.development.js:26466:5)
at renderRootSync (/home/runner/work/manifest-app/manifest-app/node_modules/react-dom/cjs/react-dom.development.js:26434:7)
at recoverFromConcurrentError (/home/runner/work/manifest-app/manifest-app/node_modules/react-dom/cjs/react-dom.development.js:25850:20)
at performConcurrentWorkOnRoot (/home/runner/work/manifest-app/manifest-app/node_modules/react-dom/cjs/react-dom.development.js:25750:22)
at flushActQueue (/home/runner/work/manifest-app/manifest-app/node_modules/react/cjs/react.development.js:2667:24)
at act (/home/runner/work/manifest-app/manifest-app/node_modules/react/cjs/react.development.js:2582:11)
at <anonymous> (/home/runner/work/manifest-app/manifest-app/node_modules/@testing-library/react/dist/act-compat.js:47:25)
at renderRoot (/home/runner/work/manifest-app/manifest-app/node_modules/@testing-library/react/dist/pure.js:180:18)
at <anonymous> (/home/runner/work/manifest-app/manifest-app/components/bank/forms/__tests__/ibcSendForm.test.tsx:54:5)
at <anonymous> (/home/runner/work/manifest-app/manifest-app/components/bank/forms/__tests__/ibcSendForm.test.tsx:53:26)
[failure] 95-95: TypeError: null is not an object (evaluating 'searchParams.get')
at IbcSendForm (/home/runner/work/manifest-app/manifest-app/components/bank/forms/ibcSendForm.tsx:95:25)
at renderWithHooks (/home/runner/work/manifest-app/manifest-app/node_modules/react-dom/cjs/react-dom.development.js:16305:18)
at mountIndeterminateComponent (/home/runner/work/manifest-app/manifest-app/node_modules/react-dom/cjs/react-dom.development.js:20074:13)
at beginWork$1 (/home/runner/work/manifest-app/manifest-app/node_modules/react-dom/cjs/react-dom.development.js:27426:14)
at performUnitOfWork (/home/runner/work/manifest-app/manifest-app/node_modules/react-dom/cjs/react-dom.development.js:26560:12)
at workLoopSync (/home/runner/work/manifest-app/manifest-app/node_modules/react-dom/cjs/react-dom.development.js:26466:5)
at renderRootSync (/home/runner/work/manifest-app/manifest-app/node_modules/react-dom/cjs/react-dom.development.js:26434:7)
at recoverFromConcurrentError (/home/runner/work/manifest-app/manifest-app/node_modules/react-dom/cjs/react-dom.development.js:25850:20)
at performSyncWorkOnRoot (/home/runner/work/manifest-app/manifest-app/node_modules/react-dom/cjs/react-dom.development.js:26096:20)
at flushSyncCallbacks (/home/runner/work/manifest-app/manifest-app/node_modules/react-dom/cjs/react-dom.development.js:12042:22)
at flushActQueue (/home/runner/work/manifest-app/manifest-app/node_modules/react/cjs/react.development.js:2667:24)
at act (/home/runner/work/manifest-app/manifest-app/node_modules/react/cjs/react.development.js:2582:11)
at <anonymous> (/home/runner/work/manifest-app/manifest-app/node_modules/@testing-library/react/dist/act-compat.js:47:25)
at eventWrapper (/home/runner/work/manifest-app/manifest-app/node_modules/@testing-library/react/dist/pure.js:107:20)
at <anonymous> (/home/runner/work/manifest-app/manifest-app/components/bank/components/__tests__/sendBox.test.tsx:45:15)
at <anonymous> (/home/runner/work/manifest-app/manifest-app/components/bank/components/__tests__/sendBox.test.tsx:41:57)
[failure] 95-95: TypeError: null is not an object (evaluating 'searchParams.get')
at IbcSendForm (/home/runner/work/manifest-app/manifest-app/components/bank/forms/ibcSendForm.tsx:95:25)
at renderWithHooks (/home/runner/work/manifest-app/manifest-app/node_modules/react-dom/cjs/react-dom.development.js:16305:18)
at mountIndeterminateComponent (/home/runner/work/manifest-app/manifest-app/node_modules/react-dom/cjs/react-dom.development.js:20074:13)
at beginWork$1 (/home/runner/work/manifest-app/manifest-app/node_modules/react-dom/cjs/react-dom.development.js:27426:14)
at performUnitOfWork (/home/runner/work/manifest-app/manifest-app/node_modules/react-dom/cjs/react-dom.development.js:26560:12)
at workLoopSync (/home/runner/work/manifest-app/manifest-app/node_modules/react-dom/cjs/react-dom.development.js:26466:5)
at renderRootSync (/home/runner/work/manifest-app/manifest-app/node_modules/react-dom/cjs/react-dom.development.js:26434:7)
at recoverFromConcurrentError (/home/runner/work/manifest-app/manifest-app/node_modules/react-dom/cjs/react-dom.development.js:25850:20)
at performSyncWorkOnRoot (/home/runner/work/manifest-app/manifest-app/node_modules/react-dom/cjs/react-dom.development.js:26096:20)
at flushSyncCallbacks (/home/runner/work/manifest-app/manifest-app/node_modules/react-dom/cjs/react-dom.development.js:12042:22)
at flushActQueue (/home/runner/work/manifest-app/manifest-app/node_modules/react/cjs/react.development.js:2667:24)
at act (/home/runner/work/manifest-app/manifest-app/node_modules/react/cjs/react.development.js:2582:11)
at <anonymous> (/home/runner/work/manifest-app/manifest-app/node_modules/@testing-library/react/dist/act-compat.js:47:25)
at eventWrapper (/home/runner/work/manifest-app/manifest-app/node_modules/@testing-library/react/dist/pure.js:107:20)
at <anonymous> (/home/runner/work/manifest-app/manifest-app/components/bank/components/__tests__/sendBox.test.tsx:51:15)
at <anonymous> (/home/runner/work/manifest-app/manifest-app/components/bank/components/__tests__/sendBox.test.tsx:49:79)
🪛 GitHub Actions: Test and Coverage
components/bank/forms/ibcSendForm.tsx
[error] 95-95: TypeError: null is not an object (evaluating 'searchParams.get'). The useSearchParams hook is returning null, but the code attempts to call .get() on it without null checking.
🔇 Additional comments (19)
utils/ibc.ts (2)
131-131: Confirm inclusion criteria infilterAssetsfunctionThe
filterAssetsfunction now includes assets wheretype_assetis'ics20'or not specified (!type_asset). Please verify that this change is intentional and that it won't introduce unintended assets into the filtered list.
148-155: Ensure correct trace selection indenomToAssetfunctionIn the
denomToAssetfunction, when handling IBC hashes, you're accessingasset.traces[0].counterparty.base_denom. If an asset has multiple traces,traces[0]may not always correspond to the desired counterparty. Please confirm that using the first trace is appropriate in all cases, or consider implementing logic to select the correct trace.components/bank/forms/ibcSendForm.tsx (1)
85-90: Ensure fee estimation uses correct chainIn the
useTxanduseFeeEstimationhooks, you're conditionally checkingselectedFromChainagainstenv.osmosisTestnetChainto determine which chain to use. Double-check thatenv.osmosisTestnetChainaccurately reflects the chain ID and that this logic does not cause issues when additional chains are added.utils/constants.ts (1)
17-29: LGTM! Token metadata is correctly defined.The OSMOSIS_TOKEN_DATA constant follows the established pattern and correctly defines the token denominations with proper exponents.
components/groups/modals/voteModal.tsx (1)
18-20: LGTM! Verify chain name usage across the codebase.Successfully replaced hardcoded chain name with environment variable. Let's verify similar usage across the codebase.
✅ Verification successful
Chain name usage is consistent across the codebase
The remaining hardcoded 'manifest' references are legitimate as they are used for address validation patterns, test data, and UI-specific cases. All chain-related hooks consistently use
env.chain, with appropriate handling of IBC functionality where different chains are needed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining hardcoded 'manifest' chain references rg -i "['\"](manifest|osmosis)['\"]" --type ts --type tsx # Search for potential missed chain-related environment variable usage rg "useChain\(['\"]" --type ts --type tsxLength of output: 165
Script:
#!/bin/bash # Search for hardcoded chain references without file type restriction rg -i "['\"]manifest['\"]" rg -i "['\"]osmosis['\"]" # Search for chain-related hook usage rg "use(Chain|Tx|FeeEstimation)\(" # Search for env.chain usage to verify consistent pattern rg "env\.chain"Length of output: 14895
components/bank/modals/sendModal.tsx (1)
20-23: LGTM! Clean integration of Osmosis balance handlingThe changes properly integrate Osmosis balance functionality through well-defined props and consistent prop passing.
Also applies to: 39-42, 109-112
components/bank/modals/txInfo.tsx (1)
78-97: LGTM! Enhanced IBC token display handling.The changes improve the display of IBC tokens by retrieving and showing the base denomination instead of the raw IBC hash. The implementation correctly:
- Handles IBC token detection
- Retrieves asset metadata
- Formats amounts with proper decimal places
hooks/useTx.tsx (2)
49-50: LGTM! Dynamic explorer URL configuration.The change removes hardcoded chain references by dynamically selecting the explorer URL based on the chain name.
Line range hint
111-134: LGTM! Enhanced proposal submission feedback.The implementation properly:
- Detects proposal submission transactions
- Extracts proposal ID from events
- Provides relevant feedback with proposal-specific links
components/factory/forms/TokenDetailsForm.tsx (1)
Line range hint
48-56: LGTM! Automatic denom units management.The useEffect hook properly maintains denomUnits consistency by:
- Including both full denom and display units
- Setting correct exponent values
- Updating on relevant field changes
components/groups/forms/groups/ConfirmationForm.tsx (1)
9-9: LGTM! Removed hardcoded chain references.The changes properly:
- Import environment configuration
- Replace hardcoded chain references with env.chain
- Maintain consistent behavior across different chains
Also applies to: 33-34
pages/_app.tsx (3)
12-19: LGTM! Clean import restructuring.The imports have been updated to source chain configurations directly from the chain registry, improving maintainability by removing hardcoded values.
79-79: LGTM! Enhanced message type support.The addition of IBC proto registry and amino converters enables proper handling of IBC-related messages.
Also applies to: 85-85
192-195: Verify Osmosis testnet endpoints configuration.The Osmosis testnet endpoints and chain configuration have been added. Please ensure that:
- The environment variables
osmosisTestnetRpcUrlandosmosisTestnetApiUrlare properly configured.- The chain registry assets and configuration for both chains are compatible.
Also applies to: 205-207
✅ Verification successful
Environment variables and chain registry configuration are properly set up
The implementation shows correct configuration:
- Environment variables are properly defined in
config/env.tsand used correctly throughout the application- Chain registry configuration in
utils/ibc.tsshows compatible asset lists and correct IBC channel setupNote: Consider adding these environment variables to an
.env.examplefile for documentation purposes:NEXT_PUBLIC_OSMOSIS_TESTNET_API_URL= NEXT_PUBLIC_OSMOSIS_TESTNET_RPC_URL=🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify environment variables and chain registry configuration # Test 1: Check if environment variables are defined echo "Checking environment variables..." grep -r "osmosisTestnetRpcUrl\|osmosisTestnetApiUrl" .env* || echo "Environment variables not found in .env files" # Test 2: Verify chain registry configuration echo "Checking chain registry imports..." fd -e ts -e js -x rg -l "chain-registry/testnet/(manifesttestnet|osmosistestnet)"Length of output: 420
Script:
#!/bin/bash # Check for environment variable usage and template files echo "Checking for environment variable usage..." rg "osmosisTestnetRpcUrl|osmosisTestnetApiUrl" -A 2 -B 2 echo -e "\nChecking for .env template files..." fd -g ".env*" echo -e "\nChecking chain registry configuration..." cat ./utils/ibc.tsLength of output: 9342
components/bank/components/tokenList.tsx (2)
19-22: LGTM! Well-structured prop additions.The new Osmosis-related props are properly typed and marked as optional, maintaining backward compatibility.
251-254: LGTM! Proper prop forwarding with fallbacks.The Osmosis-related props are properly forwarded to the SendModal component with appropriate default values.
components/groups/components/groupControls.tsx (1)
17-17: LGTM! Removed hardcoded chain identifier.The change from hardcoded 'manifest' to
env.chainimproves configurability and maintainability.Also applies to: 25-25, 231-231
hooks/useQueries.ts (1)
635-659: LGTM! Well-structured hook for Osmosis token metadata.The implementation follows the established pattern and includes proper error handling.
package.json (1)
7-7: Verify the impact of removing host binding.Removing
-H 0.0.0.0might affect accessibility of the dev server from other devices on the network. Ensure this change doesn't impact your development workflow.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
components/bank/forms/ibcSendForm.tsx (1)
Line range hint
146-152: Incorrect recipient address validation based on destination chainThe recipient address validation currently uses
.manifestAddress(), which may not be appropriate when the destination chain is not Manifest. Update the validation to check the recipient address format according to theselectedToChain.Apply this diff to adjust the validation:
recipient: Yup.string().required('Recipient is required') - .manifestAddress(), + .test( + 'is-valid-address', + 'Invalid recipient address', + function (value) { + if (!value) return false; + const { selectedToChain } = this.parent; + if (selectedToChain === env.chain) { + return isValidManifestAddress(value); + } else if (selectedToChain === env.osmosisTestnetChain) { + return isValidOsmosisAddress(value); + } + return false; + } + ),You'll need to implement
isValidManifestAddressandisValidOsmosisAddressfunctions to properly validate addresses for each chain.
🧹 Nitpick comments (4)
scripts/ibcTransferAll.ts (4)
91-93: Avoid fragile URL manipulation for REST endpointReplacing
'rpc'with'rest'in theOSMOSIS_RPCURL is fragile and could lead to errors if the URL structure changes. Consider specifying the REST endpoint explicitly or parsing the URL more robustly.Apply this diff to improve URL handling:
- const response = await axios.get( - `${OSMOSIS_RPC.replace('rpc', 'rest')}/ibc/apps/transfer/v1/denom_traces/${hash}` - ); + const osmosisRestUrl = OSMOSIS_RPC.replace('rpc', 'rest'); + const denomTraceUrl = `${osmosisRestUrl}/ibc/apps/transfer/v1/denom_traces/${hash}`; + const response = await axios.get(denomTraceUrl);Alternatively, define the REST endpoint explicitly in your environment configuration:
// Environment Configuration const env = { rpcUrl: 'https://nodes.liftedinit.tech/manifest/testnet/rpc', osmosisTestnetRpcUrl: 'https://rpc.osmotest5.osmosis.zone', + osmosisTestnetRestUrl: 'https://rest.osmotest5.osmosis.zone', chain: 'manifest-testnet', osmosisTestnetChain: 'osmo-test-5', };And then use it directly:
- const response = await axios.get( - `${OSMOSIS_RPC.replace('rpc', 'rest')}/ibc/apps/transfer/v1/denom_traces/${hash}` - ); + const response = await axios.get( + `${env.osmosisTestnetRestUrl}/ibc/apps/transfer/v1/denom_traces/${hash}` + );
151-153: Make the transfer amount configurableCurrently, the script transfers
'1'unit of each token. Consider making the transfer amount configurable or transferring the entire balance to suit different use cases.Apply this diff to transfer the full balance of each token:
token: { denom: token.denom, - amount: '1', + amount: token.amount, },
167-168: Set fees and gas limits dynamicallyHardcoding the fee amount and gas limit may lead to transaction failures or unnecessary costs. Consider estimating the gas and setting the fee dynamically based on transaction requirements.
Apply this diff to estimate fees dynamically:
- const fee = { - amount: [{ denom: 'umfx', amount: '5500' }], - gas: '5000000', - }; + const estimatedFee = await manifestClient.simulate(manifestAccount.address, messages, ''); + const fee = { + amount: estimatedFee.amount, + gas: estimatedFee.gas.toString(), + };
18-26: ExtendgetIbcInfoto support additional chain combinationsCurrently,
getIbcInfoonly supports transfers betweenmanifest-testnetandosmo-test-5. Consider extending this function to support additional chain combinations for future scalability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.gitignore(1 hunks)components/bank/components/sendBox.tsx(4 hunks)components/bank/forms/ibcSendForm.tsx(11 hunks)scripts/ibcTransferAll.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Test and Coverage
components/bank/forms/ibcSendForm.tsx
[error] 476-476: TypeError: Cannot call map() on undefined. The 'availableToChains' variable is undefined when attempting to use map() function.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
.gitignore (2)
41-41: Verify the purpose and content of the certificates directory.Given this PR's focus on IBC transfers, please confirm:
- Are these certificates used for chain communication?
- Does this directory contain any sensitive production certificates that should be managed differently?
Let's check if there are any certificates committed before this change:
✅ Verification successful
Preemptive .gitignore entry for certificates is a good security practice
The absence of any certificate files or references in the codebase suggests this is a preemptive .gitignore entry, which is a recommended practice to prevent accidental commitment of sensitive security files that will be generated during IBC setup.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any certificate files that might have been committed fd -e crt -e pem -e key -e cerLength of output: 32
Script:
#!/bin/bash # Search for certificate-related patterns in code and documentation rg -i "certificate" --type-add 'docs:*.{md,txt,rst}' -g '!{node_modules,dist,build}/*' -A 2 # Look for configuration templates or examples fd -e yaml -e yml -e json -e example -e template -e sample | xargs rg -i "certificate"Length of output: 178
43-43: Clarify the purpose and naming of demons.json.The file name
demons.jsonis unclear and potentially concerning. Could you please:
- Explain its purpose in relation to the Osmosis integration
- Consider a more descriptive name that reflects its functionality
- Confirm if it contains any sensitive data that requires special handling
Let's check if this file exists and examine its structure:
scripts/ibcTransferAll.ts (1)
141-142: Verify the correctness oftimeoutInNanoscalculationEnsure that
timeoutInNanosis calculated correctly to prevent premature timeouts or excessive delays in the IBC transfers. Verify that the units are consistent and accurately represent the intended timeout duration.components/bank/components/sendBox.tsx (1)
64-70: Chain selection validation implemented correctlyThe
useEffecthook properly ensures that the source and destination chains are not the same by updating theselectedToChainwhen necessary. This addresses the previous concern about missing chain selection validation.components/bank/forms/ibcSendForm.tsx (1)
202-206: Verify the sender address corresponds to the selectedFromChainEnsure that the sender address used in the transfer message matches the address format and prefix of the
selectedFromChain. This prevents issues where an incorrect address could cause transaction failures.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
components/bank/forms/ibcSendForm.tsx (2)
159-160: Extract magic number to a named constant.The MIN_FEE_BUFFER value should be extracted to a named constant at the module level for better maintainability.
+const MIN_FEE_BUFFER = 0.09; const hasInsufficientBuffer = value > balance - MIN_FEE_BUFFER;
Line range hint
189-213: Document the timeout calculation and extract magic numbers.The timeout calculation uses magic numbers (1.2e6 and 1e6) without explanation. Consider extracting these to named constants and adding a comment explaining the timeout logic.
+// Timeout after 20 minutes (1.2e6 milliseconds) +const TIMEOUT_WINDOW_MS = 1.2e6; +// Convert to nanoseconds (1e6 nanoseconds per millisecond) +const MS_TO_NANOS = 1e6; const stamp = Date.now(); -const timeoutInNanos = (stamp + 1.2e6) * 1e6; +const timeoutInNanos = (stamp + TIMEOUT_WINDOW_MS) * MS_TO_NANOS;components/groups/components/myGroups.tsx (1)
284-285: Avoid duplication betweenurianduri_hashfieldsIn lines 284-285, both
urianduri_hashare assigned the same value, which is the URI of the asset logo. Ifuri_hashis intended to store a hash of the URI (e.g., for verification), consider computing the hash of the URI instead of assigning the URI itself.Apply this diff to compute an actual hash for
uri_hash:+ import crypto from 'crypto'; ... uri: assetInfo?.logo_URIs?.svg ?? assetInfo?.logo_URIs?.png ?? '', - uri_hash: assetInfo?.logo_URIs?.svg ?? assetInfo?.logo_URIs?.png ?? '', + uri_hash: assetInfo?.logo_URIs + ? crypto.createHash('sha256').update(assetInfo.logo_URIs.svg ?? assetInfo.logo_URIs.png).digest('hex') + : '',Ensure that you handle the import of the
cryptomodule correctly and that it's compatible with your build environment.components/bank/components/historyBox.tsx (1)
235-236: Avoid duplication betweenurianduri_hashfieldsSimilar to the previous file, in lines 235-236, both
urianduri_hashare assigned the same value. Ifuri_hashis meant to store a hash of the URI, compute the hash instead.Apply this diff:
uri: assetInfo?.logo_URIs?.svg ?? assetInfo?.logo_URIs?.png ?? '', - uri_hash: assetInfo?.logo_URIs?.svg ?? assetInfo?.logo_URIs?.png ?? '', + uri_hash: assetInfo?.logo_URIs + ? crypto.createHash('sha256').update(assetInfo.logo_URIs.svg ?? assetInfo.logo_URIs.png).digest('hex') + : '',Ensure the
cryptomodule is imported and available.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
components/bank/components/historyBox.tsx(4 hunks)components/bank/forms/ibcSendForm.tsx(11 hunks)components/bank/modals/txInfo.tsx(2 hunks)components/groups/components/myGroups.tsx(2 hunks)package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- components/bank/modals/txInfo.tsx
🔇 Additional comments (7)
components/bank/forms/ibcSendForm.tsx (5)
Line range hint
1-35: LGTM! Well-organized imports.The imports are properly structured and all necessary for the IBC transfer functionality.
Line range hint
37-81: LGTM! Well-structured props definition.Props are properly typed with Readonly and include comprehensive support for both individual and group IBC transfers.
83-102: Fix null reference error with useSearchParams.The useSearchParams hook may return null, leading to potential runtime errors.
87-92: LGTM! Well-implemented chain-specific hooks.The tx and fee estimation hooks are properly configured based on the selected chain.
276-529: LGTM! Well-implemented chain selection UI.The chain selection UI is well-structured with proper accessibility attributes and keyboard navigation support.
components/groups/components/myGroups.tsx (1)
14-14: ImportingdenomToAssetis appropriateThe import of
denomToAssetis necessary for processing IBC token denominations correctly.components/bank/components/historyBox.tsx (1)
4-4: ImportingdenomToAssetandenvis appropriateThe imports of
denomToAssetandenvare necessary for processing asset information and accessing environment variables.
| @@ -0,0 +1,232 @@ | |||
| import { DirectSecp256k1HdWallet } from '@cosmjs/proto-signing'; | |||
There was a problem hiding this comment.
Please add a comment on the purpose of this script
Co-authored-by: Félix C. Morency <[email protected]> Signed-off-by: Joseph Chalabi <[email protected]>
Co-authored-by: Félix C. Morency <[email protected]> Signed-off-by: Joseph Chalabi <[email protected]>
Co-authored-by: Félix C. Morency <[email protected]> Signed-off-by: Joseph Chalabi <[email protected]>
Co-authored-by: Félix C. Morency <[email protected]> Signed-off-by: Joseph Chalabi <[email protected]>
Co-authored-by: Félix C. Morency <[email protected]> Signed-off-by: Joseph Chalabi <[email protected]>
Co-authored-by: Félix C. Morency <[email protected]> Signed-off-by: Joseph Chalabi <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
hooks/useQueries.ts (2)
635-771: Consider extracting common hook logic to reduce duplication.The Osmosis hooks mirror their non-Osmosis counterparts with very similar implementations. Consider creating a higher-order hook factory to reduce code duplication.
Example implementation:
const createTokenBalancesHook = (useClient: () => { lcdQueryClient: any }, prefix: string) => (address: string) => { const { lcdQueryClient } = useClient(); const fetchBalances = async () => { if (!lcdQueryClient) { throw new Error('LCD Client not ready'); } return await lcdQueryClient.cosmos.bank.v1beta1.allBalances({ address, resolveDenom: false, }); }; const balancesQuery = useQuery({ queryKey: [`${prefix}Balances`, address], queryFn: fetchBalances, enabled: !!lcdQueryClient && !!address, staleTime: Infinity, }); return { balances: balancesQuery.data?.balances, isBalancesLoading: balancesQuery.isLoading, isBalancesError: balancesQuery.isError, refetchBalances: balancesQuery.refetch, }; }; // Usage: export const useTokenBalances = createTokenBalancesHook(useLcdQueryClient, ''); export const useTokenBalancesOsmosis = createTokenBalancesHook(useOsmosisLcdQueryClient, 'osmosis');
773-863: Enhance type safety and error handling in message formatting.While the message formatting implementation is solid, consider these improvements:
- Add type annotations for the message parameter
- Consider using a discriminated union type for different message types
- Add error handling for malformed messages
Example implementation:
type BaseMessage = { '@type': string; }; type SendMessage = BaseMessage & { '@type': '/cosmos.bank.v1beta1.MsgSend'; fromAddress: string; toAddress: string; amount: TransactionAmount[]; }; type MintMessage = BaseMessage & { '@type': '/osmosis.tokenfactory.v1beta1.MsgMint'; sender: string; mintToAddress: string; amount: TransactionAmount; }; // Add other message types... type Message = SendMessage | MintMessage; // | other message types const _formatMessage = ( message: Message, address: string ): { data: { tx_type: HistoryTxType; from_address: string; to_address: string; amount: TransactionAmount[]; }; }[] => { try { switch (message['@type']) { // ... existing cases ... default: console.warn(`Unsupported message type: ${message['@type']}`); return []; } } catch (error) { console.error('Error formatting message:', error); return []; } };components/react/modal.tsx (1)
97-108: Consider memoizing chain-specific values individually.While the current implementation works, memoizing individual chain values could prevent unnecessary re-renders when only one chain's state changes.
- const chainStates = useMemo(() => { - return Object.values(chains).map(chain => ({ - connect: chain.connect, - openView: chain.openView, - status: chain.status, - username: chain.username, - address: chain.address, - disconnect: chain.disconnect, - })); - }, [chains]); + const manifestChainState = useMemo(() => ({ + connect: chains?.manifesttestnet?.connect, + openView: chains?.manifesttestnet?.openView, + status: chains?.manifesttestnet?.status, + username: chains?.manifesttestnet?.username, + address: chains?.manifesttestnet?.address, + disconnect: chains?.manifesttestnet?.disconnect, + }), [chains?.manifesttestnet]); + + const osmosisChainState = useMemo(() => ({ + connect: chains?.osmosis?.connect, + openView: chains?.osmosis?.openView, + status: chains?.osmosis?.status, + username: chains?.osmosis?.username, + address: chains?.osmosis?.address, + disconnect: chains?.osmosis?.disconnect, + }), [chains?.osmosis]);pages/bank.tsx (1)
167-238: Refactor duplicated IBC token handling logic.The IBC token handling logic is duplicated between
combinedBalancesandcombinedOsmosisBalances. Consider extracting this into a shared utility function.+ const processIbcToken = (assetInfo: any, coreBalance: any) => { + const baseDenom = assetInfo?.traces?.[1]?.counterparty?.base_denom ?? coreBalance.denom; + return { + denom: baseDenom, + coreDenom: coreBalance.denom, + amount: coreBalance.amount, + metadata: { + description: assetInfo?.description ?? '', + denom_units: assetInfo?.denom_units?.map(unit => unit ? ({ + ...unit, + aliases: unit.aliases || [], + }) : null).filter(Boolean) ?? [], + base: assetInfo?.base ?? '', + display: assetInfo?.display ?? '', + name: assetInfo?.name ?? '', + symbol: assetInfo?.symbol ?? '', + uri: assetInfo?.logo_URIs?.svg ?? assetInfo?.logo_URIs?.png ?? undefined, + uri_hash: assetInfo?.logo_URIs?.svg ?? assetInfo?.logo_URIs?.png ?? undefined, + }, + }; + };Then use it in both places:
if (coreBalance.denom.startsWith('ibc/')) { const assetInfo = denomToAsset(env.chain, coreBalance.denom); - // ... existing implementation + return processIbcToken(assetInfo, coreBalance); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
components/groups/components/myGroups.tsx(2 hunks)components/react/modal.tsx(3 hunks)hooks/useQueries.ts(4 hunks)package.json(3 hunks)pages/_app.tsx(6 hunks)pages/bank.tsx(6 hunks)tests/mock.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/mock.ts
- package.json
- components/groups/components/myGroups.tsx
🧰 Additional context used
📓 Learnings (1)
pages/bank.tsx (1)
Learnt from: chalabi2
PR: liftedinit/manifest-app#194
File: pages/bank.tsx:257-268
Timestamp: 2025-01-11T03:29:20.686Z
Learning: In the bank page of manifest-app, only assets in the user's Manifest wallet are displayed in the main asset list with search functionality. IBC assets (including Osmosis balances) are handled separately within the ibcSendForm component.
🔇 Additional comments (13)
hooks/useQueries.ts (1)
5-5: LGTM! Import changes align with IBC support.The added imports for Osmosis LCD client and IBC denom normalization are necessary for the cross-chain functionality.
Also applies to: 7-7
components/react/modal.tsx (3)
39-40: LGTM! New imports support multi-chain functionality.The new imports enable access to chain configurations and multi-chain state management, aligning with the PR's objective of adding Osmosis chain support.
110-112: LGTM! Robust disconnect implementation.The concurrent disconnection of all chains using Promise.all is an efficient approach.
467-467: LGTM! Updated disconnect handler.The Connected view now uses the new multi-chain disconnect function.
pages/_app.tsx (5)
54-54: Track the TODO for future cleanup.The current manual asset list injection is a temporary solution. Once the chain registry is updated, this should be revisited to use the official registry.
Could you create a tracking issue to ensure this technical debt is addressed when the chain registry is updated?
72-94: LGTM! Comprehensive registry configuration.The signerOptions configuration correctly merges all necessary registries and amino converters, including the new IBC support.
192-195: LGTM! Endpoint configuration follows established pattern.The Osmosis testnet endpoint configuration correctly follows the same pattern as the Manifest chain, using environment variables for the URLs.
228-266: LGTM! Clean component hierarchy with proper provider order.The component structure maintains a clean hierarchy with proper provider order and responsive layout design. The addition of SkipProvider is well-placed for IBC transfer support.
205-207: Verify environment variable configuration.The ChainProvider configuration looks correct, but ensure that all required environment variables (
osmosisRpcUrl,osmosisApiUrl) are properly documented and configured in the deployment environment.Run this script to verify the environment variables:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check if required environment variables are defined in .env files # Test: Search for Osmosis-related environment variables rg -l "osmosisRpcUrl|osmosisApiUrl" .env*Length of output: 47
🏁 Script executed:
#!/bin/bash # Script: List all occurrences of osmosis-related environment variables in .env* files # This helps verify that both osmosisRpcUrl and osmosisApiUrl are properly configured. echo "Searching for osmosisRpcUrl and osmosisApiUrl in all .env* files..." rg -n "osmosisRpcUrl|osmosisApiUrl" .env*Length of output: 192
🏁 Script executed:
#!/bin/bash # Script: Search the entire repository for osmosis-related environment variable usage # This will help us determine if 'osmosisRpcUrl' and 'osmosisApiUrl' are referenced anywhere, # which might indicate they are defined or expected to be provided in another configuration file. echo "Searching repository for 'osmosisRpcUrl'..." rg -n "osmosisRpcUrl" . echo "-----------------------------------------" echo "Searching repository for 'osmosisApiUrl'..." rg -n "osmosisApiUrl" .Length of output: 852
Environment Variable Configuration Verified
The repository properly references
osmosisRpcUrlandosmosisApiUrlin its configuration viaprocess.env.NEXT_PUBLIC_OSMOSIS_RPC_URLandprocess.env.NEXT_PUBLIC_OSMOSIS_API_URL(see./config/env.tsand their usage inpages/_app.tsxand hooks). Although these values aren’t explicitly defined in the tracked.envfiles, they are expected to be provided by the deployment environment. Ensure that the deployment documentation clearly specifies these environment variables.pages/bank.tsx (4)
1-24: LGTM! Import changes for multi-chain support.The addition of Osmosis-specific hooks and the switch to
useChainsfor multi-chain support is well-structured.
310-323: 🛠️ Refactor suggestionAdd default values for Osmosis-related props.
The TokenList component receives new props without default values, which could lead to runtime errors.
<TokenList refetchBalances={refetchBalances || resolveRefetch} - refetchOsmosisBalances={refetchOsmosisBalances} - resolveOsmosisRefetch={resolveOsmosisRefetch} - isOsmosisBalancesLoading={isOsmosisBalancesLoading} - osmosisBalances={combinedOsmosisBalances ?? []} + refetchOsmosisBalances={refetchOsmosisBalances || (() => {})} + resolveOsmosisRefetch={resolveOsmosisRefetch || (() => {})} + isOsmosisBalancesLoading={isOsmosisBalancesLoading || false} + osmosisBalances={combinedOsmosisBalances ?? []} isLoading={isLoading} balances={combinedBalances} refetchHistory={refetchHistory} address={chains.manifesttestnet.address ?? ''} pageSize={tokenListPageSize} searchTerm={searchTerm} chains={chains} />Likely invalid or redundant comment.
129-153:⚠️ Potential issueImprove null handling in IBC token processing.
The current implementation has potential null reference issues when accessing nested properties.
if (coreBalance.denom.startsWith('ibc/')) { const assetInfo = denomToAsset(env.chain, coreBalance.denom); - const baseDenom = assetInfo?.traces?.[1]?.counterparty?.base_denom; + const baseDenom = assetInfo?.traces?.[1]?.counterparty?.base_denom ?? coreBalance.denom; return { - denom: baseDenom ?? '', // normalized denom (e.g., 'umfx') + denom: baseDenom, // normalized denom (e.g., 'umfx') coreDenom: coreBalance.denom, // full IBC trace amount: coreBalance.amount, metadata: { description: assetInfo?.description ?? '', denom_units: - assetInfo?.denom_units?.map(unit => ({ + assetInfo?.denom_units?.map(unit => unit ? ({ ...unit, aliases: unit.aliases || [], - })) ?? [], + }) : null).filter(Boolean) ?? [], base: assetInfo?.base ?? '', display: assetInfo?.display ?? '', name: assetInfo?.name ?? '', symbol: assetInfo?.symbol ?? '', - uri: assetInfo?.logo_URIs?.svg ?? assetInfo?.logo_URIs?.png ?? '', - uri_hash: assetInfo?.logo_URIs?.svg ?? assetInfo?.logo_URIs?.png ?? '', + uri: assetInfo?.logo_URIs?.svg ?? assetInfo?.logo_URIs?.png ?? undefined, + uri_hash: assetInfo?.logo_URIs?.svg ?? assetInfo?.logo_URIs?.png ?? undefined, }, }; }Likely invalid or redundant comment.
32-37: 🛠️ Refactor suggestionAdd error handling for wallet connections.
The wallet connection check should handle potential connection errors to improve user experience.
+ const [connectionError, setConnectionError] = useState<string | null>(null); const chains = useChains([env.chain, env.osmosisChain]); const isWalletConnected = useMemo( () => Object.values(chains).every(chain => chain.isWalletConnected), [chains] ); + + useEffect(() => { + if (!isWalletConnected) { + Object.values(chains).forEach(chain => { + chain.connect().catch(error => { + console.error(`Failed to connect to chain: ${chain.chain.chain_name}`, error); + setConnectionError(`Failed to connect to ${chain.chain.chain_name}`); + }); + }); + } + }, [chains, isWalletConnected]);Likely invalid or redundant comment.
| await Promise.all(chainStates.map(chain => chain.disconnect())); | ||
| }; | ||
|
|
||
| const current = chains?.manifesttestnet?.walletRepo?.current; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify chain identifier consistency.
Ensure that 'manifesttestnet' matches the chain identifier defined in the environment configuration.
🏁 Script executed:
#!/bin/bash
# Description: Check chain identifier consistency
# Search for chain identifier definitions
rg -A 2 "chain\s*=\s*['\"]manifesttestnet['\"]" Length of output: 53
🏁 Script executed:
#!/bin/bash
echo "=== Searching for 'manifesttestnet' across the entire repository ==="
rg -i "manifesttestnet" .
echo ""
echo "=== Searching for potential chain configuration definitions (e.g., using ':' or '=' operators) ==="
rg -i "chain\s*[:=]" .Length of output: 9679
Chain Identifier Inconsistency Detected
- The code in
components/react/modal.tsx(line 114) usesmanifesttestnet. - The environment configuration (e.g., in
.env.testviaNEXT_PUBLIC_CHAIN) is set tomanifest. - This discrepancy may lead to mismatches when fetching chain-related configurations or assets.
Please review and align the chain identifiers either by updating the environment variable to "manifesttestnet" or by modifying the code to match the configured value.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/render.tsx (1)
5-5: LGTM! Consider adding test configurations for chain-specific features.The integration of Osmosis chain and asset lists is well-structured. To enhance test coverage, consider adding test configurations for:
- Different chain combinations
- Asset list variations
- Error states
const defaultOptions = { chains: [defaultChain, osmosisChain], assetLists: [...defaultAssetLists, ...osmosisAssetList], + // Add test-specific configurations + testConfigs: { + skipChainValidation: false, + mockIBCTransfers: false, + simulateNetworkErrors: false + }, wallets: [], };Also applies to: 10-11
components/bank/components/__tests__/sendBox.test.tsx (1)
29-46: Consider enhancing the IbcSendForm mock with error states.While the mock provides basic chain selection functionality, it could be enhanced to simulate error conditions.
mock.module('@/components/bank/forms/ibcSendForm', () => ({ default: (props: any) => { return ( <div data-testid="ibc-send-form"> + {props.error && <div data-testid="error-message">{props.error}</div>} <div className="dropdown"> <label tabIndex={0} aria-label="from-chain-selector" className="btn"> {props.selectedFromChain?.name || 'Select Chain'} </label> + {props.isFromChainError && ( + <div data-testid="from-chain-error">Invalid selection</div> + )} </div> <div className="dropdown"> <label tabIndex={0} aria-label="to-chain-selector" className="btn"> {props.selectedToChain?.name || 'Select Chain'} </label> + {props.isToChainError && ( + <div data-testid="to-chain-error">Invalid selection</div> + )} </div> </div> ); }, }));components/bank/forms/__tests__/ibcSendForm.test.tsx (2)
20-35: Consider parameterizing chain configurations for better test maintainability.Extract chain configurations to a separate test utility file for reuse across tests.
+// testUtils/chainConfigs.ts +export const testChainConfigs = { + manifest: { + id: 'manifest', + name: 'Manifest', + icon: 'https://osmosis.zone/assets/icons/osmo-logo-icon.svg', + prefix: 'manifest', + chainID: 'manifest-1', + }, + osmosis: { + id: 'osmosistestnet', + name: 'Osmosis', + icon: 'https://osmosis.zone/assets/icons/osmo-logo-icon.svg', + prefix: 'osmo', + chainID: 'osmo-test-1', + }, +};
155-176: Add tests for chain selection error handling.Consider adding tests for:
- Network errors during chain selection
- Invalid chain configurations
- Chain validation failures
test('handles chain selection errors', async () => { const mockError = new Error('Chain validation failed'); renderWithProps({ setSelectedFromChain: jest.fn().mockRejectedValue(mockError) }); const fromChainSelector = screen.getByLabelText('from-chain-selector'); fireEvent.click(fromChainSelector); // Verify error handling expect(await screen.findByTestId('error-message')).toBeInTheDocument(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/bank/components/__tests__/sendBox.test.tsx(3 hunks)components/bank/forms/__tests__/ibcSendForm.test.tsx(4 hunks)tests/mock.ts(1 hunks)tests/render.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/mock.ts
🔇 Additional comments (6)
tests/render.tsx (1)
21-23: LGTM! Provider hierarchy is correctly structured.The SkipProvider is properly placed to wrap the ToastProvider, maintaining the correct context hierarchy.
components/bank/components/__tests__/sendBox.test.tsx (3)
21-26: LGTM! Image mock is properly implemented.The next/image mock correctly handles required props and maintains accessibility with alt text.
74-88: Enhance test coverage for cross-chain transfer edge cases.While the basic flow is tested, consider adding tests for:
- Invalid chain selections
- Network errors during chain switching
- Validation failures
90-98: LGTM! Chain selection test is well-structured.The test properly verifies the presence of chain selectors after mode switch.
components/bank/forms/__tests__/ibcSendForm.test.tsx (2)
37-65: LGTM! Props configuration is comprehensive.The default props properly cover all required IBC transfer functionality.
178-200: LGTM! Same chain prevention test is well-implemented.The test properly verifies that the same chain cannot be selected for both source and destination.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
components/bank/components/sendBox.tsx (1)
91-118: Enhance accessibility for tab buttons.Add ARIA roles and keyboard navigation for better accessibility.
+ const handleKeyDown = (e: React.KeyboardEvent) => { + if (e.key === 'ArrowLeft' || e.key === 'ArrowRight') { + setActiveTab(activeTab === 'send' ? 'cross-chain' : 'send'); + } + }; return ( - <div className="rounded-2xl w-full "> + <div className="rounded-2xl w-full" role="tablist"> <div className="flex mb-4 md:mb-6 w-full h-[3.5rem] rounded-xl p-1 bg-[#0000000A] dark:bg-[#FFFFFF0F]"> <button aria-label="send-tab" + role="tab" + aria-selected={activeTab === 'send'} + onKeyDown={handleKeyDown} className={`flex-1 py-2 px-4 text-sm font-medium rounded-xl transition-colors ${
♻️ Duplicate comments (1)
components/react/modal.tsx (1)
114-114:⚠️ Potential issueFix chain identifier inconsistency.
The code uses
manifesttestnetbut environment configuration usesmanifest. This mismatch could cause runtime errors.Apply this diff to fix the chain identifier:
- const current = chains?.manifesttestnet?.walletRepo?.current; + const current = chains?.[env.chain]?.walletRepo?.current;
🧹 Nitpick comments (6)
utils/format.ts (1)
34-50: LGTM! Consider caching asset info lookup.The IBC denom handling is well-implemented. For performance optimization, consider memoizing the asset info lookup if this function is called frequently with the same parameters.
+const assetInfoCache = new Map<string, Asset>(); + export function formatDenom(denom: string): string { - const assetInfo = denomToAsset(env.chain, denom); + const cacheKey = `${env.chain}:${denom}`; + let assetInfo = assetInfoCache.get(cacheKey); + if (!assetInfo) { + assetInfo = denomToAsset(env.chain, denom); + if (assetInfo) { + assetInfoCache.set(cacheKey, assetInfo); + } + }utils/ibc.ts (1)
19-25: Add type assertion for chain_name comparison.Consider adding type assertion to ensure chain_name is string for comparison.
const filterAssets = (chainName: string, assetList: AssetList[]): Asset[] => { return ( assetList - .find(({ chain_name }) => chain_name === chainName) + .find(({ chain_name }: { chain_name: string }) => chain_name === chainName) ?.assets?.filter(({ type_asset }) => type_asset === 'ics20' || !type_asset) || [] ); };components/bank/components/sendBox.tsx (1)
47-72: Consider extracting chain configuration to a separate file.The chain configuration could be moved to a dedicated configuration file for better maintainability.
+// chains.config.ts +export const chainConfig: IbcChain[] = [ + { + id: env.chain, + name: 'Manifest', + icon: 'logo.svg', + prefix: 'manifest', + chainID: env.chainId, + }, + // ... other chains +]; +// sendBox.tsx -const ibcChains = useMemo<IbcChain[]>( - () => [ - { - id: env.chain, - // ... chain config - }, - // ... other chains - ], - [] -); +const ibcChains = useMemo<IbcChain[]>(() => chainConfig, []);components/bank/forms/ibcSendForm.tsx (2)
87-95: Extract token display name formatting to a utility function.The token display name formatting logic is duplicated in multiple places.
+// utils/formatting.ts +export const formatTokenDisplayName = (displayName: string) => { + if (displayName.startsWith('factory')) { + return displayName.split('/').pop()?.toUpperCase(); + } + if (displayName.startsWith('u')) { + return displayName.slice(1).toUpperCase(); + } + return truncateString(displayName, 10).toUpperCase(); +}; -const formatTokenDisplayName = (displayName: string) => { - if (displayName.startsWith('factory')) { - return displayName.split('/').pop()?.toUpperCase(); - } - if (displayName.startsWith('u')) { - return displayName.slice(1).toUpperCase(); - } - return truncateString(displayName, 10).toUpperCase(); -};
169-185: Improve fee buffer configuration.The fee warning logic uses hardcoded values and only applies to 'umfx' tokens.
+// Constants for fee calculation +const FEE_CONFIG = { + MIN_FEE_BUFFER: 0.09, + FEE_TOKENS: ['umfx', 'uosmo'], + WARNING_MESSAGE: (token: string) => `Remember to leave ${token} for fees!`, +}; .test('leave-for-fees', '', function (value) { const { selectedToken } = this.parent; - if (!selectedToken || !value || selectedToken.denom !== 'umfx') return true; + if (!selectedToken || !value || !FEE_CONFIG.FEE_TOKENS.includes(selectedToken.denom)) { + return true; + } const exponent = selectedToken.metadata?.denom_units[1]?.exponent ?? 6; const balance = parseFloat(selectedToken.amount) / Math.pow(10, exponent); - const MIN_FEE_BUFFER = 0.09; - const hasInsufficientBuffer = value > balance - MIN_FEE_BUFFER; + const hasInsufficientBuffer = value > balance - FEE_CONFIG.MIN_FEE_BUFFER; if (hasInsufficientBuffer) { - setFeeWarning('Remember to leave tokens for fees!'); + setFeeWarning(FEE_CONFIG.WARNING_MESSAGE(selectedToken.denom)); } else { setFeeWarning(''); }.env.test (1)
6-13: Chain Configuration: Validate Variable Consistency and Intent
The chain configuration now supports Manifest, Osmosis, and Axelar testnets. Consider standardizing the formatting—specifically, review the use of quotes for chain IDs (e.g., NEXT_PUBLIC_CHAIN_ID and NEXT_PUBLIC_AXELAR_CHAIN_ID) to ensure consistency across all variables.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockbis excluded by!**/bun.lockb
📒 Files selected for processing (13)
.env.test(1 hunks)components/bank/components/sendBox.tsx(4 hunks)components/bank/forms/ibcSendForm.tsx(10 hunks)components/bank/handlers/createMessageUtils.tsx(2 hunks)components/factory/forms/TokenDetailsForm.tsx(1 hunks)components/react/modal.tsx(3 hunks)components/wallet.tsx(2 hunks)config/env.ts(1 hunks)package.json(3 hunks)pages/_app.tsx(6 hunks)pages/bank.tsx(6 hunks)utils/format.ts(2 hunks)utils/ibc.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- components/factory/forms/TokenDetailsForm.tsx
- package.json
🧰 Additional context used
📓 Learnings (1)
pages/bank.tsx (1)
Learnt from: chalabi2
PR: liftedinit/manifest-app#194
File: pages/bank.tsx:257-268
Timestamp: 2025-01-11T03:29:20.686Z
Learning: In the bank page of manifest-app, only assets in the user's Manifest wallet are displayed in the main asset list with search functionality. IBC assets (including Osmosis balances) are handled separately within the ibcSendForm component.
🔇 Additional comments (24)
components/bank/handlers/createMessageUtils.tsx (2)
17-17: LGTM! Good variable extraction.Extracting
formatDenom(denom)to a variable improves code readability.
2-2: Verify if thedenomToAssetimport is needed.The
denomToAssetfunction is imported but not used in this file. Consider removing it if it's not needed.Let's verify the usage:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for denomToAsset usage in the file rg "denomToAsset" "components/bank/handlers/createMessageUtils.tsx"Length of output: 151
Unused Import & Approving Variable Extraction
- The
denomToAssetimport is only found in the import statement and is not used anywhere in the file. Consider removing it if it's not needed.- The extraction of
formatDenom(denom)into a variable improves clarity and is approved.components/react/modal.tsx (3)
99-108: LGTM! Well-structured chain state management.The memoized
chainStatesimplementation efficiently extracts common properties from each chain and prevents unnecessary re-renders.
110-112: LGTM! Efficient concurrent disconnection.The
disconnectfunction efficiently handles multiple chain disconnections usingPromise.all.
97-97: Verify chain configuration consistency.The chain configuration uses environment variables. Ensure that
env.chain,env.osmosisChain, andenv.axelarChainare properly defined in the environment configuration.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check chain configuration in environment files echo "=== Searching for chain configuration in environment files ===" fd -e env -e env.local -e env.development -e env.production -e env.test --exec rg -l "chain|CHAIN" echo -e "\n=== Searching for chain-related environment variable usage ===" rg "env\.(chain|osmosisChain|axelarChain)"Length of output: 8955
Chain configuration consistency verified.
The search results confirm that the chain identifiers (env.chain,env.osmosisChain, andenv.axelarChain) are consistently referenced across multiple components, including the modal in question. There’s no evidence of inconsistency in their usage.config/env.ts (1)
1-34: LGTM! Well-organized environment configuration.The environment variables are logically grouped and consistently formatted. The use of comments to separate different categories (Wallet, Chains, Ops, Explorer URLs, RPC and API URLs) improves readability.
components/bank/components/sendBox.tsx (1)
77-85: LGTM! Chain selection validation is well-implemented.The useEffect hook properly handles the case when the same chain is selected for both source and destination.
pages/_app.tsx (5)
12-20: LGTM! Chain registry imports look good.The imports from the chain registry are correctly structured and include all necessary assets and chains for IBC support.
55-55: Address the TODO comment.The comment indicates that asset list injections are temporary until the chain registry is updated.
Would you like me to help track this technical debt by creating an issue to remove asset list injections once the chain registry is updated?
206-208: LGTM! Chain configuration looks good.The ChainProvider is correctly configured with all necessary chains and their respective asset lists.
229-268: LGTM! Component hierarchy and styling look good.The updated component hierarchy with SkipProvider wrapping ThemeProvider and ContactsModalProvider is correctly implemented.
193-196: Verify RPC and REST endpoints for Osmosis.The endpoint configuration for Osmosis looks correct, but let's verify the endpoints are accessible.
components/wallet.tsx (2)
38-88: LGTM! Multi-chain wallet management looks good.The implementation correctly handles multiple chains using the useChains hook and properly manages chain states.
59-70: LGTM! Chain status management looks good.The status computation correctly aggregates the state of all chains.
pages/bank.tsx (2)
312-315: 🛠️ Refactor suggestionAdd default values for Osmosis-related props.
The TokenList component receives new props without default values.
<TokenList refetchBalances={refetchBalances || resolveRefetch} - refetchOsmosisBalances={refetchOsmosisBalances} - resolveOsmosisRefetch={resolveOsmosisRefetch} - isOsmosisBalancesLoading={isOsmosisBalancesLoading} - osmosisBalances={combinedOsmosisBalances ?? []} + refetchOsmosisBalances={refetchOsmosisBalances || (() => {})} + resolveOsmosisRefetch={resolveOsmosisRefetch || (() => {})} + isOsmosisBalancesLoading={isOsmosisBalancesLoading || false} + osmosisBalances={combinedOsmosisBalances ?? []} isLoading={isLoading} balances={combinedBalances} refetchHistory={refetchHistory}Likely invalid or redundant comment.
129-153:⚠️ Potential issueAdd null checks for IBC token handling.
The current implementation has potential null reference issues when accessing nested properties.
if (coreBalance.denom.startsWith('ibc/')) { const assetInfo = denomToAsset(env.chain, coreBalance.denom); - const baseDenom = assetInfo?.traces?.[1]?.counterparty?.base_denom; + const baseDenom = assetInfo?.traces?.[1]?.counterparty?.base_denom ?? coreBalance.denom; return { - denom: baseDenom ?? '', // normalized denom (e.g., 'umfx') + denom: baseDenom, // normalized denom (e.g., 'umfx') coreDenom: coreBalance.denom, // full IBC trace amount: coreBalance.amount, metadata: { description: assetInfo?.description ?? '', denom_units: - assetInfo?.denom_units?.map(unit => ({ + assetInfo?.denom_units?.map(unit => unit ? ({ ...unit, aliases: unit.aliases || [], - })) ?? [], + }) : null).filter(Boolean) ?? [], base: assetInfo?.base ?? '', display: assetInfo?.display ?? '', name: assetInfo?.name ?? '', symbol: assetInfo?.symbol ?? '', - uri: assetInfo?.logo_URIs?.svg ?? assetInfo?.logo_URIs?.png ?? '', - uri_hash: assetInfo?.logo_URIs?.svg ?? assetInfo?.logo_URIs?.png ?? '', + uri: assetInfo?.logo_URIs?.svg ?? assetInfo?.logo_URIs?.png ?? undefined, + uri_hash: assetInfo?.logo_URIs?.svg ?? assetInfo?.logo_URIs?.png ?? undefined, }, }; }Likely invalid or redundant comment.
components/bank/forms/ibcSendForm.tsx (1)
861-863: 🛠️ Refactor suggestionRemove unimplemented function.
The
useSkipfunction is not implemented and appears to be unused.-function useSkip(getCosmosSigner: () => Promise<import('@cosmjs/amino').OfflineAminoSigner>) { - throw new Error('Function not implemented.'); -}Likely invalid or redundant comment.
.env.test (7)
1-5: Wallet Configuration: Ensure Test Keys Are Appropriately Scoped
The wallet section includes test keys and identifiers for WalletConnect and Web3Auth. Please double-check that these dummy values remain confined to the test environment and are not inadvertently promoted to production.
14-16: Ops and Environment Tier: Confirm Test Settings
The operational variable (NEXT_PUBLIC_CHAIN_TIER) is correctly set to testnet. Verify that this configuration aligns with your intended test environment and update as needed for production deployments.
17-19: Explorer URLs: Validate Endpoint Accessibility
The explorer endpoints for Manifest, Osmosis, and Axelar have been defined. Please ensure that these URLs point to the correct testnet explorers and are accessible, as they are critical for transaction tracking and debugging.
20-21: Indexer URL: Confirm Intentional Placeholder
The indexer URL is left empty in this test configuration. Ensure that this is deliberate and document the future plans (if any) for indexer integration so that it doesn’t cause confusion later.
22-24: RPC URLs (Main Chain): Placeholder Values Check
The main RPC and API URLs are currently unset. If these are intended to be provided dynamically or not needed for testnet operations, please confirm that the application logic correctly handles empty values.
25-27: Osmosis RPC URLs: Update When Available
The Osmosis API and RPC URLs are placeholders at this time. Verify whether these will be populated later or if additional default handling is implemented in the code; otherwise, update these values once the endpoints are defined.
28-30: Axelar RPC URLs: Ensure Future Configuration
Similar to the Osmosis configuration, the Axelar API and RPC URLs are empty. Confirm that there’s a plan to update these values when integrating with the Axelar testnet, avoiding potential connectivity issues.
| export const getIbcDenom = (chainName: string, denom: string) => { | ||
| const allAssets = getAllAssets(chainName); | ||
|
|
||
| // Find the asset that has this denom as its counterparty base_denom | ||
| const ibcAsset = allAssets.find(asset => asset.traces?.[0]?.counterparty?.base_denom === denom); | ||
|
|
||
| // Return the IBC hash (base) if found | ||
| return ibcAsset?.base; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for chainName parameter.
The function should validate the chainName parameter to prevent undefined behavior.
export const getIbcDenom = (chainName: string, denom: string) => {
+ if (!chainName) {
+ throw new Error('chainName is required');
+ }
const allAssets = getAllAssets(chainName);
// Find the asset that has this denom as its counterparty base_denom
const ibcAsset = allAssets.find(asset => asset.traces?.[0]?.counterparty?.base_denom === denom);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getIbcDenom = (chainName: string, denom: string) => { | |
| const allAssets = getAllAssets(chainName); | |
| // Find the asset that has this denom as its counterparty base_denom | |
| const ibcAsset = allAssets.find(asset => asset.traces?.[0]?.counterparty?.base_denom === denom); | |
| // Return the IBC hash (base) if found | |
| return ibcAsset?.base; | |
| }; | |
| export const getIbcDenom = (chainName: string, denom: string) => { | |
| if (!chainName) { | |
| throw new Error('chainName is required'); | |
| } | |
| const allAssets = getAllAssets(chainName); | |
| // Find the asset that has this denom as its counterparty base_denom | |
| const ibcAsset = allAssets.find(asset => asset.traces?.[0]?.counterparty?.base_denom === denom); | |
| // Return the IBC hash (base) if found | |
| return ibcAsset?.base; | |
| }; |
| export const denomToAsset = (chainName: string, denom: string) => { | ||
| return getAllAssets(chainName).find(asset => asset.base === denom); | ||
| const allAssets = getAllAssets(chainName); | ||
|
|
||
| // Only handle IBC hashes | ||
| if (denom.startsWith('ibc/')) { | ||
| // Find the asset that has this IBC hash as its base | ||
| const asset = allAssets.find(asset => asset.base === denom); | ||
| if (asset?.traces?.[0]?.counterparty?.base_denom) { | ||
| // Return the original denom from the counterparty chain | ||
| return { | ||
| ...asset, | ||
| base: asset.traces[0].counterparty.base_denom, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| // Return original asset if not an IBC hash | ||
| return allAssets.find(asset => asset.base === denom); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for invalid IBC denoms.
The function should handle malformed IBC denoms gracefully.
export const denomToAsset = (chainName: string, denom: string) => {
+ if (!denom) {
+ return undefined;
+ }
const allAssets = getAllAssets(chainName);
// Only handle IBC hashes
if (denom.startsWith('ibc/')) {
+ if (denom.length < 5) { // "ibc/" + at least 1 char
+ return undefined;
+ }
// Find the asset that has this IBC hash as its base
const asset = allAssets.find(asset => asset.base === denom);
if (asset?.traces?.[0]?.counterparty?.base_denom) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const denomToAsset = (chainName: string, denom: string) => { | |
| return getAllAssets(chainName).find(asset => asset.base === denom); | |
| const allAssets = getAllAssets(chainName); | |
| // Only handle IBC hashes | |
| if (denom.startsWith('ibc/')) { | |
| // Find the asset that has this IBC hash as its base | |
| const asset = allAssets.find(asset => asset.base === denom); | |
| if (asset?.traces?.[0]?.counterparty?.base_denom) { | |
| // Return the original denom from the counterparty chain | |
| return { | |
| ...asset, | |
| base: asset.traces[0].counterparty.base_denom, | |
| }; | |
| } | |
| } | |
| // Return original asset if not an IBC hash | |
| return allAssets.find(asset => asset.base === denom); | |
| }; | |
| export const denomToAsset = (chainName: string, denom: string) => { | |
| if (!denom) { | |
| return undefined; | |
| } | |
| const allAssets = getAllAssets(chainName); | |
| // Only handle IBC hashes | |
| if (denom.startsWith('ibc/')) { | |
| if (denom.length < 5) { // "ibc/" + at least 1 char | |
| return undefined; | |
| } | |
| // Find the asset that has this IBC hash as its base | |
| const asset = allAssets.find(asset => asset.base === denom); | |
| if (asset?.traces?.[0]?.counterparty?.base_denom) { | |
| // Return the original denom from the counterparty chain | |
| return { | |
| ...asset, | |
| base: asset.traces[0].counterparty.base_denom, | |
| }; | |
| } | |
| } | |
| // Return original asset if not an IBC hash | |
| return allAssets.find(asset => asset.base === denom); | |
| }; |
| const connect = async () => { | ||
| await Promise.all(chainStates.map(chain => chain.connect())); | ||
| }; | ||
|
|
||
| const openView = () => { | ||
| chainStates[0]?.openView(); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for chain connection.
The connect function should handle potential connection failures.
const connect = async () => {
- await Promise.all(chainStates.map(chain => chain.connect()));
+ try {
+ await Promise.all(chainStates.map(chain => chain.connect()));
+ } catch (error) {
+ console.error('Failed to connect to chains:', error);
+ throw new Error('Failed to connect to one or more chains');
+ }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const connect = async () => { | |
| await Promise.all(chainStates.map(chain => chain.connect())); | |
| }; | |
| const openView = () => { | |
| chainStates[0]?.openView(); | |
| }; | |
| const connect = async () => { | |
| try { | |
| await Promise.all(chainStates.map(chain => chain.connect())); | |
| } catch (error) { | |
| console.error('Failed to connect to chains:', error); | |
| throw new Error('Failed to connect to one or more chains'); | |
| } | |
| }; | |
| const openView = () => { | |
| chainStates[0]?.openView(); | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
components/wallet.tsx (1)
222-224: Update IconWallet and WalletNotConnected components for multi-chain support.These components still use the single-chain approach (
useChain) whileWalletSectionhas been updated to support multiple chains. This inconsistency could lead to maintenance issues and confusion.Consider updating these components to use the same multi-chain approach as
WalletSection.Also applies to: 333-334
🧹 Nitpick comments (6)
components/wallet.tsx (1)
40-40: Extract chain configuration to a constant or configuration file.Consider moving the chain array to a configuration constant or file for better maintainability and reusability.
- const chains = useChains([env.chain, env.osmosisChain, env.axelarChain]); + const SUPPORTED_CHAINS = [env.chain, env.osmosisChain, env.axelarChain] as const; + const chains = useChains(SUPPORTED_CHAINS);components/bank/forms/__tests__/ibcSendForm.test.tsx (4)
20-35: Move chain configuration to a separate mock file.The chain configuration data should be moved to a dedicated mock file (e.g.,
@/tests/mockChains.ts) to improve maintainability and reusability across test files.
24-24: Update chain icons to use distinct values.Both Manifest and Osmosis chains are using the same icon URL. Consider using distinct icons to better reflect the actual implementation.
- icon: 'https://osmosis.zone/assets/icons/osmo-logo-icon.svg', + icon: 'https://manifest.zone/assets/icons/manifest-logo-icon.svg',Also applies to: 31-31
90-92: Improve debug logging in CI.Consider using a more structured debug output that includes the test name and expectations:
- console.log('Container HTML:', container.innerHTML); + console.log(` + Test: renders form with correct details + Expected elements: + - IBC send form + Actual HTML: + ${container.innerHTML} + `);
172-218: Refactor duplicate chain selection logic.The chain selection logic is duplicated between
handles chain selection correctlyandprevents selecting same chain for source and destinationtests. Consider extracting this into a helper function:const selectFromChain = async (chainName: string) => { const fromChainSelector = screen.getByLabelText('from-chain-selector'); fireEvent.click(fromChainSelector); const options = screen.getAllByRole('option', { name: chainName }); const enabledOption = options.find( option => !option.className.includes('opacity-50') ); fireEvent.click(enabledOption!); };tests/render.tsx (1)
18-22: Consider documenting the chain order significance.The configuration looks good and supports multiple chains as intended. Consider adding a comment to explain if the order of chains in the array is significant (e.g., if the first chain is considered the default).
const defaultOptions = { + // Order matters: First chain is considered the default chains: [manifestChain, osmosisChain, axelarChain], assetLists: [manifestAssets, osmosisAssets, axelarAssets], wallets: [], };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/bank/forms/__tests__/ibcSendForm.test.tsx(3 hunks)components/wallet.tsx(2 hunks)package.json(3 hunks)tests/render.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🔇 Additional comments (7)
components/wallet.tsx (3)
53-55: Add error handling for chain connection.The connect function should handle potential connection failures.
57-59: Clarify the behavior of openView function.The function only opens the view for the first chain, which might be confusing for users. Consider either:
- Opening views for all chains
- Adding a parameter to specify which chain's view to open
- Adding a comment explaining why only the first chain's view is opened
74-84: Review the strategy for selecting user information.The current implementation takes the first available username/address from any chain. This might lead to inconsistencies if different chains have different user information. Consider:
- Using the primary chain's information consistently
- Showing information from all chains
- Adding a comment explaining the selection strategy
components/bank/forms/__tests__/ibcSendForm.test.tsx (2)
78-81: LGTM! Good test hygiene.Adding
jest.clearAllMocks()to the cleanup is a good practice to ensure proper test isolation.
83-96: Document why the test is skipped.The test is marked with
test.skipwithout explanation. Either:
- Add a comment explaining why it's skipped and create a tracking issue, or
- Fix and enable the test if it's no longer needed to be skipped
tests/render.tsx (2)
6-16: LGTM! Verify testnet package versions.The imports are well-organized and align with the PR objectives for cross-chain support.
Let's verify we're using the latest versions of the testnet packages:
✅ Verification successful
🌐 Web query:
What is the latest version of chain-registry npm package?Length of output: 2232
Testnet Package Versions Verified
The test file's imports from
chain-registry/testnet/...are correctly aligned with the latest release. The web query confirms that thechain-registrypackage is at version 1.69.114, so the usage intests/render.tsxis up-to-date.
24-37: Consider adding tests for SkipProvider integration.The updated provider hierarchy looks good. Since SkipProvider is a new addition, ensure it's properly tested in the context of this render utility.
Let's check if there are tests for the SkipProvider:
| test('handles chain selection correctly', async () => { | ||
| renderWithProps(); | ||
|
|
||
| const fromChainSelector = screen.getByLabelText('from-chain-selector'); | ||
| fireEvent.click(fromChainSelector); | ||
|
|
||
| const manifestOptions = screen.getAllByRole('option', { name: 'Manifest' }); | ||
| const enabledManifestOption = manifestOptions.find( | ||
| option => !option.className.includes('opacity-50') | ||
| ); | ||
| fireEvent.click(enabledManifestOption!); | ||
|
|
||
| expect(screen.getByLabelText('from-chain-selector')).toHaveTextContent('Manifest'); | ||
|
|
||
| const toChainSelector = screen.getByLabelText('to-chain-selector'); | ||
| fireEvent.click(toChainSelector); | ||
|
|
||
| const osmosisOption = screen.getAllByRole('option', { name: 'Osmosis' }); | ||
| fireEvent.click(osmosisOption[0]); | ||
|
|
||
| expect(screen.getByLabelText('to-chain-selector')).toHaveTextContent('Osmosis'); | ||
| }); | ||
|
|
||
| test('prevents selecting same chain for source and destination', async () => { | ||
| renderWithProps(); | ||
|
|
||
| const fromChainSelector = screen.getByLabelText('from-chain-selector'); | ||
| fireEvent.click(fromChainSelector); | ||
|
|
||
| const manifestOptions = screen.getAllByRole('option', { name: 'Manifest' }); | ||
| const enabledManifestOption = manifestOptions.find( | ||
| option => !option.className.includes('opacity-50') | ||
| ); | ||
| fireEvent.click(enabledManifestOption!); | ||
|
|
||
| // Verify Manifest is not available in destination chain options | ||
| const toChainSelector = screen.getByLabelText('to-chain-selector'); | ||
| fireEvent.click(toChainSelector); | ||
|
|
||
| // Check that there's only one active Manifest option (the source) | ||
| const activeManifestOptions = screen.getAllByRole('option', { name: 'Manifest' }); | ||
| const activeManifestOption = activeManifestOptions.find( | ||
| option => !option.className.includes('opacity-50') | ||
| ); | ||
| expect(activeManifestOption).toBeDefined(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test cases for error scenarios.
The test suite is missing coverage for important error scenarios:
- Invalid addresses
- Insufficient balances
- Network errors during chain selection
- Failed transactions
Consider adding these test cases to ensure robust error handling.
Would you like me to help generate the missing test cases?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
components/bank/forms/__tests__/ibcSendForm.test.tsx (3)
20-35: Move chain configuration to a separate mock file.Consider extracting the
defaultChainsconfiguration to a separate mock file (e.g.,@/tests/mockChains.ts) to improve maintainability and reusability across test files.
24-24: Update chain icons to use distinct values.Both Manifest and Osmosis chains are using the same icon URL. Consider using distinct mock icons for better test clarity.
- icon: 'https://osmosis.zone/assets/icons/osmo-logo-icon.svg', + icon: 'https://manifest.example.com/assets/icons/manifest-logo-icon.svg',Also applies to: 31-31
207-207: Use optional chaining for safer property access.Use optional chaining to safely access the textContent property.
- return link && link.textContent?.includes('Manifest'); + return link?.textContent?.includes('Manifest');🧰 Tools
🪛 Biome (1.9.4)
[error] 207-207: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/bank/forms/__tests__/ibcSendForm.test.tsx(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
components/bank/forms/__tests__/ibcSendForm.test.tsx
[error] 207-207: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 GitHub Actions: Test and Coverage
components/bank/forms/__tests__/ibcSendForm.test.tsx
[error] 1-1: Ignored nodes: comments, script, style
🔇 Additional comments (1)
components/bank/forms/__tests__/ibcSendForm.test.tsx (1)
73-213: Add test coverage for error scenarios.The test suite is missing coverage for important error scenarios:
- Invalid addresses
- Insufficient balances
- Network errors during chain selection
- Failed transactions
Would you like me to help generate the missing test cases?
🧰 Tools
🪛 Biome (1.9.4)
[error] 207-207: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
components/bank/components/__tests__/tokenList.test.tsx (1)
1-168:🧩 Analysis chain
Add test coverage for new Osmosis-related functionality.
Based on the AI summary, the
TokenListcomponent has been updated with several new Osmosis-related props (osmosisBalances,isOsmosisBalancesLoading,refetchOsmosisBalances, etc.), but the test suite doesn't cover these new features. Please add test cases to verify:
- Rendering of Osmosis balances
- Loading states for Osmosis data
- Refetch functionality for Osmosis balances
- Integration with the new
chainspropLet me verify the new props in the TokenList component:
🏁 Script executed:
#!/bin/bash # Description: Verify the new Osmosis-related props in TokenList component # Search for the TokenList component definition ast-grep --pattern 'interface TokenListProps { $$$ }'Length of output: 1307
Missing test coverage for new Osmosis functionality
The new Osmosis-related props (such as
osmosisBalances,isOsmosisBalancesLoading,refetchOsmosisBalances,resolveOsmosisRefetch, andchains) have been added to theTokenListcomponent. However, the current test suite incomponents/bank/components/__tests__/tokenList.test.tsxdoes not include any test cases that verify:
- Correct rendering of Osmosis balances when provided.
- Display of loading states when
isOsmosisBalancesLoadingis true.- Proper functionality of the
refetchOsmosisBalances(and optionallyresolveOsmosisRefetch) callback.- Integration of the new
chainsprop.Actionable Items:
- Add a test case to render the
TokenListwith non-emptyosmosisBalancesand verify the correct presentation.- Create a test to ensure that the loading skeleton (or equivalent indicator) appears when
isOsmosisBalancesLoadingis true.- Implement a test that simulates a trigger for
refetchOsmosisBalancesand potentiallyresolveOsmosisRefetch, confirming they are called.- Include a test case that verifies proper handling/integration of the
chainsprop.
🧹 Nitpick comments (4)
components/bank/forms/__tests__/sendForm.test.tsx (1)
81-95: Improve test readability and reliability.The current test implementation could be more maintainable. Consider:
- Breaking down the test into smaller, focused test cases
- Using test helpers for common setup operations
- Following the Arrange-Act-Assert pattern more explicitly
Here's a suggested refactor:
- test('send button is enabled when inputs are valid', () => { + test('enables send button when all inputs are valid', () => { + // Arrange renderWithProps(); + const recipientInput = screen.getByPlaceholderText('Enter address'); + const amountInput = screen.getByPlaceholderText('0.00'); + const tokenSelector = screen.getByLabelText('token-selector'); + + // Act + fireEvent.change(recipientInput, { + target: { value: 'manifest1recipient' }, + }); + fireEvent.change(amountInput, { + target: { value: '100' }, + }); - fireEvent.change(screen.getByPlaceholderText('Enter address'), { - target: { value: 'manifest1recipient' }, - }); - fireEvent.change(screen.getByPlaceholderText('0.00'), { - target: { value: '100' }, - }); - const tokenSelector = screen.getByLabelText('token-selector'); fireEvent.click(tokenSelector); - const dropdownItems = screen.getAllByText('TOKEN 1'); - fireEvent.click(dropdownItems[dropdownItems.length - 1]); + const tokenOption = screen.getAllByText('TOKEN 1')[0]; + fireEvent.click(tokenOption); + + // Assert const sendButton = screen.getByRole('button', { name: 'send-btn' }); expect(sendButton).not.toBeDisabled(); });components/bank/forms/__tests__/ibcSendForm.test.tsx (3)
1-78: LGTM! Consider extracting test data to a separate file.The mock setup and helper function are well-structured. The defaultChains array and defaultProps object provide comprehensive test data.
Consider extracting the
defaultChainsarray to a separate test data file (e.g.,__mocks__/chains.ts) to improve reusability across test files and maintain consistency.
168-189: Enhance chain selection test coverage.While the test verifies basic chain selection functionality, consider adding assertions for:
- Callback functions (
setSelectedFromChain,setSelectedToChain)- Chain address updates
test('handles chain selection correctly', async () => { - renderWithProps(); + const setSelectedFromChain = jest.fn(); + const setSelectedToChain = jest.fn(); + renderWithProps({ setSelectedFromChain, setSelectedToChain }); const fromChainSelector = screen.getByLabelText('from-chain-selector'); fireEvent.click(fromChainSelector); // ... existing code ... + expect(setSelectedFromChain).toHaveBeenCalledWith(expect.objectContaining({ + name: 'Manifest' + })); + expect(setSelectedToChain).toHaveBeenCalledWith(expect.objectContaining({ + name: 'Osmosis' + })); });
191-236: LGTM! Consider these minor improvements.The test effectively verifies the prevention of same chain selection. Two suggestions for improvement:
- Use optional chaining for safer property access
- Add assertions for error messages or tooltips shown to users when attempting to select the same chain
-return link && link.textContent?.includes('Manifest'); +return link?.textContent?.includes('Manifest');🧰 Tools
🪛 Biome (1.9.4)
[error] 229-229: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.env.test(1 hunks)components/bank/components/__tests__/sendBox.test.tsx(3 hunks)components/bank/components/__tests__/tokenList.test.tsx(1 hunks)components/bank/forms/__tests__/ibcSendForm.test.tsx(4 hunks)components/bank/forms/__tests__/sendForm.test.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- components/bank/components/tests/sendBox.test.tsx
- .env.test
🧰 Additional context used
🪛 Biome (1.9.4)
components/bank/forms/__tests__/ibcSendForm.test.tsx
[error] 229-229: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (5)
components/bank/forms/__tests__/sendForm.test.tsx (2)
38-41: LGTM! Enhanced test cleanup implementation.The addition of
mock.restore()aftercleanup()improves test isolation by ensuring both DOM elements and mocked modules are properly reset between tests.
24-31: Add test coverage for IBC transfer functionality.While the test props include Osmosis chain configuration, there are no test cases verifying IBC transfer behavior. Consider adding tests for:
- IBC transfer form validation
- Chain selection handling
- Cross-chain address validation
- IBC transfer specific error cases
Would you like me to provide example test cases for these scenarios?
components/bank/components/__tests__/tokenList.test.tsx (1)
59-62: LGTM! Good practice adding mock cleanup.The addition of
mock.restore()in theafterEachblock is a good practice as it ensures proper cleanup of mocked modules between tests, preventing potential test interference.components/bank/forms/__tests__/ibcSendForm.test.tsx (2)
80-92: LGTM! Test setup and basic rendering test are well-structured.The cleanup is thorough with
mock.restore(), and the basic rendering test verifies essential form elements.
94-166: Add test cases for error scenarios.The test suite is missing coverage for important error scenarios:
- Invalid addresses
- Insufficient balances
- Network errors during chain selection
- Failed transactions
Would you like me to help generate the missing test cases?
| @@ -31,6 +31,10 @@ NEXT_PUBLIC_RPC_URL= | |||
| NEXT_PUBLIC_API_URL= | |||
| NEXT_PUBLIC_EXPLORER_URL= | |||
| NEXT_PUBLIC_INDEXER_URL= | |||
| NEXT_PUBLIC_OSMOSIS_CHAIN_ID= | |||
| @@ -45,6 +49,10 @@ where | |||
| - `NEXT_PUBLIC_API_URL` is the chain API URL | |||
| - `NEXT_PUBLIC_EXPLORER_URL` is the block explorer URL | |||
| - `NEXT_PUBLIC_INDEXER_URL` is the YACI indexer URL | |||
| - `NEXT_PUBLIC_OSMOSIS_CHAIN_ID` is the osmosis chain ID | |||
| { | ||
| id: 'manifest', | ||
| name: 'Manifest', | ||
| icon: 'https://osmosis.zone/assets/icons/osmo-logo-icon.svg', |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
components/bank/forms/ibcSendForm.tsx (4)
87-95: 🛠️ Refactor suggestionExtract duplicated token display name formatting logic.
The token display name formatting logic is duplicated in multiple places (lines 682, 730-732, 768-770). Extract this into a reusable utility function to improve maintainability.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 89-89: components/bank/forms/ibcSendForm.tsx#L89
Added line #L89 was not covered by tests
[warning] 92-92: components/bank/forms/ibcSendForm.tsx#L92
Added line #L92 was not covered by tests
119-123:⚠️ Potential issueAdd missing dependencies to useEffect.
The useEffect hook's dependency array is missing
ibcChains. This could lead to stale closures ifibcChainschanges.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 121-121: components/bank/forms/ibcSendForm.tsx#L121
Added line #L121 was not covered by tests
218-262:⚠️ Potential issueAdd error handling for Skip protocol integration.
The Skip protocol integration lacks proper error handling and validation of the route response.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 216-262: components/bank/forms/ibcSendForm.tsx#L216-L262
Added lines #L216 - L262 were not covered by tests
866-868: 🛠️ Refactor suggestionRemove unimplemented function.
The
useSkipfunction is not implemented and appears to be unused. Consider removing it.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 866-866: components/bank/forms/ibcSendForm.tsx#L866
Added line #L866 was not covered by tests
🧹 Nitpick comments (2)
components/bank/forms/ibcSendForm.tsx (2)
297-348: Remove commented code.Large blocks of commented code should be removed if they are no longer needed. If they are kept for reference, add a comment explaining why.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 296-348: components/bank/forms/ibcSendForm.tsx#L296-L348
Added lines #L296 - L348 were not covered by tests
457-483: Extract duplicated chain selection event handlers.The chain selection event handlers for keyboard and click events are duplicated. Extract these into a reusable function to reduce code duplication and improve maintainability.
Apply this diff to extract the event handlers:
+const handleChainSelection = ( + chain: IbcChain, + selectedChain: IbcChain, + setSelectedChain: (chain: IbcChain) => void, + event: React.MouseEvent | React.KeyboardEvent +) => { + if (chain.id === selectedChain.id) { + return; + } + setSelectedChain(chain); + // Get the dropdown element and remove focus + const dropdown = (event.target as HTMLElement).closest('.dropdown'); + if (dropdown) { + (dropdown as HTMLElement).removeAttribute('open'); + (dropdown.querySelector('label') as HTMLElement)?.focus(); + (dropdown.querySelector('label') as HTMLElement)?.blur(); + } +}; -onClick={e => { - if (chain.id === selectedFromChain.id) { - return; - } - setSelectedFromChain(chain); - // Get the dropdown element and remove focus - const dropdown = (e.target as HTMLElement).closest('.dropdown'); - if (dropdown) { - (dropdown as HTMLElement).removeAttribute('open'); - (dropdown.querySelector('label') as HTMLElement)?.focus(); - (dropdown.querySelector('label') as HTMLElement)?.blur(); - } -}} +onClick={e => handleChainSelection(chain, selectedFromChain, setSelectedFromChain, e)} -onKeyDown={e => { - if (e.key === 'Enter' || e.key === ' ') { - e.preventDefault(); - if (chain.id === selectedFromChain.id) { - return; - } - setSelectedFromChain(chain); - // Get the dropdown element and remove focus - const dropdown = (e.target as HTMLElement).closest('.dropdown'); - if (dropdown) { - (dropdown as HTMLElement).removeAttribute('open'); - (dropdown.querySelector('label') as HTMLElement)?.focus(); - (dropdown.querySelector('label') as HTMLElement)?.blur(); - } - } -}} +onKeyDown={e => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + handleChainSelection(chain, selectedFromChain, setSelectedFromChain, e); + } +}}Also applies to: 600-613
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 457-467: components/bank/forms/ibcSendForm.tsx#L457-L467
Added lines #L457 - L467 were not covered by tests
[warning] 470-483: components/bank/forms/ibcSendForm.tsx#L470-L483
Added lines #L470 - L483 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/bank/forms/ibcSendForm.tsx(10 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
components/bank/forms/ibcSendForm.tsx
[warning] 89-89: components/bank/forms/ibcSendForm.tsx#L89
Added line #L89 was not covered by tests
[warning] 92-92: components/bank/forms/ibcSendForm.tsx#L92
Added line #L92 was not covered by tests
[warning] 109-110: components/bank/forms/ibcSendForm.tsx#L109-L110
Added lines #L109 - L110 were not covered by tests
[warning] 121-121: components/bank/forms/ibcSendForm.tsx#L121
Added line #L121 was not covered by tests
[warning] 176-177: components/bank/forms/ibcSendForm.tsx#L176-L177
Added lines #L176 - L177 were not covered by tests
[warning] 179-182: components/bank/forms/ibcSendForm.tsx#L179-L182
Added lines #L179 - L182 were not covered by tests
[warning] 185-185: components/bank/forms/ibcSendForm.tsx#L185
Added line #L185 was not covered by tests
[warning] 206-206: components/bank/forms/ibcSendForm.tsx#L206
Added line #L206 was not covered by tests
[warning] 216-262: components/bank/forms/ibcSendForm.tsx#L216-L262
Added lines #L216 - L262 were not covered by tests
[warning] 265-267: components/bank/forms/ibcSendForm.tsx#L265-L267
Added lines #L265 - L267 were not covered by tests
[warning] 269-293: components/bank/forms/ibcSendForm.tsx#L269-L293
Added lines #L269 - L293 were not covered by tests
[warning] 296-348: components/bank/forms/ibcSendForm.tsx#L296-L348
Added lines #L296 - L348 were not covered by tests
[warning] 381-382: components/bank/forms/ibcSendForm.tsx#L381-L382
Added lines #L381 - L382 were not covered by tests
[warning] 385-387: components/bank/forms/ibcSendForm.tsx#L385-L387
Added lines #L385 - L387 were not covered by tests
[warning] 389-401: components/bank/forms/ibcSendForm.tsx#L389-L401
Added lines #L389 - L401 were not covered by tests
[warning] 457-467: components/bank/forms/ibcSendForm.tsx#L457-L467
Added lines #L457 - L467 were not covered by tests
[warning] 470-483: components/bank/forms/ibcSendForm.tsx#L470-L483
Added lines #L470 - L483 were not covered by tests
[warning] 516-519: components/bank/forms/ibcSendForm.tsx#L516-L519
Added lines #L516 - L519 were not covered by tests
[warning] 587-591: components/bank/forms/ibcSendForm.tsx#L587-L591
Added lines #L587 - L591 were not covered by tests
[warning] 600-613: components/bank/forms/ibcSendForm.tsx#L600-L613
Added lines #L600 - L613 were not covered by tests
[warning] 866-866: components/bank/forms/ibcSendForm.tsx#L866
Added line #L866 was not covered by tests
🔇 Additional comments (2)
components/bank/forms/ibcSendForm.tsx (2)
169-185: Improve fee buffer configuration and documentation.The fee warning logic uses a hardcoded buffer value and only applies to 'umfx' tokens.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 176-177: components/bank/forms/ibcSendForm.tsx#L176-L177
Added lines #L176 - L177 were not covered by tests
[warning] 179-182: components/bank/forms/ibcSendForm.tsx#L179-L182
Added lines #L179 - L182 were not covered by tests
[warning] 185-185: components/bank/forms/ibcSendForm.tsx#L185
Added line #L185 was not covered by tests
216-293: Add test coverage for Skip protocol integration.The Skip protocol integration code lacks test coverage. Add unit tests to verify the functionality and error handling.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 216-262: components/bank/forms/ibcSendForm.tsx#L216-L262
Added lines #L216 - L262 were not covered by tests
[warning] 265-267: components/bank/forms/ibcSendForm.tsx#L265-L267
Added lines #L265 - L267 were not covered by tests
[warning] 269-293: components/bank/forms/ibcSendForm.tsx#L269-L293
Added lines #L269 - L293 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
components/bank/forms/ibcSendForm.tsx (2)
216-262:⚠️ Potential issueAdd error handling for Skip protocol integration.
The Skip protocol integration lacks proper error handling and validation of the route response.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 216-262: components/bank/forms/ibcSendForm.tsx#L216-L262
Added lines #L216 - L262 were not covered by tests
119-123:⚠️ Potential issueAdd missing dependencies to useEffect.
The useEffect hook's dependency array is missing
ibcChains.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 121-121: components/bank/forms/ibcSendForm.tsx#L121
Added line #L121 was not covered by tests
🧹 Nitpick comments (1)
components/bank/components/tokenList.tsx (1)
20-24: Add default values for optional props.The new optional props for Osmosis integration should have default values in the destructuring to prevent potential undefined errors.
- osmosisBalances?: CombinedBalanceInfo[] | undefined; - isOsmosisBalancesLoading?: boolean; - refetchOsmosisBalances?: () => void; - resolveOsmosisRefetch?: () => void; + osmosisBalances?: CombinedBalanceInfo[] | undefined = []; + isOsmosisBalancesLoading?: boolean = false; + refetchOsmosisBalances?: () => void = () => {}; + resolveOsmosisRefetch?: () => void = () => {};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockbis excluded by!**/bun.lockb
📒 Files selected for processing (3)
components/bank/components/tokenList.tsx(6 hunks)components/bank/forms/ibcSendForm.tsx(8 hunks)components/groups/components/groupControls.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/groups/components/groupControls.tsx
🧰 Additional context used
🪛 GitHub Check: codecov/patch
components/bank/forms/ibcSendForm.tsx
[warning] 87-93: components/bank/forms/ibcSendForm.tsx#L87-L93
Added lines #L87 - L93 were not covered by tests
[warning] 109-110: components/bank/forms/ibcSendForm.tsx#L109-L110
Added lines #L109 - L110 were not covered by tests
[warning] 121-121: components/bank/forms/ibcSendForm.tsx#L121
Added line #L121 was not covered by tests
[warning] 176-177: components/bank/forms/ibcSendForm.tsx#L176-L177
Added lines #L176 - L177 were not covered by tests
[warning] 179-182: components/bank/forms/ibcSendForm.tsx#L179-L182
Added lines #L179 - L182 were not covered by tests
[warning] 185-185: components/bank/forms/ibcSendForm.tsx#L185
Added line #L185 was not covered by tests
[warning] 206-206: components/bank/forms/ibcSendForm.tsx#L206
Added line #L206 was not covered by tests
[warning] 216-262: components/bank/forms/ibcSendForm.tsx#L216-L262
Added lines #L216 - L262 were not covered by tests
[warning] 265-267: components/bank/forms/ibcSendForm.tsx#L265-L267
Added lines #L265 - L267 were not covered by tests
[warning] 269-293: components/bank/forms/ibcSendForm.tsx#L269-L293
Added lines #L269 - L293 were not covered by tests
[warning] 296-348: components/bank/forms/ibcSendForm.tsx#L296-L348
Added lines #L296 - L348 were not covered by tests
[warning] 381-382: components/bank/forms/ibcSendForm.tsx#L381-L382
Added lines #L381 - L382 were not covered by tests
[warning] 385-387: components/bank/forms/ibcSendForm.tsx#L385-L387
Added lines #L385 - L387 were not covered by tests
[warning] 389-401: components/bank/forms/ibcSendForm.tsx#L389-L401
Added lines #L389 - L401 were not covered by tests
[warning] 457-467: components/bank/forms/ibcSendForm.tsx#L457-L467
Added lines #L457 - L467 were not covered by tests
[warning] 470-483: components/bank/forms/ibcSendForm.tsx#L470-L483
Added lines #L470 - L483 were not covered by tests
[warning] 516-519: components/bank/forms/ibcSendForm.tsx#L516-L519
Added lines #L516 - L519 were not covered by tests
[warning] 587-591: components/bank/forms/ibcSendForm.tsx#L587-L591
Added lines #L587 - L591 were not covered by tests
[warning] 600-613: components/bank/forms/ibcSendForm.tsx#L600-L613
Added lines #L600 - L613 were not covered by tests
[warning] 848-848: components/bank/forms/ibcSendForm.tsx#L848
Added line #L848 was not covered by tests
🔇 Additional comments (6)
components/bank/components/tokenList.tsx (2)
114-114: LGTM! Improved modal state management.The change from using
document.getElementByIdto React state for modal management is a good improvement in line with React best practices.
240-244: LGTM! Proper prop forwarding with defaults.The SendModal component properly forwards all required props with appropriate default values.
components/bank/forms/ibcSendForm.tsx (4)
87-95: Extract token display name formatting to a shared utility.The token display name formatting logic is duplicated in multiple places. Extract it to a utility function for reuse.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 87-93: components/bank/forms/ibcSendForm.tsx#L87-L93
Added lines #L87 - L93 were not covered by tests
848-850: Remove unimplemented function.The
useSkipfunction is not implemented and appears to be unused.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 848-848: components/bank/forms/ibcSendForm.tsx#L848
Added line #L848 was not covered by tests
176-186: Improve fee buffer configuration and documentation.The fee warning logic uses hardcoded values and only applies to 'umfx' tokens.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 176-177: components/bank/forms/ibcSendForm.tsx#L176-L177
Added lines #L176 - L177 were not covered by tests
[warning] 179-182: components/bank/forms/ibcSendForm.tsx#L179-L182
Added lines #L179 - L182 were not covered by tests
[warning] 185-185: components/bank/forms/ibcSendForm.tsx#L185
Added line #L185 was not covered by tests
87-850: Add test coverage for critical paths.Several critical code paths lack test coverage, including:
- Token display name formatting
- Skip protocol integration
- Chain selection logic
- Fee warning logic
Consider adding tests for these scenarios to improve code reliability.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 87-93: components/bank/forms/ibcSendForm.tsx#L87-L93
Added lines #L87 - L93 were not covered by tests
[warning] 109-110: components/bank/forms/ibcSendForm.tsx#L109-L110
Added lines #L109 - L110 were not covered by tests
[warning] 121-121: components/bank/forms/ibcSendForm.tsx#L121
Added line #L121 was not covered by tests
[warning] 176-177: components/bank/forms/ibcSendForm.tsx#L176-L177
Added lines #L176 - L177 were not covered by tests
[warning] 179-182: components/bank/forms/ibcSendForm.tsx#L179-L182
Added lines #L179 - L182 were not covered by tests
[warning] 185-185: components/bank/forms/ibcSendForm.tsx#L185
Added line #L185 was not covered by tests
[warning] 206-206: components/bank/forms/ibcSendForm.tsx#L206
Added line #L206 was not covered by tests
[warning] 216-262: components/bank/forms/ibcSendForm.tsx#L216-L262
Added lines #L216 - L262 were not covered by tests
[warning] 265-267: components/bank/forms/ibcSendForm.tsx#L265-L267
Added lines #L265 - L267 were not covered by tests
[warning] 269-293: components/bank/forms/ibcSendForm.tsx#L269-L293
Added lines #L269 - L293 were not covered by tests
[warning] 296-348: components/bank/forms/ibcSendForm.tsx#L296-L348
Added lines #L296 - L348 were not covered by tests
[warning] 381-382: components/bank/forms/ibcSendForm.tsx#L381-L382
Added lines #L381 - L382 were not covered by tests
[warning] 385-387: components/bank/forms/ibcSendForm.tsx#L385-L387
Added lines #L385 - L387 were not covered by tests
[warning] 389-401: components/bank/forms/ibcSendForm.tsx#L389-L401
Added lines #L389 - L401 were not covered by tests
[warning] 457-467: components/bank/forms/ibcSendForm.tsx#L457-L467
Added lines #L457 - L467 were not covered by tests
[warning] 470-483: components/bank/forms/ibcSendForm.tsx#L470-L483
Added lines #L470 - L483 were not covered by tests
[warning] 516-519: components/bank/forms/ibcSendForm.tsx#L516-L519
Added lines #L516 - L519 were not covered by tests
[warning] 587-591: components/bank/forms/ibcSendForm.tsx#L587-L591
Added lines #L587 - L591 were not covered by tests
[warning] 600-613: components/bank/forms/ibcSendForm.tsx#L600-L613
Added lines #L600 - L613 were not covered by tests
[warning] 848-848: components/bank/forms/ibcSendForm.tsx#L848
Added line #L848 was not covered by tests
This PR introduces full support for IBC transfer transactions to and from Manifest, enabling functionality for both users and groups. Initially, support is limited to Osmosis as the additional chain.
When reviewing, please note that we currently need to manually configure the ibc and assetList settings while waiting for updates to the GitHub chain-registry and the Cosmology chain-registry npm package. To eliminate this dependency, we could consider maintaining our own registry or npm package. This approach would accommodate token factory tokens originating from either side, as well as other IBC assets. As we expand support to additional chains—including EVM chains and other Cosmos networks—this self-managed registry could streamline the integration process.
Summary by CodeRabbit
New Features
• Enhanced cross-chain transfers with distinct source and destination selectors, including auto-switch logic when both selections match.
• Expanded support for Osmosis and Axelar networks, enabling users to view and manage token balances and transfers across multiple blockchains.
• Improved wallet connectivity that now connects to multiple chains concurrently with streamlined connection and disconnection flows.
Chores
• Updated environment configurations and dependencies, plus new utility scripts to support the augmented multi-chain capabilities.