[EXTENSION] Decrypt vault in SecuritySettings re-auth flow#749
Open
Trovic1 wants to merge 11 commits into
Open
[EXTENSION] Decrypt vault in SecuritySettings re-auth flow#749Trovic1 wants to merge 11 commits into
Trovic1 wants to merge 11 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 40 minutes and 41 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR implements password re-verification for sensitive settings in the extension wallet, refactors the web dashboard's account overview fetching from direct Horizon calls to a typed-error API-based approach with user-facing retry alerts, and narrows the smart contract's upgrade WASM-hash validation to reject only all-zero hashes. Related tests and infrastructure are updated throughout. ChangesTransaction History Empty State Update
Extension Wallet Security Settings Password Re-verification Flow
Web Dashboard Account Overview API Migration and Error Handling
Account Contract Upgrade WASM Hash Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
@Trovic1 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
apps/extension-wallet/src/screens/Settings/SecuritySettings.tsx (1)
23-29: 💤 Low value
currentValueis stored but never used.The
currentValuefield inSensitiveToggleis set inhandleSensitiveToggleRequest(line 554) but is never passed toSensitiveSettingConfirmDialogor referenced elsewhere. If it's not needed, remove it to avoid confusion.🧹 Remove unused field
type SensitiveToggle = { label: string; description: string; - currentValue: boolean; nextValue: boolean; onConfirm: (value: boolean) => void; } | null;function handleSensitiveToggleRequest(nextValue: boolean) { setSensitiveToggle({ label: 'Require password for exports', description: 'Re-enter your password to change this sensitive setting.', - currentValue: requirePasswordForSensitiveActions, nextValue, onConfirm: onRequirePasswordForSensitiveActionsChange, }); }Also applies to: 550-558
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/extension-wallet/src/screens/Settings/SecuritySettings.tsx` around lines 23 - 29, Remove the unused currentValue property from the SensitiveToggle type and stop setting it in handleSensitiveToggleRequest; update any code that constructs SensitiveToggle (e.g., where you create the object in handleSensitiveToggleRequest) to only include label, description, nextValue, and onConfirm, and ensure SensitiveSettingConfirmDialog remains passed the fields it actually uses (label, description, nextValue, onConfirm) so there are no lingering references to currentValue.apps/web-dashboard/src/hooks/useAccountOverview.ts (2)
54-57: 💤 Low valueStale doc comment. The hook no longer calls Horizon directly; it fetches the internal
/api/account-overviewendpoint. Update to avoid misleading readers.📝 Proposed wording
/** * Hook to fetch account overview metrics (balance, nonce, status). - * Uses Horizon API to fetch real Stellar account data. + * Fetches from the internal `/api/account-overview` endpoint. */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web-dashboard/src/hooks/useAccountOverview.ts` around lines 54 - 57, The doc comment above the useAccountOverview hook is stale—update it to state that the hook fetches the internal "/api/account-overview" endpoint (not Horizon) and returns parsed account overview metrics (balance, nonce, status) from our backend; edit the block comment for useAccountOverview to clearly describe the current behavior and data shape returned so readers aren't misled about calling Horizon directly.
63-100: ⚡ Quick winConsider aborting in-flight fetches on unmount/refetch.
fetchDatahas noAbortController, so a pending request can resolve after unmount (state-update-after-unmount) or, ifpublicKeychanges /refetchis invoked while a request is in flight, a stale response can overwrite newer state (last-write-wins).♻️ Proposed approach
- const fetchData = useCallback(async () => { + const fetchData = useCallback(async (signal?: AbortSignal) => { if (!publicKey) { setData(null); setError(null); setIsLoading(false); return; } setIsLoading(true); setError(null); try { const response = await fetch( - `/api/account-overview?publicKey=${encodeURIComponent(publicKey)}` + `/api/account-overview?publicKey=${encodeURIComponent(publicKey)}`, + { signal } ); if (!response.ok) { throw classifyAccountOverviewError(response.status); } const accountOverview = (await response.json()) as AccountOverview; setData(accountOverview); } catch (err) { + if (signal?.aborted) return; setError( err instanceof AccountOverviewError ? err : new AccountOverviewError('Failed to fetch account data', 'FETCH_FAILED') ); setData(null); } finally { - setIsLoading(false); + if (!signal?.aborted) setIsLoading(false); } }, [publicKey]); useEffect(() => { - fetchData(); + const controller = new AbortController(); + fetchData(controller.signal); + return () => controller.abort(); }, [fetchData]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web-dashboard/src/hooks/useAccountOverview.ts` around lines 63 - 100, fetchData currently starts a fetch without cancellation, which can cause state updates after unmount or stale responses overwriting newer state; fix by creating an AbortController inside fetchData (or in the effect) and pass its signal to fetch, bail out on abort (do not call setData/setError/setIsLoading when signal.aborted or when the caught error is an AbortError), and ensure the useEffect cleanup aborts the controller when publicKey changes or the component unmounts; update fetchData/useEffect to use the controller's signal, handle AbortError specially (skip setting state), and keep existing error handling (AccountOverviewError/classifyAccountOverviewError) for non-abort failures so stale requests cannot overwrite fresh state.contracts/account/src/lib.rs (1)
1310-1324: ⚡ Quick winTest name contradicts its assertion; missing coverage for zero-hash rejection.
The test is named
test_upgrade_rejects_same_wasm_hashbut assertsis_ok(), which is confusing. It no longer tests rejection behavior—it just verifies that a non-zero hash is accepted.Consider:
- Renaming this test to
test_upgrade_accepts_non_zero_hash(or similar)- Adding a separate test for the zero-hash rejection path to cover line 407
♻️ Proposed refactor: rename and add zero-hash test
#[test] - fn test_upgrade_rejects_same_wasm_hash() { + fn test_upgrade_accepts_non_zero_hash() { let env = Env::default(); let contract_id = env.register_contract(None, AncoreAccount); let client = AncoreAccountClient::new(&env, &contract_id); let owner = Address::generate(&env); init(&env, &client, &owner); env.mock_all_auths(); let upgrade_hash = BytesN::from_array(&env, &[7; 32]); let result = client.try_upgrade(&upgrade_hash); assert!(result.is_ok()); } + + #[test] + fn test_upgrade_rejects_zero_hash() { + let env = Env::default(); + let contract_id = env.register_contract(None, AncoreAccount); + let client = AncoreAccountClient::new(&env, &contract_id); + + let owner = Address::generate(&env); + init(&env, &client, &owner); + env.mock_all_auths(); + + let zero_hash = BytesN::from_array(&env, &[0u8; 32]); + let result = client.try_upgrade(&zero_hash); + assert_eq!(result, Err(Ok(ContractError::InvalidWasmHash))); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/account/src/lib.rs` around lines 1310 - 1324, The test currently named test_upgrade_rejects_same_wasm_hash actually asserts success; rename it to test_upgrade_accepts_non_zero_hash and keep the existing setup (Env::default, register_contract, AncoreAccountClient::new, init, env.mock_all_auths) and the non-zero upgrade_hash BytesN::from_array(&env, &[7; 32]) with assert!(result.is_ok()) to reflect acceptance of a non-zero hash. Add a new test named test_upgrade_rejects_zero_hash that uses the same setup but passes a zero hash BytesN::from_array(&env, &[0; 32]) to client.try_upgrade and asserts failure (assert!(result.is_err())) to cover the zero-hash rejection code path in try_upgrade.apps/web-dashboard/src/hooks/__tests__/useAccountOverview.test.ts (1)
68-109: ⚡ Quick winRefetch test cannot detect a broken
refetch.Both queued responses return
balance: 50, so the post-refetch assertion on Line 107-108 passes even ifrefetch()never re-fetched. Make the second response differ to actually prove the data was refreshed.♻️ Use a distinct second payload
.mockResolvedValueOnce( new Response( JSON.stringify({ - balance: 50, - nonce: 100, + balance: 75, + nonce: 101, status: 'active', }), { status: 200, headers: { 'Content-Type': 'application/json' }, } ) ); @@ - await waitFor(() => expect(result.current.data?.balance).toBe(50)); - expect(result.current.data?.balance).toBe(50); + await waitFor(() => expect(result.current.data?.balance).toBe(75)); + expect(result.current.data?.balance).toBe(75);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web-dashboard/src/hooks/__tests__/useAccountOverview.test.ts` around lines 68 - 109, The test for useAccountOverview's refetch is flawed because both mocked fetch responses return identical payloads so refetch could be a no-op; update the mocked responses in the 'allows refetch' test to return a different second payload (e.g., change balance or nonce) for the second mockResolvedValueOnce so that calling result.current.refetch() (refetch) actually changes result.current.data and the assertion verifies a real refetch; locate the test in useAccountOverview.test.ts and modify the second Response payload to a distinct value to ensure the post-refetch expect checks the changed field.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/extension-wallet/src/security/__tests__/vault-export.test.ts`:
- Around line 14-27: The mocked StorageAdapter created in vi.mock keeps a
singleton adapter instance (adapter closure) whose MockStorageAdapter store
persists across tests, breaking isolation; change the mock for
createStorageAdapter (or MockStorageAdapter) so tests get a fresh, cleared
adapter each run—either return a new MockStorageAdapter instance on each
createStorageAdapter() call instead of reusing the closure-scoped adapter, or
add a clear/reset method on MockStorageAdapter and call that from the test
beforeEach (alongside resetVaultStorageManagerForTests and localStorage.clear())
to ensure the in-memory Map is emptied between tests.
In `@apps/web-dashboard/src/hooks/useAccountOverview.ts`:
- Around line 11-52: Update the outdated doc comment to reflect that
useAccountOverview fetches from the internal "/api/account-overview" endpoint
(not Horizon directly), and modify useAccountOverview's fetchData to prevent
stale responses: create and use an AbortController (or a request-version token)
tied to each fetch and signal.abort() on effect cleanup or when publicKey
changes, ensure fetch honors the controller and that state updates
(setData/setError/setIsLoading) are skipped if the request was aborted or the
component is unmounted; apply the same cancellation logic to refetch and keep
classifyAccountOverviewError, AccountOverviewError, AccountNotFoundError, and
HorizonUnavailableError intact for error classification and handling.
---
Nitpick comments:
In `@apps/extension-wallet/src/screens/Settings/SecuritySettings.tsx`:
- Around line 23-29: Remove the unused currentValue property from the
SensitiveToggle type and stop setting it in handleSensitiveToggleRequest; update
any code that constructs SensitiveToggle (e.g., where you create the object in
handleSensitiveToggleRequest) to only include label, description, nextValue, and
onConfirm, and ensure SensitiveSettingConfirmDialog remains passed the fields it
actually uses (label, description, nextValue, onConfirm) so there are no
lingering references to currentValue.
In `@apps/web-dashboard/src/hooks/__tests__/useAccountOverview.test.ts`:
- Around line 68-109: The test for useAccountOverview's refetch is flawed
because both mocked fetch responses return identical payloads so refetch could
be a no-op; update the mocked responses in the 'allows refetch' test to return a
different second payload (e.g., change balance or nonce) for the second
mockResolvedValueOnce so that calling result.current.refetch() (refetch)
actually changes result.current.data and the assertion verifies a real refetch;
locate the test in useAccountOverview.test.ts and modify the second Response
payload to a distinct value to ensure the post-refetch expect checks the changed
field.
In `@apps/web-dashboard/src/hooks/useAccountOverview.ts`:
- Around line 54-57: The doc comment above the useAccountOverview hook is
stale—update it to state that the hook fetches the internal
"/api/account-overview" endpoint (not Horizon) and returns parsed account
overview metrics (balance, nonce, status) from our backend; edit the block
comment for useAccountOverview to clearly describe the current behavior and data
shape returned so readers aren't misled about calling Horizon directly.
- Around line 63-100: fetchData currently starts a fetch without cancellation,
which can cause state updates after unmount or stale responses overwriting newer
state; fix by creating an AbortController inside fetchData (or in the effect)
and pass its signal to fetch, bail out on abort (do not call
setData/setError/setIsLoading when signal.aborted or when the caught error is an
AbortError), and ensure the useEffect cleanup aborts the controller when
publicKey changes or the component unmounts; update fetchData/useEffect to use
the controller's signal, handle AbortError specially (skip setting state), and
keep existing error handling (AccountOverviewError/classifyAccountOverviewError)
for non-abort failures so stale requests cannot overwrite fresh state.
In `@contracts/account/src/lib.rs`:
- Around line 1310-1324: The test currently named
test_upgrade_rejects_same_wasm_hash actually asserts success; rename it to
test_upgrade_accepts_non_zero_hash and keep the existing setup (Env::default,
register_contract, AncoreAccountClient::new, init, env.mock_all_auths) and the
non-zero upgrade_hash BytesN::from_array(&env, &[7; 32]) with
assert!(result.is_ok()) to reflect acceptance of a non-zero hash. Add a new test
named test_upgrade_rejects_zero_hash that uses the same setup but passes a zero
hash BytesN::from_array(&env, &[0; 32]) to client.try_upgrade and asserts
failure (assert!(result.is_err())) to cover the zero-hash rejection code path in
try_upgrade.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b8440885-d94d-4ee4-a93b-7da731002e66
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
apps/extension-wallet/src/router/__tests__/router.test.tsxapps/extension-wallet/src/screens/Settings/SecuritySettings.tsxapps/extension-wallet/src/screens/Settings/__tests__/settings.test.tsxapps/extension-wallet/src/security/__tests__/vault-export.test.tsapps/web-dashboard/src/hooks/__tests__/useAccountOverview.test.tsapps/web-dashboard/src/hooks/useAccountOverview.tsapps/web-dashboard/src/hooks/useWidgetErrorLogger.tsapps/web-dashboard/src/pages/Dashboard.tsxapps/web-dashboard/src/pages/__tests__/Dashboard.test.tsxapps/web-dashboard/src/widgets/__tests__/AccountWidgets.test.tsxapps/web-dashboard/src/widgets/__tests__/WidgetErrorBoundary.test.tsxcontracts/account/src/lib.rs
Comment on lines
+11
to
+52
| export class AccountOverviewError extends Error { | ||
| code: 'ACCOUNT_NOT_FOUND' | 'HORIZON_UNAVAILABLE' | 'FETCH_FAILED'; | ||
|
|
||
| constructor(message: string, code: 'ACCOUNT_NOT_FOUND' | 'HORIZON_UNAVAILABLE' | 'FETCH_FAILED') { | ||
| super(message); | ||
| this.name = 'AccountOverviewError'; | ||
| this.code = code; | ||
| } | ||
| } | ||
|
|
||
| export class AccountNotFoundError extends AccountOverviewError { | ||
| constructor() { | ||
| super('Account not found on network', 'ACCOUNT_NOT_FOUND'); | ||
| this.name = 'AccountNotFoundError'; | ||
| } | ||
| } | ||
|
|
||
| export class HorizonUnavailableError extends AccountOverviewError { | ||
| constructor() { | ||
| super('Horizon is temporarily unavailable', 'HORIZON_UNAVAILABLE'); | ||
| this.name = 'HorizonUnavailableError'; | ||
| } | ||
| } | ||
|
|
||
| export interface UseAccountOverviewReturn { | ||
| data: AccountOverview | null; | ||
| isLoading: boolean; | ||
| error: Error | null; | ||
| error: AccountOverviewError | null; | ||
| refetch: () => Promise<void>; | ||
| } | ||
|
|
||
| function classifyAccountOverviewError(status?: number): AccountOverviewError { | ||
| if (status === 404) { | ||
| return new AccountNotFoundError(); | ||
| } | ||
|
|
||
| if (status && status >= 500) { | ||
| return new HorizonUnavailableError(); | ||
| } | ||
|
|
||
| return new AccountOverviewError('Failed to fetch account data', 'FETCH_FAILED'); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect tsconfig target and any compiler target for the web-dashboard build.
fd -t f 'tsconfig*.json' apps/web-dashboard 2>/dev/null --exec cat {}
fd -t f 'vite.config.*' apps/web-dashboard 2>/dev/null --exec cat {}Repository: ancore-org/ancore
Length of output: 1844
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the reviewed hook file with line numbers (small enough window)
sed -n '1,120p' apps/web-dashboard/src/hooks/useAccountOverview.ts | cat -n
# Search for instanceof / error class usage in dashboard/tests
rg -n "instanceof\s+AccountOverviewError|instanceof\s+AccountNotFoundError|instanceof\s+HorizonUnavailableError|AccountOverviewError|AccountNotFoundError|HorizonUnavailableError" apps/web-dashboard/src apps/web-dashboard/tests || true
# Inspect tsconfig base target to ensure no override
fd -t f 'tsconfig.base.json' . --exec cat {} 2>/dev/null | head -n 120Repository: ancore-org/ancore
Length of output: 7501
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the reviewed hook file with line numbers (small enough window)
sed -n '1,140p' apps/web-dashboard/src/hooks/useAccountOverview.ts | cat -n
# Search for instanceof / error class usage in dashboard/tests
rg -n "instanceof\s+AccountOverviewError|instanceof\s+AccountNotFoundError|instanceof\s+HorizonUnavailableError|AccountOverviewError|AccountNotFoundError|HorizonUnavailableError" apps/web-dashboard/src apps/web-dashboard/tests || true
# Inspect tsconfig base target to ensure no override
fd -t f 'tsconfig.base.json' . --exec cat {} 2>/dev/null | head -n 120Repository: ancore-org/ancore
Length of output: 7501
Fix stale doc + prevent stale responses in useAccountOverview
- Error hierarchy/classifier look correct, and
instanceof-based handling in Dashboard/tests matches the current implementation; TS targets are ES2022/ESNext (no ES5 downleveling risk). - Doc comment is outdated: it claims the hook “Uses Horizon API … directly”, but it fetches
/api/account-overview. fetchDatalacks cancellation/cleanup (no AbortController/stale-request guard), so late responses can overwrite state after unmount or apublicKeychange.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web-dashboard/src/hooks/useAccountOverview.ts` around lines 11 - 52,
Update the outdated doc comment to reflect that useAccountOverview fetches from
the internal "/api/account-overview" endpoint (not Horizon directly), and modify
useAccountOverview's fetchData to prevent stale responses: create and use an
AbortController (or a request-version token) tied to each fetch and
signal.abort() on effect cleanup or when publicKey changes, ensure fetch honors
the controller and that state updates (setData/setError/setIsLoading) are
skipped if the request was aborted or the component is unmounted; apply the same
cancellation logic to refetch and keep classifyAccountOverviewError,
AccountOverviewError, AccountNotFoundError, and HorizonUnavailableError intact
for error classification and handling.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/extension-wallet/src/security/__tests__/vault-export.test.ts (1)
83-94:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix test data plumbing in vault-export password verification
verifyVaultPassword(password)doesn’t usegetSharedStorageManager()—it constructsnew SecureStorageManager(createStorageAdapter())directly. So, if the test seeds viaseedVaultAccount(storage, …)using a differentMockStorageAdapterinstance than the one returned by the mockedcreateStorageAdapter(), the password check will read an empty store and theresolves.toBe(true)assertion should fail. Ensure both seeding and verification target the same mocked adapter instance (or seed through the samecreateStorageAdapter()singleton used byverifyVaultPassword).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/extension-wallet/src/security/__tests__/vault-export.test.ts` around lines 83 - 94, The test fails because verifyVaultPassword creates its own SecureStorageManager via createStorageAdapter(), while the test seeds a different MockStorageAdapter instance; ensure both use the same adapter by changing the test to obtain the singleton adapter used by verifyVaultPassword (e.g., call createStorageAdapter() or getSharedStorageManager() to get the shared MockStorageAdapter) and seed via that instance instead of new MockStorageAdapter(), or alternatively mock createStorageAdapter() to return the test's storage so seedVaultAccount(storage, ...) and verifyVaultPassword(PASSWORD) operate on the same backing store.
♻️ Duplicate comments (1)
apps/extension-wallet/src/security/__tests__/vault-export.test.ts (1)
14-27:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSingleton mock adapter is never reset between tests.
The closure-scoped
adapteris created once and reused for the lifetime of the suite; its in-memoryMapis not cleared bybeforeEach(onlylocalStorage.clear()andresetVaultStorageManagerForTests()run). Any data written through the shared/default manager persists acrossit()blocks, making tests that depend on it order-sensitive. Adding aclear()toMockStorageAdapterand invoking it inbeforeEach(see the related comment above) resolves this.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/extension-wallet/src/security/__tests__/vault-export.test.ts` around lines 14 - 27, The singleton mock adapter created in the vi.mock closure (the closure-scoped adapter returned by createStorageAdapter) retains state across tests; add a clear() method to MockStorageAdapter (or expose a reset on that mock) and invoke it from your test suite's beforeEach (alongside localStorage.clear() and resetVaultStorageManagerForTests()) so adapter.clear() runs before each it(); ensure createStorageAdapter still returns the same mocked instance but that its internal Map is emptied between tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/extension-wallet/src/security/__tests__/vault-export.test.ts`:
- Around line 83-94: The test fails because verifyVaultPassword creates its own
SecureStorageManager via createStorageAdapter(), while the test seeds a
different MockStorageAdapter instance; ensure both use the same adapter by
changing the test to obtain the singleton adapter used by verifyVaultPassword
(e.g., call createStorageAdapter() or getSharedStorageManager() to get the
shared MockStorageAdapter) and seed via that instance instead of new
MockStorageAdapter(), or alternatively mock createStorageAdapter() to return the
test's storage so seedVaultAccount(storage, ...) and
verifyVaultPassword(PASSWORD) operate on the same backing store.
---
Duplicate comments:
In `@apps/extension-wallet/src/security/__tests__/vault-export.test.ts`:
- Around line 14-27: The singleton mock adapter created in the vi.mock closure
(the closure-scoped adapter returned by createStorageAdapter) retains state
across tests; add a clear() method to MockStorageAdapter (or expose a reset on
that mock) and invoke it from your test suite's beforeEach (alongside
localStorage.clear() and resetVaultStorageManagerForTests()) so adapter.clear()
runs before each it(); ensure createStorageAdapter still returns the same mocked
instance but that its internal Map is emptied between tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0048bc67-e086-4d0e-aec5-8928288b73a6
📒 Files selected for processing (2)
apps/extension-wallet/src/security/__tests__/vault-export.test.tscontracts/account/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/account/src/lib.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #689
Summary
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Chores