Skip to content

feat(wallet): surface multi-chain balances in Settings#2686

Open
obchain wants to merge 26 commits into
tinyhumansai:mainfrom
obchain:feat/wallet-balances-panel
Open

feat(wallet): surface multi-chain balances in Settings#2686
obchain wants to merge 26 commits into
tinyhumansai:mainfrom
obchain:feat/wallet-balances-panel

Conversation

@obchain
Copy link
Copy Markdown
Contributor

@obchain obchain commented May 26, 2026

Summary

  • Surfaces the existing wallet.balances RPC in the UI by adding a new Settings → Account → Wallet Balances panel.
  • Multi-chain rows (EVM, BTC, Solana, Tron) with chain badges, truncated addresses, copy-to-clipboard, formatted balances, provider-status chips, and a Refresh button.
  • No behaviour change in the core — backend was already complete; this fills the UI gap.

Problem

The wallet.balances RPC has been live in the Rust core for some time and is listed as a capability in about_app/catalog.rs, but the frontend never had a caller. The only way to read multi-chain balances today is to issue a raw JSON-RPC request — inaccessible to normal users, which breaks the obvious "I set up a wallet → what is in it?" mental model and blocks the broader crypto / portfolio workflows tracked in #1394.

Solution

  1. app/src/services/walletApi.ts — adds BalanceInfo type + fetchWalletBalances() calling wallet.balances via the existing coreRpcClient, mirroring the fetchWalletStatus pattern. New tests added in the same file.
  2. app/src/components/settings/panels/WalletBalancesPanel.tsx — new self-contained panel that renders four states (loading, error with Retry, empty with Recovery Phrase hint, loaded). Loaded state renders one BalanceRow per balance with chain badge (color-coded per Tailwind palette), truncated address (first 6 + last 4 chars), copy-to-clipboard button, formatted amount + symbol, optional "provider unavailable" chip, and a Refresh button.
  3. app/src/pages/Settings.tsx — registers the new entry under Account section, routed at /settings/wallet-balances.
  4. i18n — 13 new keys in app/src/lib/i18n/en.ts plus matching English-value entries in every locale chunk (chunks/{ar,bn,de,en,es,fr,hi,id,it,ko,pt,ru,zh-CN}-4.ts) so pnpm i18n:check parity passes and translators can fill non-English values later.
  5. Capability catalog (src/openhuman/about_app/catalog.rs) — single-line update to the skills.wallet_execution how_to field pointing users at the new panel.
  6. Coverage matrix — new row in docs/TEST-COVERAGE-MATRIX.md tracking the panel.

Submission Checklist

  • Tests added or updated (happy path + at least one failure / edge case) per Testing Strategy — 9 Vitest panel tests across loading / error+retry / empty / loaded / refresh states, plus 5 walletApi tests for the new fetchWalletBalances function (14/14 passing locally).
  • Diff coverage ≥ 80% — changed lines are largely the new panel + new RPC client function, both fully covered by the new test files.
  • Coverage matrix updated — added row 13.1.4 ("Wallet Balances Panel") under the wallet feature group; covered count bumped 69→70.
  • All affected feature IDs from the matrix are listed in the PR description under ## Relatedskills.wallet_execution (UI surfacing of an existing capability — no new feature ID needed).
  • No new external network dependencies introduced (mock backend used per Testing Strategy) — the new tests mock fetchWalletBalances directly; no network calls.
  • Manual smoke checklist updated if this touches release-cut surfaces (docs/RELEASE-MANUAL-SMOKE.md) — N/A: additive UI surface, no release-cut behaviour change.
  • Linked issue closed via Closes #NNN in the ## Related section — Closes #2682 below.

Impact

  • Runtime / platform: pure UI addition — desktop only, no behaviour change on existing surfaces.
  • Performance: negligible — one RPC call on panel mount + on user-triggered Refresh.
  • Security / privacy: unchanged — wallet.balances only uses the public address; the recovery phrase never leaves the local core.
  • Migration / compatibility: none.

Manual visual verification

Verified locally via pnpm dev:app:

  • Setting up a fresh wallet via Settings → Recovery Phrase and then opening Settings → Wallet Balances renders all four chain rows (EVM, BTC, SOL, TRX) with the expected zero-balance loaded state.
  • Sample output observed:
    EVM   0x9B99…2660      0.000000000000000000  ETH
    BTC   1BnHT1…77G3      0.00000000            BTC
    SOL   51xrwi…aWRF      0.000000000           SOL
    TRX   TAWYnc…q4q9      0.000000              TRX
    
  • Addresses are derived correctly per chain, decimals match the chain conventions (18/8/9/6), and chain badges colour-match the Tailwind palette tokens.

Related


AI Authored PR Metadata (required for Codex/Linear PRs)

Keep this section for AI-authored PRs. For human-only PRs, mark each field N/A.

Linear Issue

  • Key: N/A
  • URL: N/A

Commit & Branch

  • Branch: N/A
  • Commit SHA: N/A

Validation Run

  • pnpm --filter openhuman-app format:check — passes (Prettier auto-fixes applied during quality-gate commit).
  • pnpm typecheck — clean.
  • Focused tests: pnpm test -- WalletBalancesPanel walletApi — 14/14 passed.
  • Rust fmt/check (if changed): cargo check --manifest-path Cargo.toml clean for the one-line catalog.rs touch.
  • Tauri fmt/check (if changed): pnpm rust:check clean.

Validation Blocked

  • command: N/A
  • error: N/A
  • impact: N/A

Behavior Changes

  • Intended behavior change: Users can now view multi-chain wallet balances inside the app instead of having to issue raw RPC calls.
  • User-visible effect: New Settings → Account → Wallet Balances panel listing per-chain balance rows.

Parity Contract

  • Legacy behavior preserved: No existing surface touched. Backend RPC unchanged.
  • Guard/fallback/dispatch parity checks: N/A — additive UI only.

Summary by CodeRabbit

  • New Features
    • Wallet Balances panel in Settings > Account: shows wallets with chain badges, formatted balances, asset symbols, provider-status chips, refresh and copy-to-clipboard with feedback.
  • Tests
    • End-to-end behavioral tests covering loading, error+retry, empty, loaded, and refresh flows.
  • Documentation
    • Added wallet-balances UI translations across multiple languages and updated test coverage matrix.

Review Change Stack

vaddisrinivas and others added 10 commits May 23, 2026 11:44
Extend walletApi.ts with BalanceInfo type and fetchWalletBalances() calling
openhuman.wallet_balances. Add 3 Vitest tests (happy path, error propagation,
empty array). Add walletBalances.* and pages.settings.account.walletBalances*
keys to en.ts, en-4.ts, and all 12 non-English chunk-4 locale files. Update
wallet.execution how_to in catalog.rs to reference Settings > Wallet Balances.
New panel under Settings > Account with loading/error/empty/loaded states.
Chain badges (EVM/BTC/SOL/TRX) with color-coded design tokens; truncated
address (first 6 + last 4) with copy-to-clipboard; formatted balance + symbol;
providerStatus chip for missing providers; Refresh button. Vitest+RTL tests
cover all five UI state describe-blocks including Retry re-invocation.
Add wallet-balances route and accountSettingsItems entry in Settings.tsx so
the panel is reachable at Settings > Account > Wallet Balances. Update
TEST-COVERAGE-MATRIX.md row 13.1.4 and bump covered/total counts.
Apply Prettier auto-format, collapse duplicate-named imports into a
single type+value import, switch the panel test to a static import +
JSX render (no dynamic import per the app/src ban), and correct the
truncated-address expectation to match the implementation's first-6 +
last-4 character form.

Vitest 14/14 pass; pnpm compile, lint, rust:check all green.
@obchain obchain requested a review from a team May 26, 2026 10:36
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 26, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds a Settings > Account > Wallet Balances panel that fetches multi-chain wallet balances via a new fetchWalletBalances() API, displays rows with chain badges, truncated copyable addresses, formatted balances and provider-status badges, includes request-sequencing guards, loading/error/empty states, refresh control, tests, i18n across language chunks, settings routing integration, and a separate Rust admission pipeline for generated tools with provenance/risk fields and admission logic.

Changes

Wallet Balances Feature

Layer / File(s) Summary
API Contract & Data Shape
app/src/services/walletApi.ts, app/src/services/walletApi.test.ts
Exports BalanceInfo and fetchWalletBalances() calling openhuman.wallet_balances; tests verify RPC invocation, error propagation, and empty results.
Panel Component Implementation
app/src/components/settings/panels/WalletBalancesPanel.tsx
Implements WalletBalancesPanel and BalanceRow: monotonic request-id guards to avoid stale updates, loading/error/empty/loaded UI branches, refresh button (disabled while loading) with spinner, truncated address + copy-to-clipboard with transient “copied” feedback, formatted balances and asset symbols, and provider-unavailable badge.
Panel Behavioral Tests
app/src/components/settings/panels/__tests__/WalletBalancesPanel.test.tsx
Vitest suite (mocked fetchWalletBalances and navigation hook) covering loading spinner, generic error with Retry flow, empty-state with Recovery Phrase hint, loaded rows (chain badges, balances, symbols, truncated addresses), provider-unavailable chip, and refresh-triggered reloads.
Settings Navigation & Routing
app/src/pages/Settings.tsx
Imports WalletBalancesPanel, defines WalletIcon, adds wallet-balances to accountSettingsItems with localized strings, and registers /settings/wallet-balances route rendering the panel via wrapSettingsPage.
Navigation Hook Update
app/src/components/settings/hooks/useSettingsNavigation.ts
Adds 'wallet-balances' to SettingsRoute, maps /settings/wallet-balances to that route, and reuses recovery-phrase breadcrumbs.
Multi-Language i18n Translations
app/src/lib/i18n/en.ts, app/src/lib/i18n/chunks/*-4.ts
Adds pages.settings.account.walletBalances and walletBalances.* keys (title/description, refresh/loading/retry, empty-state guidance, copy-address, provider-missing, raw-balance template, generic error) across the main English map and language chunk files.
Documentation & Capability Catalog
docs/TEST-COVERAGE-MATRIX.md, src/openhuman/about_app/catalog.rs
Adds Wallet Balances Panel (13.1.4) as covered in the test matrix and updates skills.wallet_execution how_to text to mention Settings > Wallet Balances.

Generated Tools Admission (Rust)

Layer / File(s) Summary
Structs, Enums & Public Types
src/openhuman/tools/generated.rs
Extends GeneratedToolDefinition with optional provenance/policy fields, adds GeneratedToolRisk enum, and introduces admission-related public types (GeneratedToolAdmissionConfig, GeneratedToolAdmissionRejection, GeneratedToolAdmissionReport).
Admission Entrypoint & Validation
src/openhuman/tools/generated.rs
Implements admit_generated_tool_definitions with normalization and validation helpers enforcing provenance/risk/provider/capability checks and returning admitted/rejected reports.
Admission Tests & Helpers
src/openhuman/tools/generated.rs (tests)
Expands tests to cover trusted acceptance, provider normalization, invalid-provider rejection, legacy preservation when enforcement is off, duplicate/unsafe-name rejection, missing-risk rejection, and verifies external_effect() semantics.

Sequence Diagram

sequenceDiagram
  participant User
  participant WalletBalancesPanel
  participant fetchWalletBalances
  participant CoreRPC as openhuman.wallet_balances
  User->>WalletBalancesPanel: Click Refresh
  WalletBalancesPanel->>WalletBalancesPanel: increment latestRequestId
  WalletBalancesPanel->>fetchWalletBalances: call loadBalances()
  fetchWalletBalances->>CoreRPC: call openhuman.wallet_balances
  CoreRPC-->>fetchWalletBalances: return BalanceInfo[]
  fetchWalletBalances-->>WalletBalancesPanel: Promise resolves
  WalletBalancesPanel->>WalletBalancesPanel: setState(balances)
  WalletBalancesPanel-->>User: render BalanceRow list
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • tinyhumansai/openhuman#2519: Changes to backend wallet_balances output (provider status, optional evm_network) that this UI and fetchWalletBalances consume.
  • tinyhumansai/openhuman#1858: Also modifies useSettingsNavigation.ts/Settings route handling; similar integration pattern for adding settings panels.

Suggested reviewers

  • graycyrus
  • M3gA-Mind
  • sanil-23

Poem

🐰 Hoppy code hops through the settings door,
Wallet balances shimmer, chained and pure.
Click to copy, refresh to see,
Multi-chain friends, all safe and free. ✨

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning Most changes are in-scope, but the Rust changes to src/openhuman/tools/generated.rs introducing admission pipeline for generated tools appear unrelated to wallet balances display. Clarify the relationship between generated tool admission and wallet balances, or move those Rust changes to a separate PR focused on tool security/provenance.
Docstring Coverage ⚠️ Warning Docstring coverage is 57.58% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title clearly and concisely describes the main feature: adding a wallet balances panel to Settings UI for viewing multi-chain balances.
Linked Issues check ✅ Passed All primary objectives from #2682 are fully implemented: Settings entry, multi-chain rows with badges/addresses/copy, provider health visibility, refresh functionality, all UI states, privacy preservation, i18n coverage, and unit tests.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot added feature Net-new user-facing capability or product behavior. working A PR that is being worked on by the team. labels May 26, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 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 `@app/src/components/settings/panels/WalletBalancesPanel.tsx`:
- Around line 139-151: loadBalances currently allows out-of-order async
responses to overwrite newer state; add a sequencing guard (e.g., a local
incrementing requestId stored in a ref or closure) so each call to loadBalances
increments the id and only commits results (setBalances/setError/setLoading) if
the response id matches the latest id; implement the same pattern for the other
async calls noted (the other balance/fetch handlers around the later
occurrences) to ignore stale responses and ensure only the freshest fetch
updates state.
- Line 199: The UI currently renders the raw backend error variable `error`
directly in WalletBalancesPanel, which bypasses localization and may expose
backend phrasing; replace this with a localized user-facing string via the
`useT()` hook (e.g., `t('wallet.balance_error')` or an appropriate key), and
stop rendering `{error}` verbatim in the paragraph; if you need to surface
details for diagnostics, add a sanitized/non-user-facing place such as a
console.warn, a data-attribute (e.g., `data-error-detail`) or an internal log
entry rather than visible text; ensure `useT` is imported and used in the
`WalletBalancesPanel` component and keep the displayed message generic and
localized while preserving sanitized error detail only for diagnostics.

In `@app/src/pages/Settings.tsx`:
- Around line 239-245: The route id 'wallet-balances' is registered in the
settings menu but missing from the SettingsRoute union and getCurrentRoute()
mapping in useSettingsNavigation, causing the page to fall back to 'home';
update the SettingsRoute type (in useSettingsNavigation.ts) to include
'wallet-balances' and add a corresponding case/branch in getCurrentRoute() that
returns 'wallet-balances' when the path or route id matches, ensuring
breadcrumbs/back behavior resolves to the new route; confirm any related
switch/lookup tables (e.g., routeToTitle or routeToPath helpers) also include
'wallet-balances' so navigation and labels render correctly.

In `@app/src/services/walletApi.ts`:
- Around line 66-71: The doc for fetchWalletBalances() is contradictory about
handling an unconfigured wallet—update the comment to match the actual
implementation: either (A) state that fetchWalletBalances calls wallet.balances
via the core RPC relay and will surface the core RPC error when the wallet is
not configured (do not claim it returns an empty array), or (B) change the
implementation to catch the specific core error and return an empty array, and
then document that behavior; refer to fetchWalletBalances and the
wallet.balances/core RPC relay in the comment so callers know whether they
should expect an error or an empty array.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a0ec19da-97e2-4299-9033-8768354a8413

📥 Commits

Reviewing files that changed from the base of the PR and between 844a26c and 98d2a86.

📒 Files selected for processing (21)
  • app/src/components/settings/panels/WalletBalancesPanel.tsx
  • app/src/components/settings/panels/__tests__/WalletBalancesPanel.test.tsx
  • app/src/lib/i18n/chunks/ar-4.ts
  • app/src/lib/i18n/chunks/bn-4.ts
  • app/src/lib/i18n/chunks/de-4.ts
  • app/src/lib/i18n/chunks/en-4.ts
  • app/src/lib/i18n/chunks/es-4.ts
  • app/src/lib/i18n/chunks/fr-4.ts
  • app/src/lib/i18n/chunks/hi-4.ts
  • app/src/lib/i18n/chunks/id-4.ts
  • app/src/lib/i18n/chunks/it-4.ts
  • app/src/lib/i18n/chunks/ko-4.ts
  • app/src/lib/i18n/chunks/pt-4.ts
  • app/src/lib/i18n/chunks/ru-4.ts
  • app/src/lib/i18n/chunks/zh-CN-4.ts
  • app/src/lib/i18n/en.ts
  • app/src/pages/Settings.tsx
  • app/src/services/walletApi.test.ts
  • app/src/services/walletApi.ts
  • docs/TEST-COVERAGE-MATRIX.md
  • src/openhuman/about_app/catalog.rs

Comment thread app/src/components/settings/panels/WalletBalancesPanel.tsx
Comment thread app/src/components/settings/panels/WalletBalancesPanel.tsx Outdated
Comment thread app/src/pages/Settings.tsx
Comment thread app/src/services/walletApi.ts
- Guard `loadBalances` with a monotonic request id so a slow earlier
  fetch can no longer overwrite a newer Refresh/Retry result.
- Render a translated, user-facing error string (`walletBalances.errorGeneric`)
  instead of leaking raw backend phrasing to the UI; raw error stays in
  `console.debug` for diagnostics.
- Wire `wallet-balances` into `useSettingsNavigation` (route union, path
  matcher, breadcrumb mapping) so breadcrumbs + back navigation behave.
- Clarify the `fetchWalletBalances` contract — empty array vs reject
  when the wallet is not configured.
coderabbitai[bot]
coderabbitai Bot previously approved these changes May 26, 2026
graycyrus
graycyrus previously approved these changes May 26, 2026
Copy link
Copy Markdown
Contributor

@graycyrus graycyrus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work here. Clean additive feature that fills a real UI gap — the wallet.balances RPC was already live but had no frontend caller, and this fixes that.

What I checked

Area Files Verdict
Frontend panel WalletBalancesPanel.tsx Request-sequencing guard handles concurrent fetches correctly. Four UI states (loading, error, empty, loaded) all covered. Translated error message instead of raw backend text.
Service layer walletApi.ts fetchWalletBalances follows the existing callCoreRpc pattern. BalanceInfo type matches the Rust serde output. JSDoc is accurate.
Navigation useSettingsNavigation.ts, Settings.tsx Route, breadcrumbs, and SettingsRoute union all wired up.
i18n en.ts + 13 locale chunks All 13 keys present in every chunk with English fallback values. Parity check passes.
Rust core catalog.rs Single-line docs pointer. No logic change.
Tests Panel tests (9) + API tests (5) Comprehensive coverage across all states, retry/refresh flows, address truncation, and provider chip rendering.
Coverage matrix TEST-COVERAGE-MATRIX.md Row 13.1.4 added, count bumped 69→70.

CodeRabbit findings — all addressed

All four CodeRabbit findings (race condition guard, raw error text, missing navigation route, contradictory JSDoc) were fixed in 9096c6b. Nothing left to flag there.

Minor observations (non-blocking)

  • walletBalances.addressCopied i18n key is defined but unused — the component shows a checkmark SVG instead of text for the copied state. Harmless, but dead keys accumulate.
  • Copy-address timer doesn't clear on rapid re-clicks (multiple setTimeouts stack), so "Copied" could disappear based on the first click's timer rather than the latest. Cosmetic edge case.

Neither warrants blocking. Ship it.

vaddisrinivas and others added 4 commits May 26, 2026 19:21
…ed-tool-admission

# Conflicts:
#	app/test/e2e/specs/mega-flow.spec.ts
Previously each click in BalanceRow spawned an independent setTimeout via
the local 'timer' variable returned from an async handler — but onClick
discards that cleanup, so the older 2s timer would still fire and flip
'copied' back to false even after the latest click should still be showing
the checkmark.

Track the active timer in a useRef, clear it before scheduling the next
one, and tear down on unmount.
BalanceRow renders a checkmark SVG (not text) for the copied state, so
the key has been dead since the panel was wired up. Remove from en.ts
and all 13 locale chunk-4 files.
@obchain obchain dismissed stale reviews from graycyrus and coderabbitai[bot] via 1d7eb5e May 27, 2026 14:26
@obchain
Copy link
Copy Markdown
Contributor Author

obchain commented May 27, 2026

@graycyrus addressed both non-blocking nits in d524a5de + 1d7eb5eb:- Copy-address timer stacking: switched to a useRef-tracked timer that clears on each re-click and on unmount, so rapid clicks reset the 2s window cleanly instead of letting an older setTimeout flip copied back to false.- Dead walletBalances.addressCopied key: removed from en.ts and all 13 locale chunk-4 files. pnpm i18n:check is clean (0 missing/extra/drift). Existing Vitest suite (3551 tests) still green.

coderabbitai[bot]
coderabbitai Bot previously approved these changes May 27, 2026
@obchain obchain requested a review from graycyrus May 27, 2026 16:29
@oxoxDev oxoxDev assigned oxoxDev and unassigned oxoxDev May 28, 2026
sanil-23 added 3 commits May 28, 2026 17:00
`unicode-normalization` is a direct dep of the root `openhuman` crate
(added on main in tinyhumansai#2756 for the cross-thread memory search index) but
was not reflected in `app/src-tauri/Cargo.lock`. `cargo check` against
the Tauri shell re-locks it on first build. Commit the synced lockfile
so CI doesn't dirty the workspace.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
src/openhuman/tools/generated.rs (2)

650-664: 💤 Low value

Consider adding tests for untested admission rejection paths.

The test suite covers the main admission flows well, but a few rejection paths lack coverage:

  1. Dangerous risk → external_effect() (only ExternalWrite and Execute are tested)
  2. disabled_providers rejection (only untrusted_provider is tested)
  3. disabled_capabilities rejection
  4. Missing source_digest when provenance is enforced (only missing risk is tested)

These are low-priority since the logic is straightforward, but adding them would improve confidence in the validation paths.

Example test for disabled_capabilities
#[test]
fn admission_rejects_disabled_capability() {
    let definition = sample_definition();
    let config = GeneratedToolAdmissionConfig {
        enforce_provenance: true,
        trusted_providers: BTreeSet::from(["trusted.runtime".to_string()]),
        disabled_capabilities: BTreeSet::from(["updates.send".to_string()]),
        ..Default::default()
    };

    let report = admit_generated_tool_definitions(vec![definition], &config);

    assert!(report.admitted.is_empty());
    assert!(report.rejected[0].reason.contains("capability") && report.rejected[0].reason.contains("disabled"));
}
🤖 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 `@src/openhuman/tools/generated.rs` around lines 650 - 664, Add unit tests
covering the missing rejection/edge paths: 1) verify
GeneratedTool::external_effect() returns true for GeneratedToolRisk::Dangerous
by creating a definition with risk = Some(GeneratedToolRisk::Dangerous) and
asserting external_effect(); 2) add an admission test that sets
GeneratedToolAdmissionConfig.disabled_providers to include the provider from
sample_definition() and assert admit_generated_tool_definitions rejects it with
a reason mentioning provider/untrusted; 3) add an admission test that sets
GeneratedToolAdmissionConfig.disabled_capabilities (e.g., "updates.send") and
asserts admit_generated_tool_definitions rejects with a reason containing
"capability" and "disabled"; and 4) add an admission test for provenance
enforcement by enabling enforce_provenance and providing a definition with
missing source_digest (but present risk) and assert rejection mentions
provenance/source_digest; reuse sample_definition(),
GeneratedToolAdmissionConfig, and admit_generated_tool_definitions to locate the
code.

307-318: 💤 Low value

Document (or adjust) “reserve name on first encounter” semantics for duplicates

In src/openhuman/tools/generated.rs, validate_admission inserts definition.name into seen immediately after the safe-name check and before provenance enforcement (config.enforce_provenance / provider trust checks). If a definition fails provenance, its name is still reserved, so a later definition with the same name—otherwise valid—will be rejected as a duplicate.

if !seen.insert(definition.name.clone()) {
    return Err(format!("duplicate generated tool `{}`", definition.name));
}
if !config.enforce_provenance {
    return Ok(());
}

Add a brief comment clarifying that names become reserved on first encounter (even if provenance fails), or move the seen.insert(...) until after all provenance validations succeed.

🤖 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 `@src/openhuman/tools/generated.rs` around lines 307 - 318, The current
validate_admission flow inserts a definition name into the seen set
(seen.insert(definition.name.clone())) before provenance/trust checks
(config.enforce_provenance), which causes names to be reserved even when
provenance validation fails; either move the seen.insert(...) so it runs after
all provenance and provider-trust checks succeed, or add a clear comment at the
insertion site stating that names are intentionally reserved on first encounter
(even on provenance failure) to document the semantics; update references around
validate_admission, seen, definition.name, and config.enforce_provenance
accordingly.
🤖 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.

Nitpick comments:
In `@src/openhuman/tools/generated.rs`:
- Around line 650-664: Add unit tests covering the missing rejection/edge paths:
1) verify GeneratedTool::external_effect() returns true for
GeneratedToolRisk::Dangerous by creating a definition with risk =
Some(GeneratedToolRisk::Dangerous) and asserting external_effect(); 2) add an
admission test that sets GeneratedToolAdmissionConfig.disabled_providers to
include the provider from sample_definition() and assert
admit_generated_tool_definitions rejects it with a reason mentioning
provider/untrusted; 3) add an admission test that sets
GeneratedToolAdmissionConfig.disabled_capabilities (e.g., "updates.send") and
asserts admit_generated_tool_definitions rejects with a reason containing
"capability" and "disabled"; and 4) add an admission test for provenance
enforcement by enabling enforce_provenance and providing a definition with
missing source_digest (but present risk) and assert rejection mentions
provenance/source_digest; reuse sample_definition(),
GeneratedToolAdmissionConfig, and admit_generated_tool_definitions to locate the
code.
- Around line 307-318: The current validate_admission flow inserts a definition
name into the seen set (seen.insert(definition.name.clone())) before
provenance/trust checks (config.enforce_provenance), which causes names to be
reserved even when provenance validation fails; either move the seen.insert(...)
so it runs after all provenance and provider-trust checks succeed, or add a
clear comment at the insertion site stating that names are intentionally
reserved on first encounter (even on provenance failure) to document the
semantics; update references around validate_admission, seen, definition.name,
and config.enforce_provenance accordingly.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6835308f-554d-4fe7-88d4-12871dd637d9

📥 Commits

Reviewing files that changed from the base of the PR and between b2cc75f and fc457fc.

⛔ Files ignored due to path filters (1)
  • app/src-tauri/Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (3)
  • app/src/lib/i18n/en.ts
  • docs/TEST-COVERAGE-MATRIX.md
  • src/openhuman/tools/generated.rs
✅ Files skipped from review due to trivial changes (1)
  • docs/TEST-COVERAGE-MATRIX.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • app/src/lib/i18n/en.ts

coderabbitai[bot]
coderabbitai Bot previously approved these changes May 28, 2026
sanil-23
sanil-23 previously approved these changes May 28, 2026
Copy link
Copy Markdown
Contributor

@sanil-23 sanil-23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rfix4 round shepherd: CI green/converging, prior reviewer + CodeRabbit findings addressed in the worktree. LGTM.

@sanil-23 sanil-23 requested a review from oxoxDev May 28, 2026 17:41
@sanil-23
Copy link
Copy Markdown
Contributor

Hi @graycyrus @oxoxDev — round-4 rfix4 shepherding complete: CR comments addressed, CI green/converging, I've approved. Would appreciate a second-maintainer review when you have a moment 🙏

Copy link
Copy Markdown
Contributor

@graycyrus graycyrus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Continuation review — wallet panel is clean, all prior CodeRabbit findings and my previous non-blocking observations are addressed (timer stacking fixed, dead i18n key dropped). Two things still need attention.

Findings

[major] Undocumented bundled scope in generated.rs

This PR is described as "surface multi-chain balances in Settings" but commits 6226ed4, 21a7bf3, and 46631365 add ~150 lines of generated-tool admission control infrastructure that has nothing to do with wallet balances: GeneratedToolRisk, GeneratedToolAdmissionConfig, GeneratedToolAdmissionReport, GeneratedToolAdmissionRejection, admit_generated_tool_definitions(), validate_admission(), and associated helpers. None of this appears in the PR title, summary, checklist, or Related section. The commit messages reference a codex/oh-2542 branch that appears to have been merged in inadvertently.

This matters for a few reasons: it makes the history harder to bisect (a wallet UI regression search will land in admission control code), and the admission control logic has gaps in test coverage that CodeRabbit flagged (missing coverage for disabled_providers rejection, disabled_capabilities rejection, Dangerous risk via external_effect(), and source_digest enforcement). If this was intentional co-shipping, describe it in the PR body and address the coverage gaps. If it was an accidental merge-in from another branch, clean up the history.

[minor] Name reservation before provenance in validate_admission

In validate_admission, seen.insert(definition.name.clone()) runs before provider trust and capability checks. If an untrusted provider submits a tool with the same name as a trusted provider's tool in the same admission batch, the trusted definition is rejected as a duplicate. This is a name-squatting surface for the case where definitions from multiple providers are batched together. If "deny on first encounter regardless of provenance outcome" is intentional, add a comment saying so. If not, move the insert after provenance passes.

Wallet panel

Clean. Request-sequencing guard, translated error state, navigation wiring, and test coverage are all solid. No issues there.

graycyrus identified that commits 6226ed4, 21a7bf3, 4663136 (~393 lines in
src/openhuman/tools/generated.rs) came from vaddisrinivas's pr/2549
(generated-tool admission, issue tinyhumansai#2542) and got cross-pollinated into this
wallet-balances PR via an upstream/main merge that pulled in the codex/oh-2542
branch tip.

Reverting restores the PR scope to wallet balances only. Once tinyhumansai#2549 merges to
upstream/main, those changes will arrive here naturally via the next main
merge.

Refs: graycyrus's review comment on tinyhumansai#2686 (CHANGES_REQUESTED 2026-05-28T18:27Z)
@sanil-23 sanil-23 dismissed stale reviews from coderabbitai[bot] and themself via 50dee06 May 28, 2026 18:56
@sanil-23
Copy link
Copy Markdown
Contributor

Hi @graycyrus — addressed your major finding directly: pushed 50dee06c which reverts the 3 bundled commits from pr/2549 (6226ed4, 21a7bf3, 46631365 — the 393 lines of admission-control code in src/openhuman/tools/generated.rs). Local cargo check clean; no other code referenced the reverted types. Once #2549 merges to main, the admission-control code will arrive here naturally via the next main merge. Re-requesting review.

@sanil-23 sanil-23 requested a review from graycyrus May 28, 2026 18:56
Copy link
Copy Markdown
Contributor

@graycyrus graycyrus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! The revert removed the out-of-scope generated-tool admission control code, and the wallet balances panel is back to its proper scope. All the code looks solid — timer fix is there, nav wiring is correct, tests are comprehensive.

However, there are still several CI checks in progress (Frontend Coverage, Rust Core Coverage, e2e tests, etc). The code looks clean to me, but I want to see those finish green before approving. Let me come back once CI is fully done.

sanil-23 and others added 2 commits May 28, 2026 21:57
…ssionExpired

The TAURI-RUST-4K5 wire-shape (`Embedding API error (401 Unauthorized):
{"success":false,"error":"Invalid token"}`) is the OpenHuman backend's
session-expired envelope reaching the embeddings worker, not a backend
4xx bug. PR tinyhumansai#2786 added a SessionExpired classification + test for this
shape but left the obsolete TAURI-RUST-T BackendUserError test in place,
producing a contradiction: two tests assert different classifications for
identical input. On main, `is_embedding_backend_auth_failure` claims the
envelope first (line 392) and short-circuits before SessionExpired (line
335) can match, failing the newer test.

Three coordinated edits:

1. `is_embedding_backend_auth_failure` now skips the OpenHuman-backend
   envelope `"\"error\":\"invalid token\""` so BYO-key 401s (no envelope)
   still classify as BackendUserError but the 4K5 envelope falls through
   to SessionExpired. The function's narrative (third-party BYO-key
   rejection) is preserved.

2. `is_session_expired_message` now matches both the parenthesized
   (`Embedding API error (401`) and bare-status (`Embedding API error
   401`) wire shapes. Both are observed in production per the older
   TAURI-RUST-T test.

3. The obsolete `classifies_embedding_backend_auth_failure` test now
   asserts SessionExpired for both wire shapes — kept as a regression
   guard against `is_embedding_backend_auth_failure` re-claiming the
   envelope.

121 observability tests green.
@sanil-23
Copy link
Copy Markdown
Contributor

@graycyrus @oxoxDev — addressed the 2 CI failures in 711e01fa: the classifies_embedding_api_invalid_token_401_as_session_expired test was failing on main itself (a regression from #2786 leaving the obsolete TAURI-RUST-T BackendUserError test contradicting the new SessionExpired classification). Fix scoped to src/core/observability.rs: guard is_embedding_backend_auth_failure so the OpenHuman-backend {"error":"Invalid token"} envelope falls through to SessionExpired; extend is_session_expired_message to match both parenthesized and bare-status wire forms; update the obsolete TAURI-RUST-T test to assert SessionExpired (now a regression guard). 121 observability tests green locally. Re-requesting review.

@sanil-23 sanil-23 requested a review from graycyrus May 28, 2026 23:40
sanil-23 and others added 2 commits May 29, 2026 01:47
# Conflicts:
#	src/core/observability.rs
…s-panel

# Conflicts:
#	app/src/lib/i18n/en.ts
@obchain
Copy link
Copy Markdown
Contributor Author

obchain commented May 29, 2026

Resolved the merge conflict with main in 97b07007 — merged latest upstream db3fdc2e. The only conflict was in en.ts (our walletBalances.* keys vs upstream's new skills.dashboard.* keys); kept both. Verified locally: pnpm i18n:check parity clean (0 missing/extra/drifted across all locales), pnpm typecheck clean, cargo check clean. Branch is mergeable again.

oxoxDev
oxoxDev previously approved these changes May 29, 2026
Copy link
Copy Markdown
Contributor

@oxoxDev oxoxDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Walkthrough

2nd-maintainer pass after sanil-23's rfix4 shepherding + graycyrus's CHANGES_REQUESTED-then-verbal-nod cycle. Wallet panel is clean; the obs.rs hunk that initially looked like scope creep is actually 711e01fa's regression fix for #2786 (is_embedding_backend_auth_failure had started claiming the 4K5 wire shape after #2786 merged — explicit short-circuit guard + bare-status Embedding API error 401 arm restore the contract). Pulled the full activity log: admission-control bundle from codex/oh-2542 already reverted in 50dee06c, final merge conflict on en.ts (walletBalances vs upstream skills.dashboard) resolved cleanly in 97b07007. All CI green.

Verified

  • Wallet panel — request-sequencing guard, useT-routed error, navigation/breadcrumb wiring, address truncation, copy-timer cleanup all match graycyrus's earlier audit + sanil-23's rfix4 sign-off ✓
  • observability.rs is_embedding_backend_auth_failure short-circuit on "error":"invalid token" correctly cedes the shape to is_session_expired_message; bare-status Embedding API error 401 arm extends the #2786 conjunctive matcher to a real production wire shape; regression-tested by the new polarity guard ✓
  • Catalog one-line how_to pointer is a docs nudge, no logic change ✓
  • 11 wallet keys mirror across en.ts + 12 locale chunks (ar/bn/de/es/fr/hi/id/it/ko/pt/ru/zh-CN) ✓
  • Merge conflict resolution in en.ts (97b07007) kept both sides: walletBalances.* + upstream skills.dashboard.* ✓

Nits

  • pl-4.ts (Polish, added 2026-05-27 via #2731) is not touched — 11 wallet keys missing in the Polish chunk. pnpm i18n:check is tolerating it, so this is not a CI blocker, but Polish users see English fallback strings for the new Settings → Account → Wallet Balances panel. Quick follow-up: copy the 11 keys with English values to pl-4.ts (same English-placeholder pattern used by the other 12 chunks), or land via the next i18n sweep. Don't block on it.
  • PR body claims "13 new keys in en.ts" but actually 11 — minor count drift, cosmetic.

Questions

  • @graycyrus stale CHANGES_REQUESTED predates 50dee06c (admission-control revert) + 711e01fa (4K5 regression fix) + 97b07007 (final main merge). Re-requested for a fresh maintainer pass — would appreciate your re-review.
  • Author/shepherd: any reason pl-4.ts was deferred specifically, or just missed because the branch base predates #2731? Heads-up so the next i18n sweep PR scoops these.

senamakel
senamakel previously approved these changes May 29, 2026
@senamakel senamakel self-assigned this May 29, 2026
@senamakel
Copy link
Copy Markdown
Member

really great PR. merging shortly.

# Conflicts:
#	app/src/lib/i18n/chunks/ar-4.ts
#	app/src/lib/i18n/chunks/bn-4.ts
#	app/src/lib/i18n/chunks/de-4.ts
#	app/src/lib/i18n/chunks/es-4.ts
#	app/src/lib/i18n/chunks/fr-4.ts
#	app/src/lib/i18n/chunks/hi-4.ts
#	app/src/lib/i18n/chunks/id-4.ts
#	app/src/lib/i18n/chunks/it-4.ts
#	app/src/lib/i18n/chunks/ko-4.ts
#	app/src/lib/i18n/chunks/pt-4.ts
#	app/src/lib/i18n/chunks/ru-4.ts
#	app/src/lib/i18n/chunks/zh-CN-4.ts
#	app/src/lib/i18n/en.ts
@senamakel senamakel dismissed stale reviews from oxoxDev and themself via 3072f48 May 29, 2026 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature Net-new user-facing capability or product behavior. working A PR that is being worked on by the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Show multi-chain wallet balances in Settings

6 participants