feat: add master password #1963
Conversation
|
Warning Rate limit exceeded@Nick-1979 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 35 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughAdds a master-password migration flow, global unlock/expiry and external signing APIs, metadata/registry enhancements (knownTypes/signedExtensions), many new/updated hooks and UI components, Lottie animation support, storage/constant changes, and bumps @polkadot/* deps to ^16.4.8. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor UI as UI (SignArea / SignUsingPassword)
participant Hooks as Hooks (build extrinsicPayload)
participant BG as Background (Extension)
participant Node as Node (RPC)
UI->>Hooks: Build extrinsicPayload & signerPayload
UI->>BG: pri(signing.getSignature, signerPayload)
BG->>BG: apply registry (knownTypes, signedExtensions) & produce signature
BG-->>UI: return signature (Hex) or null
UI->>Node: submit extrinsic with payloadHex + signature
Node-->>UI: Broadcast → InBlock → Finalized (TxResult)
sequenceDiagram
autonumber
actor User as User
participant Login as Login Screen
participant Check as useCheckMasterPassword
participant Nav as Router
participant Migr as MigratePasswords
participant BG as Background
User->>Login: Enter password
Login->>Check: validate local accounts
alt accounts need migration
Login->>Nav: navigate('/migratePasswords')
User->>Migr: set master password
loop per local account
Migr->>BG: pri(accounts.changePassword or accounts.changePasswordAll)
BG-->>Migr: boolean success/fail
end
Migr->>BG: pri(lockExtension)
Migr->>Nav: navigate('/')
else no migration
Login->>BG: pri(accounts.unlockAll, password, cacheTime)
BG-->>Login: success / failure
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Potential areas needing extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/extension-polkagate/src/components/HasProxyIndicator.tsx (2)
5-5: Inconsistent genesis hash constant sources.The code mixes genesis hash constants from two different sources:
WESTEND_GENESIS_HASHandKUSAMA_GENESIS_HASHfrom../util/constants(line 11)POLKADOT_GENESISfrom@polkagate/apps-config(line 5)This inconsistency is error-prone. All three chains should use constants from the same source to ensure they represent the same data format and are maintained consistently.
Apply this diff to use constants consistently from
@polkagate/apps-config:-import { KUSAMA_GENESIS, POLKADOT_GENESIS, WESTEND_GENESIS } from '@polkagate/apps-config'; +import { KUSAMA_GENESIS_HASH, POLKADOT_GENESIS_HASH, WESTEND_GENESIS_HASH } from '@polkagate/apps-config'; import { Data } from 'iconsax-react'; import React, { useCallback, useEffect, useState } from 'react'; import { useNavigate } from 'react-router-dom'; import { useProxies, useSelectedAccount, useTranslation } from '../hooks'; -import { KUSAMA_GENESIS_HASH, WESTEND_GENESIS_HASH } from '../util/constants'; import MyTooltip from './MyTooltip';Then update the hook calls:
- const westendProxies = useProxies(WESTEND_GENESIS_HASH, account?.address); - const kusamaProxies = useProxies(KUSAMA_GENESIS_HASH, account?.address); - const polkadotProxies = useProxies(POLKADOT_GENESIS, account?.address); + const westendProxies = useProxies(WESTEND_GENESIS_HASH, account?.address); + const kusamaProxies = useProxies(KUSAMA_GENESIS_HASH, account?.address); + const polkadotProxies = useProxies(POLKADOT_GENESIS_HASH, account?.address);And update line 68:
- const genesisHash = hasProxy?.polkadot ? POLKADOT_GENESIS : hasProxy?.kusama ? KUSAMA_GENESIS : WESTEND_GENESIS; + const genesisHash = hasProxy?.polkadot ? POLKADOT_GENESIS_HASH : hasProxy?.kusama ? KUSAMA_GENESIS_HASH : WESTEND_GENESIS_HASH;Also applies to: 11-11, 24-26
37-61: Fix state mutation and stale flag issues.The
useEffecthas two critical problems:
Direct state mutation: The code mutates the previous state object (
pre.westend = true) and returns the same reference. React requires immutable updates—mutating state can prevent re-renders and violates React principles.Stale flags: Once a proxy flag is set to
true, it never resets if proxies later become empty or undefined, leading to incorrect UI state.Apply this diff to fix both issues:
useEffect((): void => { - if (westendProxies?.length) { - setHasProxy((pre) => { - pre.westend = true; - - return pre; - }); - } - - if (kusamaProxies?.length) { - setHasProxy((pre) => { - pre.kusama = true; - - return pre; - }); - } - - if (polkadotProxies?.length) { - setHasProxy((pre) => { - pre.polkadot = true; - - return pre; - }); - } + setHasProxy({ + westend: !!westendProxies?.length, + kusama: !!kusamaProxies?.length, + polkadot: !!polkadotProxies?.length + }); }, [kusamaProxies, polkadotProxies, westendProxies]);packages/extension-polkagate/src/hooks/useProxies.ts (1)
28-46: Stale data when proxiedAddress changes.The
getProxiescallback only fetches if!proxies, which means once proxies are fetched for one address, changingproxiedAddresswon't trigger a refetch. The hook will return stale proxies from the previous address.Example scenario:
- Hook called with
proxiedAddress = 'address_A'→ fetches and sets proxies for A- Component updates
proxiedAddress = 'address_B'→useEffectruns, callsgetProxies(), but condition!proxiesis false, so no refetch occurs- Hook returns proxies for address A instead of B
Apply this diff to reset proxies when the address changes:
+ useEffect(() => { + // Reset proxies when address changes to trigger refetch + setProxies(undefined); + setProxiesWithAvailability(undefined); + }, [proxiedAddress]); + const getProxies = useCallback(() => { if (!proxies && api) {packages/extension-polkagate/src/components/SignArea3.tsx (1)
102-137: Fix SignerPayload construction to use correct fields and types
- method: use
preparedTransaction.method.toHex()instead ofapi.createType('Call', preparedTransaction)- replace
runtimeVersionwith
specVersion: api.runtimeVersion.specVersiontransactionVersion: api.runtimeVersion.transactionVersion- use
api.registry.signedExtensionsinstead of the hardcoded extension list- wrap
tipinapi.registry.createType('Compact<Balance>', 0)and ensurenonceisCompact<Index>packages/extension-polkagate/src/components/SignUsingPassword.tsx (1)
87-121: Allow retrial when signature retrieval failsIf
getSignaturereturnsnull(account locked) or throws, we sethasErrortotrueand never clear it. Because the buttons are disabled whenhasErroris true, the user gets stuck with no way to try again after unlocking the account. Instead of latching the error flag, clear it before the next attempt (or gate the disable condition on something retryable) and surface the error via existing flows so the user can recover.
🧹 Nitpick comments (8)
packages/extension-polkagate/src/components/ShortAddress.tsx (1)
29-29: Consider using a functional updater for consistency.For consistency with the improvement made on line 43, the
decreaseCharactersCountcallback should also use a functional state updater. This prevents potential stale closure issues and aligns with React best practices.Apply this diff:
- const decreaseCharactersCount = useCallback(() => clipped && setCharactersCount(charactersCount - 1), [charactersCount, clipped]); + const decreaseCharactersCount = useCallback(() => clipped && setCharactersCount((prev) => prev - 1), [clipped]);packages/extension-polkagate/src/hooks/useProxies.ts (1)
40-40: Improve error handling beyond console logging.Using
console.errorprovides no user feedback or recovery mechanism. Consider propagating errors to the caller or logging to a monitoring service.Example approach:
- }).catch(console.error); + }).catch((error) => { + console.error('Failed to fetch proxies:', error); + // Optionally: set an error state that callers can check + // setProxies([]); + });packages/extension-polkagate/src/components/PasswordInput.tsx (2)
66-66: Fix spacing in type annotation.The type annotation has a missing space after the colon.
Apply this diff:
- value?:string; + value?: string;
80-82: Remove unnecessary nullish coalescing operator.The
?? nullin the onChange handler is confusing and serves no purpose.HTMLInputElement.valueis always a string (nevernullorundefined), so the nullish coalescing operator will never trigger. The code should simply passvaluedirectly.Apply this diff to simplify the onChange handler:
const onChange = useCallback(({ target: { value } }: React.ChangeEvent<HTMLInputElement>) => { - onPassChange(value ?? null); + onPassChange(value); }, [onPassChange]);packages/extension-polkagate/src/fullscreen/onboarding/Bread.tsx (1)
77-83: Align the cursor with the disabled state.While MIGRATE_PASSWORDS reuses the disabled color, the stack still shows a pointer cursor, suggesting it's clickable. Switch the cursor to
defaultfor both WELCOME and MIGRATE states to match the navigation guard.packages/extension-polkagate/src/components/MatchPasswordField.tsx (1)
90-99: Promise handling: prefer await or drop try/catchtry/catch here won’t catch async rejections; you already chain .catch. Either await or remove try/catch.
Suggested:
- const handleConfirm = useCallback(() => { + const handleConfirm = useCallback(async () => { if (passwordStatus === PASSWORD_STATUS.MATCHED && onSetPassword) { - try { - onSetPassword()?.catch(console.error); - } catch (error) { - console.error('Error setting password:', error); - } + try { + await onSetPassword?.(); + } catch (error) { + console.error('Error setting password:', error); + } } - }, [onSetPassword, passwordStatus]); + }, [onSetPassword, passwordStatus]);packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts (1)
14-24: Redundant isExternal checkYou already filter non-external accounts; remove the inner isExternal guard.
- const results = await Promise.all(nonExternalAccount.map(async (account) => { - const { address, isExternal } = account; - - if (isExternal) { - return; - } + const results = await Promise.all(nonExternalAccount.map(async (account) => { + const { address } = account;packages/extension-polkagate/src/util/workers/utils/metadataFromApi.ts (1)
28-40: metaCalls size and schema alignment
- Using full runtimeMetadata increases payload size; calls-only is lighter and commonly used for injection metadata.
- metadataVersion is not part of MetadataDef (in this PR). Either add it to the interface or omit.
Suggested:
- knownTypes: api.registry.knownTypes, - metaCalls: base64Encode(api.runtimeMetadata.toU8a()), - metadataVersion: api.runtimeMetadata.version, - signedExtensions: api.registry.signedExtensions, + knownTypes: api.registry.knownTypes, + // keep calls-only to minimize payload + metaCalls: base64Encode(api.runtimeMetadata.asCallsOnly.toU8a()), + signedExtensions: api.registry.signedExtensions, @@ - types: getSpecTypes(api.registry, chainName, api.runtimeVersion.specName, api.runtimeVersion.specVersion) as Record<string, string | Record<string, string>>, - userExtensions: (api.registry as any).userExtensions + types: getSpecTypes(api.registry, chainName, api.runtimeVersion.specName, api.runtimeVersion.specVersion) as Record<string, string | Record<string, string>>, + userExtensions: (api.registry as any).userExtensionsIf you need metadataVersion, please add it as an optional field to MetadataDef for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (41)
package.json(3 hunks)packages/extension-base/package.json(1 hunks)packages/extension-base/src/background/handlers/Extension.ts(6 hunks)packages/extension-base/src/background/types.ts(4 hunks)packages/extension-base/src/defaults.ts(1 hunks)packages/extension-chains/src/bundle.ts(2 hunks)packages/extension-inject/package.json(1 hunks)packages/extension-inject/src/types.ts(2 hunks)packages/extension-polkagate/src/class/endpointManager.ts(1 hunks)packages/extension-polkagate/src/class/endpointManager2.ts(1 hunks)packages/extension-polkagate/src/components/HasProxyIndicator.tsx(2 hunks)packages/extension-polkagate/src/components/Loading.tsx(2 hunks)packages/extension-polkagate/src/components/MatchPasswordField.tsx(3 hunks)packages/extension-polkagate/src/components/PasswordInput.tsx(5 hunks)packages/extension-polkagate/src/components/ShortAddress.tsx(1 hunks)packages/extension-polkagate/src/components/SignArea3.tsx(8 hunks)packages/extension-polkagate/src/components/SignUsingPassword.tsx(7 hunks)packages/extension-polkagate/src/context/ExtensionLockContext.tsx(2 hunks)packages/extension-polkagate/src/fullscreen/onboarding/Bread.tsx(3 hunks)packages/extension-polkagate/src/fullscreen/settings/partials/Password.tsx(1 hunks)packages/extension-polkagate/src/hooks/index.ts(1 hunks)packages/extension-polkagate/src/hooks/useAutoLockPeriod.ts(1 hunks)packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts(1 hunks)packages/extension-polkagate/src/hooks/useIsFlying.tsx(1 hunks)packages/extension-polkagate/src/hooks/useIsPasswordMigrated.ts(1 hunks)packages/extension-polkagate/src/hooks/useLoginInfo.tsx(1 hunks)packages/extension-polkagate/src/hooks/useProxies.ts(1 hunks)packages/extension-polkagate/src/messaging.ts(3 hunks)packages/extension-polkagate/src/partials/Review.tsx(1 hunks)packages/extension-polkagate/src/popup/home/ManageHome.tsx(1 hunks)packages/extension-polkagate/src/popup/passwordManagement/Login.tsx(4 hunks)packages/extension-polkagate/src/popup/passwordManagement/MigratePasswords.tsx(1 hunks)packages/extension-polkagate/src/popup/passwordManagement/types.ts(1 hunks)packages/extension-polkagate/src/util/api/signAndSend.ts(1 hunks)packages/extension-polkagate/src/util/chains.ts(2 hunks)packages/extension-polkagate/src/util/constants.ts(2 hunks)packages/extension-polkagate/src/util/workers/utils/metadataFromApi.ts(1 hunks)packages/extension-ui/src/Popup/routes/featuresRoutes.ts(2 hunks)packages/extension/package.json(1 hunks)packages/extension/public/locales/en/translation.json(1 hunks)packages/extension/src/packageInfo.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (20)
packages/extension/src/packageInfo.ts (6)
packages/extension-chains/src/bundle.ts (1)
packageInfo(11-11)packages/extension-base/src/packageInfo.ts (1)
packageInfo(6-6)packages/extension-compat-metamask/src/packageInfo.ts (1)
packageInfo(6-6)packages/extension-chains/src/packageInfo.ts (1)
packageInfo(6-6)packages/extension-dapp/src/packageInfo.ts (1)
packageInfo(6-6)packages/extension-inject/src/packageInfo.ts (1)
packageInfo(6-6)
packages/extension-polkagate/src/popup/home/ManageHome.tsx (1)
packages/extension-polkagate/src/popup/home/index.tsx (1)
Home(25-90)
packages/extension-polkagate/src/hooks/useIsPasswordMigrated.ts (2)
packages/extension-polkagate/src/components/Loading.tsx (1)
getStorage(38-48)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(302-320)
packages/extension-polkagate/src/context/ExtensionLockContext.tsx (2)
packages/extension-polkagate/src/hooks/useIsPasswordMigrated.ts (1)
useIsPasswordMigrated(9-19)packages/extension-polkagate/src/messaging.ts (1)
areAccountsLocksExpired(100-102)
packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts (4)
packages/extension-base/src/background/types.ts (1)
AccountJson(40-62)packages/extension-base/src/background/handlers/Extension.ts (1)
accountsValidate(218-226)packages/extension-polkagate/src/messaging.ts (1)
accountsValidate(84-86)packages/extension-polkagate/src/components/contexts.tsx (1)
AccountContext(34-34)
packages/extension-base/src/background/types.ts (1)
packages/extension-inject/src/types.ts (1)
MetadataDef(65-75)
packages/extension-polkagate/src/util/api/signAndSend.ts (1)
packages/extension-polkagate/src/util/types.ts (1)
TxResult(149-155)
packages/extension-polkagate/src/hooks/useLoginInfo.tsx (5)
packages/extension-polkagate/src/hooks/useAutoLockPeriod.ts (1)
useAutoLockPeriod(30-44)packages/extension-polkagate/src/context/ExtensionLockContext.tsx (1)
useExtensionLockContext(16-24)packages/extension-polkagate/src/components/Loading.tsx (2)
getStorage(38-48)setStorage(71-84)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(302-320)packages/extension-polkagate/src/popup/passwordManagement/types.ts (2)
LoginInfo(15-21)LOGIN_STATUS(4-11)
packages/extension-polkagate/src/components/Loading.tsx (3)
packages/extension-polkagate/src/hooks/useIsFlying.tsx (1)
useIsFlying(10-30)packages/extension-polkagate/src/hooks/useLoginInfo.tsx (1)
useLoginInfo(13-64)packages/extension-polkagate/src/util/constants.ts (1)
ALLOWED_URL_ON_RESET_PASSWORD(230-230)
packages/extension-polkagate/src/popup/passwordManagement/Login.tsx (10)
packages/extension-polkagate/src/hooks/useIsExtensionPopup.ts (1)
useIsExtensionPopup(4-21)packages/extension-polkagate/src/hooks/useIsPasswordMigrated.ts (1)
useIsPasswordMigrated(9-19)packages/extension-polkagate/src/context/ExtensionLockContext.tsx (1)
useExtensionLockContext(16-24)packages/extension-polkagate/src/hooks/useAutoLockPeriod.ts (1)
useAutoLockPeriod(30-44)packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts (1)
useCheckMasterPassword(29-45)packages/extension-polkagate/src/popup/settings/extensionSettings/ManagePassword.tsx (1)
isPasswordCorrect(18-23)packages/extension-polkagate/src/messaging.ts (2)
unlockAllAccounts(96-98)windowOpen(259-261)packages/extension-polkagate/src/components/Loading.tsx (2)
setStorage(71-84)updateStorage(19-36)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(302-320)packages/extension-polkagate/src/popup/passwordManagement/types.ts (1)
LOGIN_STATUS(4-11)
packages/extension-polkagate/src/popup/passwordManagement/MigratePasswords.tsx (7)
packages/extension-polkagate/src/hooks/useAlerts.ts (1)
useAlerts(12-31)packages/extension-polkagate/src/context/ExtensionLockContext.tsx (1)
useExtensionLockContext(16-24)packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts (1)
useCheckMasterPassword(29-45)packages/extension-base/src/background/handlers/Extension.ts (2)
accountsChangePassword(91-109)lockExtension(134-162)packages/extension-polkagate/src/messaging.ts (2)
accountsChangePassword(88-90)lockExtension(80-82)packages/extension-polkagate/src/components/Loading.tsx (1)
setStorage(71-84)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(302-320)
packages/extension-polkagate/src/hooks/useProxies.ts (2)
packages/extension-polkagate/src/util/types.ts (2)
ProxyTypes(582-582)Proxy(584-589)packages/extension-polkagate/src/hooks/useChainInfo.ts (1)
useChainInfo(42-73)
packages/extension-polkagate/src/components/SignArea3.tsx (2)
packages/extension-polkagate/src/hooks/useProxies.ts (1)
useProxies(21-61)packages/extension-polkagate/src/util/api/signAndSend.ts (1)
send(93-150)
packages/extension-polkagate/src/hooks/useIsFlying.tsx (1)
packages/extension-polkagate/src/hooks/useIsExtensionPopup.ts (1)
useIsExtensionPopup(4-21)
packages/extension-polkagate/src/util/chains.ts (1)
replacements/interfaces.js (2)
selectableNetworks(150-150)selectableNetworks(150-150)
packages/extension-chains/src/bundle.ts (1)
packages/extension-inject/src/types.ts (1)
MetadataDef(65-75)
packages/extension-polkagate/src/messaging.ts (2)
packages/extension-base/src/background/handlers/Extension.ts (3)
accountsValidate(218-226)accountsChangePassword(91-109)getSignature(506-525)packages/extension-inject/src/types.ts (1)
MetadataDef(65-75)
packages/extension-polkagate/src/components/SignUsingPassword.tsx (3)
packages/extension-base/src/background/handlers/Extension.ts (1)
getSignature(506-525)packages/extension-polkagate/src/messaging.ts (1)
getSignature(92-94)packages/extension-polkagate/src/util/api/signAndSend.ts (1)
submitExtrinsic(152-204)
packages/extension-polkagate/src/components/HasProxyIndicator.tsx (2)
packages/extension-polkagate/src/hooks/useProxies.ts (1)
useProxies(21-61)packages/extension-polkagate/src/util/constants.ts (2)
WESTEND_GENESIS_HASH(60-60)KUSAMA_GENESIS_HASH(59-59)
packages/extension-base/src/background/handlers/Extension.ts (1)
packages/extension-base/src/background/types.ts (3)
RequestUnlockAllAccounts(402-405)RequestSigninSignature(313-315)RequestUpdateMeta(350-353)
🪛 Biome (2.1.2)
packages/extension-polkagate/src/popup/passwordManagement/Login.tsx
[error] 52-52: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
[error] 112-112: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (23)
packages/extension-polkagate/src/fullscreen/settings/partials/Password.tsx (2)
11-11: LGTM! Import consolidation improves code cleanliness.The import statement consolidation removes duplication and maintains the same functionality. The hook is correctly destructured and all values are used appropriately in the component.
18-18: LGTM! Formatting adjustment.The space before the closing parenthesis is a formatting preference with no functional impact.
packages/extension-polkagate/src/components/ShortAddress.tsx (1)
42-44: Good improvement: functional state updater.The change to use
setCharactersCount((prev) => prev + 1)instead of referencingcharactersCountdirectly is a best practice for state updates that depend on the previous state value. This ensures the latest state is always used, especially in concurrent rendering scenarios.packages/extension-polkagate/src/hooks/useProxies.ts (1)
21-22: LGTM! Clean API simplification.The refactor to accept
genesisHashand derive the API internally viauseChainInfosimplifies caller code and maintains a clear separation of concerns.packages/extension-polkagate/src/components/PasswordInput.tsx (3)
112-112: Good accessibility improvement!Adding
tabIndex={-1}to the password visibility toggle button prevents it from being included in the keyboard tab order. This allows users to tab directly from the password input to the next focusable element, improving the keyboard navigation experience.
4-4: LGTM!The Grid wrapper replacement is a straightforward layout change that maintains the component's functionality.
Also applies to: 97-97, 150-150
66-66: Consistent uncontrolled usage confirmed. All PasswordInput instances omit the value prop (uncontrolled mode) consistently; no risk of switching modes.packages/extension-polkagate/src/hooks/useAutoLockPeriod.ts (1)
17-29: LGTM! Clear documentation added.The JSDoc provides a clear description of the hook's behavior, return values, and usage example without altering functionality.
packages/extension-base/package.json (1)
24-24: LGTM! Consistent Polkadot JS dependency updates.The updates to
@polkadot/api,@polkadot/rpc-provider, and@polkadot/typesare aligned with the broader PR-wide upgrade to ^16.4.8. The verification requested inpackages/extension/package.jsonwill cover these dependencies as well.Also applies to: 31-32
packages/extension-polkagate/src/popup/home/ManageHome.tsx (1)
13-13: No functional changes detected.Despite the change marker, this line appears to contain the same logic as before. If this was only a formatting adjustment, no review is needed.
packages/extension-polkagate/src/popup/passwordManagement/types.ts (1)
21-21: Cosmetic change: trailing newline added.This is a standard formatting convention with no functional impact.
packages/extension-inject/package.json (1)
21-23: LGTM! Consistent Polkadot JS dependency updates.These updates align with the broader PR-wide upgrade to ^16.4.8, maintaining consistency across packages.
packages/extension/src/packageInfo.ts (1)
6-6: LGTM! Version bump aligns with package.json.The version update from
1.12.1to1.13.1is consistent with the corresponding change inpackages/extension/package.json. As this is an auto-generated file, ensure the build process regenerates it correctly.packages/extension-polkagate/src/class/endpointManager2.ts (1)
6-6: Confirm ENDPOINT_TIMEOUT value remains unchanged.Please ensure the new
ENDPOINT_TIMEOUT = AUTO_LOCK_PERIOD_DEFAULT * 60 * 1000(15 minutes) matches the previousNO_PASS_PERIODvalue to maintain backward compatibility.packages/extension/package.json (1)
18-20: Validated @polkadot/api@^16.4.8 with no security advisories Confirmed the package version is published and no vulnerabilities were found. Safe to merge.packages/extension-polkagate/src/components/MatchPasswordField.tsx (2)
101-114: Grid swap LGTMSwitch to MUI Grid with sx={style} is fine.
4-4: LGTM: import updateImporting Grid from MUI matches the JSX change.
packages/extension-inject/src/types.ts (2)
9-9: Import RegisteredTypes LGTM
72-75: Extend MetadataDef LGTMOptional knownTypes and signedExtensions keep backward compat.
packages/extension-polkagate/src/partials/Review.tsx (1)
281-282: Prop passthrough LGTMwithCancel is correctly passed to SignArea3.
packages/extension-polkagate/src/hooks/index.ts (3)
47-47: Export useIsFlying LGTM
52-52: Export useIsPasswordMigrated LGTM
56-56: Export useLoginInfo LGTM
packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts
Outdated
Show resolved
Hide resolved
packages/extension-polkagate/src/popup/passwordManagement/Login.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/extension-polkagate/src/util/api/signAndSend.ts (1)
166-201: Extrinsics that fail now hang and incorrectly report successThis implementation never inspects
dispatchErrororsystem.ExtrinsicFailedevents, so any on-chain failure (e.g., insufficient balance, module error) is still reported assuccess: true. Additionally, if the node rejects or drops the extrinsic before it appears in a finalized block, the promise never resolves and the UI hangs indefinitely. The previoussendfunction (lines 93-150) correctly handles dispatch errors via the status callback. Switch toapi.rpc.author.submitAndWatchExtrinsicto monitor tx status and events, inspectresult.dispatchErrorto populatefailureTextand setsuccess: falseon failures, and always unsubscribe on terminal states.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/extension-polkagate/src/util/api/signAndSend.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/extension-polkagate/src/util/api/signAndSend.ts (1)
packages/extension-polkagate/src/util/types.ts (1)
TxResult(149-155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/extension-base/src/background/handlers/Extension.ts (2)
481-486: Return after rejecting on missing password to avoid decode with undefined.Currently, execution continues and pair.decodePkcs8(password) may throw with undefined.
// if the keyring pair is locked, the password is needed if (pair.isLocked && !password) { reject(new Error('Password needed to unlock the account')); + return false; }
491-496: Use the per-request registry when signing.Adopt createSigningRegistry for JSON payloads and avoid the global registry.
- if (isJsonPayload(payload)) { - this.handleRegistry(payload); - } - - const result = request.sign(registry, pair); + const reg = isJsonPayload(payload) + ? this.createSigningRegistry(payload) + : new TypeRegistry(); + + const result = request.sign(reg, pair);
♻️ Duplicate comments (2)
packages/extension-polkagate/src/components/Loading.tsx (1)
93-105: Still normalize the reset URL before allow-listingReset routes that carry query strings (
/forgot-password?address=...) or nested segments still miss the allow-list because we compare the raw hash viaincludes. The reset flow stays locked behind the login overlay, same bug as previously reported.Normalize the hash (strip query/fragment) and compare with
some/startsWithso reset URLs continue to work even with params.- const extensionUrl = window.location.hash.replace('#', ''); + const extensionUrl = window.location.hash.replace('#', ''); + const currentPath = extensionUrl.split(/[?#]/)[0]; const condition = isExtensionLocked || !children || isFlying; - const isResettingWallet = step === STEPS.SHOW_LOGIN && ALLOWED_URL_ON_RESET_PASSWORD.includes(extensionUrl); + const isResettingWallet = step === STEPS.SHOW_LOGIN && + ALLOWED_URL_ON_RESET_PASSWORD.some((allowedPath) => currentPath.startsWith(allowedPath));packages/extension-base/src/background/handlers/Extension.ts (1)
136-145: Lock loop: handle forgotten accounts and purge stale cache entries (duplicate of prior review).keyring.getPair(address) can throw for forgotten accounts, aborting the loop and leaving other accounts unlocked. Catch and delete the stale cache entry, then continue.
- Object.entries(this.#cachedUnlocks).forEach(([address]) => { - const pair = keyring.getPair(address); - - if (pair && !pair.isLocked) { - pair.lock(); - } - - this.#cachedUnlocks[address] = 0; - }); + Object.entries(this.#cachedUnlocks).forEach(([address]) => { + try { + const pair = keyring.getPair(address); + + if (!pair.isLocked) { + pair.lock(); + } + + this.#cachedUnlocks[address] = 0; + } catch { + delete this.#cachedUnlocks[address]; + } + });
🧹 Nitpick comments (2)
packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/index.tsx (1)
46-46: Remove or document redundantas voidon navigate calls.The codebase consistently annotates
navigate(…)withas void, but in React Router v6navigatealready returnsvoid. Unless a specific lint rule or TypeScript setting mandates these assertions, they’re unnecessary. Either drop them or add a comment/guideline explaining why they’re needed.packages/extension-base/src/background/handlers/Extension.ts (1)
407-437: Unlock‑all: clamp cacheTime and remove debug logs.
- console.log of accounts leaks PII in prod logs.
- cacheTime is unbounded; clamp to a safe max (e.g., PASSWORD_EXPIRY_MS) and validate > 0.
- console.log('get message to unlock accounts ...'); + // unlocking multiple accounts with provided password @@ - console.log('localAccounts:', localAccounts); + // local (non-external) accounts only @@ - this.#cachedUnlocks[address] = Date.now() + cacheTime; + const safeCache = + Number.isFinite(cacheTime) && cacheTime > 0 + ? Math.min(cacheTime, PASSWORD_EXPIRY_MS) + : PASSWORD_EXPIRY_MS; + this.#cachedUnlocks[address] = Date.now() + safeCache;Please confirm whether different per-account passwords are still supported post‑migration; if yes, consider short‑circuiting on the first decodePkcs8 failure and returning false instead of throwing so the caller can surface a friendly error.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/extension-base/src/background/handlers/Extension.ts(7 hunks)packages/extension-polkagate/src/components/Loading.tsx(2 hunks)packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/index.tsx(2 hunks)packages/extension-polkagate/src/util/defaultType.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/extension-polkagate/src/util/defaultType.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/extension-polkagate/src/components/Loading.tsx (3)
packages/extension-polkagate/src/hooks/useIsFlying.tsx (1)
useIsFlying(10-30)packages/extension-polkagate/src/hooks/useLoginInfo.tsx (1)
useLoginInfo(13-64)packages/extension-polkagate/src/util/constants.ts (1)
ALLOWED_URL_ON_RESET_PASSWORD(230-230)
packages/extension-base/src/background/handlers/Extension.ts (1)
packages/extension-base/src/background/types.ts (3)
RequestUnlockAllAccounts(402-405)RequestSigninSignature(313-315)RequestUpdateMeta(350-353)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Release
🔇 Additional comments (1)
packages/extension-base/src/background/handlers/Extension.ts (1)
439-441: Empty cache returns true — confirm semantics.areLocksExpired() returns true when cache is empty. If the intent is “no active unlocks ⇒ expired,” this is fine; otherwise, adjust to return false when there are no entries.
since login is mandatory
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/extension-polkagate/src/popup/settings/partials/actions/Lock.tsx (1)
25-34: Strengthen validation forautoLockPeriod.The guard only checks for
undefined, which may not catch all invalid states:
- Incomplete validation: If
autoLockPeriodcan benull,0, or negative, these values would pass the check but could produce incorrect behavior.- Line 29 calculation risk:
Date.now() - autoLockPeriodwithautoLockPeriod = 0yields the current time (no lock), and negative values could set a future timestamp.- No user feedback: When
autoLockPeriod === undefined, the button silently does nothing.Consider strengthening the validation:
const onClick = useCallback((): void => { - if (autoLockPeriod === undefined) { + if (autoLockPeriod === undefined || autoLockPeriod === null || autoLockPeriod <= 0) { return; } updateStorage(STORAGE_KEY.LOGIN_INFO, { lastLoginTime: Date.now() - autoLockPeriod }).then(() => { setExtensionLock(true); navigate('/') as void; lockExtension().catch(console.error); }).catch(console.error); }, [autoLockPeriod, navigate, setExtensionLock]);
♻️ Duplicate comments (5)
packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts (1)
33-44: Guard against undefined accounts and reset state properly.This issue was previously flagged but remains unresolved. The current implementation has two problems:
- Line 40 can call
getAccountsNeedsMigrationwithundefinedaccounts (when context hasn't loaded yet), causing a runtime error when trying to filter.- Lines 34-36 return early when password is missing but don't reset the state, leaving stale migration data visible.
Apply this diff to fix both issues:
useEffect(() => { - if (!pass) { + if (!pass || !accounts?.length) { + setAccountsNeedMigration(undefined); return; } setAccountsNeedMigration(undefined); getAccountsNeedsMigration(accounts, pass).then((res) => { setAccountsNeedMigration(res); }).catch(console.error); // eslint-disable-next-line react-hooks/exhaustive-deps }, [accounts?.length, pass]);packages/extension-base/src/background/handlers/Extension.ts (4)
136-145: Guard against missing pairs and clean up stale cache entries.As previously noted,
keyring.getPair(address)throws when an account is forgotten but the cache entry remains. The thrown error aborts the loop, preventing other accounts from being locked.Apply the previously suggested fix:
- Object.entries(this.#cachedUnlocks).forEach(([address]) => { - const pair = keyring.getPair(address); - - if (pair && !pair.isLocked) { - pair.lock(); - } - - this.#cachedUnlocks[address] = 0; - }); + Object.entries(this.#cachedUnlocks).forEach(([address]) => { + try { + const pair = keyring.getPair(address); + + if (!pair.isLocked) { + pair.lock(); + } + + this.#cachedUnlocks[address] = 0; + } catch (error) { + delete this.#cachedUnlocks[address]; + } + });
163-163: chrome.tabs.reload does not return a Promise.As previously flagged, calling
.catchonchrome.tabs.reloadwill throw at runtime since the Chrome API doesn't return a Promise.Apply the previously suggested fix:
- chrome.tabs.reload(id).catch(console.error); + withErrorLog(() => chrome.tabs.reload(id));
451-465: Create a per-request registry instead of mutating the global instance.As previously noted, mutating the shared global
registrycan cause race conditions when multiple signing requests are processed concurrently. Each request should use its own isolated TypeRegistry instance.Apply the previously suggested refactor:
- private handleRegistry (payload: SignerPayloadJSON): void { - // Get the metadata for the genesisHash - const currentMetadata = this.metadataGet(payload.genesisHash); - - // set the registry before calling the sign function - const signedExtensions = currentMetadata?.signedExtensions?.length - ? currentMetadata.signedExtensions - : registry.signedExtensions; - - registry.setSignedExtensions(signedExtensions, currentMetadata?.userExtensions); - - if (currentMetadata) { - registry.register(currentMetadata?.types); - } - } + private createSigningRegistry (payload: SignerPayloadJSON): TypeRegistry { + const reg = new TypeRegistry(); + const currentMetadata = this.metadataGet(payload.genesisHash); + const signedExtensions = currentMetadata?.signedExtensions?.length + ? currentMetadata.signedExtensions + : reg.signedExtensions; + + reg.setSignedExtensions(signedExtensions, currentMetadata?.userExtensions); + + if (currentMetadata?.types) { + reg.register(currentMetadata.types); + } + + return reg; + }
521-540: Guard against missing pairs and use a per-request registry.As previously noted:
keyring.getPair(address)can throw for unknown accounts - should return null insteadhandleRegistrymutates the global registry, causing potential race conditionsApply the previously suggested fix (adapted for the refactored
createSigningRegistry):private getSignature ({ payload }: RequestSigninSignature): HexString | null { const { address } = payload; if (!payload?.address || !payload.genesisHash) { throw new Error('Invalid payload: missing required fields.'); } + let pair: KeyringPair; + + try { + pair = keyring.getPair(address); + } catch { + return null; + } - const pair = keyring.getPair(address); this.refreshAccountPasswordCache(pair); // check if auto lock duration is expired if (pair.isLocked) { return null; } - this.handleRegistry(payload); - const { signature } = registry.createType('ExtrinsicPayload', payload, { version: payload.version }).sign(pair); + const reg = this.createSigningRegistry(payload); + const { signature } = reg + .createType('ExtrinsicPayload', payload, { version: payload.version }) + .sign(pair); return signature; }
🧹 Nitpick comments (5)
packages/extension-polkagate/src/fullscreen/settings/partials/AutoLockTimer.tsx (1)
78-78: LGTM! Label change improves clarity.The label change from "Enable Auto-Lock" to "Adjust Auto-Lock" better reflects that this control manages both the enabled state and timer configuration, not just enabling the feature.
Optional formatting nitpick: Remove the space after the
=sign for consistency with standard JSX formatting conventions (label={t('Adjust Auto-Lock')}).packages/extension-polkagate/src/hooks/index.ts (1)
47-47: Clarify theuseIsFlyinghook nameRename
useIsFlyingto something more descriptive—e.g.useIsInitializing,useIsBooting, oruseIsStartingUp—to better convey its purpose.packages/extension-polkagate/src/hooks/useAutoLock.ts (2)
32-32: Add runtime validation for storage data.The type assertion
as AutoLock|undefinedbypasses compile-time type checking. IfgetStoragereturns malformed or unexpected data (e.g., from corrupted storage or version migration), this could lead to runtime issues downstream when the invalid object is used.Consider adding a type guard or validation function:
+function isValidAutoLock(value: unknown): value is AutoLock { + if (!value || typeof value !== 'object') return false; + const obj = value as Record<string, unknown>; + return ( + typeof obj.enabled === 'boolean' && + typeof obj.delay === 'object' && + obj.delay !== null && + typeof (obj.delay as Record<string, unknown>).value === 'number' && + ['min', 'hour', 'day'].includes((obj.delay as Record<string, unknown>).type as string) + ); +} + useEffect(() => { getStorage('autoLock').then((res) => { - setAutoLock(res as AutoLock|undefined ?? DEFAULT_AUTO_LOCK); + setAutoLock(isValidAutoLock(res) ? res : DEFAULT_AUTO_LOCK); }).catch(console.error);
35-41: Apply the same validation to storage change events.The type assertion on line 37 has the same type-safety concern as line 32. The Chrome storage API doesn't enforce type constraints, so
newValueshould be validated before use.If you implement the validation guard suggested for line 32, apply it here as well:
chrome.storage.onChanged.addListener(function (changes, areaName) { if (areaName === 'local' && 'autoLock' in changes) { - const newValue = changes['autoLock'].newValue as AutoLock; - - setAutoLock(newValue); + const newValue = changes['autoLock'].newValue; + if (isValidAutoLock(newValue)) { + setAutoLock(newValue); + } } });packages/extension-polkagate/src/popup/settings/partials/actions/Lock.tsx (1)
41-41: Consider using theme colors instead of hardcoded values.The color logic is now cleaner, but hardcoded hex values
'#AA83DC'and'#745D8B'could be replaced with theme constants for better maintainability and consistency.If your theme defines these purple shades, reference them instead:
- <Unlock color={isDark ? '#AA83DC' : '#745D8B'} size={18} variant='Bulk' /> + <Unlock color={theme.palette.primary.light} size={18} variant='Bulk' />Or add the colors to your theme if they're not already defined.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
packages/extension-base/src/background/handlers/Extension.ts(7 hunks)packages/extension-polkagate/src/fullscreen/settings/partials/AutoLockTimer.tsx(1 hunks)packages/extension-polkagate/src/hooks/index.ts(1 hunks)packages/extension-polkagate/src/hooks/useAssetsBalances.ts(2 hunks)packages/extension-polkagate/src/hooks/useAutoLock.ts(1 hunks)packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts(1 hunks)packages/extension-polkagate/src/hooks/useIsLoginEnabled.ts(0 hunks)packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/index.tsx(5 hunks)packages/extension-polkagate/src/popup/settings/extensionSettings/partials/AutoLockTimer.tsx(1 hunks)packages/extension-polkagate/src/popup/settings/partials/actions/Lock.tsx(3 hunks)packages/extension-ui/src/Popup/contexts/AccountAssetProvider.tsx(2 hunks)packages/extension/public/locales/en/translation.json(1 hunks)packages/extension/public/locales/es/translation.json(0 hunks)packages/extension/public/locales/fr/translation.json(0 hunks)packages/extension/public/locales/hi/translation.json(0 hunks)packages/extension/public/locales/ru/translation.json(0 hunks)packages/extension/public/locales/zh/translation.json(0 hunks)
💤 Files with no reviewable changes (6)
- packages/extension/public/locales/ru/translation.json
- packages/extension/public/locales/es/translation.json
- packages/extension/public/locales/fr/translation.json
- packages/extension/public/locales/hi/translation.json
- packages/extension/public/locales/zh/translation.json
- packages/extension-polkagate/src/hooks/useIsLoginEnabled.ts
🧰 Additional context used
🧬 Code graph analysis (4)
packages/extension-polkagate/src/hooks/useAssetsBalances.ts (2)
packages/extension-base/src/background/types.ts (1)
AccountJson(40-62)packages/extension-polkagate/src/util/types.ts (2)
DropdownOption(877-880)UserAddedChains(722-722)
packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts (3)
packages/extension-base/src/background/types.ts (1)
AccountJson(40-62)packages/extension-base/src/background/handlers/Extension.ts (1)
accountsValidate(225-233)packages/extension-polkagate/src/messaging.ts (1)
accountsValidate(84-86)
packages/extension-ui/src/Popup/contexts/AccountAssetProvider.tsx (2)
packages/extension-polkagate/src/context/ExtensionLockContext.tsx (1)
useExtensionLockContext(16-24)packages/extension-polkagate/src/hooks/useAssetsBalances.ts (1)
useAssetsBalances(38-171)
packages/extension-base/src/background/handlers/Extension.ts (1)
packages/extension-base/src/background/types.ts (3)
RequestUnlockAllAccounts(402-405)RequestSigninSignature(313-315)RequestUpdateMeta(350-353)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (10)
packages/extension-polkagate/src/hooks/index.ts (2)
51-51: LGTM!The export is correctly formatted, alphabetically ordered, and the hook name clearly conveys its purpose.
55-55: LGTM!The export is correctly formatted, alphabetically ordered, and the hook name clearly conveys its purpose for providing login flow information.
packages/extension-polkagate/src/hooks/useAssetsBalances.ts (2)
38-38: LGTM: Parameter addition is sound.The optional
isExtensionLockedparameter maintains backward compatibility and follows good naming conventions.
47-47: AddshouldFetchAssetsto the useEffect dependency array.The computed value
shouldFetchAssetsdepends onisExtensionLocked, but it's not listed in the useEffect dependency array at line 168. When the extension is unlocked (i.e.,isExtensionLockedtransitions fromtruetofalse), the useEffect won't re-trigger to fetch assets because it only sees the other dependencies.Apply this diff to add the missing dependency:
- }, [shouldFetchAssets, addresses, fetchAssets, worker, isTestnetEnabled, isUpdate, selectedChains, genesisOptions]); + }, [shouldFetchAssets, addresses, fetchAssets, worker, isTestnetEnabled, isUpdate, selectedChains, genesisOptions, isExtensionLocked, pathname]);Alternatively, if you prefer to keep the dependency list minimal, add only
shouldFetchAssets:- }, [shouldFetchAssets, addresses, fetchAssets, worker, isTestnetEnabled, isUpdate, selectedChains, genesisOptions]); + }, [shouldFetchAssets, addresses, fetchAssets, worker, isTestnetEnabled, isUpdate, selectedChains, genesisOptions, shouldFetchAssets]);However, the first approach (adding
isExtensionLockedandpathname) is clearer because it makes the actual dependencies explicit rather than relying on the derived value.Likely an incorrect or invalid review comment.
packages/extension-ui/src/Popup/contexts/AccountAssetProvider.tsx (1)
10-10: LGTM: Lock state integration is correct.The changes properly:
- Import the lock context hook
- Retrieve the lock state
- Pass it to the assets balance hook
The integration follows React best practices and correctly threads the lock state through the component hierarchy.
Also applies to: 22-23
packages/extension-polkagate/src/popup/settings/partials/actions/Lock.tsx (2)
44-44: LGTM!Simplifying to always use
'text.primary'improves consistency and ensures proper theming.
13-13: Safe removal ofuseIsLoginEnabled
Verified no imports or usages remain in.ts/.tsxfiles; removal is safe.packages/extension-base/src/background/handlers/Extension.ts (3)
8-8: LGTM!The new type imports align with the new method signatures and are properly defined in the background types.
Also applies to: 11-11
439-449: LGTM!The lock expiration logic correctly handles edge cases: returns false when all accounts are external, and properly checks cached unlock times against the current time.
690-705: LGTM!The new message handlers correctly route to the corresponding methods with appropriate type casting, following the established pattern in this file.
packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts
Outdated
Show resolved
Hide resolved
packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/index.tsx
Outdated
Show resolved
Hide resolved
packages/extension-polkagate/src/popup/settings/extensionSettings/partials/AutoLockTimer.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (7)
packages/extension-base/src/background/handlers/Extension.ts (6)
136-145: Locking fails when cached entry belongs to a forgotten account
keyring.getPair(address)throws once an unlocked account gets forgotten, but we keep the stale cache entry. The thrown error aborts the loop, so the extension never finishes locking remaining accounts (and the request rejects). Please guard missing pairs and purge the stale cache entry before continuing.- Object.entries(this.#cachedUnlocks).forEach(([address]) => { - const pair = keyring.getPair(address); - - if (pair && !pair.isLocked) { - pair.lock(); - } - - this.#cachedUnlocks[address] = 0; - }); + Object.entries(this.#cachedUnlocks).forEach(([address]) => { + try { + const pair = keyring.getPair(address); + + if (!pair.isLocked) { + pair.lock(); + } + + this.#cachedUnlocks[address] = 0; + } catch (error) { + delete this.#cachedUnlocks[address]; + } + });
163-163: chrome.tabs.reload does not return a Promise;.catchwill throw.Use the existing withErrorLog helper or a callback to avoid runtime errors.
- chrome.tabs.reload(id).catch(console.error); + withErrorLog(() => chrome.tabs.reload(id));
407-443: Handle individual account unlock failures gracefully.If
pair.decodePkcs8(password)throws for any account (e.g., wrong password, corrupted keystore), the entire operation fails and no accounts are unlocked. Consider wrapping the unlock logic in a try-catch to handle failures per account and return partial success information.- const res = localAccounts.map(({ address }) => { - const pair = keyring.getPair(address); - - if (!pair) { - throw new Error('Unable to find pair'); - } - - if (pair.isLocked) { - pair.decodePkcs8(password); - } - - this.#cachedUnlocks[address] = Date.now() + cacheTime; - - return true; - }); + const res = localAccounts.map(({ address }) => { + try { + const pair = keyring.getPair(address); + + if (!pair) { + return false; + } + + if (pair.isLocked) { + pair.decodePkcs8(password); + } + + this.#cachedUnlocks[address] = Date.now() + cacheTime; + + return true; + } catch (error) { + console.error(`Failed to unlock account ${address}:`, error); + return false; + } + });
457-471: Avoid global TypeRegistry mutation; create a per-request registry for signing.Global registry is shared across concurrent requests and may be reconfigured mid‑sign, causing non‑deterministic behavior. Build and use a local registry per payload.
- private handleRegistry (payload: SignerPayloadJSON): void { - // Get the metadata for the genesisHash - const currentMetadata = this.metadataGet(payload.genesisHash); - - // set the registry before calling the sign function - const signedExtensions = currentMetadata?.signedExtensions?.length - ? currentMetadata.signedExtensions - : registry.signedExtensions; - - registry.setSignedExtensions(signedExtensions, currentMetadata?.userExtensions); - - if (currentMetadata) { - registry.register(currentMetadata?.types); - } - } + private createSigningRegistry (payload: SignerPayloadJSON): TypeRegistry { + const reg = new TypeRegistry(); + const currentMetadata = this.metadataGet(payload.genesisHash); + const signedExtensions = currentMetadata?.signedExtensions?.length + ? currentMetadata.signedExtensions + : reg.signedExtensions; + + reg.setSignedExtensions(signedExtensions, currentMetadata?.userExtensions); + + if (currentMetadata?.types) { + reg.register(currentMetadata.types); + } + + return reg; + }
506-506: Use the per-request registry from the refactored method.Once
handleRegistryis refactored tocreateSigningRegistry(returning a local registry), update this call site to use the returned registry for signing instead of the global instance.- this.handleRegistry(payload); + const reg = this.createSigningRegistry(payload);Then pass
regtorequest.sign(reg, pair)on Line 509.
527-546: Harden getSignature: handle unknown accounts and use a local registry.
keyring.getPair(address)can throw; return null instead of throwing.- Use a per‑request registry to avoid races.
- Remove global registry mutation.
- private getSignature ({ payload }: RequestSigninSignature): HexString | null { - const { address } = payload; - - if (!payload?.address || !payload.genesisHash) { - throw new Error('Invalid payload: missing required fields.'); - } - - const pair = keyring.getPair(address); - - this.refreshAccountPasswordCache(pair); // check if auto lock duration is expired - - if (pair.isLocked) { - return null; - } - - this.handleRegistry(payload); - const { signature } = registry.createType('ExtrinsicPayload', payload, { version: payload.version }).sign(pair); - - return signature; - } + private getSignature ({ payload }: RequestSigninSignature): HexString | null { + if (!payload?.address || !payload.genesisHash) { + throw new Error('Invalid payload: missing required fields.'); + } + + const { address } = payload; + let pair: KeyringPair; + + try { + pair = keyring.getPair(address); + } catch { + return null; + } + + this.refreshAccountPasswordCache(pair); // check if auto lock duration is expired + + if (pair.isLocked) { + return null; + } + + const reg = this.createSigningRegistry(payload); + const { signature } = reg + .createType('ExtrinsicPayload', payload, { version: payload.version }) + .sign(pair); + + return signature; + }packages/extension-polkagate/src/popup/passwordManagement/Login.tsx (1)
105-109: Resolved: unlocking state now resets via finallysetUnlocking(false) and resetting ref in finally addresses the spinner-stuck issue from the prior review.
🧹 Nitpick comments (4)
packages/extension-base/src/background/handlers/Extension.ts (1)
408-408: Remove debug console.log statements.Production code should not contain debug logging statements.
- console.log('get message to unlock accounts ...');- console.log('localAccounts:', localAccounts);Also applies to: 419-419
packages/extension-polkagate/src/popup/passwordManagement/Login.tsx (3)
96-99: Avoid type-cast; use void operator for ignored navigate resultCleaner and avoids unnecessary casting.
- : navigate(path) as void; + : void navigate(path);
85-86: Prefer void operator with error handling over type-cast for setStorageSuppressing the Promise via cast hides errors; use void and catch.
- setStorage(STORAGE_KEY.IS_PASSWORD_MIGRATED, true) as unknown as void; + void setStorage(STORAGE_KEY.IS_PASSWORD_MIGRATED, true).catch(console.error);
156-160: Disable Unlock until prerequisites are ready (better UX)Prevent no-op Enter/clicks before autoLockPeriod and isPasswordMigrated resolve.
- disabled={!hashedPassword} + disabled={ + !hashedPassword || + autoLockPeriod === undefined || + isPasswordMigrated === undefined + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/extension-base/src/background/handlers/Extension.ts(7 hunks)packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/index.tsx(4 hunks)packages/extension-polkagate/src/popup/passwordManagement/Login.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/index.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
packages/extension-base/src/background/handlers/Extension.ts (1)
packages/extension-base/src/background/types.ts (3)
RequestUnlockAllAccounts(402-405)RequestSigninSignature(313-315)RequestUpdateMeta(350-353)
packages/extension-polkagate/src/popup/passwordManagement/Login.tsx (10)
packages/extension-polkagate/src/hooks/useIsExtensionPopup.ts (1)
useIsExtensionPopup(4-21)packages/extension-polkagate/src/hooks/useIsPasswordMigrated.ts (1)
useIsPasswordMigrated(9-19)packages/extension-polkagate/src/hooks/useAutoLockPeriod.ts (1)
useAutoLockPeriod(30-44)packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts (1)
useCheckMasterPassword(29-47)packages/extension-polkagate/src/popup/settings/extensionSettings/ManagePassword.tsx (1)
isPasswordCorrect(18-23)packages/extension-polkagate/src/messaging.ts (2)
unlockAllAccounts(96-98)windowOpen(259-261)packages/extension-polkagate/src/components/Loading.tsx (2)
setStorage(71-84)updateStorage(19-36)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(302-320)packages/extension-polkagate/src/popup/passwordManagement/types.ts (1)
LOGIN_STATUS(4-11)packages/extension-polkagate/src/fullscreen/accountDetails/components/CommonTasks.tsx (1)
openOrFocusTab(35-60)
🪛 Biome (2.1.2)
packages/extension-polkagate/src/popup/passwordManagement/Login.tsx
[error] 53-53: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
[error] 123-124: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
packages/extension-polkagate/src/popup/passwordManagement/Login.tsx (1)
7-9: react-router-dom is v7.8.1 (v6+), useNavigate is supported
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (11)
packages/extension-polkagate/src/components/Loading.tsx (1)
102-102: Allow-list should ignore search/hash suffixes.This is the same issue flagged in the previous review:
ALLOWED_URL_ON_RESET_PASSWORD.includes(extensionUrl)only matches exact strings. When routes carry query params (e.g.,/forgot-password?address=...), the check fails and the login overlay incorrectly appears, blocking the reset flow.Apply the previously suggested fix:
- const isResettingWallet = isForgotten?.status || ALLOWED_URL_ON_RESET_PASSWORD.includes(window.location.hash.replace('#', '')); + const extensionUrl = window.location.hash.replace('#', ''); + const currentPath = extensionUrl.split('?')[0]; + const isResettingWallet = isForgotten?.status || + ALLOWED_URL_ON_RESET_PASSWORD.some((allowed) => currentPath.startsWith(allowed));packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts (1)
6-6: Guard undefined/empty accounts, reset state on missing pass, and add cleanupAvoid calling with undefined/empty accounts, reset state when
passis missing, and prevent stale updates with a cancellation guard. This aligns with the earlier review note.-import { useContext, useEffect, useState } from 'react'; +import { useContext, useEffect, useMemo, useState } from 'react'; @@ - const localAccounts = accounts.filter(({ isExternal }) => !isExternal); + const localAccounts = useMemo( + () => (accounts || []).filter(({ isExternal }) => !isExternal), + [accounts] + ); @@ useEffect(() => { - if (!pass) { - return; - } - - setAccountsNeedMigration(undefined); - - getAccountsNeedsMigration(localAccounts, pass).then((res) => { - setAccountsNeedMigration(res); - }).catch(console.error); + if (!pass || !accounts?.length) { + setAccountsNeedMigration(undefined); + return; + } + + let isCurrent = true; + setAccountsNeedMigration(undefined); + + void getAccountsNeedsMigration(localAccounts, pass) + .then((res) => { if (isCurrent) setAccountsNeedMigration(res); }) + .catch((e) => { console.error(e); if (isCurrent) setAccountsNeedMigration([]); }); + + return () => { isCurrent = false; }; // eslint-disable-next-line react-hooks/exhaustive-deps }, [accounts?.length, pass]);Also applies to: 26-26, 30-41
packages/extension-polkagate/src/popup/passwordManagement/Login.tsx (3)
83-96: Clear secrets from state and handle storage Promises explicitly
- Keep plaintext and hash lifetime minimal; clear both after success/migration.
- Don’t discard Promises with a cast; catch errors. Use
voidto drop result.if (success) { setExtensionLock(false); - hasLocalAccounts && setStorage(STORAGE_KEY.IS_PASSWORD_MIGRATED, true) as unknown as void; - setStorage(STORAGE_KEY.IS_FORGOTTEN, undefined) as unknown as void; + if (hasLocalAccounts) { + void setStorage(STORAGE_KEY.IS_PASSWORD_MIGRATED, true).catch(console.error); + } + void setStorage(STORAGE_KEY.IS_FORGOTTEN, undefined).catch(console.error); + setHashedPassword(undefined); } else { setExtensionLock(true); setIsPasswordError(true); } - setPlainPassword(undefined); + setPlainPassword(undefined); } else if (accountsNeedMigration?.length && isOldPasswordCorrect) { // needs migration await updateStorage(STORAGE_KEY.LOGIN_INFO, { lastLoginTime: Date.now(), status: LOGIN_STATUS.SET }); // DEPRECATED, will be removed in future releases - setStorage(STORAGE_KEY.IS_FORGOTTEN, undefined) as unknown as void; + void setStorage(STORAGE_KEY.IS_FORGOTTEN, undefined).catch(console.error); setHashedPassword(undefined); + setPlainPassword(undefined); setExtensionLock(false); const path = '/migratePasswords'; isExtension ? windowOpen(path).catch(console.error) - : navigate(path) as void; + : void navigate(path);Also applies to: 100-103
51-61: Fix void callback: don’t return setter and clear stale plaintext on empty inputReturning
setHashedPassword(...)from a void handler breaks lint and leavesplainPasswordstale.Apply:
const onPassChange = useCallback((pass: string | null): void => { if (!pass) { - return setHashedPassword(undefined); + setHashedPassword(undefined); + setPlainPassword(undefined); + return; } setPlainPassword(pass); setIsPasswordError(false); const hashedPassword = blake2AsHex(pass, 256); // Hash the string with a 256-bit output setHashedPassword(hashedPassword); }, []);
125-133: Fix void callback: don’t return setter in onForgotPasswordReturning
setStep(...)from a void handler breaks lint.const onForgotPassword = useCallback((): void => { if (isExtension) { - return setStep(STEPS.SHOW_DELETE_ACCOUNT_CONFIRMATION); + setStep(STEPS.SHOW_DELETE_ACCOUNT_CONFIRMATION); + return; } setStep(STEPS.SHOW_DELETE_ACCOUNT_CONFIRMATION_FS); openOrFocusTab('/forgot-password', true); }, [isExtension, setStep]);packages/extension-base/src/background/handlers/Extension.ts (6)
146-155: Lock loop can throw on forgotten/missing pairs; purge stale cache and continue
keyring.getPair(address)may throw if an account was forgotten; this aborts locking the rest.- Object.entries(this.#cachedUnlocks).forEach(([address]) => { - const pair = keyring.getPair(address); - - if (pair && !pair.isLocked) { - pair.lock(); - } - - this.#cachedUnlocks[address] = 0; - }); + Object.entries(this.#cachedUnlocks).forEach(([address]) => { + try { + const pair = keyring.getPair(address); + if (!pair.isLocked) { + pair.lock(); + } + this.#cachedUnlocks[address] = 0; + } catch { + delete this.#cachedUnlocks[address]; + } + });
173-175: chrome.tabs.reload is callback-style;.catchwill throwUse helper or callback to log errors; remove
.catch.- chrome.tabs.reload(id).catch(console.error); + withErrorLog(() => chrome.tabs.reload(id));
428-456: Unlock-all should handle per-account failures and continueA single
decodePkcs8error aborts the whole batch; wrap each attempt to allow partial progress and return aggregate result.- try { - const accountsLocal = this.localAccounts(); - const res = accountsLocal.map(({ address }) => { - const pair = keyring.getPair(address); - - if (!pair) { - throw new Error('Unable to find pair'); - } - - if (pair.isLocked) { - pair.decodePkcs8(password); - } - - this.#cachedUnlocks[address] = Date.now() + cacheTime; - - return true; - }); - - return res.every((success) => success); - } catch (error) { - console.error('accountsUnlockAll failed:', error); - - return false; // return false if any decode fails - } + const accountsLocal = this.localAccounts(); + const res = accountsLocal.map(({ address }) => { + try { + const pair = keyring.getPair(address); + if (pair.isLocked) { + pair.decodePkcs8(password); + } + this.#cachedUnlocks[address] = Date.now() + cacheTime; + return true; + } catch (error) { + console.error(`Failed to unlock ${address}:`, error); + return false; + } + }); + return res.every(Boolean);
459-467: Expiry check ignores local accounts not present in cacheIterating cache entries only can report expired while some locals remain unlocked/uncached. Check all local accounts.
- return Object.entries(this.#cachedUnlocks).every(([, time]) => time < Date.now()); + return accountsLocal.every(({ address }) => { + const until = this.#cachedUnlocks[address]; + return !until || until < Date.now(); + });
469-483: Avoid mutating global TypeRegistry; use a per-request registryGlobal
registryis shared across requests and can be reconfigured mid-sign. Build a local registry and use it for signing.Replace handleRegistry with a factory and use it:
- private handleRegistry (payload: SignerPayloadJSON): void { + private createSigningRegistry (payload: SignerPayloadJSON): TypeRegistry { const currentMetadata = this.metadataGet(payload.genesisHash); - const signedExtensions = currentMetadata?.signedExtensions?.length - ? currentMetadata.signedExtensions - : registry.signedExtensions; - - registry.setSignedExtensions(signedExtensions, currentMetadata?.userExtensions); - - if (currentMetadata) { - registry.register(currentMetadata?.types); - } - } + const reg = new TypeRegistry(); + const signedExtensions = currentMetadata?.signedExtensions?.length + ? currentMetadata.signedExtensions + : reg.signedExtensions; + reg.setSignedExtensions(signedExtensions, currentMetadata?.userExtensions); + if (currentMetadata?.types) { + reg.register(currentMetadata.types); + } + return reg; + }And in password-approve path:
- if (isJsonPayload(payload)) { - this.handleRegistry(payload); - } - - const result = request.sign(registry, pair); + let reg = registry; + if (isJsonPayload(payload)) { + reg = this.createSigningRegistry(payload); + } + const result = request.sign(reg, pair);Also applies to: 517-522
539-558: Harden getSignature: validate first, guard getPair, and use local registry
- Don’t destructure before null checks.
keyring.getPaircan throw; return null.- Use per-request registry, not the shared global.
- private getSignature ({ payload }: RequestSigninSignature): HexString | null { - const { address } = payload; - - if (!payload?.address || !payload.genesisHash) { - throw new Error('Invalid payload: missing required fields.'); - } - - const pair = keyring.getPair(address); + private getSignature ({ payload }: RequestSigninSignature): HexString | null { + if (!payload?.address || !payload.genesisHash) { + throw new Error('Invalid payload: missing required fields.'); + } + const { address } = payload; + let pair: KeyringPair; + try { + pair = keyring.getPair(address); + } catch { + return null; + } this.refreshAccountPasswordCache(pair); // check if auto lock duration is expired if (pair.isLocked) { return null; } - this.handleRegistry(payload); - const { signature } = registry.createType('ExtrinsicPayload', payload, { version: payload.version }).sign(pair); + const reg = this.createSigningRegistry(payload); + const { signature } = reg.createType('ExtrinsicPayload', payload, { version: payload.version }).sign(pair); return signature; }
🧹 Nitpick comments (14)
packages/extension-polkagate/src/components/Loading.tsx (1)
20-85: Relocate utility functions to a dedicated file.These storage utility functions (
updateStorage,getStorage,watchStorage,setStorage) are exported but defined within a component file. They should be moved to a separate utility module (e.g.,util/storage.ts) for better maintainability and reusability.packages/extension-polkagate/src/fullscreen/home/index.tsx (1)
24-30: Ensure navigation waits for real account data
AccountContext defaultsaccountsto[]—it’s neverundefined, soaccounts?.length === 0always fires on mount. Introduce a loading or null‐check (e.g. guard on the rawaccountsstate) before redirecting to avoid premature onboarding navigation.packages/extension-polkagate/src/popup/import/importSeedFullScreen/index.tsx (2)
98-101: Handle storage-set errors consistentlyAvoid casting away the promise; add error handling like the previous storage write.
- setStorage(STORAGE_KEY.IS_PASSWORD_MIGRATED, true) as unknown as void; + setStorage(STORAGE_KEY.IS_PASSWORD_MIGRATED, true).catch(console.error);
208-225: Password entry UX looks good; minor nitConditional MatchPasswordField vs PasswordInput is appropriate. Optionally consider passing a controlled value to PasswordInput for consistency with other controlled fields.
packages/extension-polkagate/src/popup/settings/extensionSettings/ManagePassword.tsx (4)
38-39: Avoid name collision with isPasswordCorrectShadowing the exported function name is confusing. Alias the hook result and update usages.
- const { isPasswordCorrect } = useIsPasswordCorrect(oldPass, readyToGo); + const { isPasswordCorrect: isOldPassValid } = useIsPasswordCorrect(oldPass, readyToGo); @@ - if (!isPasswordCorrect) { + if (!isOldPassValid) { @@ - }, [isPasswordCorrect, newPass, oldPass, readyToGo, t]); + }, [isOldPassValid, newPass, oldPass, readyToGo, t]);Also applies to: 50-73
5-10: Remove legacy storage-based password check helperWith useIsPasswordCorrect and accountsChangePasswordAll in place, the exported isPasswordCorrect helper (LOGIN_INFO + blake2) is obsolete and risks drift. Drop it and clean up imports.
-import React, { useCallback, useEffect, useState } from 'react'; +import React, { useCallback, useEffect, useState } from 'react'; @@ -import useIsPasswordCorrect from '@polkadot/extension-polkagate/src/hooks/useIsPasswordCorrect'; -import { accountsChangePasswordAll } from '@polkadot/extension-polkagate/src/messaging'; -import { getStorage, setStorage } from '@polkadot/extension-polkagate/src/util'; +import useIsPasswordCorrect from '@polkadot/extension-polkagate/src/hooks/useIsPasswordCorrect'; +import { accountsChangePasswordAll } from '@polkadot/extension-polkagate/src/messaging'; +import { setStorage } from '@polkadot/extension-polkagate/src/util'; @@ -import { blake2AsHex } from '@polkadot/util-crypto'; +// (removed blake2 import) @@ -import { type LoginInfo } from '../../passwordManagement/types'; +// (removed unused LoginInfo type) @@ -export const isPasswordCorrect = async (password: string, isHashed?: boolean) => { - const hashedPassword = isHashed ? password : blake2AsHex(password, 256); - const info = await getStorage(STORAGE_KEY.LOGIN_INFO) as LoginInfo; - - return info?.hashedPassword === hashedPassword; -}; +// removed legacy isPasswordCorrect()Also applies to: 14-18, 20-25
86-93: Optional: make current password field controlledKeeps state in sync and simplifies debugging.
- <PasswordInput + <PasswordInput + value={oldPass} focused hasError={passwordError} onPassChange={onCurrentPasswordChange} style={{ marginBottom: '18px' }} title={t('Current Password')} />
93-99: Remove ts-ignore by fixing MatchPasswordField prop typesDon’t suppress types. Update MatchPasswordField’s onSetPassword signature to accept a no-arg callback (or the expected type) and drop the ts-ignore.
packages/extension-polkagate/src/components/PasswordInput.tsx (1)
80-83: Simplify onChangeValue is always a string; remove the null coalescing for clarity.
- const onChange = useCallback(({ target: { value } }: React.ChangeEvent<HTMLInputElement>) => { - onPassChange(value ?? null); - }, [onPassChange]); + const onChange = useCallback(({ target: { value } }: React.ChangeEvent<HTMLInputElement>) => { + onPassChange(value); + }, [onPassChange]);packages/extension-polkagate/src/hooks/useIsPasswordCorrect.ts (1)
22-35: Prevent stale state updates; add cleanup and narrow depsAdd a cancellation guard to avoid setting state after unmount/race, use
firstLocal.addressdirectly, and narrow deps.useEffect(() => { setCorrect(undefined); if (!password || ready === false || !firstLocal) { return; } - accountsValidate(localAccounts[0].address, password).then((status) => { - setCorrect(status); - }).catch((error) => { - console.error(error); - setCorrect(false); - }); - }, [firstLocal, localAccounts, password, ready]); + let isCurrent = true; + void accountsValidate(firstLocal.address, password) + .then((status) => { if (isCurrent) setCorrect(status); }) + .catch((error) => { console.error(error); if (isCurrent) setCorrect(false); }); + + return () => { isCurrent = false; }; + }, [firstLocal?.address, password, ready]);packages/extension-polkagate/src/fullscreen/settings/partials/Password.tsx (1)
26-36: Robust timestamp parsing and placeholderHandle numeric/string timestamps safely and fix the placeholder format.
- getStorage(STORAGE_KEY.LAST_PASS_CHANGE).then((timestamp) => { - if (timestamp) { - const date = new Date(timestamp as unknown as number); + getStorage(STORAGE_KEY.LAST_PASS_CHANGE).then((timestamp) => { + const ms = Number(timestamp); + if (Number.isFinite(ms) && ms > 0) { + const date = new Date(ms); @@ - <Typography color='text.secondary' variant='B-5'> - {lastEditDate ?? '__,__,__'} + <Typography color='text.secondary' variant='B-5'> + {lastEditDate ?? '__.__.__'}Also applies to: 73-73
packages/extension-polkagate/src/messaging.ts (1)
84-87: Avoid duplicate wrappers for the same messageBoth
accountsValidateandvalidateAccountcallpri(accounts.validate). This duplicates API surface and invites confusion.Options:
- Prefer
accountsValidateand removevalidateAccount, or- Re-export for backward compat:
-export async function validateAccount (address: string, password: string): Promise<boolean> { - return sendMessage('pri(accounts.validate)', { address, password }); -} +// Deprecated: use accountsValidate +export const validateAccount = accountsValidate;Also applies to: 125-127
packages/extension-polkagate/src/popup/passwordManagement/MigratePasswords.tsx (2)
146-149: Remove ts-ignore by aligning callback typeAvoid
// @ts-ignoreonMatchPasswordField. If it expects a parameter, wrap your handler to accept and ignore it, or update prop typing.Example:
- // @ts-ignore - onSetPassword={onContinue} + onSetPassword={(): void => onContinue()}Or adjust
MatchPasswordFieldprop type to accept() => voidas well.
56-58: Prefervoid navigate(...)and clear secrets on completionMinor cleanup for intent clarity; also clear in-memory passwords when done.
- navigate('/') as void; + void navigate('/');setStorage(STORAGE_KEY.IS_PASSWORD_MIGRATED, true) .then(() => { setExtensionLock(true); - navigate('/') as void; - lockExtension().catch(console.error); + void navigate('/'); + void lockExtension().catch(console.error); + // minimize secret lifetime + setCurrentPass(''); + setMasterPass(undefined); }).catch(console.error);Also applies to: 101-106
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
packages/extension-base/src/background/handlers/Extension.ts(9 hunks)packages/extension-base/src/background/types.ts(5 hunks)packages/extension-polkagate/src/components/Loading.tsx(2 hunks)packages/extension-polkagate/src/components/PasswordInput.tsx(5 hunks)packages/extension-polkagate/src/fullscreen/home/DeriveAccount/ChildInfo.tsx(5 hunks)packages/extension-polkagate/src/fullscreen/home/DeriveAccount/ParentInfo.tsx(1 hunks)packages/extension-polkagate/src/fullscreen/home/SelectAccount.tsx(2 hunks)packages/extension-polkagate/src/fullscreen/home/index.tsx(1 hunks)packages/extension-polkagate/src/fullscreen/settings/partials/Password.tsx(2 hunks)packages/extension-polkagate/src/hooks/index.ts(1 hunks)packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts(1 hunks)packages/extension-polkagate/src/hooks/useIsForgotten.ts(1 hunks)packages/extension-polkagate/src/hooks/useIsPasswordCorrect.ts(1 hunks)packages/extension-polkagate/src/hooks/useLocalAccounts.ts(1 hunks)packages/extension-polkagate/src/messaging.ts(3 hunks)packages/extension-polkagate/src/popup/home/index.tsx(4 hunks)packages/extension-polkagate/src/popup/import/importRawSeedFullScreen/index.tsx(3 hunks)packages/extension-polkagate/src/popup/import/importSeedFullScreen/index.tsx(4 hunks)packages/extension-polkagate/src/popup/import/restoreJSONFullScreen/index.tsx(5 hunks)packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/index.tsx(5 hunks)packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/resetAccounts.ts(2 hunks)packages/extension-polkagate/src/popup/passwordManagement/FirstTimeSetPassword.tsx(0 hunks)packages/extension-polkagate/src/popup/passwordManagement/ForgotPassword.tsx(2 hunks)packages/extension-polkagate/src/popup/passwordManagement/Login.tsx(4 hunks)packages/extension-polkagate/src/popup/passwordManagement/MigratePasswords.tsx(1 hunks)packages/extension-polkagate/src/popup/passwordManagement/Reset.tsx(2 hunks)packages/extension-polkagate/src/popup/passwordManagement/constants.ts(0 hunks)packages/extension-polkagate/src/popup/passwordManagement/types.ts(1 hunks)packages/extension-polkagate/src/popup/settings/extensionSettings/ManagePassword.tsx(3 hunks)packages/extension-polkagate/src/popup/settings/partials/actions/Lock.tsx(2 hunks)packages/extension-polkagate/src/util/constants.ts(2 hunks)packages/extension-ui/src/Popup/contexts/AccountProvider.tsx(3 hunks)packages/extension/public/locales/en/translation.json(1 hunks)
💤 Files with no reviewable changes (2)
- packages/extension-polkagate/src/popup/passwordManagement/constants.ts
- packages/extension-polkagate/src/popup/passwordManagement/FirstTimeSetPassword.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/index.tsx
🧰 Additional context used
🧬 Code graph analysis (22)
packages/extension-polkagate/src/popup/passwordManagement/ForgotPassword.tsx (4)
packages/extension-polkagate/src/context/ExtensionLockContext.tsx (1)
useExtensionLockContext(16-24)packages/extension-polkagate/src/hooks/useIsExtensionPopup.ts (1)
useIsExtensionPopup(4-21)packages/extension-polkagate/src/components/Loading.tsx (1)
updateStorage(20-37)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(302-322)
packages/extension-polkagate/src/hooks/useLocalAccounts.ts (2)
packages/extension-base/src/background/types.ts (1)
AccountJson(40-62)packages/extension-polkagate/src/components/contexts.tsx (1)
AccountContext(34-34)
packages/extension-polkagate/src/popup/import/importSeedFullScreen/index.tsx (3)
packages/extension-polkagate/src/hooks/useLocalAccounts.ts (1)
useLocalAccounts(10-18)packages/extension-polkagate/src/components/Loading.tsx (1)
setStorage(72-85)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(302-322)
packages/extension-polkagate/src/hooks/useIsForgotten.ts (3)
packages/extension-polkagate/src/popup/passwordManagement/types.ts (1)
ForgottenInfo(23-26)packages/extension-polkagate/src/components/Loading.tsx (2)
watchStorage(51-70)getStorage(39-49)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(302-322)
packages/extension-base/src/background/handlers/Extension.ts (1)
packages/extension-base/src/background/types.ts (4)
RequestAccountChangePasswordAll(236-239)RequestUnlockAllAccounts(408-411)RequestSigninSignature(319-321)RequestUpdateMeta(356-359)
packages/extension-polkagate/src/popup/settings/extensionSettings/ManagePassword.tsx (5)
packages/extension-polkagate/src/hooks/useIsPasswordCorrect.ts (1)
useIsPasswordCorrect(9-41)packages/extension-polkagate/src/components/Loading.tsx (1)
setStorage(72-85)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(302-322)packages/extension-base/src/background/handlers/Extension.ts (1)
accountsChangePasswordAll(92-100)packages/extension-polkagate/src/messaging.ts (1)
accountsChangePasswordAll(92-94)
packages/extension-polkagate/src/fullscreen/settings/partials/Password.tsx (3)
packages/extension-polkagate/src/util/handleExtensionPopup.ts (1)
useExtensionPopups(11-18)packages/extension-polkagate/src/components/Loading.tsx (1)
getStorage(39-49)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(302-322)
packages/extension-polkagate/src/popup/passwordManagement/Reset.tsx (3)
packages/extension-polkagate/src/context/ExtensionLockContext.tsx (1)
useExtensionLockContext(16-24)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(302-322)packages/extension-polkagate/src/messaging.ts (1)
lockExtension(80-82)
packages/extension-polkagate/src/popup/import/importRawSeedFullScreen/index.tsx (2)
packages/extension-base/src/background/handlers/Extension.ts (1)
localAccounts(422-426)packages/extension-polkagate/src/hooks/useLocalAccounts.ts (1)
useLocalAccounts(10-18)
packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/resetAccounts.ts (3)
packages/extension-polkagate/src/components/Loading.tsx (2)
getStorage(39-49)updateStorage(20-37)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(302-322)packages/extension-polkagate/src/popup/passwordManagement/types.ts (1)
ForgottenInfo(23-26)
packages/extension-polkagate/src/hooks/useIsPasswordCorrect.ts (2)
packages/extension-base/src/background/handlers/Extension.ts (2)
localAccounts(422-426)accountsValidate(240-248)packages/extension-polkagate/src/messaging.ts (1)
accountsValidate(84-86)
packages/extension-base/src/background/types.ts (1)
packages/extension-inject/src/types.ts (1)
MetadataDef(65-75)
packages/extension-ui/src/Popup/contexts/AccountProvider.tsx (3)
packages/extension-polkagate/src/hooks/useIsForgotten.ts (1)
useIsForgotten(11-20)packages/extension-polkagate/src/components/Loading.tsx (1)
updateStorage(20-37)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(302-322)
packages/extension-polkagate/src/fullscreen/home/DeriveAccount/ChildInfo.tsx (1)
packages/extension-polkagate/src/messaging.ts (1)
deriveAccount(259-261)
packages/extension-polkagate/src/popup/passwordManagement/MigratePasswords.tsx (8)
packages/extension-polkagate/src/components/translate.ts (1)
useTranslation(8-10)packages/extension-polkagate/src/hooks/useAlerts.ts (1)
useAlerts(12-31)packages/extension-polkagate/src/context/ExtensionLockContext.tsx (1)
useExtensionLockContext(16-24)packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts (1)
useCheckMasterPassword(21-47)packages/extension-base/src/background/handlers/Extension.ts (2)
accountsChangePassword(102-120)lockExtension(145-180)packages/extension-polkagate/src/messaging.ts (2)
accountsChangePassword(88-90)lockExtension(80-82)packages/extension-polkagate/src/components/Loading.tsx (1)
setStorage(72-85)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(302-322)
packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts (4)
packages/extension-base/src/background/handlers/Extension.ts (2)
localAccounts(422-426)accountsValidate(240-248)packages/extension-base/src/background/types.ts (1)
AccountJson(40-62)packages/extension-polkagate/src/messaging.ts (1)
accountsValidate(84-86)packages/extension-polkagate/src/components/contexts.tsx (1)
AccountContext(34-34)
packages/extension-polkagate/src/popup/settings/partials/actions/Lock.tsx (2)
packages/extension-base/src/background/handlers/Extension.ts (1)
lockExtension(145-180)packages/extension-polkagate/src/messaging.ts (1)
lockExtension(80-82)
packages/extension-polkagate/src/popup/import/restoreJSONFullScreen/index.tsx (4)
packages/extension-base/src/background/types.ts (1)
ResponseJsonGetAccountInfo(454-459)packages/extension-polkagate/src/messaging.ts (1)
unlockAllAccounts(100-102)packages/extension-polkagate/src/util/constants.ts (2)
AUTO_LOCK_PERIOD_DEFAULT(226-226)STORAGE_KEY(302-322)packages/extension-polkagate/src/components/Loading.tsx (1)
setStorage(72-85)
packages/extension-polkagate/src/popup/home/index.tsx (1)
packages/extension-polkagate/src/hooks/useIsForgotten.ts (1)
useIsForgotten(11-20)
packages/extension-polkagate/src/components/Loading.tsx (6)
packages/extension-polkagate/src/hooks/useIsFlying.tsx (1)
useIsFlying(10-30)packages/extension-polkagate/src/context/ExtensionLockContext.tsx (1)
useExtensionLockContext(16-24)packages/extension-polkagate/src/hooks/useLocalAccounts.ts (1)
useLocalAccounts(10-18)packages/extension-polkagate/src/hooks/useIsForgotten.ts (1)
useIsForgotten(11-20)packages/extension-polkagate/src/util/constants.ts (1)
ALLOWED_URL_ON_RESET_PASSWORD(230-230)packages/extension-polkagate/src/popup/passwordManagement/ForgotPassword.tsx (1)
ForgotPassword(72-95)
packages/extension-polkagate/src/messaging.ts (2)
packages/extension-base/src/background/handlers/Extension.ts (4)
accountsValidate(240-248)accountsChangePassword(102-120)accountsChangePasswordAll(92-100)getSignature(539-558)packages/extension-inject/src/types.ts (1)
MetadataDef(65-75)
packages/extension-polkagate/src/popup/passwordManagement/Login.tsx (8)
packages/extension-polkagate/src/hooks/useIsExtensionPopup.ts (1)
useIsExtensionPopup(4-21)packages/extension-polkagate/src/hooks/useIsPasswordMigrated.ts (1)
useIsPasswordMigrated(9-19)packages/extension-polkagate/src/hooks/useAutoLockPeriod.ts (1)
useAutoLockPeriod(30-44)packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts (1)
useCheckMasterPassword(21-47)packages/extension-polkagate/src/popup/settings/extensionSettings/ManagePassword.tsx (1)
isPasswordCorrect(20-25)packages/extension-polkagate/src/messaging.ts (2)
unlockAllAccounts(100-102)windowOpen(263-265)packages/extension-polkagate/src/components/Loading.tsx (2)
setStorage(72-85)updateStorage(20-37)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(302-322)
🪛 Biome (2.1.2)
packages/extension-polkagate/src/popup/passwordManagement/MigratePasswords.tsx
[error] 53-53: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
packages/extension-polkagate/src/popup/passwordManagement/Login.tsx
[error] 53-53: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
[error] 127-128: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: build
🔇 Additional comments (24)
packages/extension-polkagate/src/fullscreen/home/DeriveAccount/ParentInfo.tsx (1)
127-127: LGTM! Simplified password label aligns with master password feature.The change from a dynamic translation (likely including the parent account name) to a static "Password" label is appropriate, especially given the master password implementation mentioned in the PR objectives. The
SelectAccountcomponent above (lines 114-121) already provides visual context about which account's password is being requested, so the simpler label maintains clarity while supporting the unified password workflow.packages/extension-polkagate/src/popup/home/index.tsx (4)
9-9: LGTM: Clean import addition.The import of
useIsForgottenhook is correctly added to support the new forgotten-password flow.
24-24: LGTM: Explicit void cast clarifies intent.The explicit
as voidcast clearly communicates that the return value ofuseBackgroundis intentionally unused here. This is a clean way to indicate side-effect-only hook usage.
33-33: LGTM: Hook usage follows React patterns.The
useIsForgottenhook is correctly invoked to retrieve the forgotten-password state from storage.
70-72: LGTM: Conditional logic correctly handles forgotten state.The refactored rendering logic cleanly replaces the previous LOGIN_INFO flow with the new
isForgotten?.statuscheck. The optional chaining correctly handles the initialundefinedstate when storage hasn't loaded yet, defaulting to showing the Welcome component until the storage read completes.packages/extension-polkagate/src/fullscreen/home/DeriveAccount/ChildInfo.tsx (3)
87-87: LGTM!The simplified heading text appropriately reflects the master password flow where users no longer need to set a separate password for the derived account.
113-113: LGTM!The disabled state logic is correctly simplified to only check for
childName, consistent with removing the password requirement from the UI.
47-61: Master password flow verified – no changes needed.The backend’s
derivationCreateusesparentPasswordto derive the child key and thepasswordparameter to encrypt it. PassingparentPasswordfor both meets the master-password design without validation issues.packages/extension-polkagate/src/components/Loading.tsx (2)
96-100: Verify step state management.The effect only sets
steptoSTEPS.SHOW_LOGINwhen the extension becomes locked, but doesn't clear or updatestepwhenisExtensionLockedbecomesfalse. Ensure this doesn't cause stale state issues if the extension is unlocked while the component is mounted.
117-119: Original review comment is incorrect:Login’s onForgotPassword handler calls setStep(STEPS.SHOW_DELETE_ACCOUNT_CONFIRMATION), so ForgotPassword does render as expected.Likely an incorrect or invalid review comment.
packages/extension-polkagate/src/fullscreen/home/index.tsx (1)
22-22: LGTM! Clean context integration.The transition to
AccountContextfor account data aligns well with the master password migration architecture described in the PR.packages/extension-polkagate/src/fullscreen/home/SelectAccount.tsx (1)
52-76: No layout or styling impact from added wrapper div – there are no CSS selectors or spacing rules targeting the Container as a direct child, so the extra<div>does not affect the flex column layout.packages/extension-polkagate/src/components/PasswordInput.tsx (1)
97-101: Layout wrapper changeSwitching to Grid wrapper can affect spacing/margins. Please eyeball a few screens using PasswordInput to ensure no regressions.
packages/extension-polkagate/src/hooks/index.ts (1)
47-55: New exports approved; useIsLoginEnabled removal verified
Search across.ts/.tsxfiles shows no remaining imports or usages ofuseIsLoginEnabled.packages/extension-polkagate/src/popup/passwordManagement/types.ts (1)
23-26: LGTM!The new
ForgottenInfointerface is well-defined and properly supports the forgotten-password workflow. The tri-statestatusfield (boolean | undefined) appropriately handles initialization scenarios.packages/extension-polkagate/src/popup/import/importRawSeedFullScreen/index.tsx (1)
198-215: LGTM!The conditional rendering correctly handles the master-password migration flow:
- New users (no local accounts) set up their initial password via
MatchPasswordField- Existing users (with local accounts) enter their existing password via
PasswordInputThe optional chaining on Line 198 properly handles the undefined case.
packages/extension-polkagate/src/popup/passwordManagement/ForgotPassword.tsx (1)
33-36: LGTM!The storage update correctly uses the new
IS_FORGOTTENkey with the appropriate data structure. The flow properly clears the extension lock before navigation.packages/extension-polkagate/src/popup/passwordManagement/Reset.tsx (1)
112-120: LGTM!The refactoring from
ActionContexttouseNavigatefollows modern React Router patterns. The storage update correctly resets theIS_FORGOTTENstate before re-locking the extension.packages/extension-polkagate/src/hooks/useLocalAccounts.ts (1)
13-17: Verify that the shallow dependency is safe.The
useMemodepends onaccounts?.lengthrather than the fullaccountsarray. This optimization assumes that theisExternalproperty of existing accounts doesn't change during the component lifecycle.While this is typically safe (accounts don't usually flip between external/internal), consider whether there are any edge cases where an account's
isExternalstatus might change without modifying the array length.If there are no such scenarios, this optimization is appropriate. You may want to add a comment explaining the rationale:
return useMemo( () => accounts.filter(({ isExternal }) => !isExternal), - // eslint-disable-next-line react-hooks/exhaustive-deps + // Only re-filter when account count changes; isExternal property is immutable after account creation + // eslint-disable-next-line react-hooks/exhaustive-deps [accounts?.length] );packages/extension-polkagate/src/hooks/useIsForgotten.ts (1)
11-19: LGTM!The hook correctly implements real-time storage synchronization:
- Subscribes to storage changes via
watchStorage- Retrieves initial value on mount
- Properly handles the tri-state return type
The nullish coalescing operator on Line 16 appropriately provides a fallback to
nullwhen storage is empty.packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/resetAccounts.ts (1)
20-37: LGTM!The refactoring improves error handling with
Promise.all:
- Executes account forgetting operations in parallel for better performance
- Validates that all operations succeeded before returning
- Properly catches and logs errors
The storage update correctly resets the
IS_FORGOTTENstatus before processing the account deletions.packages/extension-polkagate/src/util/constants.ts (1)
310-313: Storage keys: validate writes/reads are updatedDouble-check read/write sites use
IS_FORGOTTEN,IS_PASSWORD_MIGRATED, andLAST_PASS_CHANGE.You can extend the script above to search for usages and mismatches.
packages/extension-polkagate/src/popup/passwordManagement/Login.tsx (1)
109-115: Good: unlocking state reset in finally
setUnlocking(false)andisUnlockingRef.current = falsein finally reliably clear busy state after all paths. LGTM.packages/extension-base/src/background/types.ts (1)
105-112: Types and mappings look consistent with new handlersNew request interfaces and pri(...) mappings align with handlers and messaging. No issues spotted.
Also applies to: 121-123, 236-240, 408-411
packages/extension-polkagate/src/fullscreen/home/SelectAccount.tsx
Outdated
Show resolved
Hide resolved
packages/extension-polkagate/src/popup/import/restoreJSONFullScreen/index.tsx
Outdated
Show resolved
Hide resolved
packages/extension-polkagate/src/popup/passwordManagement/MigratePasswords.tsx
Show resolved
Hide resolved
packages/extension-polkagate/src/popup/settings/extensionSettings/ManagePassword.tsx
Outdated
Show resolved
Hide resolved
packages/extension-polkagate/src/popup/settings/partials/actions/Lock.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/extension-polkagate/src/fullscreen/settings/partials/Password.tsx (1)
26-26: Consider safer type handling for timestamp.The double cast
as unknown as numberbypasses type safety. If the stored value is a string representation of a timestamp, theDateconstructor will handle it, but this defeats TypeScript's type checking.Consider one of these approaches:
Option 1: Type guard with explicit conversion
- const date = new Date(timestamp as unknown as number); + const timestampNum = typeof timestamp === 'number' ? timestamp : parseInt(String(timestamp), 10); + const date = new Date(timestampNum);Option 2: Let Date constructor handle it (it accepts string | number)
- const date = new Date(timestamp as unknown as number); + const date = new Date(timestamp as string | number);Option 3: Update
getStoragereturn type to includenumberin its signature (if timestamps are always stored as numbers).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/extension-polkagate/src/fullscreen/settings/partials/Password.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/extension-polkagate/src/fullscreen/settings/partials/Password.tsx (4)
packages/extension-polkagate/src/components/translate.ts (1)
useTranslation(8-10)packages/extension-polkagate/src/util/handleExtensionPopup.ts (1)
useExtensionPopups(11-18)packages/extension-polkagate/src/components/Loading.tsx (1)
getStorage(39-49)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(302-322)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: build
🔇 Additional comments (1)
packages/extension-polkagate/src/fullscreen/settings/partials/Password.tsx (1)
34-34: Good addition of error handling.The
.catch(console.error)prevents unhandled promise rejection and logs errors. The fallback display'__,__,__'(line 71) provides adequate user feedback when the timestamp is unavailable.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/extension-polkagate/src/popup/import/addWatchOnlyFullScreen/index.tsx (1)
31-47: Fix error-handling flow: navigation runs before user sees errors.The
.finally()block (lines 39-41) executes before the.catch()block (lines 42-45) due to promise-chain ordering. This causes the component to navigate away even whencreateAccountExternalfails, preventing users from seeing error feedback. Additionally, settingisBusyto false after navigation is ineffective since the component has unmounted.Apply this diff to fix the error-handling flow:
const onAdd = useCallback(() => { if (name && realAddress) { setIsBusy(true); createAccountExternal(name, realAddress, undefined) .then(() => { setStorage(STORAGE_KEY.SELECTED_PROFILE, PROFILE_TAGS.WATCH_ONLY).catch(console.error); + switchToOrOpenTab('/', true); }) - .finally(() => - switchToOrOpenTab('/', true) - ) .catch((error: Error) => { setIsBusy(false); console.error(error); + // TODO: Show error message to user }); } }, [name, realAddress]);This ensures navigation only happens on success, and errors are handled while the component is still mounted.
Consider adding user-facing error feedback (e.g., a toast or error message state) instead of only logging to console.
packages/extension-polkagate/src/components/SignArea3.tsx (1)
100-123: Fix payload construction: wrong Call source + avoid hard‑coded signedExtensions + make runtimeVersion explicit.Using createType('Call', preparedTransaction) risks mis-encoding; pass the Call. Also use chain‑provided signedExtensions (and include assetId when ChargeAssetTxPayment is active). Prefer explicit specVersion/transactionVersion.
- try { - const _payload = { + try { + const signedExt = api.registry.signedExtensions; + const includeAssetId = signedExt.includes('ChargeAssetTxPayment'); + const _payload = { address: from, - // assetId: null, // fee asset Id, singerOption?.assetId + ...(includeAssetId ? { assetId: signerOption?.assetId ?? null } : {}), blockHash: lastHeader.hash, blockNumber: api.registry.createType('BlockNumber', lastHeader.number.toNumber()), era: api.registry.createType('ExtrinsicEra', { current: lastHeader.number.toNumber(), period: 64 }), genesisHash: api.genesisHash, - method: api.createType('Call', preparedTransaction), // TODO: DOES SUPPORT nested calls, batches , ... + method: preparedTransaction.method, // pass Call directly nonce: api.registry.createType('Compact<Index>', rawNonce), - runtimeVersion: api.runtimeVersion, - signedExtensions: [ - 'CheckNonZeroSender', - 'CheckSpecVersion', - 'CheckTxVersion', - 'CheckGenesis', - 'CheckMortality', - 'CheckNonce', - 'CheckWeight', - 'ChargeTransactionPayment' - ], + specVersion: api.runtimeVersion.specVersion, + transactionVersion: api.runtimeVersion.transactionVersion, + signedExtensions: signedExt, tip: 0, version: preparedTransaction.version };
🧹 Nitpick comments (4)
packages/extension-polkagate/src/components/SignArea3.tsx (4)
73-73: Pass proxyTypeFilter to useProxies for consistency and fewer UI checks.Hook already supports filtering; wire it here.
- const proxies = useProxies(genesisHash, formatted); + const proxies = useProxies(genesisHash, formatted, proxyTypeFilter);
145-145: Avoid unsafe double cast; use toString() for delegate.- const selectedProxyAddress = selectedProxy?.delegate as unknown as string; + const selectedProxyAddress = selectedProxy?.delegate?.toString();
147-152: Use accountNextIndex for nonce to avoid stale reads and pending tx issues.system.account.nonce can lag; accountNextIndex is the canonical next nonce.
- api.rpc.chain.getHeader().then(setLastHeader).catch(console.error); - api.query['system']['account'](from).then((res) => setRawNonce((res as FrameSystemAccountInfo)?.nonce.toNumber() || 0)).catch(console.error); + void Promise.all([ + api.rpc.chain.getHeader().then(setLastHeader), + api.rpc.system.accountNextIndex(from).then((idx) => setRawNonce(idx.toNumber())) + ]).catch(console.error);
229-242: Make step transition robust around send().Optional: wrap send in try/finally so CONFIRMATION always runs; log errors in catch.
- const txResult = await send(from, api, preparedTransaction, extrinsicPayload.toHex(), signature); - setFlowStep(TRANSACTION_FLOW_STEPS.CONFIRMATION); - handleTxResult(txResult); + try { + const txResult = await send(from, api, preparedTransaction, extrinsicPayload.toHex(), signature); + handleTxResult(txResult); + } finally { + setFlowStep(TRANSACTION_FLOW_STEPS.CONFIRMATION); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/extension-polkagate/src/components/SignArea3.tsx(8 hunks)packages/extension-polkagate/src/fullscreen/home/RemoveAccount.tsx(3 hunks)packages/extension-polkagate/src/fullscreen/sendFund/index.tsx(0 hunks)packages/extension-polkagate/src/partials/RemoveAccount.tsx(2 hunks)packages/extension-polkagate/src/popup/home/index.tsx(3 hunks)packages/extension-polkagate/src/popup/import/addWatchOnlyFullScreen/index.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- packages/extension-polkagate/src/fullscreen/sendFund/index.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/extension-polkagate/src/popup/home/index.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
packages/extension-polkagate/src/partials/RemoveAccount.tsx (1)
packages/extension-polkagate/src/messaging.ts (1)
validateAccount(125-127)
packages/extension-polkagate/src/fullscreen/home/RemoveAccount.tsx (1)
packages/extension-polkagate/src/messaging.ts (1)
validateAccount(125-127)
packages/extension-polkagate/src/components/SignArea3.tsx (2)
packages/extension-polkagate/src/hooks/useProxies.ts (1)
useProxies(21-61)packages/extension-polkagate/src/util/api/signAndSend.ts (1)
send(93-150)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: Analyze (javascript)
🔇 Additional comments (8)
packages/extension-polkagate/src/popup/import/addWatchOnlyFullScreen/index.tsx (1)
6-6: LGTM: Import and formatting changes align with simplified flow.The removal of crypto initialization imports and the updated
setStorageimport path are consistent with the PR's shift toward a streamlined account creation flow without in-browser keyring dependencies.Also applies to: 10-10, 24-24
packages/extension-polkagate/src/partials/RemoveAccount.tsx (1)
12-12: LGTM!The
validateAccountimport is correctly added and aligns with the new password validation flow.packages/extension-polkagate/src/fullscreen/home/RemoveAccount.tsx (2)
61-67: Password validation approach looks solid.The shift from local keyring unlocking (
signer.unlock) to centralized password validation (validateAccount) aligns well with the master-password architecture introduced in this PR. The error handling correctly updates the UI state when validation fails.
52-59: Critical bug: Promise never resolves, blocking removal indefinitely.The pattern
await new Promise(requestAnimationFrame)is incorrect.requestAnimationFrameexpects a callback function as its argument, but here it's being passed theresolveandrejectparameters from the Promise constructor. This Promise will never resolve, causing the UI to freeze withisBusystuck attrue.Apply this diff to fix the frame-delay pattern:
const onRemove = useCallback(async () => { try { if (!account || (account?.isExternal && !acknowledged) || (!account?.isExternal && !password)) { return; } setIsBusy(true); - await new Promise(requestAnimationFrame); + await new Promise((resolve) => requestAnimationFrame(resolve)); if (!account.isExternal && password) {Likely an incorrect or invalid review comment.
packages/extension-polkagate/src/components/SignArea3.tsx (4)
9-9: LGTM: correct generic import for SubmittableExtrinsic.
125-129: LGTM: SignerPayload created with version option and serialized via toPayload().
137-143: LGTM: ExtrinsicPayload instantiation from SignerPayload JSON looks correct.
263-263: Downstream prop types verified — interfaces correctly updated to GenericExtrinsicPayload.The component interfaces have been properly updated:
- SignUsingPassword, SignWithLedger, and SignUsingQR all define
payload: GenericExtrinsicPayload | undefined- The
extrinsicPayloadin SignArea3.tsx is created viaapi.registry.createType('ExtrinsicPayload', ...), which produces the correct GenericExtrinsicPayload runtime type- Legacy
SignerPayloadis not used in any of these component payload propsType compatibility is correct. The distinction between
GenericExtrinsicPayload(polkadot runtime type) andExtrinsicPayload(TypeScript interface) is properly handled.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/extension-polkagate/src/partials/WaitScreen.tsx (2)
25-45: Add runtime type validation before casting detail to string.The type assertion
as stringon line 29 bypasses TypeScript's type checking without runtime validation. Ifdetailis not actually a string (e.g., an object, number, or array), the.toLowerCase()call on line 31 will throw a runtime TypeError.Apply this diff to add proper type guarding:
const handleTxEvent = useCallback((s: CustomEventInit<unknown>) => { const { detail } = s; - if (detail) { + if (detail && typeof detail === 'string') { - const state = detail as string; + const state = detail; switch (state.toLowerCase()) { case ('ready'): setText({ title: t('The transaction is ready.'), inColor: t('ready') }); break; case ('broadcast'): setText({ title: t('The transaction has been sent.'), inColor: t('sent') }); break; case ('inblock'): setText({ title: t('The transaction is now on-chain.'), inColor: t('on-chain') }); break; default: setText({ title: t(`The transaction is in ${state} state`), inColor: t(`${state}`) }); } } }, [t]);
47-49: Add event listener cleanup to prevent memory leak.The
useEffectregisters an event listener but never removes it. If the component unmounts and remounts (e.g., during navigation or re-renders), multiple listeners will accumulate, causing memory leaks and duplicate event handling.Apply this diff to add the cleanup function:
useEffect(() => { window.addEventListener('transactionState', handleTxEvent); + + return () => { + window.removeEventListener('transactionState', handleTxEvent); + }; }, [handleTxEvent]);packages/extension-polkagate/src/components/SignArea3.tsx (1)
100-130: Fix payload construction to use extrinsic method and derive signedExtensions from registryThe review identifies valid and critical issues confirmed by polkadot.js best practices:
method field – Currently wraps the entire extrinsic; should use
preparedTransaction.methoddirectly. SubmittableExtrinsic already exposes the method property that matches the payload signature.signedExtensions – Hardcoding the list breaks when the runtime updates signed extensions (e.g., adding CheckMetadataHash). Use
api.registry.signedExtensions(already used inmetadataFromApi.ts:31) to pull the canonical list from chain metadata.tip encoding – Should be
api.registry.createType('Compact<Balance>', 0)to ensure proper SCALE encoding rather than a primitive number.blockNumber conversion – Passing
lastHeader.number.toNumber()to createType is redundant; pass the value directly since createType handles type coercion.assetId support – Optional but recommended:
signerOptionparameter is available (line 51) and the codebase tracks assetId elsewhere; conditionally include it when present.Regarding lines 137-143: this is a separate extrinsicPayload computation with no payload construction logic. The reference likely indicates formatting consistency only (the suggested diff shows the second createType call on one line).
Apply the suggested changes to ensure the payload matches the runtime's expectations and aligns with polkadot.js API design.
♻️ Duplicate comments (1)
packages/extension-polkagate/src/util/api/signAndSend.ts (1)
44-55: Unify transactionState event.detail shapePreviously flagged: detail differs across emitters. Emit a consistent object, e.g. { state: result.status.type } for all occurrences. This avoids breaking listeners.
- const event = new CustomEvent('transactionState', { detail: result.status.type }); + const event = new CustomEvent('transactionState', { detail: { state: result.status.type } });
🧹 Nitpick comments (4)
packages/extension-polkagate/src/partials/WaitScreen.tsx (1)
41-42: Consider more user-friendly messaging for unknown transaction states.The default case displays the raw state string directly to users, which may expose technical internal state names (e.g., "txpool", "pending_validation"). While this provides good coverage for unexpected states, consider whether these technical terms are appropriate for end users or if a more generic fallback message would be better.
For example:
default: - setText({ title: t(`The transaction is in ${state} state`), inColor: t(`${state}`) }); + setText({ title: t('The transaction is being processed.'), inColor: t('processing') });packages/extension-polkagate/src/util/api/signAndSend.ts (1)
57-72: Dispatch error text: include section.name alongside docsDocs alone can be empty; surfacing section.name improves UX.
- failureText = `${docs.join(' ')}`; + failureText = `${section}.${name}${docs.length ? `: ${docs.join(' ')}` : ''}`;packages/extension-polkagate/src/components/SignUsingPassword.tsx (2)
6-9: Avoid shadowing global Proxy; alias the type importBiome warns about Proxy name. Alias the type and update usages.
-import type { Proxy } from '../util/types'; +import type { Proxy as ProxyAccount } from '../util/types'; @@ -interface UseProxyProps { - proxies: Proxy[] | undefined; +interface UseProxyProps { + proxies: ProxyAccount[] | undefined; @@ -export interface SignUsingPasswordProps { +export interface SignUsingPasswordProps { @@ - proxies: Proxy[] | undefined; + proxies: ProxyAccount[] | undefined;Also applies to: 21-24, 60-73
82-99: Null signature path: surface actionable UIThrowing an error is caught and only sets hasError; consider surfacing a clear “Session locked—please unlock” toast/dialog and route user to the unlock flow.
Example:
- if (!signature) { - // TODO: show login page - throw new Error('account is locked need to login again!'); - } + if (!signature) { + setHasError(true); + // TODO: open unlock modal or navigate to Manage Password screen + return; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/extension-polkagate/src/components/SignArea3.tsx(8 hunks)packages/extension-polkagate/src/components/SignUsingPassword.tsx(4 hunks)packages/extension-polkagate/src/partials/WaitScreen.tsx(1 hunks)packages/extension-polkagate/src/util/api/signAndSend.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/extension-polkagate/src/util/api/signAndSend.ts (1)
packages/extension-polkagate/src/util/types.ts (1)
TxResult(149-155)
packages/extension-polkagate/src/components/SignArea3.tsx (2)
packages/extension-polkagate/src/hooks/useProxies.ts (1)
useProxies(21-61)packages/extension-polkagate/src/util/api/signAndSend.ts (1)
send(125-144)
packages/extension-polkagate/src/components/SignUsingPassword.tsx (4)
packages/extension-polkagate/src/components/VariantButton.tsx (1)
DecisionButtonProps(9-17)packages/extension-polkagate/src/util/types.ts (1)
Proxy(584-589)packages/extension-polkagate/src/messaging.ts (1)
getSignature(96-98)packages/extension-base/src/background/handlers/Extension.ts (1)
getSignature(543-562)
🪛 Biome (2.1.2)
packages/extension-polkagate/src/components/SignUsingPassword.tsx
[error] 8-8: Do not shadow the global "Proxy" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
packages/extension-polkagate/src/components/SignArea3.tsx (3)
229-241: onSignature: OK path, but verify payload type expected by send()Passing extrinsicPayload.toHex() matches send(from, api, extrinsic, payloadHex, signature). Ensure any downstream that inspects payload tolerates HexString. LGTM otherwise.
72-74: useProxies signature change alignmentSwapping to useProxies(genesisHash, formatted) aligns with the updated hook. Confirm all other call sites use the new (genesisHash, proxiedAddress) order.
255-268: Child props: payload/extrinsicPayload wiringPassing payload={extrinsicPayload} and signerPayload to SignWithLedger/SignUsingPassword/SignUsingQR looks consistent with their new props. Ensure those components treat payload as ExtrinsicPayload (not SignerPayloadJSON).
Also applies to: 271-283, 295-302
packages/extension-polkagate/src/components/SignUsingPassword.tsx (1)
96-99: Pass signerOption intent through the pipeline (fee asset, tips, era)You log signerOption but don’t use it. After fixing payload construction in SignArea3, ensure signerOption fields (tip, era, assetId) are reflected in the payload/signature and send path.
Also applies to: 106-107
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/extension-polkagate/src/popup/passwordManagement/MigratePasswords.tsx (1)
186-193: Verify PasswordInput null handling.If
PasswordInputcan callonPassChangewithnull, passingsetCurrentPassdirectly may cause issues sincecurrentPasswordis typed asstring(line 38). Consider wrapping the callback to coerce null values.If needed, apply this diff:
<PasswordInput focused hasError={isIncorrectPassword} onEnterPress={onNext} - onPassChange={setCurrentPass} + onPassChange={(pass) => setCurrentPass(pass ?? '')} title={t('Enter {{name}} Password', { replace: { name: accountToMigrate.name } })} value={currentPassword} />Run the following script to verify PasswordInput's type signature:
#!/bin/bash # Search for PasswordInput component definition to check onPassChange prop type ast-grep --pattern $'interface $_ { $$$ onPassChange: $_ $$$ }' rg -nP --type=tsx --type=ts -A5 'onPassChange.*:.*\?'
🧹 Nitpick comments (3)
packages/extension-polkagate/src/popup/passwordManagement/MigratePasswords.tsx (2)
58-58: Remove unnecessary void cast.The
as voidcast is unnecessary when callingnavigate.Apply this diff:
- navigate('/') as void; + navigate('/');
105-105: Remove unnecessary void cast.The
as voidcast is unnecessary when callingnavigate.Apply this diff:
- navigate('/') as void; + navigate('/');packages/extension-polkagate/src/assets/animations/index.ts (1)
4-4: Consider using ambient type declarations instead of @ts-nocheck.While
@ts-nocheckworks, a more maintainable approach would be to create an ambient type declaration file (e.g.,types.d.ts) for.lottiefiles:declare module '*.lottie' { const content: string; export default content; }This would provide type safety while properly handling the asset imports.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
package.json(3 hunks)packages/extension-polkagate/src/assets/animations/index.ts(1 hunks)packages/extension-polkagate/src/popup/passwordManagement/MigratePasswords.tsx(1 hunks)packages/extension/webpack.shared.cjs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/extension-polkagate/src/popup/passwordManagement/MigratePasswords.tsx (8)
packages/extension-polkagate/src/components/translate.ts (1)
useTranslation(8-10)packages/extension-polkagate/src/hooks/useAlerts.ts (1)
useAlerts(12-31)packages/extension-polkagate/src/context/ExtensionLockContext.tsx (1)
useExtensionLockContext(16-24)packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts (1)
useCheckMasterPassword(21-47)packages/extension-polkagate/src/messaging.ts (2)
accountsChangePassword(88-90)lockExtension(80-82)packages/extension-base/src/background/handlers/Extension.ts (2)
accountsChangePassword(102-120)lockExtension(145-180)packages/extension-polkagate/src/components/Loading.tsx (1)
setStorage(72-85)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(309-329)
🪛 Biome (2.1.2)
packages/extension-polkagate/src/popup/passwordManagement/MigratePasswords.tsx
[error] 54-54: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
package.json (3)
30-30: LGTM: Development build script added.The
build:devscript correctly setsNODE_ENV=developmentfor faster development builds without the production pipeline overhead.
57-57: LGTM: Lottie dependency addition.The
@lottiefiles/dotlottie-reactdependency is correctly added to support the new Lottie animations feature, and it integrates well with the webpack configuration and animation assets.
62-73: Polkadot package upgrades verified as valid and secure.All @polkadot/* packages are uniformly upgraded from
^16.4.6to^16.4.8. Version 16.4.8 exists and has no known security vulnerabilities. The coordinated upgrade maintains compatibility with resolutions properly aligned.packages/extension/webpack.shared.cjs (1)
45-48: LGTM: Webpack rule for Lottie files.The module rule correctly handles
.lottiefiles usingasset/resource, which will emit the files to the output directory and return their URLs. This integrates properly with the new@lottiefiles/dotlottie-reactdependency.packages/extension-polkagate/src/assets/animations/index.ts (1)
6-8: All three.lottiefiles are confirmed to exist inpackages/extension-polkagate/src/assets/animations/and correctly match the exports in index.ts. The code is valid with no issues.
packages/extension-polkagate/src/popup/passwordManagement/MigratePasswords.tsx
Show resolved
Hide resolved
packages/extension-polkagate/src/popup/passwordManagement/MigratePasswords.tsx
Show resolved
Hide resolved
packages/extension-polkagate/src/popup/passwordManagement/MigratePasswords.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/extension-base/src/background/handlers/Extension.ts (1)
521-526: Use the per‑request registry during signingSwitch from global registry to the local one returned above.
- if (isJsonPayload(payload)) { - this.handleRegistry(payload); - } - - const result = request.sign(registry, pair); + const reg = isJsonPayload(payload) + ? this.createSigningRegistry(payload) + : new TypeRegistry(); + + const result = request.sign(reg, pair);
♻️ Duplicate comments (16)
packages/extension-polkagate/src/popup/import/importRawSeedFullScreen/index.tsx (1)
41-53: Critical: Validation persists keys to storage.The
keyring.addUrimethod persists keys to the keyring as a side effect. Validation should be pure and not mutate storage. Additionally, the error message is not translated.Use the background
validateSeedmethod instead:+import { validateSeed as validateSeedBg } from '../../../messaging'; + const validateSeed = useCallback(async (seed: string, type?: KeypairType): Promise<AccountInfo> => { if (!(seed.startsWith('0x') && seed.length === 66)) { - throw new Error('The raw seed is invalid. It should be 66 characters long and start with 0x'); + throw new Error(t('The raw seed is invalid. It should be 66 characters long and start with 0x')); } - const { pair } = keyring.addUri(seed, undefined, {}, type || DEFAULT_TYPE); + const { address, suri } = await validateSeedBg(seed, type || DEFAULT_TYPE); return { - address: pair.address, + address, genesis: POLKADOT_GENESIS, - suri: seed + suri }; -}, []); +}, [t]);packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts (1)
29-41: Guard on falsy pass and add cancellation to avoid stale state updates
- When
passbecomes falsy, previous results linger; reset state and return early.- Cancel in‑flight validations when
passorlocalAccountschange to prevent late resolves overwriting newer state.useEffect(() => { - if (!pass) { - return; - } - - setAccountsNeedMigration(undefined); - - getAccountsNeedsMigration(localAccounts, pass) - .then(setAccountsNeedMigration) - .catch(console.error); + if (!pass) { + setAccountsNeedMigration(undefined); + return; + } + + setAccountsNeedMigration(undefined); + + let cancelled = false; + void getAccountsNeedsMigration(localAccounts, pass) + .then((res) => { + if (!cancelled) setAccountsNeedMigration(res); + }) + .catch((e) => { + console.error(e); + if (!cancelled) setAccountsNeedMigration([]); + }); // eslint-disable-next-line react-hooks/exhaustive-deps - }, [localAccounts?.length, pass]); + return () => { cancelled = true; }; + }, [localAccounts?.length, pass]);packages/extension-polkagate/src/popup/passwordManagement/Login.tsx (2)
128-133: LGTM: void callback no longer returns a valueThe early return now avoids returning the setter result.
86-96: Clear hashed password on success to minimize secret lifetime
plainPasswordis cleared, buthashedPasswordremains in memory after a successful unlock.if (success) { setExtensionLock(false); - hasLocalAccounts && setStorage(STORAGE_KEY.IS_PASSWORD_MIGRATED, true) as unknown as void; - setStorage(STORAGE_KEY.IS_FORGOTTEN, undefined) as unknown as void; + if (hasLocalAccounts) { + void setStorage(STORAGE_KEY.IS_PASSWORD_MIGRATED, true).catch(console.error); + } + void setStorage(STORAGE_KEY.IS_FORGOTTEN, undefined).catch(console.error); } else { setExtensionLock(true); setIsPasswordError(true); } - setPlainPassword(undefined); + setPlainPassword(undefined); + setHashedPassword(undefined);packages/extension-base/src/background/handlers/Extension.ts (7)
174-175: LGTM: usewithErrorLogforchrome.tabs.reloadAvoids
.catchon a non‑Promise API and logs lastError safely.
145-155: Locking can throw for forgotten accounts; purge stale cache and continue
keyring.getPair(address)may throw after an account is forgotten; the loop aborts and remaining accounts aren’t locked.- Object.entries(this.#cachedUnlocks).forEach(([address]) => { - const pair = keyring.getPair(address); - - if (pair && !pair.isLocked) { - pair.lock(); - } - - this.#cachedUnlocks[address] = 0; - }); + Object.entries(this.#cachedUnlocks).forEach(([address]) => { + try { + const pair = keyring.getPair(address); + if (!pair.isLocked) { + pair.lock(); + } + this.#cachedUnlocks[address] = 0; + } catch { + delete this.#cachedUnlocks[address]; + } + });
433-461: Handle per‑account unlock failures without aborting the whole operationA single bad keystore/password currently flips the entire result to
false. Capture failures per account and keep going.- try { - const accountsLocal = this.localAccounts(); - const res = accountsLocal.map(({ address }) => { - const pair = keyring.getPair(address); - - if (!pair) { - throw new Error('Unable to find pair'); - } - - if (!pair.isLocked) { - pair.lock(); - } - - if (pair.isLocked) { - pair.decodePkcs8(password); - } - - this.#cachedUnlocks[address] = Date.now() + cacheTime; - - return true; - }); - - return res.every((success) => success); - } catch (error) { - console.error('accountsUnlockAll failed:', error); - - return false; // return false if any decode fails - } + const accountsLocal = this.localAccounts(); + const results = accountsLocal.map(({ address }) => { + try { + const pair = keyring.getPair(address); + if (!pair.isLocked) pair.lock(); + if (pair.isLocked) pair.decodePkcs8(password); + this.#cachedUnlocks[address] = Date.now() + cacheTime; + return true; + } catch (error) { + console.error(`accountsUnlockAll: failed to unlock ${address}`, error); + return false; + } + }); + + return results.every(Boolean);
463-471:areLocksExpiredignores accounts without cache entriesIterate local accounts and treat missing cache entries as expired.
- return Object.entries(this.#cachedUnlocks).every(([, time]) => time < Date.now()); + return accountsLocal.every(({ address }) => { + const time = this.#cachedUnlocks[address]; + return !time || time < Date.now(); + });
473-487: Avoid mutating the global TypeRegistry; create a per‑request registryGlobal registry mutation is a race across concurrent requests; use a local registry instead.
- private handleRegistry (payload: SignerPayloadJSON): void { - // Get the metadata for the genesisHash - const currentMetadata = this.metadataGet(payload.genesisHash); - - // set the registry before calling the sign function - const signedExtensions = currentMetadata?.signedExtensions?.length - ? currentMetadata.signedExtensions - : registry.signedExtensions; - - registry.setSignedExtensions(signedExtensions, currentMetadata?.userExtensions); - - if (currentMetadata) { - registry.register(currentMetadata?.types); - } - } + private createSigningRegistry (payload: SignerPayloadJSON): TypeRegistry { + const reg = new TypeRegistry(); + const currentMetadata = this.metadataGet(payload.genesisHash); + const signedExtensions = currentMetadata?.signedExtensions?.length + ? currentMetadata.signedExtensions + : reg.signedExtensions; + reg.setSignedExtensions(signedExtensions, currentMetadata?.userExtensions); + if (currentMetadata?.types) { + reg.register(currentMetadata.types); + } + return reg; + }
543-562: HardengetSignatureand sign with a local registryHandle unknown accounts and avoid global registry mutation.
private getSignature ({ payload }: RequestSigningSignature): HexString | null { if (!payload?.address || !payload.genesisHash) { throw new Error('Invalid payload: missing required fields.'); } - const { address } = payload; - - const pair = keyring.getPair(address); - - this.refreshAccountPasswordCache(pair); // check if auto lock duration is expired - - if (pair.isLocked) { - return null; - } - - this.handleRegistry(payload); - const { signature } = registry.createType('ExtrinsicPayload', payload, { version: payload.version }).sign(pair); + const { address } = payload; + let pair: KeyringPair; + try { + pair = keyring.getPair(address); + } catch { + return null; + } + + this.refreshAccountPasswordCache(pair); + if (pair.isLocked) { + return null; + } + + const reg = this.createSigningRegistry(payload); + const { signature } = reg + .createType('ExtrinsicPayload', payload, { version: payload.version }) + .sign(pair); return signature; }
92-100: Gracefully handle per‑account failures inaccountsChangePasswordAllOne failure currently throws and aborts the whole request; return overall success while logging failures.
private accountsChangePasswordAll ({ newPass, oldPass }: RequestAccountChangePasswordAll): boolean { const accountsLocal = this.localAccounts(); - const res = accountsLocal.map(({ address }) => { - return this.accountsChangePassword({ address, newPass, oldPass }); - }); + const res = accountsLocal.map(({ address }) => { + try { + return this.accountsChangePassword({ address, newPass, oldPass }); + } catch (e) { + console.error(`accountsChangePasswordAll: failed for ${address}`, e); + return false; + } + }); return res.every(Boolean); }packages/extension-base/src/background/types.ts (2)
121-123: LGTM: consistent namingRequestSigningSignatureThe request/response map uses the corrected name.
361-364: LGTM:RequestUnlockAllAccountsplaced before Responses with clear commentInterface is in the Requests section and uses “milliseconds”.
packages/extension-polkagate/src/popup/passwordManagement/MigratePasswords.tsx (3)
53-56: Don’t return a setter fromuseEffectReturning
setStep(...)is a no‑op and confuses the effect’s cleanup semantics.- if (accountsNeedMigration?.length) { - return setStep(STEP.MIGRATING); - } + if (accountsNeedMigration?.length) { + setStep(STEP.MIGRATING); + return; + }
185-192: NormalizePasswordInputnulls before updating string state
PasswordInputpassesstring | null; writingnullintostringstate is unsafe.- onPassChange={setCurrentPass} + onPassChange={(pass) => setCurrentPass(pass ?? '')}
37-39: AlignmasterPasstype with initializationAvoid
string | undefinedbut initialize with''. Pick one convention.- const [masterPass, setMasterPass] = useState<string | undefined>(''); + const [masterPass, setMasterPass] = useState<string>('');
🧹 Nitpick comments (6)
packages/extension-polkagate/src/popup/staking/solo-new/settings/ChooseAccount.tsx (1)
60-60: Consider using a stable identifier for the Fragment key.The Fragment currently uses
indexas the key, which can cause issues if the accounts list is reordered or filtered. Consider usingaddressorformattedinstead, as they provide stable unique identifiers.Apply this diff:
- <Fragment key={index}> + <Fragment key={address}> <Container disableGutters onClick={handleSelect(formatted)} sx={{ alignItems: 'center', cursor: 'pointer', display: 'flex', flexDirection: 'row', justifyContent: 'space-between' }}>packages/extension-polkagate/src/popup/import/importRawSeedFullScreen/index.tsx (1)
168-186: Recommended: Always pass onImport to avoid race conditions.Conditionally passing
undefinedtoonSetPasswordbased on parent state can race with child component updates. If the user presses Enter while the parent hasn't updated, the action may silently no-op.Let
MatchPasswordFieldgate the action based on its own validation state:- {localAccounts?.length === 0 - ? (<MatchPasswordField - onSetPassword={(password && name && !error && !!seed) ? onImport : undefined} + {localAccounts?.length === 0 + ? (<MatchPasswordField + onSetPassword={onImport} setConfirmedPassword={setPassword} spacing='20px'packages/extension-polkagate/src/popup/passwordManagement/Login.tsx (2)
88-90: Avoid casting Promises to void; handle rejections explicitlyReplace
as unknown as voidwith a safe fire‑and‑forget pattern.- hasLocalAccounts && setStorage(STORAGE_KEY.IS_PASSWORD_MIGRATED, true) as unknown as void; - setStorage(STORAGE_KEY.IS_FORGOTTEN, undefined) as unknown as void; + if (hasLocalAccounts) { + void setStorage(STORAGE_KEY.IS_PASSWORD_MIGRATED, true).catch(console.error); + } + void setStorage(STORAGE_KEY.IS_FORGOTTEN, undefined).catch(console.error); @@ - setHashedPassword(undefined); + setHashedPassword(undefined); + void setStorage(STORAGE_KEY.IS_FORGOTTEN, undefined).catch(console.error);Also applies to: 98-101
103-106: Prefervoid navigate(path)overas voidcastMinor readability/style improvement.
- isExtension - ? windowOpen(path).catch(console.error) - : navigate(path) as void; + isExtension + ? windowOpen(path).catch(console.error) + : void navigate(path);packages/extension-polkagate/src/popup/passwordManagement/MigratePasswords.tsx (2)
99-108: Clear sensitive state on completionUnset
masterPass/currentPasswordin memory once migration is finalized.const onDone = useCallback(() => { setIsBusy(true); setStorage(STORAGE_KEY.IS_PASSWORD_MIGRATED, true) .then(() => { setExtensionLock(true); - navigate('/') as void; + navigate('/') as void; lockExtension().catch(console.error); - }).catch(console.error); + }).catch(console.error) + .finally(() => { + // minimize secret lifetime + // eslint-disable-next-line no-unused-expressions + setCurrentPass(''); + // eslint-disable-next-line no-unused-expressions + // masterPass is used only for this flow; clear it + // @ts-expect-error: allowed for secret clearing + setMasterPass(''); + }); }, [navigate, setExtensionLock]);
70-97: EnsureisBusyresets in all pathsUse
finallyto guaranteesetIsBusy(false)regardless of success/error/early exit.const onNext = useCallback(() => { if (accountToMigrate && masterPass) { - setIsBusy(true); - accountsChangePassword(accountToMigrate.address, currentPassword, masterPass) - .then((success) => { + setIsBusy(true); + accountsChangePassword(accountToMigrate.address, currentPassword, masterPass) + .then((success) => { if (success) { if (accountIndexToChangePassword + 1 === accountsNeedMigration?.length) { - setIsBusy(false); - - return setStep(STEP.COMPLETED); + setStep(STEP.COMPLETED); + return; } setAccountIndex((pre) => pre + 1); setCurrentPass(''); } else { notify(t('Something went wrong while migrating password!'), 'error'); } - setIsBusy(false); }).catch((error) => { console.error(error); setIncorrectPassword(true); - setIsBusy(false); - }); + }).finally(() => setIsBusy(false)); } else { - setIsBusy(false); + setIsBusy(false); } }, [accountIndexToChangePassword, accountToMigrate, accountsNeedMigration?.length, currentPassword, masterPass, notify, t]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/extension-base/src/background/handlers/Extension.ts(9 hunks)packages/extension-base/src/background/types.ts(5 hunks)packages/extension-polkagate/src/fullscreen/home/SelectAccount.tsx(2 hunks)packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts(1 hunks)packages/extension-polkagate/src/hooks/useIsForgotten.ts(1 hunks)packages/extension-polkagate/src/popup/import/importRawSeedFullScreen/index.tsx(5 hunks)packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/useAccountImportOrCreate.ts(1 hunks)packages/extension-polkagate/src/popup/passwordManagement/Login.tsx(4 hunks)packages/extension-polkagate/src/popup/passwordManagement/MigratePasswords.tsx(1 hunks)packages/extension-polkagate/src/popup/staking/solo-new/settings/ChooseAccount.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/useAccountImportOrCreate.ts
🧰 Additional context used
🧬 Code graph analysis (7)
packages/extension-polkagate/src/popup/passwordManagement/Login.tsx (10)
packages/extension-polkagate/src/hooks/useIsExtensionPopup.ts (1)
useIsExtensionPopup(4-21)packages/extension-polkagate/src/hooks/useIsPasswordMigrated.ts (1)
useIsPasswordMigrated(9-19)packages/extension-polkagate/src/context/ExtensionLockContext.tsx (1)
useExtensionLockContext(16-24)packages/extension-polkagate/src/hooks/useAutoLockPeriod.ts (1)
useAutoLockPeriod(30-44)packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts (1)
useCheckMasterPassword(21-46)packages/extension-polkagate/src/popup/settings/extensionSettings/ManagePassword.tsx (1)
isPasswordCorrect(21-26)packages/extension-polkagate/src/messaging.ts (2)
unlockAllAccounts(100-102)windowOpen(263-265)packages/extension-polkagate/src/components/Loading.tsx (2)
setStorage(72-85)updateStorage(20-37)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(309-329)packages/extension-polkagate/src/popup/passwordManagement/types.ts (1)
LOGIN_STATUS(4-11)
packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts (4)
packages/extension-base/src/background/handlers/Extension.ts (2)
localAccounts(422-426)accountsValidate(240-248)packages/extension-base/src/background/types.ts (1)
AccountJson(40-62)packages/extension-polkagate/src/messaging.ts (1)
accountsValidate(84-86)packages/extension-polkagate/src/hooks/useLocalAccounts.ts (1)
useLocalAccounts(10-18)
packages/extension-polkagate/src/hooks/useIsForgotten.ts (3)
packages/extension-polkagate/src/popup/passwordManagement/types.ts (1)
ForgottenInfo(23-26)packages/extension-polkagate/src/components/Loading.tsx (2)
watchStorage(51-70)getStorage(39-49)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(309-329)
packages/extension-base/src/background/types.ts (1)
packages/extension-inject/src/types.ts (1)
MetadataDef(65-75)
packages/extension-polkagate/src/popup/passwordManagement/MigratePasswords.tsx (7)
packages/extension-polkagate/src/hooks/useAlerts.ts (1)
useAlerts(12-31)packages/extension-polkagate/src/context/ExtensionLockContext.tsx (1)
useExtensionLockContext(16-24)packages/extension-polkagate/src/hooks/useCheckMasterPassword.ts (1)
useCheckMasterPassword(21-46)packages/extension-base/src/background/handlers/Extension.ts (2)
accountsChangePassword(102-120)lockExtension(145-180)packages/extension-polkagate/src/messaging.ts (2)
accountsChangePassword(88-90)lockExtension(80-82)packages/extension-polkagate/src/components/Loading.tsx (1)
setStorage(72-85)packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(309-329)
packages/extension-polkagate/src/popup/import/importRawSeedFullScreen/index.tsx (5)
packages/extension-polkagate/src/hooks/useLocalAccounts.ts (1)
useLocalAccounts(10-18)packages/extension-polkagate/src/messaging.ts (1)
validateSeed(251-253)packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/types.ts (1)
AccountInfo(4-8)packages/extension-polkagate/src/util/defaultType.ts (1)
DEFAULT_TYPE(6-6)packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/useAccountImportOrCreate.ts (1)
useAccountImportOrCreate(19-112)
packages/extension-base/src/background/handlers/Extension.ts (1)
packages/extension-base/src/background/types.ts (4)
RequestAccountChangePasswordAll(236-239)RequestUnlockAllAccounts(361-364)RequestSigningSignature(319-321)RequestUpdateMeta(356-359)
🪛 Biome (2.1.2)
packages/extension-polkagate/src/popup/passwordManagement/MigratePasswords.tsx
[error] 54-54: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
packages/extension-polkagate/src/popup/staking/solo-new/settings/ChooseAccount.tsx (1)
61-61: LGTM: Correct removal of redundant key.Removing the
keyprop from theContaineris appropriate since the parentFragment(line 60) already provides the key for the list item.packages/extension-polkagate/src/fullscreen/home/SelectAccount.tsx (1)
52-76: Duplicate key issue resolved - LGTM!The previous review flagged a duplicate
key={address}prop on both the outer div and inner Container. The current implementation correctly places the key only on the outermostdivelement (line 52), with the Container (line 53) no longer having a duplicate key. This follows React best practices for list rendering.packages/extension-base/src/background/handlers/Extension.ts (1)
192-196: LGTM: removing cached unlock on forgetStale cache entry is deleted before forgetting the account.
… after tx submission
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/extension-polkagate/src/components/SignArea3.tsx (1)
100-129: Critical: signer payload builds Call from the whole extrinsic (invalid) and hardcodes extensions.
methodmust be the call, not the full extrinsic. Using the extrinsic will produce an invalid payload/signature.- Prefer
api.registry.signedExtensionsover a hardcoded list to match the runtime and avoid silent mismatches.- Prefer
api.extrinsicVersionas the payload version source of truth.- Optional but safer: provide
specVersionandtransactionVersionexplicitly instead ofruntimeVersion.Apply this minimal fix first (correct Call + version):
- method: api.createType('Call', preparedTransaction), // TODO: DOES SUPPORT nested calls, batches , ... + // Use the call only; supports nested/batch calls via .method + method: preparedTransaction.method, @@ - signedExtensions: [ - 'CheckNonZeroSender', - 'CheckSpecVersion', - 'CheckTxVersion', - 'CheckGenesis', - 'CheckMortality', - 'CheckNonce', - 'CheckWeight', - 'ChargeTransactionPayment' - ], + // Derive from registry to match the chain at runtime + signedExtensions: api.registry.signedExtensions, @@ - version: preparedTransaction.version + version: api.extrinsicVersionOptional robustness (if you prefer explicit versions):
- runtimeVersion: api.runtimeVersion, + specVersion: api.runtimeVersion.specVersion, + transactionVersion: api.runtimeVersion.transactionVersion,
♻️ Duplicate comments (3)
packages/extension-polkagate/src/util/api/signAndSend.ts (3)
50-53: Unify transactionState event detail shape and guard for non-window contextsEmit a consistent detail object (e.g., { state }) and guard for environments without window (bg service worker).
- const event = new CustomEvent('transactionState', { detail: result.status.type }); - - window.dispatchEvent(event); + const detail = { state: result.status.type }; + if (typeof window !== 'undefined' && typeof window.dispatchEvent === 'function') { + window.dispatchEvent(new CustomEvent('transactionState', { detail })); + }Verification (grep listeners expecting a string vs object):
#!/bin/bash rg -nP "addEventListener\\(.*transactionState|on\\(['\"]transactionState['\"]" -A3 -B3
122-128: Capture and dispose unsubscribe from signAndSend to prevent leaksThe callback variant returns an unsubscribe function; not capturing it leaks the subscription. Mirror the send() pattern.
- extrinsic.signAndSend(pair, options ?? {}, async (result) => { - await handleResult(api, resolve, result); - }).catch((e) => { + let unsub: (() => void) | undefined; + const onResult = async (result: ISubmittableResult) => { + const { status } = result; + await handleResult(api, resolve, result); + if ( + status.isInBlock || + status.isFinalized || + status.isInvalid || + status.isDropped || + status.isUsurped || + status.isRetracted || + status.isFinalityTimeout + ) { + unsub?.(); + } + }; + extrinsic.signAndSend(pair, options ?? {}, onResult).then((u) => { unsub = u; }).catch((e) => { console.log('⚠️ Catch error', e); resolve({ block: 0, failureText: String(e), fee: '', success: false, txHash: '' }); - }); + unsub?.(); + });
79-104: Handle Retracted and always return after resolving terminal statesRetracted is a terminal failure and should resolve. Also add an early return after the failure resolve to prevent accidental double-resolve if this function grows.
- try { - const { isCompleted, isError, status, txHash } = result; - const { isDropped, isFinalityTimeout, isFinalized, isInBlock, isInvalid, isUsurped, type } = status; + try { + const { isCompleted, isError, status, txHash } = result; + const { isDropped, isFinalityTimeout, isFinalized, isInBlock, isInvalid, isUsurped, isRetracted, type } = status; @@ - if (isCompleted && isError) { + if (isCompleted && isError) { resolve({ block: 0, failureText: failureText || `unknown error (${type})`, fee: '', success: false, txHash: '' }); - - return; + return; } @@ - if (isInvalid || isDropped || isUsurped || isFinalityTimeout) { - resolve({ block: 0, failureText: failureText || status.type, fee: '', success: false, txHash: txHash?.toString() || '' }); - } + if (isInvalid || isDropped || isUsurped || isRetracted || isFinalityTimeout) { + resolve({ block: 0, failureText: failureText || status.type, fee: '', success: false, txHash: txHash?.toString() || '' }); + return; + }
🧹 Nitpick comments (5)
packages/extension-polkagate/src/components/SignArea3.tsx (1)
209-223: Result mapping improvements are fine; consider defensive defaults.Destructuring with defaults for
blockandtxHashis OK. Ifdecimal/tokencan vary across chains, ensure UI formatting tolerates''or0gracefully.packages/extension-polkagate/src/util/api/signAndSend.ts (4)
8-12: Drop ts-ignore and use stable EventRecord typingAvoid importing lookup-specific FrameSystemEventRecord with a ts-ignore. Prefer EventRecord from interfaces and update the Vec generic accordingly.
import type { Vec } from '@polkadot/types'; -import type { SignedBlock } from '@polkadot/types/interfaces'; -// @ts-ignore -import type { FrameSystemEventRecord } from '@polkadot/types/lookup'; +import type { SignedBlock } from '@polkadot/types/interfaces'; +import type { EventRecord } from '@polkadot/types/interfaces'; @@ - const allEvents = await apiAt.query['system']['events']() as Vec<FrameSystemEventRecord>; + const allEvents = await apiAt.query.system.events() as Vec<EventRecord>;Also applies to: 18-18
16-24: Tx hash param typing: accept string to match toHex() and avoid TS frictionresult.txHash.toHex() is typed as string; your function expects HexString. Broaden the param to string (or HexString | string) to keep call-sites clean.
-async function getAppliedFee (api: ApiPromise, signedBlock: SignedBlock, txHashHex: HexString): Promise<string | undefined> { +async function getAppliedFee (api: ApiPromise, signedBlock: SignedBlock, txHashHex: string): Promise<string | undefined> {Also applies to: 29-41
56-70: Include section.name in failureText for better UXToday only docs are surfaced. Append section.name for clearer, actionable error messages.
- failureText = `${docs.join(' ')}`; + failureText = `${section}.${name}: ${docs.join(' ')}`.trim();
52-54: Reduce noisy console logs or gate by envThese console logs are verbose for production. Consider gating behind a debug flag.
Also applies to: 120-121, 139-140
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/extension-polkagate/src/components/SignArea3.tsx(9 hunks)packages/extension-polkagate/src/fullscreen/sendFund/index.tsx(1 hunks)packages/extension-polkagate/src/util/api/signAndSend.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/extension-polkagate/src/components/SignArea3.tsx (2)
packages/extension-polkagate/src/hooks/useProxies.ts (1)
useProxies(21-61)packages/extension-polkagate/src/util/api/signAndSend.ts (1)
send(131-172)
packages/extension-polkagate/src/util/api/signAndSend.ts (1)
packages/extension-polkagate/src/util/types.ts (1)
TxResult(149-155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: build
🔇 Additional comments (6)
packages/extension-polkagate/src/fullscreen/sendFund/index.tsx (1)
1-271: No issues found. Crypto initialization is properly centralized.The removal of local
cryptoWaitReady()from the SendFund component is correct. The crypto initialization has been properly moved topackages/extension/src/background.ts(lines 32–42), where it runs as part of the extension's service worker initialization on startup. This guarantees that crypto is ready before any UI components, including SignArea3, attempt to sign transactions. No race conditions exist because the background script initializes before the UI becomes interactive.packages/extension-polkagate/src/components/SignArea3.tsx (5)
72-74: Hook usage update looks correct.Switching to
useProxies(genesisHash, formatted)aligns with the hook’s signature and avoids coupling toapi.
125-129: Good: using SignerPayload.toPayload().Converting to the JSON payload before constructing
ExtrinsicPayloadis correct and interoperable with external signers.
137-144: LGTM: ExtrinsicPayload creation.
api.registry.createType('ExtrinsicPayload', signerPayload, { version: signerPayload.version })is the right shape foraddSignature(...).
264-269: Prop shape change topayload={extrinsicPayload}looks consistent across Ledger and QR.Assuming downstream components consume
ExtrinsicPayload, these changes are coherent with the new signing flow.If either component still expects the older
payloadshape, update their prop types accordingly. The script above will surface any mismatches.Also applies to: 300-303
231-243: Signature flow and payload types verified as correct.Both
SignWithLedgerandSignUsingQRexpectpayload: GenericExtrinsicPayload | undefined, and SignArea3.tsx correctly passesextrinsicPayloadto both components at lines 264 and 300. The code is properly typed and children receive the correct payload type.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/extension-polkagate/src/components/SignArea3.tsx (1)
100-129: Fix SignerPayload construction (missing spec/tx version, wrong method, hard-coded extensions).Current payload is not runtime-accurate and may yield invalid/malleated signatures:
- specVersion/transactionVersion are missing; runtimeVersion is not a SignerPayload field.
- method should be the call (tx.method), not the whole extrinsic.
- signedExtensions must come from registry; hard-coding omits chain-specific items (e.g., CheckMetadataHash, ChargeAssetTxPayment), breaking signature on many networks.
- tip should be Compact.
Apply:
const _payload = { address: from, - assetId: signerOption?.assetId, - blockHash: lastHeader.hash, - blockNumber: api.registry.createType('BlockNumber', lastHeader.number.toNumber()), - era: api.registry.createType('ExtrinsicEra', { current: lastHeader.number.toNumber(), period: 64 }), - genesisHash: api.genesisHash, - method: api.createType('Call', preparedTransaction), // TODO: DOES SUPPORT nested calls, batches , ... - nonce: api.registry.createType('Compact<Index>', rawNonce), - runtimeVersion: api.runtimeVersion, - signedExtensions: [ - 'CheckNonZeroSender', - 'CheckSpecVersion', - 'CheckTxVersion', - 'CheckGenesis', - 'CheckMortality', - 'CheckNonce', - 'CheckWeight', - 'ChargeTransactionPayment' - ], - tip: 0, - version: preparedTransaction.version + assetId: signerOption?.assetId, + blockHash: lastHeader.hash, + blockNumber: api.registry.createType('BlockNumber', lastHeader.number.toNumber()), + era: api.registry.createType('ExtrinsicEra', { current: lastHeader.number.toNumber(), period: 64 }), + genesisHash: api.genesisHash, + method: preparedTransaction.method, + nonce: api.registry.createType('Compact<Index>', rawNonce), + // Pull from runtime to guarantee match + specVersion: api.runtimeVersion.specVersion.toNumber(), + transactionVersion: api.runtimeVersion.transactionVersion.toNumber(), + // Ensure asset-fee extension when assetId is present + signedExtensions: signerOption?.assetId + ? Array.from(new Set([...api.registry.signedExtensions, 'ChargeAssetTxPayment'])) + : api.registry.signedExtensions, + tip: api.registry.createType('Compact<Balance>', signerOption?.tip ?? 0), + version: preparedTransaction.version }; - const raw = api.registry.createType('SignerPayload', _payload, { - version: _payload.version - }); + const raw = api.registry.createType('SignerPayload', _payload, { version: _payload.version }); return raw.toPayload();Optionally derive era/nonce using chain derive for better liveness:
- era/current from latest header subscription
- nonce from api.derive.tx.signingInfo(from)
Also applies to: 135-135
🧹 Nitpick comments (2)
packages/extension-polkagate/src/components/SignArea3.tsx (2)
209-222: TxResult mapping LGTM (new shape).Fields align with util/api/send() return; ensure fee inclusion later if needed by UI.
231-243: Signature flow OK; add minimal failure handling.Consider catching send() rejection to revert WAIT state if message port drops.
- const txResult = await send(from, api, preparedTransaction, extrinsicPayload.toHex(), signature); + let txResult: TxResult; + try { + txResult = await send(from, api, preparedTransaction, extrinsicPayload.toHex(), signature); + } catch (e) { + setFlowStep(TRANSACTION_FLOW_STEPS.REVIEW); // or an error step + throw e; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/extension-polkagate/src/components/SignArea3.tsx(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/extension-polkagate/src/components/SignArea3.tsx (2)
packages/extension-polkagate/src/hooks/useProxies.ts (1)
useProxies(21-61)packages/extension-polkagate/src/util/api/signAndSend.ts (1)
send(131-172)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
packages/extension-polkagate/src/components/SignArea3.tsx (6)
9-9: Import looks fine.Type usage aligns with SubmittableExtrinsic typing in this file.
73-74: useProxies signature update LGTM.Passing (genesisHash, formatted) matches the updated hook contract.
137-144: ExtrinsicPayload creation looks correct.Creating from payload with matching version is the right path.
264-269: Props are correctly typed and separated.The verification confirms that SignArea3.tsx correctly passes
extrinsicPayload(type: GenericExtrinsicPayload) to thepayloadprop, while separately passingsignerPayload(type: SignerPayloadJSON) to thesignerPayloadprop. TheextrinsicPayloadis properly created viaapi.registry.createType('ExtrinsicPayload', ...), which returns the expected GenericExtrinsicPayload type that SignWithLedger expects.
278-284: Incorrect analysis - all signers consistently use SignerPayloadJSON.The review comment mischaracterizes the payload usage:
- SignWithLedger receives both
payload: GenericExtrinsicPayloadandsignerPayload: SignerPayloadJSON- SignUsingQR receives both
payload: GenericExtrinsicPayloadandsignerPayload: SignerPayloadJSON- SignUsingPassword receives
signerPayload: SignerPayloadJSONAll three signers consistently use
SignerPayloadJSON. The additionalGenericExtrinsicPayloadin Ledger and QR is intentional—it's needed to display the transaction details to the user for verification before signing. Password signing doesn't require this because it doesn't display extrinsic details to the user. This is a sensible design distinction, not an inconsistency requiring unification.Likely an incorrect or invalid review comment.
300-303: QR flow prop handling is correct; verification complete.The extrinsicPayload is properly created as an
ExtrinsicPayloadviaapi.registry.createType()and includes signedExtensions through the signerPayload context. The type casts betweenGenericExtrinsicPayloadandExtrinsicPayloadare appropriate, and the Qr component correctly receives and processes the payload with.toU8a().
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/extension-polkagate/src/partials/WaitScreen.tsx (1)
57-57: Consider responsive sizing for the animation.The animation uses a fixed width of
300px, which might not scale well across different viewport sizes or devices.Consider using responsive units or percentage-based sizing:
- <DotLottieReact autoplay loop src={sendingLottie} style={{ height: 'auto', width: '300px' }} /> + <DotLottieReact autoplay loop src={sendingLottie} style={{ height: 'auto', width: '100%', maxWidth: '300px' }} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/extension-polkagate/src/assets/animations/index.ts(1 hunks)packages/extension-polkagate/src/partials/WaitScreen.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: build
🔇 Additional comments (4)
packages/extension-polkagate/src/assets/animations/index.ts (1)
6-9: LGTM! Animation exports align with PR objectives.The exported animations (lockLottie, masterKey, migratePassword, sendingLottie) are properly structured and align with the master password migration feature being added in this PR.
packages/extension-polkagate/src/partials/WaitScreen.tsx (3)
4-4: LGTM! Lottie animation imports are properly structured.The imports for
DotLottieReactandsendingLottiecorrectly reference the new animation infrastructure added in this PR.Also applies to: 10-11
33-46: LGTM! State normalization and default case handling are well-implemented.The
toLowerCase()normalization handles case variations in transaction states, and the default case provides good fallback behavior for unknown states.
56-56: LGTM! Layout adjustments accommodate the new animation.The styling changes appropriately adjust spacing and layout to accommodate the DotLottie animation replacement.
…low (#1978) * fix: gracefully handle non-existent key pair during forgot password flow * fix: ensure lockExtension handles cached unlocks safely when pairs no longer exist
…login when expired (#1980) * feat: add proactive unlock cache expiry detection and redirect UI to login when expired * refactor: use one global unlockExpiry for all accounts * chore: fix conflict issues
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
packages/extension-polkagate/src/context/ExtensionLockContext.tsx (1)
44-50: Do not skip the lock-expiry check when migration is pending.This issue has already been identified in a previous review. The lock-expiry check currently only runs when
isPasswordsMigratedis truthy, which excludes non-migrated users (whereisPasswordsMigrated === false). The check should run for both migrated and non-migrated users, gating only on whether the migration state is known (isPasswordsMigrated !== undefined).packages/extension-base/src/background/handlers/Extension.ts (4)
132-140: Handle individual account password-change failures gracefully.If
accountsChangePasswordthrows for any account (e.g., corrupted keystore, internal error), the entire operation aborts and no accounts are updated. Wrap each call in a try-catch to handle failures per account and return partial success information.Apply this diff:
private accountsChangePasswordAll ({ newPass, oldPass }: RequestAccountChangePasswordAll): boolean { const accountsLocal = this.localAccounts(); const res = accountsLocal.map(({ address }) => { - return this.accountsChangePassword({ address, newPass, oldPass }); + try { + return this.accountsChangePassword({ address, newPass, oldPass }); + } catch (error) { + console.error(`Failed to change password for account ${address}:`, error); + return false; + } }); return res.every(Boolean); }
517-531: Avoid global TypeRegistry mutation; create a per-request registry for signing.Global registry is shared across concurrent requests and may be reconfigured mid-sign, causing non-deterministic behavior. Build and use a local registry per payload.
Apply this diff to create a per-request registry:
-private handleRegistry (payload: SignerPayloadJSON): void { +private createSigningRegistry (payload: SignerPayloadJSON): TypeRegistry { + const reg = new TypeRegistry(); // Get the metadata for the genesisHash const currentMetadata = this.metadataGet(payload.genesisHash); // set the registry before calling the sign function const signedExtensions = currentMetadata?.signedExtensions?.length ? currentMetadata.signedExtensions - : registry.signedExtensions; + : reg.signedExtensions; - registry.setSignedExtensions(signedExtensions, currentMetadata?.userExtensions); + reg.setSignedExtensions(signedExtensions, currentMetadata?.userExtensions); - if (currentMetadata) { - registry.register(currentMetadata?.types); + if (currentMetadata?.types) { + reg.register(currentMetadata.types); } + + return reg; }Then update call sites (Line 561 and Line 591) to use the returned registry.
576-595: Harden getSignature: fix validation order, handle missing accounts, and use local registry.Multiple issues in this method:
- Line 577 validates
payloadbut Line 581 destructuresaddressbefore the validation check. Ifpayloadis null/undefined, Line 581 throws before validation.- Line 583:
keyring.getPair(address)can throw for unknown/forgotten accounts. Return null instead of propagating the error.- Lines 591-592: Uses global registry via
handleRegistrywhich can cause race conditions with concurrent requests.Apply this diff to fix all issues:
private getSignature ({ payload }: RequestSigningSignature): HexString | null { if (!payload?.address || !payload.genesisHash) { throw new Error('Invalid payload: missing required fields.'); } const { address } = payload; - const pair = keyring.getPair(address); + let pair: KeyringPair; + + try { + pair = keyring.getPair(address); + } catch { + return null; + } this.refreshAccountPasswordCache(pair); // check if auto lock duration is expired if (pair.isLocked) { return null; } - this.handleRegistry(payload); - const { signature } = registry.createType('ExtrinsicPayload', payload, { version: payload.version }).sign(pair); + const reg = this.createSigningRegistry(payload); + const { signature } = reg + .createType('ExtrinsicPayload', payload, { version: payload.version }) + .sign(pair); return signature; }Note: This assumes
handleRegistryis refactored tocreateSigningRegistryas suggested in the previous comment.
561-564: Update to use per-request registry from refactored method.Once
handleRegistryis refactored tocreateSigningRegistry(returning a local registry), update this call site to capture and use the returned registry for signing instead of the global instance.Example:
if (isJsonPayload(payload)) { - this.handleRegistry(payload); + const reg = this.createSigningRegistry(payload); + const result = request.sign(reg, pair); +} else { + const result = request.sign(registry, pair); } - -const result = request.sign(registry, pair);Note: Verify that
request.signaccepts a registry parameter; if not, you may need to adjust the signing flow to use the local registry.
🧹 Nitpick comments (2)
packages/extension-polkagate/src/context/ExtensionLockContext.tsx (1)
32-42: Improve type safety for the message listener.The message parameter is typed as
any, which bypasses type checking. Consider defining a proper message type.Apply this diff to add type safety:
+interface LockExpiredMessage { + type: 'LOCKED_ACCOUNTS_EXPIRED'; +} + useEffect(() => { - const handleLockExpiredMessage = (msg: any) => { + const handleLockExpiredMessage = (msg: LockExpiredMessage) => { if (msg.type === 'LOCKED_ACCOUNTS_EXPIRED') { window.location.reload(); } };Additionally, verify the hard reload behavior.
window.location.reload()discards all application state. Confirm this is the intended behavior rather than a softer state reset through React context updates.packages/extension-base/src/background/handlers/Extension.ts (1)
478-511: Consider handling individual account unlock failures gracefully.The current implementation wraps the entire unlock operation in a single try-catch. If
decodePkcs8fails for any account (e.g., corrupted keystore), the operation returns false and the global expiry is not set for any accounts. Consider wrapping each account unlock in a try-catch to allow partial success, where successfully unlocked accounts still get the benefit of the cache time.Example approach:
private accountsUnlockAll ({ cacheTime, password }: RequestUnlockAllAccounts): boolean { if (!password) { throw new Error('Password needed to unlock the account'); } - try { - const accountsLocal = this.localAccounts(); - - for (const { address } of accountsLocal) { + const accountsLocal = this.localAccounts(); + const results: boolean[] = []; + + for (const { address } of accountsLocal) { + try { const pair = keyring.getPair(address); if (!pair) { - throw new Error('Unable to find pair'); + results.push(false); + continue; } if (!pair.isLocked) { pair.lock(); } if (pair.isLocked) { pair.decodePkcs8(password); } + + results.push(true); + } catch (error) { + console.error(`Failed to unlock account ${address}:`, error); + results.push(false); } + } + if (results.some(Boolean)) { // Set a single expiry timestamp for all accounts this.setUnlockExpiry(Date.now() + cacheTime); + } - return true; - } catch (error) { - console.error('accountsUnlockAll failed:', error); - - return false; // return false if any decode fails - } + return results.every(Boolean); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/extension-base/src/background/handlers/Extension.ts(10 hunks)packages/extension-polkagate/src/context/ExtensionLockContext.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/extension-polkagate/src/context/ExtensionLockContext.tsx (2)
packages/extension-polkagate/src/hooks/useIsPasswordMigrated.ts (1)
useIsPasswordMigrated(9-19)packages/extension-polkagate/src/messaging.ts (1)
areAccountsLocksExpired(104-106)
packages/extension-base/src/background/handlers/Extension.ts (2)
packages/extension-base/src/background/handlers/State.ts (1)
State(134-584)packages/extension-base/src/background/types.ts (4)
RequestAccountChangePasswordAll(237-240)RequestUnlockAllAccounts(362-365)RequestSigningSignature(320-322)RequestUpdateMeta(357-360)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: build
🔇 Additional comments (2)
packages/extension-polkagate/src/context/ExtensionLockContext.tsx (1)
4-4: LGTM: Imports and hook integration are correct.The new imports and hook usage properly support the lock-expiry and migration workflows.
Also applies to: 6-7, 27-27
packages/extension-base/src/background/handlers/Extension.ts (1)
62-64: ****A listener for the
LOCKED_ACCOUNTS_EXPIREDmessage type is already properly configured in the codebase. Inpackages/extension-polkagate/src/context/ExtensionLockContext.tsx, the listener is registered viachrome.runtime.onMessage.addListener()at line 39 to handle this exact message type (line 34), triggering a window reload when received. The message will be successfully delivered and handled without the.catch(console.error)being invoked for a missing listener.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/extension-polkagate/src/hooks/usePool.ts(1 hunks)packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/useAccountImportOrCreate.ts(1 hunks)packages/extension/public/locales/en/translation.json(1 hunks)packages/extension/public/locales/es/translation.json(1 hunks)packages/extension/public/locales/fr/translation.json(1 hunks)packages/extension/public/locales/hi/translation.json(2 hunks)packages/extension/public/locales/ru/translation.json(1 hunks)packages/extension/public/locales/zh/translation.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/extension-polkagate/src/hooks/usePool.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/extension/public/locales/hi/translation.json
🧰 Additional context used
🧬 Code graph analysis (1)
packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/useAccountImportOrCreate.ts (6)
packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/types.ts (1)
AccountInfo(4-8)packages/extension-polkagate/src/hooks/useIsPasswordCorrect.ts (1)
useIsPasswordCorrect(9-40)packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/resetAccounts.ts (1)
resetOnForgotPassword(10-18)packages/extension-polkagate/src/messaging.ts (1)
createAccountSuri(173-175)packages/extension-polkagate/src/util/defaultType.ts (1)
DEFAULT_TYPE(6-6)packages/extension-polkagate/src/util/constants.ts (2)
STORAGE_KEY(309-330)PROFILE_TAGS(260-266)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (8)
packages/extension/public/locales/zh/translation.json (1)
852-872: Translations for master password feature are complete and properly formatted.The new translations for master password functionality (setup, migration, auto-lock, password management, and account derivation) are well-integrated into the locale file. All placeholder variables (
{{name}},{{migrated}},{{count}}) are preserved correctly, and the Chinese translations are consistent with the feature semantics.Minor observation: Line 852 ("Missing tokens? Manage Networks!") is marked with
~but the translation appears unchanged from the original. Verify this is intentional or a marker artifact.packages/extension/public/locales/ru/translation.json (1)
858-878: Master password translation strings are properly formatted and consistent.The 20 new Russian translations for the master password feature follow existing conventions:
- All template variables (
{{name}},{{migrated}},{{count}}) are preserved correctly- JSON structure is valid with no syntax errors
- Terminology (e.g., "мастер-пароль", "учетная запись", "миграция") is consistent with other translation entries
- Error and completion messages follow the established pattern
packages/extension/public/locales/fr/translation.json (1)
860-879: Master password workflow translations are well-formatted and consistent.The new French translations for the master password migration feature are properly structured with correct JSON syntax, preserved template variables (
{{name}},{{migrated}},{{count}}), and French localization conventions. All entries maintain consistency with existing translation patterns in the file.packages/extension/public/locales/es/translation.json (1)
1-880: Translation file is well-structured and complete.The JSON is syntactically valid, and the Spanish translations follow consistent terminology, proper grammar, and appropriate formality level throughout. The ~20 new master-password related entries are comprehensive and properly formatted with correct placeholder interpolation.
To ensure completeness, please verify that all new translation keys added in lines 860–879 are actually referenced in the master-password UI code. For example, keys like
"Set master password","Migrate accounts to master password", and the long-form descriptions should have corresponding usage in components handling master-password flows (e.g., migration dialogs, password setup screens).packages/extension/public/locales/en/translation.json (4)
804-804: Clarify placement of existing key refresh.Line 804 appears to be an existing key ("Missing tokens? Manage Networks!") that's now being included in the master-password translation set. Verify that this repositioning is intentional and not accidentally mixing context boundaries between feature sections.
822-822: Verify contextual appropriateness of this key within master-password feature.The phrase "This child account will be created from your parent account. Give it a name." relates to derived account creation, not master-password migration or unlock flows. Confirm this key belongs in the master-password feature translations or if it should be in a separate account-derivation section.
808-808: Ensure placeholder variable names align with actual template context.Lines 808, 816, and 817 use placeholders
{{name}},{{migrated}}, and{{count}}. Verify these placeholders are correctly supplied by the code consuming these translation strings, and that the surrounding UI correctly displays these values in context (e.g., "Migrate account 2 of 5").Also applies to: 816-817
814-818: Review lengthy onboarding/migration descriptions for clarity and tone.These multi-sentence descriptions (lines 814-815, 818) provide good context for users but are quite long. Verify that:
- They fit within intended UI layouts without truncation
- The tone and terminology ("master password," "unlock and sign") are consistent with the rest of the application
- They are tested with actual UI rendering to confirm readability
...extension-polkagate/src/popup/newAccount/createAccountFullScreen/useAccountImportOrCreate.ts
Show resolved
Hide resolved
...extension-polkagate/src/popup/newAccount/createAccountFullScreen/useAccountImportOrCreate.ts
Outdated
Show resolved
Hide resolved
...extension-polkagate/src/popup/newAccount/createAccountFullScreen/useAccountImportOrCreate.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
packages/extension-polkagate/src/hooks/useAutoLock.ts (1)
27-29: Optional: initialize state with DEFAULT_AUTO_LOCK to remove undefined states.Prevents UI flicker and repeated optional chaining.
-export default function useAutoLock (): AutoLock | undefined { - const [autoLock, setAutoLock] = useState<AutoLock>(); +export default function useAutoLock (): AutoLock { + const [autoLock, setAutoLock] = useState<AutoLock>(DEFAULT_AUTO_LOCK);packages/extension-polkagate/src/popup/settings/extensionSettings/partials/consts.ts (1)
4-8: Type the option values and keep labels i18n‑friendly.
- Tie value to AutoLockDelayType to prevent drift.
- Prefer translating labels at render time rather than hardcoding.
+import type { AutoLockDelayType } from '@polkadot/extension-polkagate/src/hooks/useAutoLock'; - -export const autoLockOptions = [ - { text: 'min', value: 'min' }, - { text: 'hour', value: 'hour' }, - { text: 'day', value: 'day' } -]; +export const autoLockOptions: { text: string; value: AutoLockDelayType }[] = [ + { text: 'min', value: 'min' }, + { text: 'hour', value: 'hour' }, + { text: 'day', value: 'day' } +];Confirm whether DropSelect expects pre‑translated text, or if we should pass translation keys and call t() in the consumer.
packages/extension-polkagate/src/fullscreen/settings/partials/AutoLockTimerAdjustment.tsx (1)
47-47: Prefer nullish coalescing once input is validated/clamped in the hook.After clamping in the hook, this avoids surprising fallback when the user types 0 while we surface a clear min constraint.
- inputValue={inputValue || autoLock?.delay?.value || AUTO_LOCK_PERIOD_DEFAULT} + inputValue={inputValue ?? autoLock?.delay?.value ?? AUTO_LOCK_PERIOD_DEFAULT}packages/extension-polkagate/src/fullscreen/settings/partials/useAdjustAutoLockTimer.tsx (1)
63-68: Optional: avoid JSON.stringify for deep equality.Stringify is O(n) and brittle. Consider shallow compare of fields or a tiny deepEqual util.
- if (!autoLock || JSON.stringify(toSave) === JSON.stringify(autoLock)) { + if ( + !autoLock || + (toSave.enabled === autoLock.enabled && + toSave.delay.type === autoLock.delay.type && + toSave.delay.value === autoLock.delay.value) + ) { return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/extension-polkagate/src/fullscreen/settings/ExtensionSettings.tsx(2 hunks)packages/extension-polkagate/src/fullscreen/settings/partials/AutoLockTimer.tsx(0 hunks)packages/extension-polkagate/src/fullscreen/settings/partials/AutoLockTimerAdjustment.tsx(1 hunks)packages/extension-polkagate/src/fullscreen/settings/partials/useAdjustAutoLockTimer.tsx(1 hunks)packages/extension-polkagate/src/hooks/useAutoLock.ts(2 hunks)packages/extension-polkagate/src/popup/settings/extensionSettings/Main.tsx(2 hunks)packages/extension-polkagate/src/popup/settings/extensionSettings/partials/AutoLockTimer.tsx(0 hunks)packages/extension-polkagate/src/popup/settings/extensionSettings/partials/consts.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/extension-polkagate/src/popup/settings/extensionSettings/partials/AutoLockTimer.tsx
- packages/extension-polkagate/src/fullscreen/settings/partials/AutoLockTimer.tsx
🧰 Additional context used
🧬 Code graph analysis (5)
packages/extension-polkagate/src/popup/settings/extensionSettings/Main.tsx (1)
packages/extension-polkagate/src/fullscreen/settings/partials/AutoLockTimerAdjustment.tsx (1)
AutoLockTimerAdjustment(15-68)
packages/extension-polkagate/src/fullscreen/settings/partials/AutoLockTimerAdjustment.tsx (5)
packages/extension-polkagate/src/components/translate.ts (1)
useTranslation(8-10)packages/extension-polkagate/src/hooks/useIsExtensionPopup.ts (1)
useIsExtensionPopup(4-21)packages/extension-polkagate/src/fullscreen/settings/partials/useAdjustAutoLockTimer.tsx (1)
useAdjustAutoLockTimer(17-78)packages/extension-polkagate/src/util/constants.ts (1)
AUTO_LOCK_PERIOD_DEFAULT(226-226)packages/extension-polkagate/src/popup/settings/extensionSettings/partials/consts.ts (1)
autoLockOptions(4-8)
packages/extension-polkagate/src/hooks/useAutoLock.ts (1)
packages/extension-polkagate/src/util/constants.ts (1)
STORAGE_KEY(309-330)
packages/extension-polkagate/src/fullscreen/settings/ExtensionSettings.tsx (1)
packages/extension-polkagate/src/fullscreen/settings/partials/AutoLockTimerAdjustment.tsx (1)
AutoLockTimerAdjustment(15-68)
packages/extension-polkagate/src/fullscreen/settings/partials/useAdjustAutoLockTimer.tsx (2)
packages/extension-polkagate/src/hooks/useAutoLock.ts (3)
AutoLock(11-17)AutoLockDelayType(9-9)useAutoLock(27-39)packages/extension-polkagate/src/util/constants.ts (2)
AUTO_LOCK_PERIOD_DEFAULT(226-226)STORAGE_KEY(309-330)
🔇 Additional comments (3)
packages/extension-polkagate/src/fullscreen/settings/ExtensionSettings.tsx (1)
11-11: Replacement to AutoLockTimerAdjustment looks correct.Import path and placement are consistent with the new flow.
Also applies to: 37-37
packages/extension-polkagate/src/popup/settings/extensionSettings/Main.tsx (1)
7-7: LGTM on swapping in AutoLockTimerAdjustment.No behavioral concerns.
Also applies to: 18-18
packages/extension-polkagate/src/fullscreen/settings/partials/useAdjustAutoLockTimer.tsx (1)
11-11: Fix import source for setStorage.The
setStoragefunction is exported from@polkadot/extension-polkagate/src/util(viautil/storage.ts), notcomponents/Loading. Using the wrong source risks circular dependencies and inconsistency across the codebase where 11 other files correctly import from util.-import { setStorage } from '../../../components/Loading'; +import { setStorage } from '@polkadot/extension-polkagate/src/util';
packages/extension-polkagate/src/fullscreen/settings/partials/AutoLockTimerAdjustment.tsx
Outdated
Show resolved
Hide resolved
packages/extension-polkagate/src/fullscreen/settings/partials/useAdjustAutoLockTimer.tsx
Show resolved
Hide resolved
packages/extension-polkagate/src/fullscreen/settings/partials/useAdjustAutoLockTimer.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
packages/extension-base/src/background/handlers/Extension.ts (4)
132-140: Handle individual account password-change failures gracefully.If
accountsChangePasswordthrows for any account, the entire operation fails and no accounts are updated. The past review comment on this issue remains unaddressed.Apply this diff to handle failures per account:
private accountsChangePasswordAll ({ newPass, oldPass }: RequestAccountChangePasswordAll): boolean { const accountsLocal = this.localAccounts(); const res = accountsLocal.map(({ address }) => { - return this.accountsChangePassword({ address, newPass, oldPass }); + try { + return this.accountsChangePassword({ address, newPass, oldPass }); + } catch (error) { + console.error(`Failed to change password for account ${address}:`, error); + return false; + } }); return res.every(Boolean); }
244-252: Handle individual account forget failures gracefully.If
accountsForgetthrows for any account, the entire operation fails and remaining accounts are not processed. The past review comment on this issue remains unaddressed.Apply this diff to handle failures per account:
private accountsForgetAll (): boolean { const accounts = keyring.getAccounts(); accounts.forEach(({ address }) => { - this.accountsForget({ address }); + try { + this.accountsForget({ address }); + } catch (error) { + console.error(`Failed to forget account ${address}:`, error); + } }); return true; }
517-531: Avoid global TypeRegistry mutation; create a per-request registry for signing.The global
registryis shared across concurrent requests and may be reconfigured mid-sign, causing non-deterministic behavior. The past review comment on this issue remains unaddressed.Refactor to create and return a per-request registry:
-private handleRegistry (payload: SignerPayloadJSON): void { +private createSigningRegistry (payload: SignerPayloadJSON): TypeRegistry { + const reg = new TypeRegistry(); const currentMetadata = this.metadataGet(payload.genesisHash); const signedExtensions = currentMetadata?.signedExtensions?.length ? currentMetadata.signedExtensions - : registry.signedExtensions; + : reg.signedExtensions; - registry.setSignedExtensions(signedExtensions, currentMetadata?.userExtensions); + reg.setSignedExtensions(signedExtensions, currentMetadata?.userExtensions); - if (currentMetadata) { - registry.register(currentMetadata?.types); + if (currentMetadata?.types) { + reg.register(currentMetadata.types); } + + return reg; }Then update call sites (lines 561, 591) to use the returned registry.
576-595: Handle unknown accounts gracefully and avoid global registry mutation.While the payload validation issue has been fixed, two issues remain:
Missing error handling:
keyring.getPair(address)at Line 583 can throw for unknown or forgotten accounts, causing the method to reject instead of returningnullas intended.Global registry mutation: Line 591 calls
handleRegistry(payload)which mutates the shared global registry, causing potential race conditions between concurrent signing requests.Apply this diff to handle unknown accounts:
private getSignature ({ payload }: RequestSigningSignature): HexString | null { if (!payload?.address || !payload.genesisHash) { throw new Error('Invalid payload: missing required fields.'); } const { address } = payload; - const pair = keyring.getPair(address); + let pair: KeyringPair; + + try { + pair = keyring.getPair(address); + } catch { + return null; + } this.refreshAccountPasswordCache(pair); if (pair.isLocked) { return null; } - this.handleRegistry(payload); - const { signature } = registry.createType('ExtrinsicPayload', payload, { version: payload.version }).sign(pair); + const reg = this.createSigningRegistry(payload); + const { signature } = reg.createType('ExtrinsicPayload', payload, { version: payload.version }).sign(pair); return signature; }Note: This assumes
handleRegistryis refactored tocreateSigningRegistryas suggested in the earlier comment.
🧹 Nitpick comments (2)
packages/extension-base/src/background/handlers/Extension.ts (2)
478-511: Consider handling individual account unlock failures gracefully.The current implementation uses a single try-catch around the entire loop. If any account fails to unlock (e.g., wrong password for one account, corrupted keystore), the operation returns
falseand no accounts are unlocked. This all-or-nothing approach may not provide the best user experience.Consider tracking which accounts succeeded and which failed, then returning a more informative result (or at least logging failures per account while continuing to process remaining accounts).
Example approach:
private accountsUnlockAll ({ cacheTime, password }: RequestUnlockAllAccounts): boolean { if (!password) { throw new Error('Password needed to unlock the account'); } - try { - const accountsLocal = this.localAccounts(); - - for (const { address } of accountsLocal) { - const pair = keyring.getPair(address); - - if (!pair) { - throw new Error('Unable to find pair'); - } - - if (!pair.isLocked) { - pair.lock(); - } - - if (pair.isLocked) { - pair.decodePkcs8(password); - } - } - - // Set a single expiry timestamp for all accounts - this.setUnlockExpiry(Date.now() + cacheTime); - - return true; - } catch (error) { - console.error('accountsUnlockAll failed:', error); - - return false; // return false if any decode fails - } + const accountsLocal = this.localAccounts(); + let successCount = 0; + + for (const { address } of accountsLocal) { + try { + const pair = keyring.getPair(address); + + if (!pair) { + console.error(`Unable to find pair for account ${address}`); + continue; + } + + if (pair.isLocked) { + pair.decodePkcs8(password); + } + + successCount++; + } catch (error) { + console.error(`Failed to unlock account ${address}:`, error); + } + } + + if (successCount > 0) { + // Set expiry if at least one account was unlocked + this.setUnlockExpiry(Date.now() + cacheTime); + return true; + } + + return false; }
513-515: Clarify the semantics ofareLocksExpiredwhen no expiry is set.The method returns
truewhen#unlockExpiry === null, which means "no unlock is active" rather than "locks have expired". This might be semantically confusing for callers expecting a distinction between "never unlocked" and "expired".Consider whether the method name and return value accurately represent the intended state. If
nullmeans "all accounts are locked" (which is effectively "expired" from an unlock perspective), the current logic is correct but could benefit from a clarifying comment.Example clarification:
private areLocksExpired (): boolean { + // Returns true if no unlock is active or if the unlock has expired return this.#unlockExpiry === null || this.#unlockExpiry < Date.now(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/extension-base/src/background/handlers/Extension.ts(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/extension-base/src/background/handlers/Extension.ts (2)
packages/extension-base/src/background/handlers/State.ts (1)
State(134-584)packages/extension-base/src/background/types.ts (4)
RequestAccountChangePasswordAll(237-240)RequestUnlockAllAccounts(362-365)RequestSigningSignature(320-322)RequestUpdateMeta(357-360)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
packages/extension-base/src/background/handlers/Extension.ts (1)
186-226: LGTM! Past issues have been addressed.The method now correctly:
- Iterates local accounts using
for...ofinstead ofObject.entries- Wraps
getPairin try-catch to handle forgotten accounts gracefully- Uses
withErrorLogwrapper forchrome.tabs.reload- Clears unlock expiry before reloading tabs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/useAccountImportOrCreate.ts (2)
69-69: Check the boolean result of resetOnForgotPassword.The function returns a boolean indicating success or failure, but the result is ignored. If the reset fails, account creation proceeds anyway, potentially leaving forgotten accounts in an inconsistent state.
Apply this diff to check the result and abort on failure:
- await resetOnForgotPassword(); + const resetOk = await resetOnForgotPassword(); + if (!resetOk) { + setIsBusy(false); + return setError(t('Failed to reset accounts')); + }
82-87: Block navigation when storage operations fail.Lines 82-84 capture storage failures but only log a warning. Navigation and page reload proceed regardless, leaving the application in an inconsistent state where the account exists but critical flags (
SELECTED_PROFILE,IS_PASSWORD_MIGRATED) are not persisted.Apply this diff to prevent navigation on storage failure:
const okProfile = await setStorage(STORAGE_KEY.SELECTED_PROFILE, PROFILE_TAGS.LOCAL); const okMigrated = await setStorage(STORAGE_KEY.IS_PASSWORD_MIGRATED, true); if (!okProfile || !okMigrated) { - console.warn('Failed to persist profile or migration flag'); + setIsBusy(false); + return setError(t('Failed to save account settings')); } navigate(onSuccessPath) as void; window.location.reload();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/useAccountImportOrCreate.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/useAccountImportOrCreate.ts (6)
packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/types.ts (1)
AccountInfo(4-8)packages/extension-polkagate/src/hooks/useIsPasswordCorrect.ts (1)
useIsPasswordCorrect(9-40)packages/extension-polkagate/src/popup/newAccount/createAccountFullScreen/resetAccounts.ts (1)
resetOnForgotPassword(10-18)packages/extension-polkagate/src/messaging.ts (1)
createAccountSuri(173-175)packages/extension-polkagate/src/util/defaultType.ts (1)
DEFAULT_TYPE(6-6)packages/extension-polkagate/src/util/constants.ts (2)
STORAGE_KEY(309-330)PROFILE_TAGS(260-266)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: build
...extension-polkagate/src/popup/newAccount/createAccountFullScreen/useAccountImportOrCreate.ts
Show resolved
Hide resolved
1 participant
Summary by CodeRabbit
New Features
UI/UX
Localization
Chores