(Deposit/Withdraw) Use denom to avoid excluding variants in deposits #3952
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
4 Skipped Deployments
|
JoseRFelix
changed the title
WalkthroughThe changes in this pull request involve significant updates to the asset management procedures within the Changes
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/trpc/src/assets.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the config "prettier" to extend from. Please check that the name of the config is correct. The config "prettier" was referenced from the config file in "/packages/trpc/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. 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: 0
🧹 Outside diff range and nitpick comments (1)
packages/trpc/src/assets.ts (1)
635-635: Simplify the type assertion inincludescheckThe type assertion
(typeof variantsNotToBeExcluded)[number]is unnecessary here. SincevariantsNotToBeExcludedis an array of strings andasset.coinDenomis a string, you can simplify the code by removing the type assertion.Apply this diff to simplify the code:
- !variantsNotToBeExcluded.includes( - asset.coinDenom as (typeof variantsNotToBeExcluded)[number] - ) + !variantsNotToBeExcluded.includes(asset.coinDenom)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/trpc/src/assets.ts(1 hunks)packages/web/components/bridge/asset-select-screen.tsx(1 hunks)
🔇 Additional comments (3)
packages/web/components/bridge/asset-select-screen.tsx (3)
27-30: LGTM: Type-safe approach for asset symbol handling
The switch from variant group keys to specific asset symbols (MainnetAssetSymbols | TestnetAssetSymbols) provides better type safety and aligns with the PR's objective of denom-based filtering.
Line range hint 89-103: LGTM: Clean integration of asset filtering constants
The queryParameters memo correctly utilizes the updated constants for asset filtering and prioritization. The implementation maintains type safety while achieving the PR's objective of proper WBTC handling.
Line range hint 27-46: Verify WBTC visibility in the deposit list
The changes appropriately handle WBTC visibility by:
- Including it in
variantsNotToBeExcludedto prevent filtering - Adding it to
prioritizedDenomsfor better placement in the list
Let's verify that WBTC appears in the deposit list and that these changes don't affect other assets:
✅ Verification successful
Based on the search results, I can now provide a final response since we have sufficient information about how WBTC is handled in the codebase:
WBTC visibility changes are correctly implemented
The changes ensure proper WBTC visibility in the deposit list through a well-integrated approach:
variantsNotToBeExcludedis used in the asset filtering logic (packages/trpc/src/assets.ts) to prevent WBTC from being excludedprioritizedDenomsarray is used in sorting logic with explicit index comparison to ensure WBTC appears in the preferred order- The implementation properly type-checks through TypeScript's
satisfiesoperator
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify WBTC handling in the codebase
# Check for any other WBTC-specific handling that might conflict with these changes
# Search for WBTC-related configurations
rg -i "wbtc" --type ts --type tsx
# Check for any variant group related configurations that might affect WBTC
rg "variantGroup|coinDenom.*wbtc" --type ts --type tsx
Length of output: 146
Script:
#!/bin/bash
# Let's try with correct file type flags and broader search patterns
# Search for WBTC-related configurations
rg -t typescript -t tsx "wbtc|WBTC"
# Look for variant groups and asset configurations
rg -t typescript -t tsx "variantGroup|assetSymbols|MainnetAssetSymbols|TestnetAssetSymbols"
# Check how these constants are used
rg -t typescript -t tsx "variantsNotToBeExcluded|prioritizedDenoms|deprioritizedDenoms"
Length of output: 326
Script:
#!/bin/bash
# Let's try without file type flags and use path filtering instead
# Search for WBTC-related configurations
rg "wbtc|WBTC" --glob "*.{ts,tsx}"
# Look for variant groups and asset configurations
rg "variantGroup|assetSymbols|MainnetAssetSymbols|TestnetAssetSymbols" --glob "*.{ts,tsx}"
# Check how these constants are used
rg "variantsNotToBeExcluded|prioritizedDenoms|deprioritizedDenoms" --glob "*.{ts,tsx}"
Length of output: 144889
What is the purpose of the change:
Since WBTC will now use “allBtc” as its variant group key, we want to ensure it remains visible in the deposit list, even though it’s categorized as a variant. This is important because users may deposit WBTC from networks other than Bitcoin, such as Ethereum.
Linear Task
https://linear.app/osmosis/issue/FE-1225/use-denom-to-avoid-excluding-variants-in-deposits
Brief Changelog
Testing and Verifying