M3-L01: fix(nitronode): persist home channel challenge state#720
Merged
Conversation
Restore persistence of Status=Challenged, ChallengeExpiresAt, and StateVersion in HandleHomeChannelChallenged, and refresh the user's enforced balance. Without this, CheckOpenChannel kept passing for a disputed channel and the node could keep accepting or signing states while the contract was in DISPUTED. Auto-checkpoint stays disabled (M2-M01 policy): non-checkpointable intents (CLOSE, escrow initiate/finalize, migration) cannot be resolved via ScheduleCheckpoint, so operator action is required. Stale events where event.StateVersion < channel.StateVersion are ignored with a warning: per protocol the challenged version cannot regress below the last known on-chain version. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThe ChangesHome Channel Challenge Persistence
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
nitronode/event_handlers/service_test.go (1)
103-148: ⚡ Quick winAdd error-path tests for the two new store calls in
HandleHomeChannelChallenged.There are no tests covering
UpdateChannelreturning an error orRefreshUserEnforcedBalancereturning an error from this handler. These are new production code paths (lines 153-159 ofservice.go) introduced in this PR and are currently untested.✅ Suggested additions
func TestHandleHomeChannelChallenged_UpdateChannelError(t *testing.T) { mockStore := new(MockStore) ctx := log.SetContextLogger(context.Background(), log.NewNoopLogger()) service := &EventHandlerService{} channelID := "0xHomeChannel123" challengeExpiry := uint64(time.Now().Add(time.Hour).Unix()) channel := &core.Channel{ ChannelID: channelID, UserWallet: "0x1234567890123456789012345678901234567890", Asset: "usdc", Type: core.ChannelTypeHome, Status: core.ChannelStatusOpen, StateVersion: 3, } event := &core.HomeChannelChallengedEvent{ ChannelID: channelID, StateVersion: 4, ChallengeExpiry: challengeExpiry, } mockStore.On("GetChannelByID", channelID).Return(channel, nil) mockStore.On("UpdateChannel", mock.Anything).Return(errors.New("db error")) err := service.HandleHomeChannelChallenged(ctx, mockStore, event) require.Error(t, err) require.Contains(t, err.Error(), "db error") mockStore.AssertExpectations(t) } func TestHandleHomeChannelChallenged_RefreshBalanceError(t *testing.T) { mockStore := new(MockStore) ctx := log.SetContextLogger(context.Background(), log.NewNoopLogger()) service := &EventHandlerService{} channelID := "0xHomeChannel123" userWallet := "0x1234567890123456789012345678901234567890" challengeExpiry := uint64(time.Now().Add(time.Hour).Unix()) channel := &core.Channel{ ChannelID: channelID, UserWallet: userWallet, Asset: "usdc", Type: core.ChannelTypeHome, Status: core.ChannelStatusOpen, StateVersion: 3, } event := &core.HomeChannelChallengedEvent{ ChannelID: channelID, StateVersion: 4, ChallengeExpiry: challengeExpiry, } mockStore.On("GetChannelByID", channelID).Return(channel, nil) mockStore.On("UpdateChannel", mock.Anything).Return(nil) mockStore.On("RefreshUserEnforcedBalance", userWallet, "usdc").Return(errors.New("refresh error")) err := service.HandleHomeChannelChallenged(ctx, mockStore, event) require.Error(t, err) require.Contains(t, err.Error(), "refresh error") mockStore.AssertExpectations(t) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nitronode/event_handlers/service_test.go` around lines 103 - 148, Add two unit tests covering error paths in HandleHomeChannelChallenged: when store.UpdateChannel returns an error and when store.RefreshUserEnforcedBalance returns an error. Create TestHandleHomeChannelChallenged_UpdateChannelError that stubs GetChannelByID to return the channel and UpdateChannel to return errors.New("db error"), then assert HandleHomeChannelChallenged(ctx, mockStore, event) returns an error containing "db error". Create TestHandleHomeChannelChallenged_RefreshBalanceError that stubs GetChannelByID and UpdateChannel to succeed but stubs RefreshUserEnforcedBalance to return errors.New("refresh error"), then assert the handler returns an error containing "refresh error"; reference the EventHandlerService.HandleHomeChannelChallenged, UpdateChannel, and RefreshUserEnforcedBalance symbols and ensure mockStore.AssertExpectations(t) is called in both tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@nitronode/event_handlers/service_test.go`:
- Around line 103-148: Add two unit tests covering error paths in
HandleHomeChannelChallenged: when store.UpdateChannel returns an error and when
store.RefreshUserEnforcedBalance returns an error. Create
TestHandleHomeChannelChallenged_UpdateChannelError that stubs GetChannelByID to
return the channel and UpdateChannel to return errors.New("db error"), then
assert HandleHomeChannelChallenged(ctx, mockStore, event) returns an error
containing "db error". Create
TestHandleHomeChannelChallenged_RefreshBalanceError that stubs GetChannelByID
and UpdateChannel to succeed but stubs RefreshUserEnforcedBalance to return
errors.New("refresh error"), then assert the handler returns an error containing
"refresh error"; reference the EventHandlerService.HandleHomeChannelChallenged,
UpdateChannel, and RefreshUserEnforcedBalance symbols and ensure
mockStore.AssertExpectations(t) is called in both tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd9af022-c3ce-49fc-8d34-2f72b9cdb6e8
📒 Files selected for processing (2)
nitronode/event_handlers/service.gonitronode/event_handlers/service_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Status=Challenged,ChallengeExpiresAt, andStateVersioninHandleHomeChannelChallenged, and refresh the user's enforced balance.CheckOpenChannel(filtersstatus <= Open) kept passing for a disputed channel, so the node could keep accepting and signing states while the contract was inDISPUTED. This addresses the round-2 audit finding flagged after fix: audit findings round 2 #713 (M2-M01).ScheduleCheckpoint, so operator action is still required and a warning is emitted.event.StateVersion < channel.StateVersionare ignored with a warning — per protocol the challenged version cannot regress below the last known on-chain version.Test plan
go test ./nitronode/event_handlers/...go vet ./nitronode/event_handlers/...TestHandleHomeChannelChallenged_PersistsChallenge— assertsUpdateChannel(Status=Challenged, expiry set, version bumped) andRefreshUserEnforcedBalance, asserts no auto-checkpoint scheduled.TestHandleHomeChannelChallenged_StaleVersionIgnored— asserts no persistence when challenged version < current._ChannelNotFoundand_TypeMismatchstill pass.🤖 Generated with Claude Code
Summary by CodeRabbit