Skip to content

refactor(tor): replace legacy Tor integration - #2583

Merged
ethicnology merged 1 commit into
developfrom
feat/tor-settings
Aug 18, 2026
Merged

refactor(tor): replace legacy Tor integration#2583
ethicnology merged 1 commit into
developfrom
feat/tor-settings

Conversation

@ethicnology

Copy link
Copy Markdown
Member

Stacked on the feat/tor-electrum PR. Review that one first.

Both Tor consumers now go through bull_tor, so the old integration has nothing left to serve and is deleted: lib/core/tor/'s socket adapter, tor_connectivity_service, tor_config_adapter, tor_locator, tor_status, the proxy-config value object and check_tor_proxy_connection_usecase.

That also drops the tor dependency (Foundation-Devices/tor, pinned at b92e2bc) and the Linux plugin registration it brought with it. Net effect on this commit is 514 deletions against 288 insertions.

The Tor settings screen moves onto the new contract: the cubit reads connection state from bull_tor, and tor_connection_status_card reports what the new state machine can actually express instead of the old boolean-ish status.

This is the PR where the migration becomes irreversible, so it is worth checking that nothing outside the deleted tree still expects the old types — whole-project analyze covers that, and it is clean.

@ethicnology ethicnology self-assigned this Aug 10, 2026
@claude

claude Bot commented Aug 10, 2026

Copy link
Copy Markdown

Claude finished @ethicnology's task in 7m 48s —— View job


Review: refactor(tor): replace legacy Tor integration

  • Gather context (diff against feat/tor-electrum, read changed files)
  • Review Tor settings cubit & status card migration
  • Review deletions for dangling references / regressions
  • Post findings

Note: I can't post formal inline review comments (no gh/review API access in this run), so findings are listed below with file/line references. CI (Analyze and Test) was still in progress when this review was posted, and this runner has no Flutter toolchain, so I verified by reading the code rather than running make analyze/make unit-test.

Summary

The deletion is clean. I searched lib/, test/, and integration_test/ for every removed type (TorStatus, TorDatasource, TorRepository, InitTorUsecase, IsTorRequiredUsecase, TorStatusUsecase, CheckTorProxyConnectionUsecase, TorConnectivityService, TorProxyConfig, any core/tor/ import) — zero dangling references. socks5_proxy is correctly retained (still used by electrum_socket_connector.dart and bull_tor's HTTP client factory). Build-system cleanup is consistent: pubspec.yaml/pubspec.lock, ios/Podfile.lock, linux/generated_plugins.cmake, makefile (TRACKED_RUST_LIBS and the per-lib rustc-pin case), Containerfile.tools comment, and the reproducibility README all drop tor/libtor.so coherently.

Migration correctness checks that passed:

  • VerifyExternalTorUsecase is registered in bull_tor's TorLocator.registerUsecases, called from core_locator.dart:83 at startup — the TorSettingsCubit factory resolves it fine.
  • Removing ..init() from tor_settings_router.dart:19 is safe and actually fixes a double-init: TorSettingsScreen.initState already called init() on the base branch, and the electrum-settings router keeps ..init() for its own cubit instance (electrum_settings_router.dart:28), so TorProxyErrorBanner's connection is TorReady check still gets populated.
  • No timeout regression: ExternalSocksTorBackend.verify keeps 3s connect + 3s read timeouts, matching the deleted TorConnectivityService. (The old 30s status cache is gone, so every check does a real SOCKS5 handshake — fine for a settings screen.)
  • Behavior improvement worth calling out: disabling the proxy now emits TorStopped instead of leaving a stale "online" status, and the new isClosed guards fix a real emit-after-close bug (covered by the new cubit test).

Findings (all low severity — nothing blocking)

1. checkConnectionStatus can get stuck on "Connecting…" if the stored port is ever invalidlib/features/tor_settings/presentation/bloc/tor_settings_cubit.dart:60-68
TorConnecting is emitted before TorProxyEndpoint(...) is constructed, and that constructor throws (RangeError/ArgumentError) on an out-of-range port or empty host. The port sheet validates 1–65535, but the repository layer (setTorProxyPort) does not, so a corrupted/legacy stored value would throw after the emit — leaving the card spinning forever, with the exception unhandled because init() is fire-and-forget in initState. Constructing the endpoint before the emit (or catching and mapping to TorUnavailable) makes the failure mode a red "Disconnected" instead of a permanent spinner. Fix this →

2. Stale verify result can overwrite a newer onelib/features/tor_settings/presentation/bloc/tor_settings_cubit.dart:69-70
Two overlapping checkConnectionStatus() calls (e.g. user changes the port while a verify against the old port is riding out its 3s timeout) race, and whichever completes last wins — so a failure for the old port can overwrite a success for the new one. The old code had the same shape, so this is not a regression, but a simple generation counter (final gen = ++_checkGeneration; ... if (gen == _checkGeneration && !isClosed) emit(...)) would close it while you're in this file.

3. refreshSettings() is missing the isClosed guard its siblings gotlib/features/tor_settings/presentation/bloc/tor_settings_cubit.dart:73-76
Harmless today (checkConnectionStatus re-checks isClosed on entry), but init() and updateTorSettings() both guard between the two awaits and this one doesn't — worth one line for consistency.

4. The censorship/progress UI added to the status card is unreachable from this screenlib/features/tor_settings/ui/widgets/tor_connection_status_card.dart:21 (_looksCensored), :91/:112 (its call sites plus the bootstrap-progress title), and the new routeLabel parameter (:9)
VerifyExternalTorUsecase only ever yields TorReady or TorUnavailable(failure: TorExternalProxyUnavailableFailure) — never TorConnecting.progress/diagnostic or TorBootstrapFailure — and no production call site passes routeLabel (only the widget test does). I assume these are staged for the embedded-Tor/Snowflake PRs in this stack; if not, they're dead code plus three new app_en.arb strings shipped early. Relatedly, the three new localization keys exist only in app_en.arb, so all 26 other locales will show English for them.

None of these should hold up the merge — the stated goal (nothing outside the deleted tree still expects the old types) checks out.


@ethicnology
ethicnology force-pushed the feat/tor-settings branch 2 times, most recently from 5b93305 to 5d4ebf1 Compare August 10, 2026 21:46
@ethicnology
ethicnology force-pushed the feat/tor-settings branch 2 times, most recently from 8007d94 to 98ece4d Compare August 11, 2026 01:59
@ethicnology
ethicnology force-pushed the feat/tor-settings branch 2 times, most recently from 7604345 to aaefccf Compare August 17, 2026 18:28
Base automatically changed from feat/tor-electrum to develop August 18, 2026 15:36
@ethicnology
ethicnology merged commit 5809b3f into develop Aug 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant