Feat/retire bitcoin #1006
Feat/retire bitcoin #1006
Conversation
WalkthroughThe pull request includes updates to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
haunv3
requested review from
perfogic,
vuonghuuhung and
quangdz1704
and removed request for
vuonghuuhung
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (14)
src/pages/UniversalSwap/Swap/hooks/useFilteredTokens.ts (1)
23-23: Consider adding logging for filtered BTC tokensThe filtering logic correctly excludes BTC tokens, aligning with the Bitcoin retirement objective. However, consider adding debug logging when tokens are filtered out to aid in troubleshooting.
- setFilteredToTokens(filteredToTokens.filter((fi) => fi?.contractAddress !== BTC_CONTRACT)); + const finalToTokens = filteredToTokens.filter((fi) => fi?.contractAddress !== BTC_CONTRACT); + if (filteredToTokens.length !== finalToTokens.length) { + console.debug('Filtered out BTC tokens from "to" tokens:', + filteredToTokens.filter((fi) => fi?.contractAddress === BTC_CONTRACT)); + } + setFilteredToTokens(finalToTokens);Similar change can be applied to the "from" tokens filtering.
Also applies to: 32-32
src/config/chainInfos.ts (1)
Line range hint
24-217: Consider documenting the new Bitcoin bridge architecture.This PR introduces significant changes to the Bitcoin integration architecture:
- New OraiBTC bridge network
- Retirement of the old Bitcoin implementation
- Token denomination changes
Consider:
- Adding architecture documentation explaining the new Bitcoin bridge flow
- Creating a migration guide for users
- Updating integration documentation for partners
- Adding monitoring for the new bridge network
Would you like help creating the architectural documentation or migration guide?
src/pages/BitcoinDashboard/components/PendingDeposits/PendingDeposits.tsx (2)
Line range hint
168-177: Consider improving fee calculation precision.The current fee calculation in the amount accessor involves multiple conversions that could lead to precision loss:
Number(data.amount).toFixed(0)converts to number and back to string- Multiple
toDisplaycalls with different decimalsConsider using a decimal handling library like
decimal.jsorbignumber.jsto maintain precision:- {( - toDisplay(BigInt(Number(data.amount).toFixed(0) || 0), 8) - - fee.relayerFee - - toDisplay(BigInt(depositFee?.deposit_fees || 0), 14) - ).toFixed(6)}{' '} + {new BigNumber(data.amount) + .times(10 ** 8) + .minus(fee.relayerFee) + .minus(new BigNumber(depositFee?.deposit_fees || 0)) + .div(10 ** 8) + .toFixed(6)}{' '}
Line range hint
91-102: Improve error handling in handleUpdateTxPending.The current error handling only logs to console, which could hide issues from users and make debugging harder.
Consider adding proper error handling:
} catch (err) { - console.log(err); + console.error('Failed to update pending deposit:', err); + // Consider showing a user-friendly error message + throw new Error('Failed to process pending deposits. Please try again later.'); }src/pages/BitcoinDashboardV2/components/ConvertBitcoinV2/index.tsx (1)
Line range hint
82-82: Update error message to match new naming.The error message still references "BTC V2" which is inconsistent with the new naming.
- if (!address) throw Error('BTC V2 deposit address is not found!'); + if (!address) throw Error('BTC deposit address is not found!');🧰 Tools
🪛 Biome
[error] 28-28: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
[error] 28-28: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
src/pages/Balance/TransferConvertToken/index.tsx (2)
Line range hint
161-176: Consider extracting fee calculation logic into a separate function.The fee calculation logic is becoming complex with multiple conditions. Consider extracting it into a dedicated function for better maintainability and testability.
+ const calculateBridgeFee = (isV2: boolean, contractConfig: any) => { + if (!isV2) return 0; + if (contractConfig?.token_fee.denominator === 0) return 0; + return (contractConfig?.token_fee.nominator * 100) / contractConfig?.token_fee.denominator; + }; // Replace the existing logic with: - if (isV2) { - if (contractConfig?.token_fee.denominator != 0) { - bridgeFee = (contractConfig?.token_fee.nominator * 100) / contractConfig?.token_fee.denominator; - } else { - bridgeFee = 0; - } - // not support relayer fee yet - relayerFeeTokenFee = 0; - } + bridgeFee = calculateBridgeFee(isV2, contractConfig); + relayerFeeTokenFee = isV2 ? 0 : relayerFeeTokenFee; // not support relayer fee yet
Line range hint
179-189: Address technical debt in decimal handling.The code contains TODO comments and magic numbers for decimal handling. Consider:
- Documenting the reason for using 14 decimal places
- Creating a constant for the decimal places
- Adding unit tests to verify decimal calculations
+ // Number of decimal places for Bitcoin amounts in satoshis + const BITCOIN_DECIMALS = 14; + const SATOSHI_DENOMINATOR = Math.pow(10, BITCOIN_DECIMALS); let toDisplayBTCFee = 0; if (depositFeeBtc && isFromBitcoinToOraichain) { - // TODO: usat decimal 14 - toDisplayBTCFee = new BigDecimal(depositFeeBtc.deposit_fees ?? 0).div(1e14).toNumber(); + toDisplayBTCFee = new BigDecimal(depositFeeBtc.deposit_fees ?? 0).div(SATOSHI_DENOMINATOR).toNumber(); } if (withdrawalFeeBtc && isFromOraichainToBitcoin) { - // TODO: usat decimal 14 - toDisplayBTCFee = new BigDecimal(withdrawalFeeBtc.withdrawal_fees ?? 0).div(1e14).toNumber(); + toDisplayBTCFee = new BigDecimal(withdrawalFeeBtc.withdrawal_fees ?? 0).div(SATOSHI_DENOMINATOR).toNumber(); }Would you like me to help create unit tests for the decimal calculations?
src/pages/Balance/index.tsx (7)
Line range hint
344-392: Consider extracting fee calculation logic.The fee calculation logic in
handleTransferOraichainToBTCis complex and could benefit from being extracted into a separate function for better maintainability and reusability.+const calculateBTCTransferFee = (depositFee, withdrawFee, isFastMode) => { + if (!withdrawFee?.withdrawal_fees) { + throw Error('Withdrawal fees are not found!'); + } + if (!depositFee?.deposit_fees) { + throw Error('Deposit fees are not found!'); + } + return isFastMode ? depositFee?.deposit_fees : withdrawFee?.withdrawal_fees; +}; const handleTransferOraichainToBTC = async (fromToken: TokenItemType, transferAmount: number, btcAddr: string) => { if (fromToken.name === 'BTC') { try { - if (!withdrawV2Fee?.withdrawal_fees) { - throw Error('Withdrawal fees are not found!'); - } - if (!depositV2Fee?.deposit_fees) { - throw Error('Deposit fees are not found!'); - } - const fee = isFastMode ? depositV2Fee?.deposit_fees : withdrawV2Fee?.withdrawal_fees; + const fee = calculateBTCTransferFee(depositV2Fee, withdrawV2Fee, isFastMode);
Line range hint
439-443: Improve error handling in handleTransferBTC.The error handling in this section could be more robust. Consider adding specific error messages and handling different failure scenarios.
const handleTransferBTC = async ({ isBTCToOraichain, fromToken, transferAmount }) => { const btcAddr = await window.Bitcoin.getAddress(); - if (!btcAddr) throw Error('Not found your bitcoin address!'); + if (!btcAddr) { + throw new Error('Bitcoin address not found. Please ensure your wallet is properly connected.'); + } if (isBTCToOraichain) { if (fromToken.name !== 'BTC') { await handleRecoveryAddress(); } return handleTransferBTCToOraichain(fromToken, transferAmount, btcAddr); } return handleTransferOraichainToBTC(fromToken, transferAmount, btcAddr); };
Line range hint
271-443: Add input validation for transfer amounts.The Bitcoin transfer functions should validate the transfer amount before processing to ensure it meets minimum requirements and doesn't exceed available balance.
+const validateTransferAmount = (amount: number, balance: bigint, minAmount: number) => { + if (amount <= 0) { + throw new Error('Transfer amount must be greater than 0'); + } + if (amount < minAmount) { + throw new Error(`Transfer amount must be at least ${minAmount} BTC`); + } + if (BigInt(amount) > balance) { + throw new Error('Insufficient balance for transfer'); + } +}; const handleTransferBTCToOraichain = async (fromToken: TokenItemType, transferAmount: number, btcAddr: string) => { + validateTransferAmount(transferAmount, amounts[fromToken.denom], 0.0001); const isV2 = fromToken.name === 'BTC'; // ... rest of the function };
Line range hint
271-443: Add transaction timeout handling.The Bitcoin transfer functions should implement a timeout mechanism to handle cases where the transaction takes too long or fails to complete.
Consider implementing a timeout mechanism using Promise.race() to ensure transactions don't hang indefinitely. This would improve user experience and system reliability.
Example implementation pattern:
const withTimeout = async (promise: Promise<any>, timeoutMs: number) => { const timeout = new Promise((_, reject) => setTimeout(() => reject(new Error('Transaction timeout')), timeoutMs) ); return Promise.race([promise, timeout]); };
Line range hint
271-443: Consider implementing retry mechanism.The Bitcoin transfer functions could benefit from a retry mechanism for handling temporary network issues or blockchain congestion.
const withRetry = async (operation: () => Promise<any>, maxRetries = 3, delay = 1000) => { for (let i = 0; i < maxRetries; i++) { try { return await operation(); } catch (error) { if (i === maxRetries - 1) throw error; await new Promise(resolve => setTimeout(resolve, delay * (i + 1))); } } };
Line range hint
271-443: Add comprehensive logging for debugging.The Bitcoin transfer functions would benefit from more detailed logging to help with debugging and monitoring.
const handleTransferBTCToOraichain = async (fromToken: TokenItemType, transferAmount: number, btcAddr: string) => { + console.log(`Initiating BTC transfer to Oraichain: Amount=${transferAmount}, Address=${btcAddr}`); const isV2 = fromToken.name === 'BTC'; try { // ... existing code ... + console.log(`Transfer completed successfully: TxHash=${rs.rawTxHex}`); } catch (error) { + console.error(`Transfer failed: ${error.message}`, { error, fromToken, transferAmount, btcAddr }); throw error; } };
Line range hint
271-443: Add unit tests for Bitcoin transfer functions.The Bitcoin transfer functions lack unit tests, which are crucial for ensuring reliability and catching regressions.
Would you like me to help generate comprehensive unit tests for the Bitcoin transfer functions? These would cover:
- Success scenarios
- Error handling
- Edge cases
- Input validation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
- package.json (2 hunks)
- src/config/chainInfos.ts (5 hunks)
- src/pages/Balance/TransferConvertToken/index.tsx (1 hunks)
- src/pages/Balance/index.tsx (3 hunks)
- src/pages/BitcoinDashboard/components/PendingDeposits/PendingDeposits.tsx (1 hunks)
- src/pages/BitcoinDashboardV2/components/ConvertBitcoinV2/index.tsx (1 hunks)
- src/pages/UniversalSwap/Swap/hooks/useFilteredTokens.ts (3 hunks)
🔇 Additional comments (7)
src/pages/UniversalSwap/Swap/hooks/useFilteredTokens.ts (2)
3-3: LGTM: Clean import additionThe BTC_CONTRACT import is properly added from the existing @oraichain/oraidex-common package.
Line range hint
23-32: Verify BTC token handling across the applicationLet's verify that this BTC token filtering is consistently applied across the application to prevent any edge cases where BTC tokens might still be accessible.
✅ Verification successful
Let me gather more specific information about BTC token handling to verify the consistency of this change.
This will help us understand:
- Where BTC_CONTRACT is used and how
- If there are similar filtering patterns elsewhere
- Any other BTC-specific filtering logic that might need alignment
Let me check one more aspect to understand how the BTC token is used in the swap functionality.
This will help us understand:
- How the BTC filtering is implemented in UniversalSwapHelper
- If there are any special cases for BTC handling in the swap logic
Based on the gathered information, I can now provide the final response:
BTC token filtering is properly isolated and consistently implemented
The BTC token filtering implementation is correctly scoped to the
useFilteredTokenshook and doesn't require changes elsewhere because:
- The
BTC_CONTRACTconstant is only used in two places:
useFilteredTokens.tsfor filtering tokens in the swap interfaceibc-chain.tsfor chain configuration, unrelated to the filtering logic- The filtering is applied consistently to both "from" and "to" token lists
- The base token filtering is handled by
UniversalSwapHelper.filterNonPoolEvmTokens, with BTC filtering as an additional layer specific to the swap interface- No other components directly filter or handle BTC tokens in a way that would be affected by this change
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other instances where BTC tokens might be handled echo "Checking for BTC token handling patterns..." rg -l "BTC" --type ts --type tsx # Look for potential swap-related components that might need similar filtering echo -e "\nChecking for other swap-related components..." fd -e ts -e tsx . | rg -i "swap|token|filter"Length of output: 3640
Script:
#!/bin/bash # Check for BTC_CONTRACT constant usage echo "Checking BTC_CONTRACT usage..." rg "BTC_CONTRACT" -A 2 # Check for specific BTC token filtering in swap-related components echo -e "\nChecking BTC token filtering patterns..." ast-grep --pattern 'filter($$$).filter((fi) => fi?.contractAddress !== $BTC)' # Look for other potential BTC filtering implementations echo -e "\nChecking for other BTC-related filtering..." rg "contractAddress.*BTC" -A 2Length of output: 2589
Script:
#!/bin/bash # Check UniversalSwapHelper implementation for BTC handling echo "Checking UniversalSwapHelper implementation..." rg -A 5 "filterNonPoolEvmTokens" # Check for any BTC-specific swap logic echo -e "\nChecking for BTC-specific swap handling..." ast-grep --pattern 'class UniversalSwapHelper { $$$ }'Length of output: 2789
package.json (2)
27-27: Caution: Beta version in production dependency.The change from
1.1.15to1.1.15-beta.1for@oraichain/oraidex-universal-swapintroduces a beta version as a production dependency, which could potentially impact stability.
143-143: Verify changelog for breaking changes.The
@oraichain/oraidex-commonpackage has been updated from1.1.23to1.1.27. Please verify the changelog for any breaking changes related to Bitcoin handling between these versions.src/config/chainInfos.ts (2)
150-158: LGTM! Verify complete removal of BTC V2 references.Good cleanup of commented-out code. However, let's ensure all BTC V2 references have been properly migrated.
#!/bin/bash # Check for any remaining BTC V2 references in the codebase echo "Checking for remaining BTC V2 references..." rg -l "BTC[ -]?V2" # Verify the integration points of the new network echo "Checking network integration points..." rg "chainInfosWithSdk|chainInfos.*?oraibtc" -A 2
73-73: Breaking Change: Verify impact of Bitcoin denomination updates.The renaming of Bitcoin denominations represents a significant change:
- Original 'BTC' → 'BTC (Legacy)'
- 'BTC V2' → 'BTC'
This change could impact:
- UI displays and sorting
- API integrations expecting specific denominations
- Historical transaction records
- User-facing documentation
Also applies to: 83-83, 98-98
src/pages/BitcoinDashboardV2/components/ConvertBitcoinV2/index.tsx (1)
Line range hint
1-300: Code cleanup and security improvements needed.
- Remove commented-out code (getBitcoinAddress and related useEffect) if it's no longer needed.
- Consider consolidating fee calculations into a dedicated hook for better maintainability.
- Add input validation for the Bitcoin address format.
Let's check for similar patterns in other files:
#!/bin/bash # Look for other commented-out Bitcoin address retrieval code echo "Searching for commented Bitcoin address retrieval..." rg "getBitcoinAddress|blockstream\.info" # Check for Bitcoin address validation patterns echo "Searching for Bitcoin address validation..." rg "bitcoin.*address.*valid|btc.*address.*valid"🧰 Tools
🪛 Biome
[error] 28-28: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
[error] 28-28: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
src/pages/BitcoinDashboard/components/PendingDeposits/PendingDeposits.tsx
Outdated
Show resolved
Hide resolved
src/pages/BitcoinDashboardV2/components/ConvertBitcoinV2/index.tsx
Outdated
Show resolved
Hide resolved
|
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
These changes enhance user experience by simplifying Bitcoin transaction handling and improving clarity in fee structures.