fix(clearnode): resolve a set of audit findings

[philanton]

Summary by CodeRabbit

• Bug Fixes

  • Wallet comparison is now case-insensitive to prevent false "same wallet" rejections.
  • Requests containing duplicate participant+asset allocations are rejected with clear error messaging.

• New Features

  • Validation of asset decimal precision for allocation amounts to enforce allowed decimals.

• Tests

  • Added tests covering duplicate-allocation rejection, deposit precision validation, rebalance duplicate handling, and same-wallet case-insensitive rejection.

[coderabbitai]

Warning

Rate limit exceeded

@philanton has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 16 minutes and 7 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 16 minutes and 7 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 790d7256-226f-40c9-a731-c7c83ffbc209

📥 Commits

Reviewing files that changed from the base of the PR and between 8343450 and c059d55.

📒 Files selected for processing (6)

• clearnode/api/app_session_v1/rebalance_app_sessions_test.go
• clearnode/api/app_session_v1/submit_app_state_test.go
• clearnode/api/app_session_v1/submit_deposit_state_test.go
• clearnode/api/channel_v1/request_creation.go
• clearnode/api/channel_v1/submit_state.go
• clearnode/api/channel_v1/submit_state_test.go

📝 Walkthrough

Walkthrough

Added duplicate-allocation validation to multiple app-session handlers, plus per-allocation asset decimal precision checks for deposits; updated channel wallet comparison to be case-insensitive. Several negative tests were added to assert rejections and prevent ledger writes on invalid input.

Changes

Cohort / File(s)	Summary
App Session — Rebalance clearnode/api/app_session_v1/rebalance_app_sessions.go, clearnode/api/app_session_v1/rebalance_app_sessions_test.go	Rejects duplicate (participant, asset) allocations in rebalance updates; new test verifies rejection and that no ledger entries are recorded.
App Session — Submit App State clearnode/api/app_session_v1/submit_app_state.go, clearnode/api/app_session_v1/submit_app_state_test.go	Added duplicate-allocation checks for operate/withdraw/close intents; tests added to assert rejection and that ledger writes are not invoked.
App Session — Submit Deposit State clearnode/api/app_session_v1/submit_deposit_state.go, clearnode/api/app_session_v1/submit_deposit_state_test.go	Per-allocation asset decimals lookup and precision validation added; duplicate-allocation detection added. Tests cover duplicate allocation rejection and invalid-decimal precision rejection.
Channel — Wallet Comparison & Tests clearnode/api/channel_v1/handler.go, clearnode/api/channel_v1/submit_state_test.go	Changed sender/receiver wallet equality check to case-insensitive (strings.EqualFold); added test ensuring TransferSend with same-wallet differing only by case is rejected and receiver persistence is not invoked.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested reviewers

• ihsraham
• dimast-x
• nksazonov

Poem

🐰 I hop through code with nimble feet,
Finding duplicates I won't let meet,
Wallets folded, case put right,
Decimals checked before the night,
Ledger sleeps — all errors beat.

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title 'fix(clearnode): resolve a set of audit findings' is vague and generic, using the non-specific term 'a set of audit findings' without describing the actual changes being made.	Consider using a more descriptive title that summarizes the main changes, such as 'fix(clearnode): validate duplicate allocations and case-insensitive wallet comparison' to convey the primary fixes being implemented.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/cn-audit-findings

---

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]

LGTM, more tests needed

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

clearnode/api/app_session_v1/submit_deposit_state_test.go (1)

803-805: Consider adding explicit rollback verification or removing the .Maybe() with stricter assertion.

The comment correctly explains that RecordLedgerEntry may be called for the first valid allocation before the duplicate is detected. However, the .Maybe() makes this test less deterministic.

Since the production code processes allocations sequentially and the first allocation (100 USDC) differs from the current amount (0), RecordLedgerEntry will be called before the duplicate check fails on the second iteration. To make the test more explicit:

-	// The first (non-duplicate) allocation may be processed before the duplicate is detected,
-	// so RecordLedgerEntry might be called for the first entry. Allow it.
-	mockStore.On("RecordLedgerEntry", participant1, appSessionID, asset, depositAmount).Return(nil).Maybe()
+	// The first allocation passes validation and triggers RecordLedgerEntry before the duplicate is detected.
+	// The transaction will be rolled back when the handler returns an error.
+	mockStore.On("RecordLedgerEntry", participant1, appSessionID, asset, depositAmount).Return(nil).Once()

This makes the test behavior deterministic and documents that the ledger write occurs but is rolled back.

🤖 Prompt for AI Agents

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

In `@clearnode/api/app_session_v1/submit_deposit_state_test.go` around lines 803 -
805, Replace the non-deterministic .Maybe() on mockStore.RecordLedgerEntry with
a strict expectation and add an explicit rollback expectation: change
mockStore.On("RecordLedgerEntry", participant1, appSessionID, asset,
depositAmount).Return(nil).Maybe() to mockStore.On("RecordLedgerEntry",
participant1, appSessionID, asset, depositAmount).Return(nil).Once() and add
mockStore.On("RollbackLedgerEntry", participant1, appSessionID, asset,
depositAmount).Return(nil).Once() (or equivalent AssertCalled/AssertExpectations
to verify RollbackLedgerEntry was invoked), so the test documents and enforces
that the ledger write happens and is subsequently rolled back.

🤖 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_state_test.go`:
- Around line 379-383: The test currently assigns receiverWallet := senderWallet
which may keep identical casing and hide regressions; explicitly change the
receiver address casing so the receiverWallet differs only by case (e.g., set
receiverWallet.Address = strings.ToUpper(senderWallet.Address) or flip a hex
letter) before calling issueTransferReceiverState so tests exercise
case-insensitive comparison (NormalizeHexAddress/strings.EqualFold). Apply the
same explicit-casing change for the other occurrence around the
receiverWallet/senderWallet block at the 443-447 region.
- Line 354: The test currently ignores errors from NewChannelDefaultSigner,
PackState, and Sign (e.g., the nodeSigner, PackState(...) and Sign(...) calls),
which can mask failures; update each ignored-error assignment to check the
returned error and fail the test on error (use t.Fatalf or require.NoError) so
failures surface immediately—specifically replace occurrences of "nodeSigner, _
:= core.NewChannelDefaultSigner(mockSigner)" and the PackState(...) and
Sign(...) calls that discard errors with proper error variables and assertions
that error == nil and include context in the failure message.

---

Nitpick comments:
In `@clearnode/api/app_session_v1/submit_deposit_state_test.go`:
- Around line 803-805: Replace the non-deterministic .Maybe() on
mockStore.RecordLedgerEntry with a strict expectation and add an explicit
rollback expectation: change mockStore.On("RecordLedgerEntry", participant1,
appSessionID, asset, depositAmount).Return(nil).Maybe() to
mockStore.On("RecordLedgerEntry", participant1, appSessionID, asset,
depositAmount).Return(nil).Once() and add mockStore.On("RollbackLedgerEntry",
participant1, appSessionID, asset, depositAmount).Return(nil).Once() (or
equivalent AssertCalled/AssertExpectations to verify RollbackLedgerEntry was
invoked), so the test documents and enforces that the ledger write happens and
is subsequently rolled back.

🪄 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: 541566c2-d851-4540-bfed-d9d00d85abb8

📥 Commits

Reviewing files that changed from the base of the PR and between 69b934a and 8343450.

📒 Files selected for processing (4)

• clearnode/api/app_session_v1/rebalance_app_sessions_test.go
• clearnode/api/app_session_v1/submit_app_state_test.go
• clearnode/api/app_session_v1/submit_deposit_state_test.go
• clearnode/api/channel_v1/submit_state_test.go