feat(signing): WalletConnect v2 backend (attested-signing PR9/10)

[zmanian]

Rebased onto current main (attested-signing cascade, 2026-07-23). The stack was 1184 commits behind (merge base 2026-05-24). Tracking issue: #6532.

Inline review threads may have re-anchored or orphaned from the force-push. Reviewers: the "Cascade changes" section states exactly what the rebase altered beyond the original work.

What this PR is

WalletConnect v2 backend in ironclaw_wallet_external, over the openssl-free fork tracecommons/walletconnect-rs (relay_client/relay_rpc, cacao deliberately disabled).

Cascade changes

• The openssl-free fork pin was re-verified against current dependencies — this was the flagged risk for this hop. The workspace still resolves and workspace_graph_is_openssl_free passes with the WalletConnect fork in the tree.
• deny.toml rebuilt as main's + the fork allowance. The branch's copy had a stale allow-git entry (astral-sh/ruff.git); main currently uses samuelcolvin/ruff.git. Taking main's file and appending only the tracecommons/walletconnect-rs line avoids reverting main-side policy drift.
• Adopted the round-2 API: VerifiedProof::new is now 3-arg (ProviderId::WalletConnect, *approved_tx_hash, proof) — binding the provider and the approved hash into the proof is what makes it unforgeable; the old 1-arg form would have dropped that. Plus the GrantError::InvalidTimestamp/InvalidExpiry fail-closed mapping.

Verification

15 + 20 + 27 crate tests pass; boundary test green including workspace_graph_is_openssl_free; clippy clean.

[gemini-code-assist]

Code Review

This pull request implements the WalletConnect v2 signing provider within the ironclaw_wallet_external crate, enabling out-of-band signing for EVM and ed25519-based chains. The implementation includes CAIP-2 chain resolution, namespace pinning to prevent scope broadening, and a verification flow that binds proofs to specific sessions and nonces. Additionally, architectural tests were added to ensure the workspace remains OpenSSL-free. Feedback from the review highlights critical security risks associated with manual byte-based string indexing in hex decoding logic, which could lead to panics on non-ASCII input. It was also recommended to implement case-insensitive comparisons for EVM addresses to avoid unexpected verification failures.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 343eb2525a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[henrypark133]

Code Review (multi-agent)

Intent: Implement WalletConnect v2 signing provider backend with namespace pinning and domain-separated attestation digest verification
Stats: 16 findings (from 24 raw) across 10 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, pattern-refactor. Reviewers failed: none. Body-only: 5

Security (2)

1. Medium Duplicate bs58 crate versions (0.4.0 and 0.5.1) in lockfile (Cargo.lock:1250-1260, confidence 75) — anchor: Cargo.lock:1253
   The diff introduces bs58 0.4.0 while 0.5.1 already exists. Having two versions of a cryptographic encoding library can lead to subtle interoperability issues.
   Fix: Consolidate to a single bs58 version by updating the dependent crate to use 0.5.1.

2. Medium Diff truncated — source code changes not visible for full security analysis (diff:1-1, confidence 75) — anchor: context_bundle:diff_truncated=true (no diff position — body only)
   The diff is truncated, so the actual implementation of WalletConnectSigningProvider cannot be fully reviewed.
   Fix: Provide the full diff or review the source files directly.

Bugs (2)

1. Medium encode_walletconnect_proof silently swallows serialization errors (crates/ironclaw_wallet_external/src/walletconnect/proof.rs:67-69, confidence 75) — anchor: crates/ironclaw_wallet_external/src/walletconnect/proof.rs:68
   serde_json::to_vec(payload).unwrap_or_default() returns empty Vec on any serialization failure.
   Fix: Return Result<Vec<u8>, SigningProviderError> and map the serde_json error to ProofInvalid.
   Also flagged by: local-patterns/Medium

2. Medium SessionBindingStore silently drops all operations on mutex poison (crates/ironclaw_wallet_external/src/walletconnect/session.rs:93-119, confidence 75) — anchor: crates/ironclaw_wallet_external/src/walletconnect/session.rs:94
   record(), peek(), and take() all silently do nothing when the Mutex is poisoned.
   Fix: Use .expect() on the lock result so a poisoned mutex panics immediately.
   Also flagged by: performance/Medium
   Also flagged by: tests/Medium
   Also flagged by: performance/Medium

Tests (5)

1. High verify_evm non-32-byte signed_payload error path not exercised (crates/ironclaw_wallet_external/src/walletconnect/signer.rs:67-110, confidence 75) — anchor: crates/ironclaw_wallet_external/src/walletconnect/signer.rs:75
   verify_evm returns ProofInvalid when signed_payload is not exactly 32 bytes. No test constructs a WC proof with a wrong-length signed_payload for EVM.
   Fix: tests::walletconnect_verify_resume::evm_signed_payload_wrong_length_is_proof_invalid
   Also flagged by: tests/High

2. Medium recovery_id_from_v invalid v-byte error path not exercised (crates/ironclaw_wallet_external/src/walletconnect/signer.rs:162-186, confidence 75) — anchor: crates/ironclaw_wallet_external/src/walletconnect/signer.rs:172
   recovery_id_from_v rejects v values outside {0,1,27,28,>=35}. No test constructs a proof with an invalid v byte.
   Fix: tests::walletconnect_verify_resume::evm_invalid_recovery_id_v_is_proof_invalid

3. High verify_chain_signature Solana missing public_key error path not exercised (crates/ironclaw_wallet_external/src/walletconnect/signer.rs:46-65, confidence 75) — anchor: crates/ironclaw_wallet_external/src/walletconnect/signer.rs:49
   verify_chain_signature returns ProofInvalid when public_key is None for Solana/ed25519 families. All existing Solana tests provide public_key.
   Fix: tests::walletconnect_verify_resume::solana_missing_public_key_is_proof_invalid

4. Medium verify_ed25519 wrong-length signature and public_key error paths not exercised (crates/ironclaw_wallet_external/src/walletconnect/signer.rs:112-160, confidence 75) — anchor: crates/ironclaw_wallet_external/src/walletconnect/signer.rs:118
   verify_ed25519 returns ProofInvalid for non-64-byte signatures and non-32-byte public keys. No test constructs a Solana proof with wrong-length crypto fields.
   Fix: tests::walletconnect_verify_resume::solana_wrong_length_crypto_fields_is_proof_invalid

5. Low WalletConnectSigningProvider::initiate not tested (crates/ironclaw_wallet_external/src/walletconnect/mod.rs:124-160, confidence 75) — anchor: crates/ironclaw_wallet_external/src/walletconnect/mod.rs:124
   The initiate method is a public trait implementation that returns AwaitingUserAction. No test calls initiate.
   Fix: tests::walletconnect_verify_resume::initiate_returns_awaiting_user_action

Conventions (4)

1. Medium Hex parsing regresses to panic on non-ASCII Unicode input (crates/ironclaw_wallet_external/src/injected/evm.rs:95-110, confidence 100) — anchor: CLAUDE.md:21 — No .unwrap() or .expect() in production code (no diff position — body only)
   The diff replaces the byte-based panic-free hex parser with u8::from_str_radix on &str which WILL PANIC on non-ASCII even-byte input. The old code was explicitly designed as panic-free for untrusted relay/wallet input. Tests verifying this were removed.
   Fix: Revert to the byte-based hex parser (as_bytes() + per-byte nibble matching). Re-add the removed Unicode panic-free tests.

2. Medium Hex parsing regresses to panic on non-ASCII Unicode input (crates/ironclaw_wallet_external/src/injected/solana.rs:64-79, confidence 100) — anchor: CLAUDE.md:21 — No .unwrap() or .expect() in production code (no diff position — body only)
   Same regression as evm.rs: replaces byte-based panic-free hex parser with u8::from_str_radix on &str slices.
   Fix: Revert to the byte-based panic-free hex parser.

3. Medium Hex decode helper regresses to panic on non-ASCII Unicode input (crates/ironclaw_wallet_external/src/injected/mod.rs:219-233, confidence 100) — anchor: CLAUDE.md:21 — No .unwrap() or .expect() in production code (no diff position — body only)
   Same regression: hex_decode replaces byte-based parsing with u8::from_str_radix on &str, panicking on non-ASCII even-byte JSON strings.
   Fix: Revert to byte-based panic-free hex parsing.
   Also flagged by: local-patterns/Medium

4. Medium Removes panic-free Unicode hex parsing tests without rationale (crates/ironclaw_wallet_external/tests/verify_resume.rs:347-454, confidence 75) — anchor: AGENTS.md:79-80 — Test through the caller, not just the helper (no diff position — body only)
   The diff removes four tests that verified panic-free behavior on attacker-supplied Unicode input. These tests covered a documented security invariant.
   Fix: Either retain the tests or add rationale explaining why Unicode panic-free parsing is no longer a requirement.

Local Patterns (1)

1. Low SessionBindingStore::new() is redundant boilerplate over derive(Default) (crates/ironclaw_wallet_external/src/walletconnect/session.rs:86-89, confidence 75) — anchor: crates/ironclaw_wallet_external/src/injected/mod.rs:94-98
   SessionBindingStore derives Default and new() is just Self::default().
   Fix: Remove SessionBindingStore::new() and let callers use SessionBindingStore::default().

Maintainability (2)

1. Medium Three copies of hex serialization/deserialization in one crate (crates/ironclaw_wallet_external/src/walletconnect/mod.rs:300-352, confidence 100) — anchor: crates/ironclaw_wallet_external/src/walletconnect/mod.rs:300
   Near-identical hex (de)serialization logic is duplicated three times.
   Fix: Extract a single crate-level hex module.
   Also flagged by: tests/Medium

2. Medium encode_walletconnect_proof silently swallows serialization errors (crates/ironclaw_wallet_external/src/walletconnect/mod.rs:67-69, confidence 75) — anchor: crates/ironclaw_wallet_external/src/walletconnect/mod.rs:68
   encode_walletconnect_proof returns Vec with no error path and uses unwrap_or_default().
   Fix: Change the return type to Result<Vec<u8>, SigningProviderError>.
   Also flagged by: conventions/Medium

[zmanian]

Review response — henrypark133 CHANGES_REQUESTED (16 findings)

Pushed 5b3b0f83c (force-with-lease). cargo test -p ironclaw_wallet_external 27/27 pass, cargo fmt --check clean, cargo clippy -p ironclaw_wallet_external --all-features --tests zero warnings. PRESERVE invariants untouched (case-insensitive-but-exact EVM session-account match, ApprovedTxHash binding, openssl-free pinned fork rev, deterministic resume).

#	Finding	Disposition
Sec 1	bs58 0.4.0/0.5.1 dup in lockfile	Decline (would-weaken-invariant) — bs58 0.4.0 is transitive-only via the pinned relay_rpc fork; this crate uses no bs58 directly. Consolidating needs a fork-rev bump or a [patch], both risking the openssl-free pinning. No interop risk for us.
Sec 2	Diff truncated	Not actionable — full source reviewed directly; no truncation in the repo.
Bug 1	encode_walletconnect_proof swallows serde errors	Fixed — returns Result<_, SigningProviderError>, maps to ProofInvalid.
Bug 2	SessionBindingStore silently drops on mutex poison	Fixed — record/peek/take recover the guard via into_inner() (no silent no-op; panic-free per the crate's no-.expect() rule; no induced-panic DoS).
Test 1 (High)	verify_evm non-32-byte payload untested	Fixed — evm_signed_payload_wrong_length_is_proof_invalid.
Test 2	recovery_id_from_v invalid-v untested	Fixed — evm_invalid_recovery_id_v_is_proof_invalid.
Test 3 (High)	Solana missing public_key untested	Fixed — solana_missing_public_key_is_proof_invalid.
Test 4	verify_ed25519 wrong-length fields untested	Fixed — solana_wrong_length_crypto_fields_is_proof_invalid (sig + pubkey branches).
Test 5 (Low)	initiate untested	Fixed — initiate_returns_awaiting_user_action + initiate_unsupported_chain_fails_closed.
Conv 1–3	Hex parsing "regresses to panic" on non-ASCII (evm.rs/solana.rs/injected/mod.rs)	Stale / out-of-scope — these paths are not in this PR's diff (PR9 vs staging touches no injected/ files). The WalletConnect hex paths the PR does own (signer::decode_hex_fixed, hex_bytes::hex_decode) are byte-based (as_bytes() + chunks_exact()) and fail closed on non-ASCII — they cannot panic.
Conv 4	Removed Unicode panic-free tests	Stale — those tests are present: evm_malformed_unicode_hex_account_fails_closed and malformed_unicode_hex_in_proof_field_fails_closed both assert ProofInvalid (not panic) on non-ASCII even-byte input.
Local 1 (Low)	SessionBindingStore::new() redundant over Default	Decline — new() is the production constructor; explicit constructor alongside Default is idiomatic.
Maint 1	Three copies of hex (de)serialization	Addressed — opt_hex_bytes already delegated to shared hex_bytes; decode_hex_fixed now delegates its nibble decode there too, leaving only the [u8; N]-vs-Vec length wrapper.
Maint 2	encode swallows errors (dup of Bug 1)	Fixed (same change).

[henrypark133]

Low — deny.toml sets multiple-versions = "warn" with no skip entries for the new duplicate crates introduced by this PR: bs58 (0.4.0 from relay_rpc + 0.5.1 already in the workspace) and ordered-float (2.10.1 from relay_rpc + 5.3.0 already in the workspace). This is not a deny-level block (just a warning), but cargo-deny warnings surfacing on every CI run from a dependency that won't be upgraded in this PR is noise. Consider adding explicit [[bans.skip]] entries with comments noting the relay_rpc version pins, to document the reason and suppress the warning cleanly.

[henrypark133]

Low — Relay URL validation and session expiry are not present in PR9, both correctly deferred to PR10. One note for the PR10 author: when the live relay connection is wired, the relay hostname should be validated against an allowlist (e.g. relay.walletconnect.com only, not arbitrary WebSocket targets) to prevent a compromised config from redirecting the session to an attacker-controlled relay. Similarly SessionBinding has no TTL; PR10 should enforce a max binding lifetime (typically 5 minutes matching WC v2 session proposal timeout) so a stale binding cannot be used for a long-delayed replay after the grant CAS and nonce checks. Neither gap affects the correctness of PR9's current surface (relay is stubbed; grant CAS + nonce binding provide replay protection for the completed-proof path). Recording here for PR10 tracking.

[henrypark133]

PR9/10 attested-signing WalletConnect v2 backend: approve.

The two security cores — namespace pinning (enforce_pinned_scope) and verify_resume's fail-closed chain of hash binding (T20) → session+nonce binding (T18) → signed-payload binding (#1) → chain-signature verification (T17) → one-shot grant CAS (T20) — are correctly implemented and thoroughly tested. CAIP-2 grammar validation, CAIP-10 account chain-pinning, singleton-exact scope enforcement, EVM ecrecover / ed25519 verification, and the peek-first / consume-on-success binding discipline are all sound. The openssl-free posture and architecture boundary tests are well-enforced.

Findings (no Critical/High):

• Medium (session.rs:85): record() doc says "propagate poison by panicking" but code calls poison.into_inner() which recovers. Misleading doc for a security-critical store.
• Low (Cargo.toml:37): PR body cites fork rev c6b528e but Cargo.toml/lock pin 7078fd3 — stale description.
• Low (deny.toml): no [[bans.skip]] for new bs58 / ordered-float version duplicates from relay_rpc; will produce CI warnings.
• Low (architectural, PR10 tracking): relay URL should be allowlisted; SessionBinding has no TTL — noted for PR10 author.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (2)

• staging
• reborn-integration

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: a87da42c-3bb1-4bf6-8d82-f45534c7d880

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[zmanian]

Superseded by #6755, part of consolidating the 20-PR attested-signing-* stack into 8 PRs against current main.

The content of this PR is carried forward in full, re-based on current main rather than on the old stack. That re-basing also fixed the CI failures that had been red here — including the missing [package.metadata.ironclaw] layer declarations, which were blocking the bottom of the stack and therefore every PR above it.

Closing to keep the queue honest. The review history stays on this PR and remains readable; reopen if the consolidation is rejected.