-
Notifications
You must be signed in to change notification settings - Fork 17
Misc bug fixes #702
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Misc bug fixes #702
Conversation
updateTransactionsFromCurrentAccount handles both btc and non btc based tx cache so use it instead of always fetching from backend when updateUserTransactionsState is called
This reverts commit ef13531.
also add pali headers to JSON-RPC requests
remove checkForPendingTransactionsUpdate as it was doing similar things but just unconfirmations, checkForUpdates had a bug which was not checking for unconfirms.. now it is so we merge in one. Also fix setting active account twice in set active account model. FIxing eip1559 checks in utxo model if not bitcoin based. Fix setting account in setAccount, which was breaking sending funds when switching accounts.
accounts were mixing up tx list because of the prev merge fix in validateAndManageUserTransactions
keep alive to fix blank screen issue, add hints for api url on ux, fix going back on menu (not to home)
also menu context navigation, as well as fix blank screen
sometimes it cant parse vault and causes enforced import seed again
… as replaced - Replace transactions immediately when speed-up or cancel is initiated - Remove complex replacement chain tracking since only one tx per nonce exists - Update setTransactionStatusToAccelerated to replace old tx with new one - Add optional cancelTransaction parameter to setTransactionStatusToCanceled - Remove updateReplacementChainOnConfirmation logic (now a no-op) - Remove all cleanup code from validateAndManageUserTransactions This matches blockchain reality where only one transaction per nonce can exist, simplifying the UI and preventing stuck 'replaced' states when wallet misses confirmation events.
- Revert immediate transaction replacement approach from commit 4675a98 - Mark old transactions as replaced instead of removing them - Add simple nonce-based cleanup: if any tx is confirmed, keep only that one - Remove 0-value transaction filtering to match blockchain explorers - Preserve transaction metadata (isReplaced, isSpeedUp) during deduplication - Fix timestamp sorting to handle both EVM and SYS transactions This approach is simpler and more robust: transactions are marked as replaced during speed-up/cancel, then cleaned up during sync when one confirms.
- Create unified `treatAndSortTransactions` function that combines deduplication, sorting, and limiting in a single pass (reduced from 3-4 loops to 2) - Remove redundant `treatDuplicatedTxs` function - Add new `setAccountTransactions` Redux action for simple transaction updates - Consolidate transaction processing in controllers instead of Redux Consistency improvements: - Both EVM and UTXO now process transactions in their controllers and dispatch to Redux consistently - Move all EVM transaction processing to `pollingEvmTransactions` to eliminate double processing in RPC fallback path - Remove unnecessary `flatMap` usage in EVM controller Performance gains: - Eliminate duplicate merging/deduplication between controllers and Redux - Process transactions only once instead of multiple times - Reduce memory allocations with optimized data structures Code cleanup: - Fix unused variable warnings in EvmList.tsx and MainController.ts - Fix interface property ordering in transaction types - Fix variable shadowing in handleListeners.ts - Remove `setMultipleTransactionToState` usage in favor of simpler action The transaction limit of 30 is now consistently applied at the processing stage rather than in Redux, making the data flow clearer and more efficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: API Change Breaks Existing Callers
Breaking API change in the findUserTxsInProviderByBlocksRange
function. Its signature changed from accepting (provider, startBlock, endBlock)
to (provider, numBlocks)
. Existing callers using the old 3-parameter format will cause runtime errors or incorrect behavior due to mismatched parameter interpretation.
source/scripts/Background/controllers/transactions/utils.ts#L42-L46
export const findUserTxsInProviderByBlocksRange = async ( | |
provider: CustomJsonRpcProvider, | |
numBlocks: number | |
): Promise<IEvmTransactionResponse[] | any> => { |
Was this report helpful? Give feedback by reacting with 👍 or 👎
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: EVM Deduplication Fails Across Accounts
The EVM transaction deduplication logic incorrectly uses nonce
alone as the unique identifier. As nonce
is only unique per account, transactions from different accounts with the same nonce are erroneously merged, leading to incorrect transaction data.
source/scripts/Background/controllers/transactions/utils.ts#L228-L236
pali-wallet/source/scripts/Background/controllers/transactions/utils.ts
Lines 228 to 236 in 1e6a215
// For EVM transactions with nonce, use nonce to detect replacements | |
// For UTXO or transactions without nonce, use hash/txid | |
let id: string; | |
if ('nonce' in tx && typeof tx.nonce === 'number') { | |
id = tx.nonce.toString(); | |
} else { | |
id = ('hash' in tx ? tx.hash : tx.txid).toLowerCase(); | |
} | |
Was this report helpful? Give feedback by reacting with 👍 or 👎
- Fix linting warnings by removing unused imports and variables across multiple components - Remove unused Button, Icon, IconButton imports from various pages - Clean up unused state variables (forceRender, inputKey, etc.) - Remove unused destructured variables in notification-manager.ts - Enhance ETH max send functionality - Refactor SendEth.tsx to better handle maximum balance transfers - Add formatFullPrecisionBalance utility for accurate balance display - Improve gas fee calculations and error handling - Add isMaxSend state tracking for better UX - Improve error messaging - Add "insufficientFundsForGas" translation across all locales - Better user feedback when balance is too low to cover gas fees - Code organization improvements - Delete unused source/scripts/Background/controllers/balances/utils.ts - Refactor balance formatting utilities in utils/format.ts - Clean up component imports and dependencies - UI/UX refinements - Update various components for better consistency - Improve form validation and error states - Enhanced visual feedback in transaction flows This commit improves the wallet's handling of maximum value transfers while cleaning up the codebase for better maintainability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: EVM Deduplication Fails with Replaced Transactions
EVM transaction deduplication incorrectly uses nonce
as the unique identifier. This prevents replaced transactions (which have the same nonce but different hashes) from being displayed, as only one transaction per nonce is stored. The transaction hash should be the primary unique identifier, with nonce used for replacement detection.
source/scripts/Background/controllers/transactions/utils.ts#L224-L239
pali-wallet/source/scripts/Background/controllers/transactions/utils.ts
Lines 224 to 239 in 4001a66
// Single pass: Group transactions by their ID and keep the best version | |
const txMap = new Map<string, UnifiedTransaction>(); | |
for (const tx of transactions) { | |
// For EVM transactions with nonce, use nonce to detect replacements | |
// For UTXO or transactions without nonce, use hash/txid | |
let id: string; | |
if ('nonce' in tx && typeof tx.nonce === 'number') { | |
id = tx.nonce.toString(); | |
} else { | |
id = ('hash' in tx ? tx.hash : tx.txid).toLowerCase(); | |
} | |
const existing = txMap.get(id); | |
if (!existing || tx.confirmations > existing.confirmations) { |
Was this report helpful? Give feedback by reacting with 👍 or 👎
…eing in blocks The blockchain explorer API incorrectly returns confirmations="0" for transactions that have valid blockNumber/blockHeight, causing wallet to show "waiting for confirmation" indefinitely. Changes: - Add isTransactionInBlock() utility to detect confirmations via block presence - Update notification manager, rapid polling, and deduplication logic to use block detection instead of confirmations > 0 - Fix transaction deduplication to update when new tx has blockNumber but existing doesn't (both confirmations=0) - Fix rapid polling timing to fetch fresh API data before checking vault state - Ensure vault state is persisted during background polling when wallet locked - Preserve pending transaction tracking when wallet locks Resolves issue where users had to reopen wallet to see confirmed transactions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Native ETH Transfer Validation Fails
The amount
field validation for native ETH transfers in SendEth.tsx
asynchronously calls calculateMaxAmount()
. This leads to performance issues and potential race conditions from repeated network requests during input. If calculateMaxAmount()
fails, the validation incorrectly checks against the total balance instead of the spendable balance (total balance minus gas fees), allowing users to input amounts that will cause transaction failures due to insufficient gas, despite appearing valid.
source/pages/Send/SendEth.tsx#L703-L724
pali-wallet/source/pages/Send/SendEth.tsx
Lines 703 to 724 in c2b6dc8
// For native ETH, also validate against max amount (balance - gas) | |
try { | |
// Calculate max sendable amount | |
const maxAmountStr = await calculateMaxAmount(); | |
const maxAmountBN = | |
ethers.utils.parseEther(maxAmountStr); | |
// If amount exceeds max (balance - gas), show specific error | |
if (valueBN.gt(maxAmountBN)) { | |
// Check if it's just slightly over (within gas fee range) | |
if (valueBN.lte(balanceBN)) { | |
return Promise.reject( | |
t('send.insufficientFundsForGas') || | |
'Insufficient funds for gas' | |
); | |
} | |
} | |
} catch (error) { | |
console.error('Error validating max amount:', error); | |
// If we can't calculate gas, just check against balance | |
// The actual transaction will fail if gas is insufficient | |
} |
Bug: MAX Send Button Fails Gas Fee Validation
The "MAX" send button and validation logic for native ETH transactions are flawed. When "MAX" is selected, the amount is set to the full ETH balance, incorrectly flagging it as a MAX send. This allows users to attempt sending their entire ETH balance, which will fail as gas fees cannot be paid from the remaining balance. The validation should instead check against the balance minus estimated gas fees.
source/pages/Send/SendEth.tsx#L119-L131
pali-wallet/source/pages/Send/SendEth.tsx
Lines 119 to 131 in c2b6dc8
// Check if this is a MAX send by comparing amount to balance | |
let isMax = false; | |
if (!selectedAsset) { | |
try { | |
const balanceEth = String(activeAccount?.balances?.ethereum || '0'); | |
const amountBN = ethers.utils.parseEther(values.amount); | |
const balanceBN = ethers.utils.parseEther(balanceEth); | |
isMax = amountBN.eq(balanceBN); | |
} catch { | |
// If parsing fails, it's not a MAX send | |
isMax = false; | |
} | |
} |
source/pages/Send/SendEth.tsx#L642-L653
pali-wallet/source/pages/Send/SendEth.tsx
Lines 642 to 653 in c2b6dc8
// If MAX is clicked for native ETH, check if value equals full balance | |
if (isMaxSend && !selectedAsset) { | |
const balanceEth = String( | |
activeAccount?.balances?.ethereum || '0' | |
); | |
const balanceBN = ethers.utils.parseEther(balanceEth); | |
// If the amount equals the full balance, it's valid | |
if (valueBN.eq(balanceBN)) { | |
return Promise.resolve(); | |
} | |
} |
Bug: Invalid Ethereum Values and Stale Dependencies
The calculateMaxAmount
function crashes due to unvalidated activeAccount?.balances?.ethereum
values (e.g., "NaN") passed to ethers.utils.parseEther
, preventing amount validation. Additionally, the calculateGasFees
useCallback has a stale isCalculatingGas
dependency, leading to concurrent gas calculations and potential race conditions.
source/pages/Send/SendEth.tsx#L282-L393
pali-wallet/source/pages/Send/SendEth.tsx
Lines 282 to 393 in c2b6dc8
} | |
}, [selectedAsset, activeAccount.address, controllerEmitter, form]); // isCalculatingGas excluded to prevent loops | |
const calculateMaxAmount = useCallback(async (): Promise<string> => { | |
if (selectedAsset) { | |
// For tokens, use full balance as fees are paid in native token | |
return String(selectedAsset.balance); | |
} | |
try { | |
// Get balance in wei using the full precision stored balance | |
const balanceEth = activeAccount?.balances?.ethereum || '0'; | |
const balanceWei = ethers.utils.parseEther(String(balanceEth)); | |
// Try to get fresh fee data or use cached data | |
let totalFeeWei = BigNumber.from(0); | |
if (!cachedFeeData) { | |
// Try to calculate fresh fee data | |
try { | |
const receiver = | |
form.getFieldValue('receiver') || activeAccount.address; | |
const { maxFeePerGas, maxPriorityFeePerGas } = | |
(await controllerEmitter([ | |
'wallet', | |
'ethereumTransaction', | |
'getFeeDataWithDynamicMaxPriorityFeePerGas', | |
])) as any; | |
// Estimate gas limit for a simple transfer | |
const gasLimit = await controllerEmitter( | |
['wallet', 'ethereumTransaction', 'getTxGasLimit'], | |
[ | |
{ | |
from: activeAccount.address, | |
to: receiver, | |
value: '0x1', // Small test value | |
maxFeePerGas, | |
maxPriorityFeePerGas, | |
}, | |
] | |
).then((gas) => BigNumber.from(gas)); | |
// Calculate total fee in wei | |
totalFeeWei = gasLimit.mul(BigNumber.from(maxFeePerGas)); | |
// Cache the fee data for future use | |
const totalFeeEth = Number(ethers.utils.formatEther(totalFeeWei)); | |
setCachedFeeData({ | |
maxFeePerGas: maxFeePerGas.toString(), | |
maxPriorityFeePerGas: maxPriorityFeePerGas.toString(), | |
gasLimit: gasLimit.toString(), | |
totalFeeEth, | |
}); | |
} catch (error) { | |
console.error('Error calculating fresh fee data:', error); | |
// Use conservative estimate based on network | |
const conservativeFeeEth = | |
activeNetwork.chainId === 570 ? '0.00001' : '0.001'; | |
totalFeeWei = ethers.utils.parseEther(conservativeFeeEth); | |
} | |
} else { | |
// Use cached fee data - reconstruct wei value from components | |
const gasLimit = BigNumber.from(cachedFeeData.gasLimit); | |
const maxFeePerGas = BigNumber.from(cachedFeeData.maxFeePerGas); | |
totalFeeWei = gasLimit.mul(maxFeePerGas); | |
} | |
// Calculate max amount in wei (no buffer needed when using BigNumber) | |
const maxAmountWei = balanceWei.sub(totalFeeWei); | |
// If result would be negative, return 0 | |
if (maxAmountWei.lt(0)) { | |
return '0'; | |
} | |
// Convert back to ETH as a string to preserve precision | |
const maxAmountEth = ethers.utils.formatEther(maxAmountWei); | |
// Return as string to preserve full precision | |
return maxAmountEth; | |
} catch (error) { | |
console.error('Error calculating max amount:', error); | |
// Fallback: return balance minus conservative fee using BigNumber | |
try { | |
const balanceEth = activeAccount?.balances?.ethereum || '0'; | |
const balanceWei = ethers.utils.parseEther(String(balanceEth)); | |
const conservativeFee = | |
activeNetwork.chainId === 570 ? '0.00001' : '0.001'; | |
const conservativeFeeWei = ethers.utils.parseEther(conservativeFee); | |
const maxAmountWei = balanceWei.sub(conservativeFeeWei); | |
if (maxAmountWei.lt(0)) { | |
return '0'; | |
} | |
return ethers.utils.formatEther(maxAmountWei); | |
} catch (fallbackError) { | |
console.error('Error in fallback calculation:', fallbackError); | |
return '0'; | |
} | |
} | |
}, [ | |
selectedAsset, | |
cachedFeeData, | |
activeAccount?.balances?.ethereum, | |
activeAccount?.address, | |
activeNetwork.chainId, | |
controllerEmitter, | |
form, | |
]); |
Was this report helpful? Give feedback by reacting with 👍 or 👎
- Add getReadOnlySigner() method to KeyringManager for read-only operations - Enhance SyscoinTransactions.decodeRawTransaction() to work with locked wallet - Update MainController.decodeRawTransaction() with isRawHex parameter - Fix notification manager to decode UTXO transactions without wallet unlock - Update transaction utils to properly validate block numbers (reject -1) - Fix test constructor to include new getReadOnlySigner parameter Resolves "Missing psbt field" error and "Block: -1" confirmation issues
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Error Handling Flaw in Response Processing
The controllerEmitter
incorrectly rejects successful responses. The condition response.code && typeof response.error === 'boolean'
mistakenly flags responses with response.error: false
as errors, as typeof response.error
is boolean
for both true
and false
. This also introduces redundant and slightly inconsistent checks for structured errors (response.code && response.error === true
), potentially leading to inconsistent error processing.
source/scripts/Background/controllers/controllerEmitter.ts#L83-L103
pali-wallet/source/scripts/Background/controllers/controllerEmitter.ts
Lines 83 to 103 in ab4d244
// Check for application-level errors | |
if (response && response.error) { | |
// If it's a structured error from syscoinjs-lib, pass it through as-is | |
if (response.error === true && response.code) { | |
return reject(response); | |
} | |
// Also check if the entire response is the error object | |
if (response.code && typeof response.error === 'boolean') { | |
return reject(response); | |
} | |
// For simple error messages, wrap in Error object | |
return reject(new Error(response.error)); | |
} | |
// Also check if response itself is a structured error | |
if (response && response.code && response.error === true) { | |
return reject(response); | |
} | |
resolve(response); | |
} |
Bug: Inconsistent Null Checks and Gas Estimation Issues
The SendEth
component exhibits inconsistent null checking for activeAccount.address
, potentially causing runtime errors. Furthermore, gas estimation for native ETH transfers (especially for 'MAX' amount calculations) uses a generic EOA address as a fallback receiver, which can lead to inaccurate gas calculations and incorrect 'MAX' amounts if the actual recipient is a contract.
source/pages/Send/SendEth.tsx#L305-L315
pali-wallet/source/pages/Send/SendEth.tsx
Lines 305 to 315 in ab4d244
const { maxFeePerGas, maxPriorityFeePerGas } = | |
(await controllerEmitter([ | |
'wallet', | |
'ethereumTransaction', | |
'getFeeDataWithDynamicMaxPriorityFeePerGas', | |
])) as any; | |
// Estimate gas limit for a simple transfer | |
const gasLimit = await controllerEmitter( | |
['wallet', 'ethereumTransaction', 'getTxGasLimit'], | |
[ |
Bug: ETH "MAX" Send Validation Ignores Gas Fees
The form validation for native ETH "MAX" sends is flawed. When isMaxSend
is true, the validation prematurely passes if the amount equals the full account balance, failing to deduct gas fees. This allows users to attempt transactions that will fail due to insufficient funds for gas.
source/pages/Send/SendEth.tsx#L641-L654
pali-wallet/source/pages/Send/SendEth.tsx
Lines 641 to 654 in ab4d244
// If MAX is clicked for native ETH, check if value equals full balance | |
if (isMaxSend && !selectedAsset) { | |
const balanceEth = String( | |
activeAccount?.balances?.ethereum || '0' | |
); | |
const balanceBN = ethers.utils.parseEther(balanceEth); | |
// If the amount equals the full balance, it's valid | |
if (valueBN.eq(balanceBN)) { | |
return Promise.resolve(); | |
} | |
} | |
Was this report helpful? Give feedback by reacting with 👍 or 👎
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: EVM Transaction Deduplication Fails with Nonce
The transaction deduplication logic in treatAndSortTransactions
incorrectly uses tx.nonce.toString()
as the unique identifier for EVM transactions. This is problematic because replacement transactions (e.g., speed-up or cancel) share the same nonce, causing different transactions to be incorrectly merged or deduplicated, potentially leading to data loss. The transaction hash/txid should be used as the primary unique identifier for all transactions, with nonce only considered for detecting replacements.
source/scripts/Background/controllers/transactions/utils.ts#L231-L241
pali-wallet/source/scripts/Background/controllers/transactions/utils.ts
Lines 231 to 241 in 31bd731
let id: string; | |
if ('nonce' in tx && typeof tx.nonce === 'number') { | |
id = tx.nonce.toString(); | |
} else { | |
id = ('hash' in tx ? tx.hash : tx.txid).toLowerCase(); | |
} | |
const existing = txMap.get(id); | |
// Update if: no existing tx, more confirmations, or transaction just got into a block | |
const shouldUpdate = |
Bug: Flawed Error Handling in `controllerEmitter`
The controllerEmitter
function contains incorrect and redundant error handling logic for structured responses. It incorrectly rejects successful responses where response.error
is false
due to a flawed boolean type check. Furthermore, multiple redundant checks for structured errors result in unreachable code.
source/scripts/Background/controllers/controllerEmitter.ts#L84-L101
pali-wallet/source/scripts/Background/controllers/controllerEmitter.ts
Lines 84 to 101 in 31bd731
if (response && response.error) { | |
// If it's a structured error from syscoinjs-lib, pass it through as-is | |
if (response.error === true && response.code) { | |
return reject(response); | |
} | |
// Also check if the entire response is the error object | |
if (response.code && typeof response.error === 'boolean') { | |
return reject(response); | |
} | |
// For simple error messages, wrap in Error object | |
return reject(new Error(response.error)); | |
} | |
// Also check if response itself is a structured error | |
if (response && response.code && response.error === true) { | |
return reject(response); | |
} | |
Was this report helpful? Give feedback by reacting with 👍 or 👎
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Incomplete Dependency Array Causes Stale Closures
The renderAccount
useCallback has an incomplete dependency array, missing canRemoveAccount
and handleRemoveClick
. This causes stale closures, leading to incorrect account removal validation and improper handling of remove clicks.
source/pages/Settings/ManageAccounts.tsx#L158-L221
pali-wallet/source/pages/Settings/ManageAccounts.tsx
Lines 158 to 221 in dcfcba2
// Memoized unified account rendering | |
const renderAccount = useCallback( | |
(account: IKeyringAccountState, accountType: KeyringAccountType) => { | |
const config = ACCOUNT_TYPE_CONFIG[accountType]; | |
const IconComponent = config.icon; | |
return ( | |
<li | |
key={`${accountType}-${account.id}`} | |
className="my-3 py-2 w-full flex justify-between items-center transition-all duration-300 border-b border-dashed border-dashed-light cursor-default" | |
> | |
<div className="flex items-center"> | |
<span | |
style={{ maxWidth: '16.25rem', textOverflow: 'ellipsis' }} | |
className="w-max flex items-center justify-start whitespace-nowrap overflow-hidden" | |
> | |
<IconComponent /> | |
{account.label} ({ellipsis(account.address, 4, 4)}) | |
</span> | |
<span | |
className={`text-xs ml-2 px-2 py-0.5 text-white ${config.bgColor} rounded-full`} | |
> | |
{config.label} | |
</span> | |
{isActiveAccount(account, accountType) && ( | |
<Icon name="greenCheck" isSvg className="ml-2 w-4" /> | |
)} | |
</div> | |
<div className="flex gap-x-2 items-center justify-between"> | |
<IconButton | |
onClick={() => editAccount(account)} | |
type="primary" | |
shape="circle" | |
> | |
<Icon | |
name="edit" | |
size={20} | |
className="hover:text-brand-royalblue text-xl" | |
/> | |
</IconButton> | |
{canRemoveAccount(account, accountType) && ( | |
<IconButton | |
onClick={() => handleRemoveClick(account, accountType)} | |
type="primary" | |
shape="circle" | |
className="hover:bg-red-500 hover:bg-opacity-20" | |
> | |
<Icon | |
name="delete" | |
size={20} | |
className="hover:text-red-500 text-xl" | |
/> | |
</IconButton> | |
)} | |
</div> | |
</li> | |
); | |
}, | |
[editAccount, isActiveAccount] | |
); | |
Was this report helpful? Give feedback by reacting with 👍 or 👎
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Missing Dependencies in `renderAccount` Hook
The renderAccount
useCallback hook is missing canRemoveAccount
and handleRemoveClick
from its dependency array. This omission causes stale closures, leading to incorrect behavior for account removal functionality.
source/pages/Settings/ManageAccounts.tsx#L158-L220
pali-wallet/source/pages/Settings/ManageAccounts.tsx
Lines 158 to 220 in 0f6fac0
// Memoized unified account rendering | |
const renderAccount = useCallback( | |
(account: IKeyringAccountState, accountType: KeyringAccountType) => { | |
const config = ACCOUNT_TYPE_CONFIG[accountType]; | |
const IconComponent = config.icon; | |
return ( | |
<li | |
key={`${accountType}-${account.id}`} | |
className="my-3 py-2 w-full flex justify-between items-center transition-all duration-300 border-b border-dashed border-dashed-light cursor-default" | |
> | |
<div className="flex items-center"> | |
<span | |
style={{ maxWidth: '16.25rem', textOverflow: 'ellipsis' }} | |
className="w-max flex items-center justify-start whitespace-nowrap overflow-hidden" | |
> | |
<IconComponent /> | |
{account.label} ({ellipsis(account.address, 4, 4)}) | |
</span> | |
<span | |
className={`text-xs ml-2 px-2 py-0.5 text-white ${config.bgColor} rounded-full`} | |
> | |
{config.label} | |
</span> | |
{isActiveAccount(account, accountType) && ( | |
<Icon name="greenCheck" isSvg className="ml-2 w-4" /> | |
)} | |
</div> | |
<div className="flex gap-x-2 items-center justify-between"> | |
<IconButton | |
onClick={() => editAccount(account)} | |
type="primary" | |
shape="circle" | |
> | |
<Icon | |
name="edit" | |
size={20} | |
className="hover:text-brand-royalblue text-xl" | |
/> | |
</IconButton> | |
{canRemoveAccount(account, accountType) && ( | |
<IconButton | |
onClick={() => handleRemoveClick(account, accountType)} | |
type="primary" | |
shape="circle" | |
className="hover:bg-red-500 hover:bg-opacity-20" | |
> | |
<Icon | |
name="delete" | |
size={20} | |
className="hover:text-red-500 text-xl" | |
/> | |
</IconButton> | |
)} | |
</div> | |
</li> | |
); | |
}, | |
[editAccount, isActiveAccount] | |
); |
Was this report helpful? Give feedback by reacting with 👍 or 👎
- Remove unused imports (AnyNaptrRecord, floor, ISupportsInterfaceProps, etc.) - Fix unused variables (currentAccountData, accounts, network parameters) - Resolve variable shadowing issue in EvmList.tsx (assets -> tokens) - Change let to const for immutable variables (assetToAdd) - Rename interfaces to follow naming conventions (TokenDetails -> ITokenDetails, TokenSearchResult -> ITokenSearchResult) - Fix TypeScript interface property ordering for consistency - Add eslint-disable comment for intentional unused destructured variable - Apply prettier formatting fixes All ESLint errors resolved, only intentional warnings remain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Gas Limit Conversion Error
Ethereum transactions incorrectly multiply the gasLimit
by 10^9
before use. Gas limits are unit counts, not wei values, and should not be converted. This leads to inaccurate gas fee calculations, causing transactions to fail, incur excessive costs, or result in "insufficient funds" errors, particularly for "MAX sends". Additionally, the basicTxValues.isMax
property, which is essential for the "MAX send" logic, is referenced but undefined, further breaking this functionality.
source/pages/Send/Confirm.tsx#L395-L428
pali-wallet/source/pages/Send/Confirm.tsx
Lines 395 to 428 in ede5ef9
// For MAX sends, deduct gas fees from the value | |
// This is required because ethers.js validates balance >= value + gas | |
if (basicTxValues.isMax) { | |
const gasLimit = BigNumber.from( | |
validateCustomGasLimit | |
? customFee.gasLimit * 10 ** 9 | |
: fee.gasLimit | |
); | |
if (isEIP1559Compatible) { | |
// EIP-1559 transaction | |
const maxFeePerGasWei = ethers.utils.parseUnits( | |
String( | |
Boolean(customFee.isCustom && customFee.maxFeePerGas > 0) | |
? customFee.maxFeePerGas.toFixed(9) | |
: fee.maxFeePerGas.toFixed(9) | |
), | |
9 | |
); | |
const maxGasFeeWei = gasLimit.mul(maxFeePerGasWei); | |
value = value.sub(maxGasFeeWei); | |
} else { | |
// Legacy transaction | |
const gasPriceWei = BigNumber.from(gasPrice); | |
const gasFeeWei = gasLimit.mul(gasPriceWei); | |
value = value.sub(gasFeeWei); | |
} | |
// Ensure value doesn't go negative | |
if (value.lt(0)) { | |
alert.error(t('send.insufficientFundsForGas')); | |
setLoading(false); | |
return; | |
} |
source/pages/Send/Confirm.tsx#L535-L540
pali-wallet/source/pages/Send/Confirm.tsx
Lines 535 to 540 in ede5ef9
gasLimit: BigNumber.from( | |
validateCustomGasLimit | |
? customFee.gasLimit * 10 ** 9 // Multiply gasLimit to reach correctly decimal value | |
: fee.gasLimit | |
), | |
}, |
Bug: Stale State in `useEffect` Causes Incorrect Wallet Handling
The useEffect
hook in useController
uses the reactive hasEncryptedVault
state (from useSelector
) within its callbacks (e.g., checkUnlockStatus
, handleLockStateMessage
) but omits it from its dependency array. This creates a stale closure, causing the polling and message listener logic to operate with an outdated hasEncryptedVault
value, leading to incorrect wallet lock state detection and navigation, particularly when the wallet is forgotten.
source/hooks/useController.ts#L14-L183
pali-wallet/source/hooks/useController.ts
Lines 14 to 183 in ede5ef9
// Get hasEncryptedVault from global state to distinguish between locked and forgotten states | |
const hasEncryptedVault = useSelector( | |
(state: RootState) => state.vaultGlobal.hasEncryptedVault | |
); | |
// Function to check unlock status | |
const checkUnlockStatus = async () => { | |
try { | |
const unlocked = await controllerEmitter(['wallet', 'isUnlocked'], []); | |
const wasUnlocked = isUnlocked; | |
const nowUnlocked = !!unlocked; | |
setIsUnlocked(nowUnlocked); | |
// If wallet was unlocked but now locked, redirect to unlock screen | |
// BUT only if there's still an encrypted vault (not forgotten) | |
if (wasUnlocked && !nowUnlocked && !isLoading && hasEncryptedVault) { | |
console.log( | |
'[useController] Wallet became locked, redirecting to unlock screen' | |
); | |
navigate('/', { replace: true }); | |
} | |
// If there's no encrypted vault, don't force redirect - let routing logic handle it | |
if (wasUnlocked && !nowUnlocked && !isLoading && !hasEncryptedVault) { | |
console.log( | |
'[useController] Wallet was forgotten, allowing routing logic to handle navigation' | |
); | |
} | |
return nowUnlocked; | |
} catch (error: any) { | |
// Don't spam console with connection errors during service worker lifecycle | |
if ( | |
!error.message?.includes('Could not establish connection') && | |
!error.message?.includes('Receiving end does not exist') && | |
!error.message?.includes('Network request timed out') | |
) { | |
console.error('[useController] Error checking unlock status:', error); | |
} | |
// If we can't check status due to service worker issues, maintain current state | |
// This prevents flickering during service worker restarts | |
return isUnlocked; | |
} | |
}; | |
// Reset auto-lock timer on user activity | |
const resetAutoLockTimer = async () => { | |
try { | |
await controllerEmitter(['wallet', 'resetAutoLockTimer'], []); | |
} catch (error: any) { | |
// Don't spam console with connection errors during service worker lifecycle | |
if ( | |
!error.message?.includes('Could not establish connection') && | |
!error.message?.includes('Receiving end does not exist') && | |
!error.message?.includes('Network request timed out') | |
) { | |
console.error( | |
'[useController] Error resetting auto-lock timer:', | |
error | |
); | |
} | |
// Silently ignore service worker connection errors - timer will be reset on next successful call | |
} | |
}; | |
// Handle lock state change messages from background | |
const handleLockStateMessage = (message: any) => { | |
if (message.type === 'CONTROLLER_STATE_CHANGE') { | |
// State changes are handled by Redux, but we might have missed a lock event | |
// Double-check unlock status to be safe | |
checkUnlockStatus(); | |
} else if (message.type === 'logout') { | |
// Explicit logout message - only redirect if there's still an encrypted vault | |
console.log('[useController] Logout message received'); | |
setIsUnlocked(false); | |
if (hasEncryptedVault) { | |
console.log( | |
'[useController] Redirecting to unlock screen after logout' | |
); | |
navigate('/', { replace: true }); | |
} else { | |
console.log( | |
'[useController] No encrypted vault, letting routing logic handle navigation' | |
); | |
} | |
} else if (message.type === 'wallet_forgotten') { | |
// Wallet was explicitly forgotten - don't redirect, let routing logic handle it | |
console.log( | |
'[useController] Wallet forgotten message received, letting routing logic handle navigation' | |
); | |
setIsUnlocked(false); | |
} | |
}; | |
useEffect(() => { | |
// Initial unlock status check | |
checkUnlockStatus().then(() => { | |
setIsLoading(false); | |
}); | |
// Listen for Chrome runtime messages about state changes | |
const messageListener = (message: any) => { | |
handleLockStateMessage(message); | |
}; | |
chrome.runtime.onMessage.addListener(messageListener); | |
// Set up periodic polling as a fallback to catch missed lock events | |
// Poll every 10 seconds when unlocked, more frequently when locked | |
let pollInterval: NodeJS.Timeout | null = null; | |
let lockedPollInterval: NodeJS.Timeout | null = null; | |
const startPolling = () => { | |
// Clear any existing intervals | |
if (pollInterval) clearInterval(pollInterval); | |
if (lockedPollInterval) clearInterval(lockedPollInterval); | |
pollInterval = setInterval(async () => { | |
try { | |
const currentlyUnlocked = await checkUnlockStatus(); | |
// If we're locked, check more frequently to detect unlock quickly | |
if (!currentlyUnlocked) { | |
clearInterval(pollInterval!); | |
pollInterval = null; | |
// Start faster polling when locked (every 2 seconds) | |
lockedPollInterval = setInterval(async () => { | |
try { | |
const stillLocked = !(await checkUnlockStatus()); | |
if (!stillLocked) { | |
// When unlocked, switch back to normal polling | |
clearInterval(lockedPollInterval!); | |
lockedPollInterval = null; | |
startPolling(); | |
} | |
} catch (error: any) { | |
// Ignore connection errors during locked polling | |
} | |
}, 2000); | |
} | |
} catch (error: any) { | |
// Only log non-connection errors | |
if ( | |
!error.message?.includes('Could not establish connection') && | |
!error.message?.includes('Receiving end does not exist') && | |
!error.message?.includes('Network request timed out') && | |
!error.message?.includes('Failed to connect to service worker') | |
) { | |
console.error('[useController] Polling error:', error); | |
} | |
} | |
}, 10000); // 10 seconds for normal polling | |
return pollInterval; | |
}; | |
// Only start polling after initial load completes | |
if (!isLoading) { | |
startPolling(); | |
} | |
// Cleanup | |
return () => { | |
chrome.runtime.onMessage.removeListener(messageListener); | |
if (pollInterval) clearInterval(pollInterval); | |
if (lockedPollInterval) clearInterval(lockedPollInterval); | |
}; | |
}, [navigate]); // Only depend on navigate - hasEncryptedVault is accessed directly in scope |
Was this report helpful? Give feedback by reacting with 👍 or 👎
No description provided.