M-I02: normalize input hex addresses

[philanton]

Description

The clearnode API accepts wallet addresses without the 0x prefix. common.IsHexAddress validates format but accepts both "0xabc123..." (42 chars) and "abc123..." (40 chars) as valid. Both representations decode to the same 20-byte address via common.HexToAddress, producing identical channel IDs, signatures, and on-chain behavior — but they are treated as different strings in the database.

The signature verification in ChannelSigValidator.Verify (pkg/core/channel_signer.go:213) compares the recovered address (always "0x..."-prefixed via EthereumAddress.String()) against the wallet using strings.EqualFold. A no-prefix wallet will always fail this comparison, causing the entire database transaction to roll back.

However, this is an accidental guard — the rejection happens at signature validation (line 150), not at input sanitization (line 27). The code between those lines (channel ID computation, existing channel lookup, state advancement validation) all execute with a wallet string that doesn't match the database's expected format before the transaction is rolled back.

Summary by CodeRabbit

Bug Fixes

• Improved Ethereum address validation with clearer, more detailed error messages across API endpoints for session and channel operations.
• Standardized address formatting for consistent handling across the platform.

Tests

• Added comprehensive test coverage for address validation and normalization functionality.

[coderabbitai]

📝 Walkthrough

Walkthrough

This pull request replaces go-ethereum/common hex address validation with a new core.NormalizeHexAddress utility function across multiple API handlers. The change standardizes address normalization to lowercase and provides more detailed error messages while maintaining existing control flow for downstream operations.

Changes

Cohort / File(s)	Summary
API Handler Address Normalization clearnode/api/app_session_v1/submit_session_key_state.go, clearnode/api/channel_v1/handler.go, clearnode/api/channel_v1/request_creation.go, clearnode/api/channel_v1/submit_session_key_state.go	Replaced common.IsHexAddress validation with core.NormalizeHexAddress normalization for user address and session key fields. Updated error messages to report normalization errors instead of generic validation failures.
Core Utility Enhancement pkg/app/app_v1.go	Updated PackAppV1 to normalize app.OwnerWallet via core.NormalizeHexAddress and return normalization errors, replacing boolean address validation with error-based handling.
Normalization Utility & Tests pkg/core/utils.go, pkg/core/utils_test.go	Added new NormalizeHexAddress function that enforces strict Ethereum address formatting (0x prefix + 40 hex chars), validates hex character validity, and returns descriptive errors. Includes comprehensive unit tests covering valid inputs (various cases), missing prefix, incorrect length, empty string, and invalid characters.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A rabbit hops through hex so bright,
Normalizing addresses left and right,
Gone are the days of loose validation loose,
Now errors speak truth, no more excuse,
From 0xABC to 0xabc we glide,
Consistent addresses with nowhere to hide! 🔐

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly matches the main change: adding normalization of input hex addresses via the new NormalizeHexAddress function applied across multiple API handlers.

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

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/clearnode-i-02

---

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 and usage tips.}

[nksazonov]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[nksazonov]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (2)

pkg/app/app_v1.go (1)

35-37: Wrap normalization error with %w to preserve error chain.

Current formatting drops typed error semantics for upstream handling.

♻️ Suggested patch

-		return nil, fmt.Errorf("invalid owner wallet address: %v", err)
+		return nil, fmt.Errorf("invalid owner wallet address: %w", err)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/app/app_v1.go` around lines 35 - 37, The error returned from
core.NormalizeHexAddress when assigning app.OwnerWallet should be wrapped using
%w to preserve the error chain; update the error return in the block that calls
core.NormalizeHexAddress (the assignment to app.OwnerWallet) so the fmt.Errorf
uses "%w" and returns the original error (err) instead of "%v", ensuring callers
can use errors.Is / errors.As on the wrapped error.

clearnode/api/channel_v1/handler.go (1)

92-93: Preserve underlying DB error details in both receiver-state reads.

Both branches now lose err, which makes incident debugging harder.

♻️ Suggested patch

-		return nil, rpc.Errorf("failed to get last %s user state for transfer receiver with address %s", senderState.Asset, receiverWallet)
+		return nil, rpc.Errorf("failed to get last %s user state for transfer receiver with address %s: %v", senderState.Asset, receiverWallet, err)
...
-		return nil, rpc.Errorf("failed to get last %s user state for transfer receiver with address %s", senderState.Asset, receiverWallet)
+		return nil, rpc.Errorf("failed to get last %s user state for transfer receiver with address %s: %v", senderState.Asset, receiverWallet, err)

Also applies to: 109-110

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@clearnode/api/channel_v1/handler.go` around lines 92 - 93, The two
error-return branches that currently call rpc.Errorf("failed to get last %s user
state for transfer receiver with address %s", senderState.Asset, receiverWallet)
are dropping the underlying err; update both failing branches (the
receiver-state read branches that call rpc.Errorf) to include the original err
details in the message (e.g., pass err as an argument or format it into the
string) so rpc.Errorf includes the underlying DB error, referencing the existing
variables err, senderState.Asset, and receiverWallet in each updated call.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@clearnode/api/channel_v1/submit_session_key_state.go`:
- Around line 35-45: Normalization of user/session keys via
core.NormalizeHexAddress (used on coreState.UserAddress and
coreState.SessionKey) can cause misses in GetLastChannelSessionKeyVersion(...)
because historical rows may use legacy (non-canonical) keys; add a migration
plan or implement a temporary dual-read reconciliation: when reading last
session version (the code path invoking GetLastChannelSessionKeyVersion),
attempt lookup by the normalized key first and if not found perform a fallback
lookup using the legacy/raw key and reconcile results (or mark for migration),
and when writing persist both canonical and legacy keys or schedule a DB
backfill to update existing rows before switching to normalized-only keys.
Ensure changes are applied around the functions NormalizeHexAddress, the submit
handler (where coreState is normalized), and the GetLastChannelSessionKeyVersion
lookup to avoid forking during rollout.

In `@pkg/core/utils_test.go`:
- Around line 42-45: The "missing_0x_prefix" test currently expects an error for
a non-0x-prefixed address; update the test to accept normalization instead: set
wantErr to false for the "missing_0x_prefix" case and assert that the function
under test returns the normalized form (i.e., the input prefixed with "0x" —
"0x1234567890abcdef1234567890abcdef12345678") or otherwise matches the canonical
normalized value produced by your NormalizeAddress/NormalizeHexAddress helper;
keep the original input and only change the expectation and add the
normalization assertion.

In `@pkg/core/utils.go`:
- Around line 28-43: NormalizeHexAddress currently rejects 40-char hex addresses
without the "0x" prefix; update it to accept and canonicalize those by
lowercasing input, allowing either a 42-char string with "0x" prefix or a
40-char hex string, validating hex characters in either case and returning the
lowercased address with a "0x" prefix; adjust the validation logic in
NormalizeHexAddress to check for len==40 (no prefix) or len==42 with prefix,
validate characters starting at the appropriate index, and return the canonical
"0x"+lowercasedHex string on success.

---

Nitpick comments:
In `@clearnode/api/channel_v1/handler.go`:
- Around line 92-93: The two error-return branches that currently call
rpc.Errorf("failed to get last %s user state for transfer receiver with address
%s", senderState.Asset, receiverWallet) are dropping the underlying err; update
both failing branches (the receiver-state read branches that call rpc.Errorf) to
include the original err details in the message (e.g., pass err as an argument
or format it into the string) so rpc.Errorf includes the underlying DB error,
referencing the existing variables err, senderState.Asset, and receiverWallet in
each updated call.

In `@pkg/app/app_v1.go`:
- Around line 35-37: The error returned from core.NormalizeHexAddress when
assigning app.OwnerWallet should be wrapped using %w to preserve the error
chain; update the error return in the block that calls core.NormalizeHexAddress
(the assignment to app.OwnerWallet) so the fmt.Errorf uses "%w" and returns the
original error (err) instead of "%v", ensuring callers can use errors.Is /
errors.As on the wrapped error.

🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 570d4409-d395-40ff-b255-656f68d044d4

📥 Commits

Reviewing files that changed from the base of the PR and between 38d2ad1 and ad81ee7.

📒 Files selected for processing (7)

• clearnode/api/app_session_v1/submit_session_key_state.go
• clearnode/api/channel_v1/handler.go
• clearnode/api/channel_v1/request_creation.go
• clearnode/api/channel_v1/submit_session_key_state.go
• pkg/app/app_v1.go
• pkg/core/utils.go
• pkg/core/utils_test.go