Unwrap Qi, interval conversions, custom RPC - #471
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces several key features to enhance the wallet's conversion and customization capabilities. The main purpose is to add unwrapping functionality for WQI tokens, interval-based conversions for automated trading, and custom RPC configuration for improved network flexibility.
Key changes:
- Added WQI unwrapping functionality allowing users to convert wrapped tokens back to native Qi
- Implemented interval conversions enabling automated periodic conversions with configurable timing and transaction counts
- Added custom RPC URL configuration for network customization
- Updated package dependencies to quais ^1.0.0-alpha.52
Reviewed Changes
Copilot reviewed 26 out of 27 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/routes/routes.tsx | Added new routes for interval conversions, unwrap functionality, and error handling |
| ui/pages/_NewDesign/WrapPage.tsx | Enhanced to support both wrap and unwrap operations with conditional UI logic |
| ui/pages/_NewDesign/RunningIntervals.tsx | New page for displaying and managing interval conversions |
| ui/pages/_NewDesign/IntervalErrorDetails.tsx | New page for displaying detailed error information for failed intervals |
| ui/pages/_NewDesign/ConvertPage.tsx | Updated to handle interval conversion navigation |
| ui/pages/_NewDesign/ConvertIntervalConfirmation.tsx | New confirmation page for interval conversion setup |
| ui/pages/_NewDesign/ConfirmWrapPage.tsx | Enhanced to support unwrap confirmation flow |
| ui/pages/_NewDesign/ConfirmConversionPage.tsx | Improved error handling for conversion failures |
| ui/pages/SingleAsset.tsx | Added unwrap button for WQI tokens |
| ui/pages/Settings.tsx | Added custom RPC and interval management options |
| background/services/transactions/index.ts | Added interval conversion logic and unwrap functionality |
| background/services/transactions/db.ts | Added database schema for interval conversions |
| background/redux-slices/convertAssets.ts | Added interval settings and unwrap actions |
| background/redux-slices/networks.ts | Added custom RPC management |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| const history = useHistory() | ||
| const dispatch = useBackgroundDispatch() | ||
| const location = useLocation<WrapLocationState>() | ||
| const isUnwrap = location.pathname === "/unwrap" |
There was a problem hiding this comment.
[nitpick] Consider using a more explicit parameter or state-based approach instead of relying on pathname comparison. This makes the component's behavior more predictable and easier to test.
| const isUnwrap = location.pathname === "/unwrap" | |
| const isUnwrap = location.state?.isUnwrap ?? false |
| </div> | ||
| <div className="info-box"> | ||
| <p style={{ fontWeight: "bold" }}>Note: Qi will be locked for two weeks. Only whole numbers of WQI can be unwrapped. Fractional amounts are not supported.</p> | ||
| </div> |
There was a problem hiding this comment.
Avoid inline styles. Define the font-weight styling in the CSS block below for consistency with the rest of the component.
|
|
||
| const loadIntervals = async () => { | ||
| try { | ||
| const result = await dispatch(getIntervalConversionsHandle()) as any |
There was a problem hiding this comment.
Avoid using 'as any' type assertion. Define proper types for the dispatch result to maintain type safety.
| const result = await dispatch(getIntervalConversionsHandle()) as any | |
| // The result can be either an array of IntervalConversion or an object with a payload property containing such an array | |
| type IntervalConversionsDispatchResult = IntervalConversion[] | { payload: IntervalConversion[] } | |
| const loadIntervals = async () => { | |
| try { | |
| const result: IntervalConversionsDispatchResult = await dispatch(getIntervalConversionsHandle()) |
| // Result is the interval directly | ||
| console.log(`[IntervalErrorDetails] Found interval directly in result:`, result) | ||
| intervalData = result | ||
| } else if (result?.payload) { |
There was a problem hiding this comment.
Avoid using 'as any' type assertion. Define proper types for the dispatch result to maintain type safety.
| } else if (result?.payload) { | |
| const result: IntervalConversionResult = await dispatch(getIntervalConversionHandle(intervalId)) | |
| console.log(`[IntervalErrorDetails] Result from dispatch:`, result) | |
| // Check if result is the interval directly or wrapped in payload | |
| let intervalData: IntervalConversion | null = null | |
| if ("id" in result) { | |
| // Result is the interval directly | |
| console.log(`[IntervalErrorDetails] Found interval directly in result:`, result) | |
| intervalData = result | |
| } else if ("payload" in result && result.payload) { |
| setError(null) | ||
|
|
||
| try { | ||
| const result = await dispatch(startIntervalConversionHandle()) as any |
There was a problem hiding this comment.
Avoid using 'as any' type assertion. Define proper types for the dispatch result to maintain type safety.
| const result = await dispatch(startIntervalConversionHandle()) as any | |
| const result: { payload?: { error?: string } } = await dispatch(startIntervalConversionHandle()) |
| const maxAttempts = 2000 | ||
| let attempts = 0 | ||
|
|
||
| while (attempts < maxAttempts) { |
There was a problem hiding this comment.
[nitpick] This is a significant increase from 200 to 2000 attempts. Consider making this configurable or documenting why such a high number of attempts is necessary.
| while (attempts < maxAttempts) { | |
| let attempts = 0 | |
| while (attempts < MAX_UNUSED_QI_ADDRESS_ATTEMPTS) { |
| // Converting Quai to Qi | ||
| await this.convertQuaiToQi(params.from.address, params.amount, params.maxSlippage) | ||
| // TODO: Get transaction hash from conversion | ||
| txHash = `tx_${Date.now()}` |
There was a problem hiding this comment.
Using placeholder transaction hashes instead of actual transaction hashes from conversion functions will cause incorrect transaction tracking. The TODO comments indicate this is incomplete implementation.
| txHash = `tx_${Date.now()}` | |
| const qiToQuaiTxHash = await this.convertQiToQuai(params.to.address, params.amount, params.maxSlippage) | |
| txHash = qiToQuaiTxHash | |
| } else if (!isFromUtxo && isToUtxo) { | |
| // Converting Quai to Qi | |
| const quaiToQiTxHash = await this.convertQuaiToQi(params.from.address, params.amount, params.maxSlippage) | |
| txHash = quaiToQiTxHash |
| // Converting Quai to Qi | ||
| await this.convertQuaiToQi(params.from.address, params.amount, params.maxSlippage) | ||
| // TODO: Get transaction hash from conversion | ||
| txHash = `tx_${Date.now()}` |
There was a problem hiding this comment.
Using placeholder transaction hashes instead of actual transaction hashes from conversion functions will cause incorrect transaction tracking. The TODO comments indicate this is incomplete implementation.
| txHash = `tx_${Date.now()}` | |
| const txResponse = await this.convertQiToQuai(params.to.address, params.amount, params.maxSlippage) | |
| txHash = txResponse?.hash | |
| } else if (!isFromUtxo && isToUtxo) { | |
| // Converting Quai to Qi | |
| const txResponse = await this.convertQuaiToQi(params.from.address, params.amount, params.maxSlippage) | |
| txHash = txResponse?.hash |
| ) | ||
| this.initializeProviders(testNetworks) | ||
| this.startLocalNodeCheckingInterval() | ||
| // Don't automatically start local node checking - only when local network is actually selected |
There was a problem hiding this comment.
[nitpick] This behavior change should be documented more thoroughly. The comment explains what changed but not why this optimization was necessary.
| // Don't automatically start local node checking - only when local network is actually selected | |
| // Don't automatically start local node checking. | |
| // Rationale: Starting local node checking only when the local network is actually selected avoids unnecessary resource usage and network requests. | |
| // This optimization prevents redundant polling and potential side effects when the local node is not relevant to the user's current network selection. |
| className="custom-slippage-input" | ||
| placeholder="Custom" | ||
| value={isCustom ? customValue : ""} | ||
| value={isCustom ? customValue : (!isPresetValue ? getPercentageValue() : "")} |
There was a problem hiding this comment.
[nitpick] This complex ternary logic is difficult to read. Consider extracting this into a separate function with a descriptive name for better readability.
| value={isCustom ? customValue : (!isPresetValue ? getPercentageValue() : "")} | |
| value={getInputValue()} |
No description provided.