refactor(wallet): remove dead self-custody branches from deploy-web consumers#3243
Merged
baktun14 merged 3 commits intoJun 3, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (43)
💤 Files with no reviewable changes (9)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (32)
📝 WalkthroughWalkthroughConsolidates the wallet model to managed-only: removes custodial branches and prerequisites, standardizes managed-wallet storage and JWT flows, switches error reporting to notificator, unconditionally applies denom/audit transformations, and updates tests and UI gating to use flags and authenticated user identity. ChangesManaged-Wallet-Only Refactor
🎯 4 (Complex) | ⏱️ ~60 minutes
Suggested reviewers:
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Base automatically changed from
refactor/deploy-web-wallet-provider-managed-only
to
main
June 3, 2026 15:19
baktun14
force-pushed
the
refactor/deploy-web-remove-dual-mode-wallet-branches
branch
from
30fa3bc to
c31eb62
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3243 +/- ##
==========================================
- Coverage 67.78% 66.62% -1.17%
==========================================
Files 1077 988 -89
Lines 26303 23955 -2348
Branches 6318 5809 -509
==========================================
- Hits 17830 15959 -1871
+ Misses 7416 6992 -424
+ Partials 1057 1004 -53
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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)
apps/deploy-web/src/components/deployments/ManifestUpdate/ManifestUpdate.tsx (1)
128-176:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't surface provider/send failures as SDL parse errors.
Any error from
signAndBroadcastTx()orsendManifest()currently falls through tosetParsingError("Error while parsing SDL file"), so network/auth/provider failures get mislabeled as editor parse errors. KeepparsingErrorfor YAML/validation failures only, and let operational failures stay on the snackbar path.Suggested fix
} catch (error: any) { if (error.name === "YAMLException" || error.name === "CustomValidationError") { setParsingError(error.message); } else { - setParsingError("Error while parsing SDL file"); console.error(error); } setIsSendingManifest(false); }🤖 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/deploy-web/src/components/deployments/ManifestUpdate/ManifestUpdate.tsx` around lines 128 - 176, The catch block in handleUpdateClick is treating all exceptions as parse errors; only YAMLException and CustomValidationError should setParsingError — operational failures from signAndBroadcastTx or sendManifest should be propagated to the app-level/snackbar path. Change the catch so that if error.name is "YAMLException" or "CustomValidationError" you call setParsingError(error.message), otherwise clear any parsing error/state (or leave it unchanged), ensure setIsSendingManifest(false) is executed, log the error if desired, and rethrow the error (or return/propagate it) so signAndBroadcastTx/sendManifest failures are handled by the global/snackbar error handler; keep references to handleUpdateClick, signAndBroadcastTx, sendManifest, setParsingError, and setIsSendingManifest to locate and update the code.
🧹 Nitpick comments (3)
apps/deploy-web/src/queries/useApiKeysQuery.spec.tsx (1)
105-109: ⚡ Quick winAvoid nested
useroverrides insidemock()here.
vitest-mock-extendedcan proxy nested overrides, so unresolveduser.*fields may become mock functions instead ofundefined. Build the mock first, then assignuserandisLoadingon the returned object.Suggested pattern
- useUser: () => - mock<ReturnType<typeof USE_API_KEYS_DEPENDENCIES.useUser>>({ - user: mockUser, - isLoading: false - }) + useUser: () => { + const useUserResult = mock<ReturnType<typeof USE_API_KEYS_DEPENDENCIES.useUser>>(); + useUserResult.user = mockUser; + useUserResult.isLoading = false; + return useUserResult; + }Based on learnings: when using
vitest-mock-extended, avoid passing nested objects as overrides tomock<T>()because recursive proxies can turn unknown properties into mock functions; assign nested properties after creating the mock instead.Also applies to: 153-157, 201-205, 231-235
🤖 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/deploy-web/src/queries/useApiKeysQuery.spec.tsx` around lines 105 - 109, The test is passing a nested override object into mock<ReturnType<typeof USE_API_KEYS_DEPENDENCIES.useUser>>() which lets vitest-mock-extended proxy nested fields into mock functions; instead, create the base mock first (e.g., const userHook = mock<ReturnType<typeof USE_API_KEYS_DEPENDENCIES.useUser>>() ), then assign userHook.user = mockUser and userHook.isLoading = false and return userHook from the useUser override. Apply the same pattern to the other occurrences referenced (around lines 153-157, 201-205, 231-235) so nested properties are set after creating the mock.apps/deploy-web/src/components/layout/AccountMenu.spec.tsx (2)
84-95: ⚡ Quick winAvoid the nested
mock()override foruseCustomUser.
vitest-mock-extendedcan proxy nested overrides likeuser: { ... }, which makes future missing fields read back as mock functions instead ofundefined. Returning a plain hook result here is safer.Based on learnings: when using `vitest-mock-extended`, avoid passing nested objects as overrides to `mock()` because recursive proxies can turn unknown nested properties into mock functions.Safer setup
const dependencies: typeof DEPENDENCIES = { - useCustomUser: () => - mock<ReturnType<typeof DEPENDENCIES.useCustomUser>>({ - user: input.username ? { username: input.username, userId: input.userId } : undefined, - isLoading: input.isLoading ?? false - }), + useCustomUser: () => ({ + user: input.username ? { username: input.username, userId: input.userId } : undefined, + isLoading: input.isLoading ?? false, + error: undefined, + checkSession: vi.fn() + }), useRouter: () => mock<ReturnType<typeof DEPENDENCIES.useRouter>>({ push }), useFlag: flagName => (flagName === "billing_usage" && input.isBillingUsageEnabled) ?? false };🤖 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/deploy-web/src/components/layout/AccountMenu.spec.tsx` around lines 84 - 95, The test's setup function nests a mock override via mock<ReturnType<typeof DEPENDENCIES.useCustomUser>>({ user: ... }) which causes vitest-mock-extended to create proxy mock functions for unknown nested fields; instead, return a plain object for the useCustomUser hook result (not wrapped by mock) inside setup, e.g. replace the mock<ReturnType<typeof DEPENDENCIES.useCustomUser>>(...) return with an actual object matching the hook shape (providing user and isLoading) so unknown nested properties remain undefined; keep useRouter and useFlag mocks as-is and reference setup, DEPENDENCIES.useCustomUser, useCustomUser, useRouter, and useFlag when making the change.
59-81: ⚡ Quick winAdd the missing
flag=true/userId-absent regression test.This change makes
user?.userIdpart of the render contract, but the suite only covers the happy path andflag=false. A regression that shows Billing & Usage for users without an internal account would still pass here.Based on learnings: `user.userId` is the field used to determine whether an internal account exists and should be the onboarding/auth-state guard.Proposed test
+ it("hides Billing & Usage when the user has no internal account yet", async () => { + setup({ + username: "erin", + isBillingUsageEnabled: true + }); + + await userEvent.click(screen.getByRole("button", { name: /account menu/i })); + + await screen.findByText("Logout"); + expect(screen.queryByText("Billing & Usage")).not.toBeInTheDocument(); + });🤖 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/deploy-web/src/components/layout/AccountMenu.spec.tsx` around lines 59 - 81, Add a regression test to cover the case where isBillingUsageEnabled is true but the user lacks an internal account (user.userId is undefined); update AccountMenu.spec.tsx by adding a new it block (similar style to existing tests) that calls setup({ username: "eve", /* no userId */, isBillingUsageEnabled: true }), opens the account menu via userEvent.click(screen.getByRole("button", { name: /account menu/i })), and asserts that "Billing & Usage" is NOT in the document, ensuring the component uses user?.userId as the guard.
🤖 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/deploy-web/src/components/deployments/DeploymentDetailTopBar/DeploymentDetailTopBar.spec.tsx`:
- Around line 112-118: The test "renders auto top-up section" currently only
asserts the static label via screen.getByText; update it to also assert the
interactive switch control itself (e.g., use screen.getByRole('switch', { name:
/auto top-up/i }) or screen.getByLabelText(/auto top-up/i)) so the test fails if
the switch is removed or not rendered. Modify the test that calls setup({
deployment: createDeployment({ state: "active" }) }) to include an additional
expectation asserting the switch control is in the document and optionally that
it has the expected checked state.
---
Outside diff comments:
In
`@apps/deploy-web/src/components/deployments/ManifestUpdate/ManifestUpdate.tsx`:
- Around line 128-176: The catch block in handleUpdateClick is treating all
exceptions as parse errors; only YAMLException and CustomValidationError should
setParsingError — operational failures from signAndBroadcastTx or sendManifest
should be propagated to the app-level/snackbar path. Change the catch so that if
error.name is "YAMLException" or "CustomValidationError" you call
setParsingError(error.message), otherwise clear any parsing error/state (or
leave it unchanged), ensure setIsSendingManifest(false) is executed, log the
error if desired, and rethrow the error (or return/propagate it) so
signAndBroadcastTx/sendManifest failures are handled by the global/snackbar
error handler; keep references to handleUpdateClick, signAndBroadcastTx,
sendManifest, setParsingError, and setIsSendingManifest to locate and update the
code.
---
Nitpick comments:
In `@apps/deploy-web/src/components/layout/AccountMenu.spec.tsx`:
- Around line 84-95: The test's setup function nests a mock override via
mock<ReturnType<typeof DEPENDENCIES.useCustomUser>>({ user: ... }) which causes
vitest-mock-extended to create proxy mock functions for unknown nested fields;
instead, return a plain object for the useCustomUser hook result (not wrapped by
mock) inside setup, e.g. replace the mock<ReturnType<typeof
DEPENDENCIES.useCustomUser>>(...) return with an actual object matching the hook
shape (providing user and isLoading) so unknown nested properties remain
undefined; keep useRouter and useFlag mocks as-is and reference setup,
DEPENDENCIES.useCustomUser, useCustomUser, useRouter, and useFlag when making
the change.
- Around line 59-81: Add a regression test to cover the case where
isBillingUsageEnabled is true but the user lacks an internal account
(user.userId is undefined); update AccountMenu.spec.tsx by adding a new it block
(similar style to existing tests) that calls setup({ username: "eve", /* no
userId */, isBillingUsageEnabled: true }), opens the account menu via
userEvent.click(screen.getByRole("button", { name: /account menu/i })), and
asserts that "Billing & Usage" is NOT in the document, ensuring the component
uses user?.userId as the guard.
In `@apps/deploy-web/src/queries/useApiKeysQuery.spec.tsx`:
- Around line 105-109: The test is passing a nested override object into
mock<ReturnType<typeof USE_API_KEYS_DEPENDENCIES.useUser>>() which lets
vitest-mock-extended proxy nested fields into mock functions; instead, create
the base mock first (e.g., const userHook = mock<ReturnType<typeof
USE_API_KEYS_DEPENDENCIES.useUser>>() ), then assign userHook.user = mockUser
and userHook.isLoading = false and return userHook from the useUser override.
Apply the same pattern to the other occurrences referenced (around lines
153-157, 201-205, 231-235) so nested properties are set after creating the mock.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c0838dfd-3215-4dfa-b6cc-5533f6372c7d
📒 Files selected for processing (41)
.eslintrc.jsapps/deploy-web/src/components/deployments/DeploymentDetailTopBar/DeploymentDetailTopBar.spec.tsxapps/deploy-web/src/components/deployments/DeploymentDetailTopBar/DeploymentDetailTopBar.tsxapps/deploy-web/src/components/deployments/DeploymentSubHeader.tsxapps/deploy-web/src/components/deployments/ManifestUpdate/ManifestUpdate.spec.tsxapps/deploy-web/src/components/deployments/ManifestUpdate/ManifestUpdate.tsxapps/deploy-web/src/components/home/YourAccount/YourAccount.spec.tsxapps/deploy-web/src/components/layout/AccountMenu.spec.tsxapps/deploy-web/src/components/layout/AccountMenu.tsxapps/deploy-web/src/components/layout/Sidebar.tsxapps/deploy-web/src/components/layout/WalletStatus.spec.tsxapps/deploy-web/src/components/layout/WalletStatus.tsxapps/deploy-web/src/components/new-deployment/CreateLease/CreateLease.spec.tsxapps/deploy-web/src/components/new-deployment/ManifestEdit/ManifestEdit.spec.tsxapps/deploy-web/src/components/new-deployment/ManifestEdit/ManifestEdit.tsxapps/deploy-web/src/components/new-deployment/SdlBuilder.spec.tsxapps/deploy-web/src/components/new-deployment/SdlBuilder.tsxapps/deploy-web/src/components/onboarding/OnboardingContainer/OnboardingContainer.tsxapps/deploy-web/src/components/sdl/SimpleServiceFormControl.tsxapps/deploy-web/src/components/sdl/TokenFormControl.tsxapps/deploy-web/src/components/shared/PriceEstimateTooltip.tsxapps/deploy-web/src/components/wallet/ConnectManagedWalletButton.spec.tsxapps/deploy-web/src/components/wallet/ConnectManagedWalletButton.tsxapps/deploy-web/src/context/WalletProvider/WalletProvider.tsxapps/deploy-web/src/context/WalletProvider/deriveWalletIsLoading.spec.tsapps/deploy-web/src/context/WalletProvider/deriveWalletIsLoading.tsapps/deploy-web/src/context/WalletProvider/useEnforceSelfCustodyFlag.spec.tsapps/deploy-web/src/context/WalletProvider/useEnforceSelfCustodyFlag.tsapps/deploy-web/src/context/WalletProvider/useSignAndBroadcast.tsxapps/deploy-web/src/hooks/useManagedWallet.tsapps/deploy-web/src/hooks/useProviderJwt/useProviderJwt.spec.tsxapps/deploy-web/src/hooks/useProviderJwt/useProviderJwt.tsapps/deploy-web/src/queries/deploymentSettingsQuery.spec.tsapps/deploy-web/src/queries/deploymentSettingsQuery.tsapps/deploy-web/src/queries/useApiKeysQuery.spec.tsxapps/deploy-web/src/queries/useApiKeysQuery.tsapps/deploy-web/src/store/walletStore.tsapps/deploy-web/src/utils/walletUtils.spec.tsapps/deploy-web/src/utils/walletUtils.tsapps/deploy-web/tests/seeders/localWallet.tsapps/deploy-web/tests/seeders/wallet.ts
💤 Files with no reviewable changes (7)
- apps/deploy-web/src/store/walletStore.ts
- apps/deploy-web/src/components/sdl/TokenFormControl.tsx
- apps/deploy-web/src/context/WalletProvider/useEnforceSelfCustodyFlag.spec.ts
- apps/deploy-web/src/components/wallet/ConnectManagedWalletButton.tsx
- apps/deploy-web/src/context/WalletProvider/useEnforceSelfCustodyFlag.ts
- apps/deploy-web/src/components/wallet/ConnectManagedWalletButton.spec.tsx
- apps/deploy-web/src/components/sdl/SimpleServiceFormControl.tsx
baktun14
force-pushed
the
refactor/deploy-web-remove-dual-mode-wallet-branches
branch
from
c31eb62 to
dc4149b
Compare
…onsumers
Drops the dual-mode wallet primitives that L-1 intentionally left in
place: the `selectedWalletType` atom, the `walletUtils` custodial
helpers, the `LocalWallet` union, `isCustodial`/`switchWalletType` on
the `useWallet()` contract, and every consumer that still branched on
them. Also removes the cosmos-kit reads (`useSelectedChain`,
`useManager`, `CURRENT_WALLET_KEY`) and custodial analytics branch
inside `WalletProvider` itself, plus `useEnforceSelfCustodyFlag`
whose input type died with the atom. Legacy `{networkId}/wallets`
storage entries are deliberately left as inert data — a separate
project sweeps localStorage. Unblocks L-3 (cosmos-kit removal) and
L-4 (self_custody flag removal).
Also sets `root: true` in the monorepo's ESLint config so working in
git worktrees nested under the repo doesn't trip the "plugin loaded
twice" error.
Fixes CON-408
…lf-custody branches Replaces the duplicated transaction-error snackbar wiring in useSignAndBroadcast with the existing useNotificator helper, and swaps the inline Add Funds <Link> for AddFundsLink so the verified-login gate is back in place. Also removes branches that are unreachable now that deploy-web is managed-wallet-only (uakt denom guard, useFlag dependency in ConnectManagedWalletButton) and trims a stale narration comment in WalletProvider.
baktun14
force-pushed
the
refactor/deploy-web-remove-dual-mode-wallet-branches
branch
from
de2e0be to
fd66c0c
Compare
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.
Why
L-1 (#3239) collapsed
signAndBroadcastTxto managed-only and droppedswitchWalletTypefrom the context value, but intentionally kept the wallet-layer artifacts alive —selectedWalletTypeatom,walletUtilscustodial helpers,isCustodial/isManagedonuseWallet(), all cosmos-kit reads insideWalletProvider. This PR picks up that cascade: it deletes the dual-mode wallet primitives, theWalletProvidercustodial branch, and every consumer that still branched on them. After L-2, the codebase no longer carries the dual-mode contract anywhere — which unblocks L-3 (remove cosmos-kit deps) and L-4 (remove theself_custodyfeature flag).Fixes CON-408
Part of Self-custody wallet extraction
PR base is
refactor/deploy-web-wallet-provider-managed-only(L-1, #3239) — GitHub will auto-retarget tomainafter L-1 merges.What
Bucket A —
WalletProvidercollapseuseSelectedChain(),useManager(),custodialWalletManager, and theCURRENT_WALLET_KEYimport/logout side effect(selectedWalletType === "managed" && managedWallet) || userWalletto justmanagedWalletidentify({ managedWallet: true })callisCustodialfromContextTypeand the context value; tightenManagedWalletMarker(now always{ isManaged: true; denom: string })useEnforceSelfCustodyFlaghook + spec (itsSelectedWalletTypeinput dies with the atom — the rest of the flag plumbing stays for L-4)Bucket B —
walletStoreSelectedWalletTypetype and theselectedWalletTypeatomderiveWalletIsLoadingsignature (noselectedWalletType/isCustodialConnecting) and its specgetSelectedStorageWallet()usage inuseManagedWalletBucket C —
walletUtilsCustodialLocalWallet, theLocalWalletunion,getStorageWallets,updateStorageWallets,updateWallet,deleteWalletFromStorage,getSelectedStorageWallet,useSelectedWalletFromStorageupdateStorageManagedWalletto{ userId } & Partial<…>so callers (e.g. JWT writes) can update a single field without re-passing the full shapeensureUserManagedWalletOwnershipto read/write via the managed helperswalletUtils.spec.ts(dual-mode scenarios removed),tests/seeders/localWallet.ts(dropbuildCustodialLocalWallet)Bucket D — consumer collapses
useWallet()contract: dropisCustodial,switchWalletType;isManagedbecomes literaltruedeploymentSettingsQuery,Sidebar,AccountMenu,ManifestEdit(denom branching + deadPrerequisiteListpath),SdlBuilder,SimpleServiceFormControl(drop unusedTokenFormControl),OnboardingContainer,useProviderJwt(switch to managed helpers, look up byuserIdviauseUser),PriceEstimateTooltip,DeploymentSubHeader,WalletStatus,DeploymentDetailTopBar,ManifestUpdate,useApiKeysQueryOut of scope (per plan)
@cosmos-kit/*/@cosmjs/*dep removal → L-3 (CON-259)self_custodyfeature-flag config +useIsSelfCustodyEnabled→ L-4 (CON-338){networkId}/walletsandCURRENT_WALLET_KEYlocalStorage entries → future localStorage sweep/v1/blockchain-status→ L-5 (CON-373)Verification
Both pass.
npx tsc --noEmitandnpm run lint -- --quietproduce no new errors beyond L-1 baseline.npm testpasses all suites except a pre-existingTrendIndicatorsnapshot drift unrelated to this PR.Net: +412 / −2005.
Summary by CodeRabbit
New Features
Changes
Removed