(Deposit/Withdraw) Address design feedback for Bitcoin deposits and withdrawals #4010
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
4 Skipped Deployments
|
JoseRFelix
changed the title
JoseRFelix
changed the title
|
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/web/components/bridge/amount-screen.tsxOops! Something went wrong! :( ESLint: 8.50.0 ESLint couldn't find the config "next/core-web-vitals" to extend from. Please check that the name of the config is correct. The config "next/core-web-vitals" was referenced from the config file in "/packages/web/.eslintrc.json". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThe pull request introduces several modifications across multiple components related to asset transfers. Key changes include updates to user interface elements, such as button text and modal titles, improved state management practices, and enhanced error handling. New components are added to streamline the display of transaction details, and visual adjustments are made to the layout and styling of existing components. These changes collectively aim to improve user interactions and the clarity of information presented during asset transfers. Changes
Possibly related PRs
Suggested reviewers
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 (3)
packages/web/components/bridge/amount-screen.tsx (1)
Line range hint
1-1180: Consider breaking down the component for better maintainability.The
AmountScreencomponent is quite large and handles multiple responsibilities. Consider the following improvements:
- Extract the chain selection logic into a separate component
- Move complex state management into custom hooks
- Split the transfer details into its own file
Example refactor for chain selection:
+ // chain-selector.tsx + export const ChainSelector: FunctionComponent<{ + direction: "deposit" | "withdraw"; + fromChain: BridgeChainWithDisplayInfo; + toChain: BridgeChainWithDisplayInfo; + onChainSelect: (chain: BridgeChainWithDisplayInfo) => void; + }> = ({ direction, fromChain, toChain, onChainSelect }) => { + // Extract chain selection logic here + }; // amount-screen.tsx export const AmountScreen = observer(({ ... }) => { - // Chain selection logic + return ( + <> + <ChainSelector + direction={direction} + fromChain={fromChain} + toChain={toChain} + onChainSelect={handleChainSelect} + /> + {/* Rest of the component */} + </> + ); });packages/web/components/nomic/nomic-pending-transfers.tsx (2)
Line range hint
47-85: LGTM! Consider adding type safety for transaction IDs.The state management improvements look good. Creating a new Map instance ensures proper immutability, and the optimization check prevents unnecessary re-renders. The logic for maintaining transaction history is well-implemented.
Consider creating a type for the transaction ID to improve type safety:
type TransactionId = string; interface TransactionStore { transactions: Map<TransactionId, /* ... */>; transactionIds: Record<TransactionId, boolean>; }
339-340: Implement Bitcoin origin address retrieval.The TODO comment indicates a missing feature to display the source Bitcoin address for pending deposits. This information would improve transaction transparency for users.
Would you like me to help implement the Bitcoin address retrieval functionality? I can:
- Add the address field to the pending deposit type
- Update the state management to handle this new field
- Implement the UI to display the address
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (17)
packages/web/localizations/de.jsonis excluded by!**/*.jsonpackages/web/localizations/en.jsonis excluded by!**/*.jsonpackages/web/localizations/es.jsonis excluded by!**/*.jsonpackages/web/localizations/fa.jsonis excluded by!**/*.jsonpackages/web/localizations/fr.jsonis excluded by!**/*.jsonpackages/web/localizations/gu.jsonis excluded by!**/*.jsonpackages/web/localizations/hi.jsonis excluded by!**/*.jsonpackages/web/localizations/ja.jsonis excluded by!**/*.jsonpackages/web/localizations/ko.jsonis excluded by!**/*.jsonpackages/web/localizations/pl.jsonis excluded by!**/*.jsonpackages/web/localizations/pt-br.jsonis excluded by!**/*.jsonpackages/web/localizations/ro.jsonis excluded by!**/*.jsonpackages/web/localizations/ru.jsonis excluded by!**/*.jsonpackages/web/localizations/tr.jsonis excluded by!**/*.jsonpackages/web/localizations/zh-cn.jsonis excluded by!**/*.jsonpackages/web/localizations/zh-hk.jsonis excluded by!**/*.jsonpackages/web/localizations/zh-tw.jsonis excluded by!**/*.json
📒 Files selected for processing (5)
packages/web/components/bridge/amount-screen.tsx(1 hunks)packages/web/components/bridge/bridge-wallet-select-modal.tsx(2 hunks)packages/web/components/bridge/deposit-address-screen.tsx(4 hunks)packages/web/components/input/textarea-box.tsx(2 hunks)packages/web/components/nomic/nomic-pending-transfers.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/web/components/input/textarea-box.tsx
🔇 Additional comments (7)
packages/web/components/bridge/bridge-wallet-select-modal.tsx (2)
69-69: LGTM: Modal title updated per design feedback
The modal title has been updated to "Enter Bitcoin address" as requested in the design feedback.
581-581: LGTM: Button text styling updated
The button styling has been updated to ensure consistent font size using the !text-subtitle1 class.
packages/web/components/bridge/deposit-address-screen.tsx (3)
Line range hint 158-197: LGTM: QR code close button implementation
The close button has been properly implemented with:
- Correct positioning using absolute positioning
- Proper accessibility attributes (aria-label, data-testid)
- Appropriate styling and hover states
453-458: LGTM: Fee display logic updated
The fee display logic has been updated to handle non-zero provider fees appropriately.
535-541: LGTM: Provider fee display updated per design feedback
The provider fee display now shows "Free" when the fee is zero, as requested in the design feedback.
packages/web/components/bridge/amount-screen.tsx (2)
1180-1180: LGTM: Button text change implemented correctly.
The button text change from "continue" to "confirmAmount" for Bitcoin withdrawals has been implemented as per the design feedback requirements.
Line range hint 1-1180: Verify coordination with related components.
This file implements the amount screen component, but some UI changes mentioned in the PR objectives (QR code close trigger, address modal title, etc.) should be coordinated with other components.
Let's verify the related component implementations:
✅ Verification successful
Based on the code analysis, I can now provide a final verification response:
UI changes have been properly implemented across components
The code shows that:
- QR code close trigger is handled in
QRCodeViewcomponent with proper state management - Bitcoin address modal title is correctly set in
BridgeWalletSelectModalusingtransfer.enterBitcoinAddresstranslation key - Fee display shows "Free" text in
ProviderFeesRowcomponent when fee is zero
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for related component implementations
# Check for QR code close implementation
echo "Checking QR code close implementation..."
rg -A 5 "QRCode|qr-code" --type tsx
# Check for address modal title implementation
echo "Checking address modal title implementation..."
rg -A 5 "transfer\.enterBitcoinAddress" --type tsx
# Check for fee display implementation
echo "Checking fee display implementation..."
rg -A 5 "0%|Free" --type tsx
Length of output: 486
Script:
#!/bin/bash
# Let's try searching without file type restriction since tsx extension wasn't recognized
# Check for QR code close implementation
echo "Checking QR code close implementation..."
rg -A 5 "QRCode|qr-code"
# Check for address modal title implementation
echo "Checking address modal title implementation..."
rg -A 5 "transfer\.enterBitcoinAddress"
# Check for fee display implementation
echo "Checking fee display implementation..."
rg -A 5 "transfer\.free|0%"
# Let's also check for modal components that might contain these implementations
echo "Checking modal components..."
rg -A 5 "Modal.*bitcoin|bitcoin.*Modal"
Length of output: 68972
Script:
#!/bin/bash
# Let's search for specific components and their implementations
# Check for QR code component implementation
echo "Checking QR code component..."
fd -e tsx -e ts "qr-code" -x cat {}
# Check for Bitcoin address modal implementation
echo "Checking Bitcoin address modal..."
fd -e tsx -e ts "bridge-wallet-select-modal" -x cat {}
# Check for fee display implementation in quote details
echo "Checking fee display in quote details..."
fd -e tsx -e ts "quote-detail" -x cat {}
Length of output: 37744
What is the purpose of the change:
Implement the following feedback items from the design team:
Linear Task
https://linear.app/osmosis/issue/FE-1278/nomic-feedback
Testing and Verifying