Skip to content

Hotfix v0.2.5 - #147

Open
KingGorrin wants to merge 15 commits into
zenon-network:masterfrom
KingGorrin:chore/reown_wallet
Open

KingGorrin wants to merge 15 commits into
zenon-network:masterfrom
KingGorrin:chore/reown_wallet

Conversation

@KingGorrin

@KingGorrin KingGorrin commented Mar 8, 2026

Copy link
Copy Markdown
Collaborator

Summary

This PR fixes a WalletConnect regression on Windows 11 introduced in Syrius v0.2.4.

The issue is caused by a bug in the WalletConnect library stack where double quotes in the generated User Agent string are not replaced with single quotes. This causes the WalletConnect session handshake to fail on Windows 11.

To address this:

  • the deprecated WalletConnect library was replaced with reown_walletkit
  • a temporary patched version of reown_core was added to work around the upstream bug

Upstream issue:
reown-com/reown_flutter#355

This fix is intended for hotfix-0.2.5.


Root cause

During User Agent construction, invalid quoting is preserved:

  • double quotes are not replaced with single quotes
  • the WalletConnect handshake fails on Windows 11
  • sessions cannot be established

Changes

WalletConnect migration

  • replaced the deprecated WalletConnect library with reown_walletkit

Temporary upstream workaround

  • added a locally patched reown_core package under patched_packages/
  • avoids maintaining a full fork of the reown_flutter repository
  • this explains the large number of added files in the PR

Dependency updates required by the migration

The migration required upgrading Zenon dependencies:

These updates are necessary for compatibility with the new WalletConnect stack.

Platform / build fixes

Some additional changes were required to keep builds working after the migration:

Linux CI

  • added libsecret-1-dev and libjsoncpp-dev
  • required by the patched reown_core dependency chain

macOS

  • minor fixes required for the new dependency stack

CI / tooling

  • workflow updates required to build with the updated dependencies

Why a local patch?

The bug is still present upstream in reown_flutter.

Instead of maintaining a fork of the entire repository, only the affected package (reown_core) was patched locally. This keeps the workaround small and easy to remove once the upstream fix is released.


Testing

Tested on Windows 11:

  • WalletConnect session creation works again
  • handshake completes successfully
  • connection flow works as expected

Follow-up

Once the upstream issue is resolved, the local reown_core patch can be removed and replaced with the official package version.

@KingGorrin KingGorrin changed the title hotfix: fix WalletConnect on Windows 11 by migrating to reown_wallet and patching reown_core Hotfix v0.2.5 Mar 8, 2026
@KingGorrin
KingGorrin marked this pull request as ready for review March 8, 2026 11:45
@KingGorrin KingGorrin added the bug Something isn't working label Mar 8, 2026
@KingGorrin KingGorrin self-assigned this Mar 8, 2026
Comment thread .github/workflows/syrius_builder.yml
Comment thread lib/widgets/reusable_widgets/dotted_border_info_widget.dart Outdated
@KingGorrin

Copy link
Copy Markdown
Collaborator Author

The following improvements have been made to the wc service.

  • Use registerRequestHandler as the primary request handling flow
  • Remove onSessionRequest subscription
  • Restore pending request polling as a fallback for handler delivery issues
  • Centralize pairings/sessions reload and UI refresh logic
  • Refresh state on relevant WalletConnect sync and lifecycle events
  • Fix disconnectSessions to use session topics instead of pairing topics
  • Add explicit helpers for pairing cleanup
  • Approve sessions using generatedNamespaces when available, with manual fallback for Zenon
  • Make WalletConnect service creation idempotent
  • Register wallet accounts only after addresses are available
  • Prevent duplicate account registration
  • Add guards before pairing and clean up orphan pairings beforehand
  • Clean up pairings after rejected session proposals
  • Automatically recover from session proposal errors
  • Improve overall disconnect flow consistency
  • Improve state synchronization, UI refresh handling, and WalletConnect logging

@KingGorrin

Copy link
Copy Markdown
Collaborator Author

Refactored parts of the codebase and improved URI parsing reliability in app_links

Changes

main_app_container.dart

  • Centralized URI handling with _handleIncomingUri
  • Stream + initial URI now both routed through it
  • Improved deep-link parsing (_getDeepLinkAction)
  • More robust WalletConnect parsing via utility

wallet_connect_uri_utils.dart (new)

  • Added extractWalletConnectUri(...) helper
  • Supports encoded links, ?uri=, Windows format
  • Returns null for invalid input

utils.dart

  • Export added

Tests

  • Added unit tests for all key WalletConnect cases

@KingGorrin

Copy link
Copy Markdown
Collaborator Author

Applied additional fixes to improve the stability of WalletConnect communication.

1) lib/services/web3wallet_service.dart

  • Added chain-routing delegation for WC methods (znn_info, znn_sign, znn_send) to IChain/NoMService.
  • Reworked pending-request polling:
    • request lifecycle sets: in-flight/completed
    • stale-topic handling + stale-topic dedupe set
    • safer dispatch with optional chainId / requestId
    • robust response/error handling for already-responded + stale-session cases
  • Removed old fallback/unimplemented method behavior.
  • Removed reject cleanup disconnect call (_cleanupAfterReject) so reject doesn’t hard-break re-proposals.

2) lib/blocs/wallet_connect/chains/nom_service.dart

  • Added structured logging (NoMWalletConnectService).
  • Removed internal request-handler registration from constructor (centralized registration now in service).
  • Added public delegated handlers:
    • handleZnnInfo, handleZnnSign, handleZnnSend
  • Added single-flight dedupe for interactive requests to prevent duplicate dialogs.
  • Added session/address helpers:
    • session lookup by topic
    • active address resolution
    • requested fromAddress extraction and signer resolution
  • Updated method implementations:
    • znn_info returns resolved active address
    • znn_sign supports map/string payloads and signs with resolved signer address
    • znn_send uses resolved signer address instead of blindly trusting request param

3) lib/utils/functions.dart

  • Updated walletSign(...) to accept optional explicit address.
  • Added guard for unresolved/invalid signer index.
  • Minor formatting cleanup.

@0x3639
0x3639 requested a review from Copilot May 22, 2026 09:55

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

@0x3639

0x3639 commented May 22, 2026

Copy link
Copy Markdown
Collaborator

PR #147 Review: Actionable Findings

Reviewed against PR head 4434285294aaa05235637cddfa01a9ac1e1b837a (kinggorrin/chore/reown_wallet).

This review was performed back and forth between Claude 4.7 and Codex 5.5

Blocking / High Priority

1. fromAddress handling can sign or send from an address the user did not approve

lib/blocs/wallet_connect/chains/nom_service.dart:126 resolves a dApp-supplied fromAddress. If the dApp supplies an address that is not wallet-owned, the wallet logs a warning and falls back to kSelectedAddress instead of rejecting the request (:149-154). If the dApp supplies a different wallet-owned address, the wallet uses it (:136-147).

The confirmation dialogs do not show the actual signer/source address. The sign dialog only shows the message (:273-274), and the send dialog only shows amount and destination (:360-362). The transfer then uses signerAddress as the source (:390-394).

Impact: a malicious or buggy dApp can request a mismatched fromAddress, and the wallet may sign/send from an address the user did not see in the prompt. For payments, this is a funds-safety issue.

Recommended fix:

  • Reject requests when fromAddress is present and not wallet-owned.
  • Show the exact signer/source address in every znn_sign and znn_send confirmation.
  • Consider requiring fromAddress == kSelectedAddress unless the user explicitly approves using a different owned address.

2. Null-session approval still reports success

Claude's latest audit is correct here. In lib/services/web3wallet_service.dart:761-770, the success notification fires before checking approveResponse.session:

final approveResponse = await _approveSession(id: event.id);
await _sendSuccessfullyApprovedSessionNotification(dAppMetadata);

if (approveResponse.session != null) {
  _upsertSession(approveResponse.session!);
}

Impact: if Reown returns ApproveResponse.session == null, the user sees "Successfully connected" even though no session was added and no error was surfaced.

Recommended fix:

final approveResponse = await _approveSession(id: event.id);

if (approveResponse.session != null) {
  _upsertSession(approveResponse.session!);
  await _sendSuccessfullyApprovedSessionNotification(dAppMetadata);
} else {
  await NotificationUtils.sendNotificationError(
    Exception('WalletConnect approveSession returned null session'),
    'WalletConnect session approval failed',
  );
  _logger.warning(
    'approveSession returned null session for ${dAppMetadata.name}',
  );
}

3. The new test and flutter analyze are broken

The PR adds test/utils/wallet_connect_uri_utils_test.dart, but pubspec.yaml:82-86 does not include flutter_test in dev_dependencies.

Verification:

flutter test test/utils/wallet_connect_uri_utils_test.dart

fails immediately with:

Couldn't resolve the package 'flutter_test'

flutter analyze also fails after flutter pub get with the same missing flutter_test errors plus stale generated mock errors under patched_packages/reown_core-1.3.8/test/.... Because the patched package is vendored inside the repo, root analysis traverses its test tree.

Recommended fix:

  • Add flutter_test under root dev_dependencies.
  • Exclude or remove vendored package tests/examples from the root analyzer, or regenerate the stale mocks if the tests are meant to be maintained in-tree.
  • Add flutter test and/or flutter analyze to CI for this PR if possible.

4. Dynamic chain approval is not matched by service/handler registration

I disagree with the optimistic read in 05-audit-current.md that chain resolution fully adapts per session. The dispatch code tries to resolve the session chain (lib/services/web3wallet_service.dart:386-407), and approval uses the current getChainIdentifier() (:824-828), but setup registers only one chain service:

sl.registerSingleton<IChain>(
  NoMService(reference: NoMChainId.mainnet),
  instanceName: NoMChainId.mainnet.chain(), // zenon:1
);

That is lib/main.dart:197-200. Also, request handlers/event emitters are registered during Web3WalletService.create() (lib/services/web3wallet_service.dart:73-76, :352-384), before InitUtils._setChainId() loads the persisted chain id (lib/utils/init_utils.dart:36-42).

Impact: if the wallet is configured for chain id 3, the PR can approve zenon:3 sessions/accounts, but later request dispatch resolves zenon:3 and no matching IChain service exists. znn_info, znn_sign, and znn_send can fail for non-zenon:1 sessions.

Recommended fix:

  • Register supported chain services for every approved chain id, or register/update them after the persisted chain id is loaded.
  • Register request handlers/event emitters using the same chain ids that can appear in approved namespaces.
  • Add a test/manual case for WalletConnect on chain id 3.

5. WalletConnect URI validation remains too loose in several entry points

Claude is right that extractWalletConnectUri(...) improves the deep-link path, but the PR still accepts generic URIs elsewhere:

  • lib/widgets/main_app_container.dart:1055-1063 accepts clipboard text with Uri.tryParse(text) != null.
  • lib/widgets/modular_widgets/wallet_connect_widgets/wallet_connect_uri_card.dart:62-66 validates manual input with Uri.tryParse.
  • lib/widgets/modular_widgets/wallet_connect_widgets/wallet_connect_uri_card.dart:100-104 enables Connect with Uri.tryParse.
  • lib/widgets/modular_widgets/wallet_connect_widgets/wallet_connect_qr_card.dart:144-146 accepts scanned QR text with Uri.tryParse.
  • lib/widgets/modular_widgets/wallet_connect_widgets/wallet_connect_camera_card.dart:134-140 uses Uri.tryParse for camera QR validation.

Uri.tryParse accepts ordinary URLs and relative strings that are not WalletConnect payloads. This can trigger false WalletConnect notifications/tab navigation or enable Connect for garbage input.

Recommended fix: route every user input, clipboard, QR, and deep-link path through extractWalletConnectUri(...) or another Reown-compatible WalletConnect parser. Validation should require a wc: payload with the required WalletConnect parameters, not any parseable URI.

6. Duplicate session proposal dialogs are still possible

Web3WalletService._onSessionProposal only checks _approvedProposalIds after the user accepts (lib/services/web3wallet_service.dart:754-760). It does not mark a proposal as in-flight before opening the dialog (:717-752).

Impact: if Reown delivers the same proposal event more than once while the first modal is still open, the wallet can stack duplicate approval dialogs for the same proposal. Reject/approve ordering then becomes ambiguous.

Recommended fix: track proposal ids before showing the dialog, clear them on rejection/failure, and ensure each proposal id is approved or rejected exactly once.

Medium / Follow-Up

7. windowManager.show() is still unawaited in request handlers

Agreed with Claude / 03-windows-rca.md. NoMService still calls windowManager.show() without await in all three request handlers:

  • lib/blocs/wallet_connect/chains/nom_service.dart:174-176
  • lib/blocs/wallet_connect/chains/nom_service.dart:241-243
  • lib/blocs/wallet_connect/chains/nom_service.dart:316-318

The corresponding _approveSession path correctly awaits windowManager.show(). The request handlers should do the same, especially for Windows where native show/focus calls are slower.

8. Vendored reown_core-1.3.8 needs an exit plan

Agreed with Claude. The override exists for a narrow upstream issue, but it vendors a large third-party package into the repo. Add a comment in pubspec.yaml explaining why the override exists and what upstream version/issue lets it be removed. Keep root analysis from traversing vendored tests unless this repo intends to maintain them.

9. Polling timer and bookkeeping sets are unbounded

Agreed with Claude, but I would not block on this after the higher-priority issues above. Web3WalletService starts a 1-second polling timer in create() and keeps request/proposal/account bookkeeping sets for the process lifetime. Gate polling on active sessions or bound the sets.

10. macOS entitlements look okay in debug, but release still needs a smoke test

Claude flagged emptied entitlement plist files. I ran:

flutter build macos --debug
codesign -d --entitlements :- "build/macos/Build/Products/Debug/s y r i u s.app"

The debug build succeeded and the signed app did contain sandbox, network, camera, JIT, and get-task-allow entitlements. I would downgrade this from an open code finding to a release-smoke-test item: verify the release DMG can use network, camera QR scan, and embedded-node/native-library flows.

11. Localization files are currently dead code

Agreed with Claude. lib/l10n/app_localizations*.dart and flutter_localizations were added, but AppLocalizations is not wired into MaterialApp and not used in lib/. Either wire it in or remove the files from this PR.

Notes On Claude Comparison

I agree with Claude that the latest head resolved the earlier triple-path request handling, znn_sign / znn_send stubs, and silent znn_info disclosure from 330d0ea.

I disagree with treating dynamic chain handling as resolved. The resolver is dynamic, but DI registration and handler registration are not. Non-zenon:1 WalletConnect sessions still look broken.

I also think the URI-validation issue is broader than the latest Claude draft says because clipboard handling in main_app_container.dart still uses Uri.tryParse, not only paste/camera/QR widgets.

Local Verification Performed

  • flutter pub get: completed.
  • flutter test test/utils/wallet_connect_uri_utils_test.dart: fails because flutter_test is missing.
  • flutter analyze: fails with missing flutter_test plus vendored reown_core stale mock errors.
  • flutter build macos --debug: succeeds locally with warnings.
  • codesign -d --entitlements :- ...: debug app contains expected entitlements.

Additional Claude Observations

Three items I want to flag that aren't in the sections above. Listed here rather than re-numbered into the main list so the prior structure stays stable.

A. Approval ignores the dApp's proposed namespaces

Web3WalletService._approveSession (lib/services/web3wallet_service.dart:818-836) hard-codes the approved namespaces regardless of what the dApp asked for:

final resolvedNamespaces = <String, Namespace>{
  'zenon': Namespace(
    chains: [_namespaceChainId],
    accounts: _walletAccounts(),
    methods: const ['znn_sign', 'znn_info', 'znn_send'],
    events: const ['chainIdChange', 'addressChange'],
  ),
};

event.params.requiredNamespaces / optionalNamespaces are never consulted. The approval dialog (:717-752) also doesn't show the user what the dApp proposed — only the dApp metadata.

Two consequences:

  • A dApp that proposes any non-zenon namespace (e.g., eip155) gets a partial approval. Depending on how Reown handles the mismatch, the session may fail at approval time or — worse — succeed and leave the dApp calling methods the wallet won't respond to.
  • The user is approving an opaque "connect" without seeing the chain id, methods, or events being granted. This is below the bar I'd expect for a wallet connection prompt.

Recommended: validate the proposed namespaces against the wallet's supported set, reject (with a clear error) if the dApp requires anything outside it, and surface chain id + methods in the approval dialog.

B. params.hashCode keying of single-flight dedup is brittle

NoMService.handleZnnSign and handleZnnSend (lib/blocs/wallet_connect/chains/nom_service.dart:85-93) key the single-flight cache by '$topic:${params.hashCode}'. params is dynamic and is usually a Dart Map. Dart's default Map.hashCode is identity-based, not value-based — two distinct Map instances with identical content can have different hash codes.

The polling fallback in Web3WalletService._drainPendingSessionRequests constructs its own view of pending requests via _safeGetPendingRequests(). If the params object the polling path reaches handleZnnSign with is a different Map instance than the one the registered-handler path receives — even if the contents are identical — the single-flight cache will miss the dedup and the user will see two Sign Message / Send Payment dialogs for the same request.

This matters specifically because the polling fallback is the mechanism this PR uses to fix the Windows bug. If dedup silently misses there, the user can approve the same payment twice.

Recommended: key the single-flight cache by requestId (which is unique and stable per request) or by a content-hash of params (e.g., jsonEncode(params).hashCode). Add a manual test that explicitly fires both paths against the same request and confirms exactly one dialog appears.

C. _cleanupStalePairings is too aggressive in the error-recovery path

_cleanupStalePairings (lib/services/web3wallet_service.dart:134-162) disconnects every pairing that has no associated session. It's called from pair() (reasonable — the user is about to create a fresh one) and from _recoverFromProposalError (called when onSessionProposalError fires).

In WalletConnect v2, a pairing without a session is the normal state between pairing and session-approval. A single onSessionProposalError doesn't mean every session-less pairing on disk is stale — it just means this one proposal failed. The recovery handler will disconnect any other pairings the user has open that haven't yet been used for a session.

Recommended: scope the cleanup to the failing pairing (use event.topic from the error), or remove the cleanup call from _recoverFromProposalError entirely and rely on the periodic cleanup at pair() time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants