YNU-422: do channel operations with the correct state signer #431
Conversation
WalkthroughAdds a new ChannelStatus value "resizing", propagates channel participant and resizing state into Cerebro configs, introduces participant-aware signer selection and a resize-in-progress guard in channel ops, updates SDK signer resolution and tests, and adds integration tests for session-key vs wallet flows. Changes
Sequence Diagram(s)sequenceDiagram
participant Operator as Operator
participant RPC as Clearnode RPC (GetChannels)
participant Config as ChainAssetConfig
Operator->>RPC: Request channels (no "open" filter)
RPC-->>Operator: Return channels list
Operator->>Operator: For each channel\n- ignore if status ∉ {"open","resizing"}\n- if status == "resizing": set ChannelResizing=true, record ChannelParticipant\n- if match: capture ChannelID/RawAmount
Operator->>Config: Build ChainAssetConfig with ChannelResizing & ChannelParticipant
note right of Config `#DDFFDD`: New fields added to config
sequenceDiagram
participant Ops as ChannelOps (Open/Close/Resize)
participant Resolver as SignerResolver
participant Wallet as WalletClient
participant Session as SessionKeySigner
Ops->>Resolver: Resolve participant signer (participant == wallet ? WalletStateSigner : session signer)
Resolver-->>Ops: Return participantSigner
Ops->>Ops: If config.ChannelResizing == true -> early exit (guard)
alt proceed
Ops->>CustodyAPI: Call Open/Close/Resize with Wallet + participantSigner
CustodyAPI-->>Ops: Return result
end
note right of Ops `#FFF2CC`: participant-aware signing applied to all custody calls
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Files meriting extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (4)📓 Common learnings📚 Learning: 2025-11-05T10:19:06.155ZApplied to files:
📚 Learning: 2025-11-04T19:14:48.960ZApplied to files:
📚 Learning: 2025-11-04T10:39:28.297ZApplied to files:
🧬 Code graph analysis (2)examples/cerebro/channel_ops.go (3)
examples/cerebro/operator.go (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (6)
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. Comment |
Summary of ChangesHello @philanton, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a 'resizing' status for channels to prevent concurrent resize operations. The changes span across the RPC API definition, cerebro's configuration, and operational logic. While the intent is good, I've found a logical flaw in how the 'resizing' status is being determined for an asset's channel in examples/cerebro/operator.go. The current implementation incorrectly flags a channel as resizing if any channel is resizing, not just the specific one for the asset. I've provided a suggestion to fix this. The rest of the changes look good.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
examples/cerebro/channel_ops.go (1)
72-75: Consider extracting participant signer selection logic.The participant signer selection pattern appears in three locations with minor variations. While the current implementation is clear, consider extracting to a helper method for better maintainability.
For example:
func (o *Operator) selectParticipantSigner(participantAddress string) sign.Signer { if participantAddress == o.config.Wallet.PublicKey().Address().String() { return o.config.Wallet } return o.config.Signer }Then use:
participantSigner := o.selectParticipantSigner(asset.ChannelParticipant)Also applies to: 154-157, 319-322
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/cerebro/channel_ops.go(4 hunks)examples/cerebro/config.go(1 hunks)examples/cerebro/operator.go(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:425-427
Timestamp: 2025-11-04T10:39:28.297Z
Learning: In clearnode/custody.go's handleResized function, when a resize event is received for a channel with status ChannelStatusOpen (not ChannelStatusResizing), the code intentionally logs an error but continues processing the resize operation normally. This is the expected behavior.
📚 Learning: 2025-11-05T10:19:06.155Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:449-465
Timestamp: 2025-11-05T10:19:06.155Z
Learning: In clearnode/custody.go's handleResized function during positive resize operations (deposits), the ledger flow is: on-chain funds first credit the channel account, are immediately debited from the channel account, then credit the unified wallet account. The channel account acts as a pass-through for deposits. The unified wallet account is where actual deposited funds are held. This is recorded as a deposit transaction from channelAccountID to walletAccountID.
Applied to files:
examples/cerebro/channel_ops.go
📚 Learning: 2025-11-04T19:14:48.960Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:542-553
Timestamp: 2025-11-04T19:14:48.960Z
Learning: In clearnode/custody.go's handleClosed function, when unlocking escrow funds on channel close, the code intentionally passes the channelEscrowAccountBalance (which may be negative) directly to ledger.Record for the wallet account. When the escrow balance is negative (e.g., resize confirmation received for a channel not in resizing state), this causes a debit to the unified wallet account, which is the expected behavior to equalize accounting. Temporary negative unified balances in this scenario are acceptable.
Applied to files:
examples/cerebro/channel_ops.go
📚 Learning: 2025-11-04T10:39:28.297Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:425-427
Timestamp: 2025-11-04T10:39:28.297Z
Learning: In clearnode/custody.go's handleResized function, when a resize event is received for a channel with status ChannelStatusOpen (not ChannelStatusResizing), the code intentionally logs an error but continues processing the resize operation normally. This is the expected behavior.
Applied to files:
examples/cerebro/channel_ops.go
🧬 Code graph analysis (2)
examples/cerebro/channel_ops.go (2)
clearnode/pkg/sign/sign.go (3)
Signer(11-15)PublicKey(23-26)Address(29-34)examples/cerebro/custody/custody_binding.go (1)
Channel(40-45)
examples/cerebro/operator.go (2)
clearnode/pkg/sign/sign.go (2)
PublicKey(23-26)Address(29-34)examples/cerebro/config.go (1)
ChainAssetConfig(75-84)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Publish (Clearnode)
- GitHub Check: Test (Integration) / Test Integration
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
examples/cerebro/config.go (1)
80-83: LGTM!The new fields
ChannelResizingandChannelParticipantcleanly extend the per-asset channel state tracking. The additions integrate well with the operator config loading and channel operations logic.examples/cerebro/channel_ops.go (3)
154-159: LGTM!The participant signer selection correctly uses the stored
ChannelParticipantfrom the asset config to determine whether to use the wallet or default signer for closing the channel.
214-217: LGTM - Guard prevents concurrent resize operations.The check correctly prevents initiating a resize when one is already in progress for the asset.
Note: This guard's effectiveness depends on the
ChannelResizingflag being correctly populated inoperator.go. Please verify that the flag is set accurately when loading channel state.
319-324: LGTM!The participant signer selection logic is consistent with the approach used in
OpenChannelandCloseChannel.examples/cerebro/operator.go (2)
256-256: LGTM - Fetching all channels now.Removing the "open" filter to fetch all channels is necessary to track channels in the "resizing" state.
288-296: ChainAssetConfig construction looks correct.The struct initialization properly includes the new
ChannelParticipantandChannelResizingfields. However, the correctness ofChannelResizingdepends on fixing the flag logic in lines 274-278.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
clearnode/auth.go (1)
23-24: ClarifyExpiresAtcomment to match thetime.Timetype
Challenge.SessionKeyExpiresAtis a Unix timestamp (uint64), butPolicy.ExpiresAtis atime.Timewhile its comment still says “Unix timestamp (in seconds)”. That can confuse readers/clients expecting a raw numeric timestamp from JSON.Consider rewording the
ExpiresAtcomment to something like “Expiration time (derived from a Unix timestamp)” or, if you truly want a numeric Unix value in the policy, switch the field type touint64for consistency.Also applies to: 47-54
sdk/src/client/state.ts (1)
1-18: Participant-aware signer selection is consistent and backward-compatibleThe new
_fetchParticipantAndGetSigner/_checkParticipantAndGetSignerhelpers and their integration into initial, challenge, resize, and final signing flows look coherent:
- Initial state uses
_checkParticipantAndGetSignerwithchannel.participants[0], matching the existing “first participant is the local user” assumption you already enforce with two participants.- Challenge/resize/final states pull participants from
nitroliteService.getChannelDataand gracefully fall back tostateSignerwhen the channel shape is unexpected (non-2 participants).- When the selected participant matches
walletClient.account.address, you switch toWalletStateSigner, correctly routing signatures through the wallet; otherwise, you retain the existingstateSignerbehavior, preserving pre-v0.5.0 compatibility.The only thing to keep in mind is the explicit reliance on
participants[0]as the user participant. If the participant ordering ever changes or multi-party channels are introduced, this will be the main place to revisit.Also applies to: 63-66, 82-95, 106-128, 147-168, 187-208
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
clearnode/auth.go(2 hunks)sdk/src/client/prepare.ts(1 hunks)sdk/src/client/state.ts(6 hunks)sdk/test/unit/client/index.test.ts(5 hunks)sdk/test/unit/client/state.test.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- sdk/src/client/prepare.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:425-427
Timestamp: 2025-11-04T10:39:28.297Z
Learning: In clearnode/custody.go's handleResized function, when a resize event is received for a channel with status ChannelStatusOpen (not ChannelStatusResizing), the code intentionally logs an error but continues processing the resize operation normally. This is the expected behavior.
📚 Learning: 2025-11-13T10:32:14.531Z
Learnt from: nksazonov
Repo: erc7824/nitrolite PR: 428
File: clearnode/rpc_router_auth.go:233-244
Timestamp: 2025-11-13T10:32:14.531Z
Learning: In clearnode/rpc_router_auth.go, when CheckSessionKeyExists returns ErrSessionKeyExistsAndExpired during authentication, the error should be returned to the client and block re-authentication. This is intentional behavior - expired session keys should not allow automatic re-authentication and replacement in the auth flow.
Applied to files:
clearnode/auth.go
🧬 Code graph analysis (1)
sdk/src/client/state.ts (3)
sdk/src/utils/channel.ts (1)
getChannelId(11-25)sdk/src/client/prepare.ts (1)
PreparerDependencies(39-48)sdk/src/client/types.ts (1)
ChannelId(8-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Publish (Clearnode)
- GitHub Check: Test (Integration) / Test Integration
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
sdk/test/unit/client/state.test.ts (3)
11-19: Utils mock shape looks compatible with updated state logicAdding
getPackedStateto the../../../src/utilsmock keeps the module shape broad enough for current and future uses instate.tswithout impacting these tests (which only hitgetChannelIdhere). No issues spotted.
35-49: Deps wiring for initial-state tests matches new signer resolutionProviding
walletClient(withaccountandsignMessage) andchainIdindepsaligns with the updatedPreparerDependenciesand ensures_checkParticipantAndGetSignercan safely inspect the wallet vs. session signer. The tests still exercise thestateSignerpath as intended since the first participant differs from the wallet account.
154-183: Final-state deps correctly exercise_fetchParticipantAndGetSignerThe added
walletClient,nitroliteService.getChannelDatamock, andchainIdlet_prepareAndSignFinalStateresolve the signer via_fetchParticipantAndGetSignerwithout runtime surprises. The mock channel data ensures the “non-wallet participant” branch usesstateSigner, matching the test expectations.sdk/test/unit/client/index.test.ts (3)
78-83: OverridingsharedDepsservices keeps client and state helpers alignedUpdating
client.sharedDeps.nitroliteServiceandclient.sharedDeps.erc20Serviceto the same mocks used for the client’s private services ensures all flows that go throughPreparerDependencies(e.g., state helpers) operate on the test doubles, avoiding accidental real calls. This is an important fix for the new sharedDeps-based design.
127-153:CreateChannelParamsand initial-state expectations updated correctlySwitching to
unsignedInitialStateplus a separateserverSignatureinCreateChannelParams, and having_prepareAndSignInitialStatereturn the fully signedinitialState, matches the updated API. The test correctly:
- Constructs
initialStateby augmentingunsignedInitialStatewith sigs.- Mocks
_prepareAndSignInitialStateto return{ initialState, channelId }.- Verifies that
createChannelpasses the signedinitialStateintonitroliteService.createChanneland returns it in the result.Looks solid.
Also applies to: 156-173
240-252:challengeChanneltests now cover participant-aware signingThe new
getChannelDatamocks in thechallengeChanneltests provide a realistic channel structure (participants, status, wallets, etc.), allowing_fetchParticipantAndGetSignerto pick the wallet-based signer path. As a result, asserting thatmockNitroService.challengereceivesmockSignatureas the last argument effectively verifies that the wallet signer is being used end-to-end, while the failure test still checks error propagation with the same setup.These changes line up well with the new signer-resolution logic.
Also applies to: 309-321
philanton
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
integration/tests/backward_compatibility/onchain_ops_with_sk.test.ts (4)
36-51: Global setup/teardown looks solid; consider initial Clearnode reset for full isolationThe shared
BlockchainUtils,DatabaseUtils,Identity,TestWebSocket, and auth setup are consistent and appropriate for this suite, and the finalws.close()/databaseUtils.close()cleanup is good.If you want this file to be completely self-contained regardless of what other integration tests do, you might also call
await databaseUtils.resetClearnodeState()once inbeforeAllto guarantee a clean DB/session-key cache before any scenarios run, not just after each innerdescribe.
60-125: Manual construction of legacy session‑key channel is correct; simplify initial state handlingThe way you manually construct a
Channelwithparticipants: [sessionKey, clearnode], derivechannelId, and signunsignedInitialStatewith both the session key andclearnodeSigneraccurately simulates the legacy “session key as participant” layout.One thing that could be simplified:
depositAndCreateChannelconceptually expects something akin to an unsigned/partially signed initial state plus a separateserverSignature. Passing aninitialStatethat already has bothuserSignatureandclearnodeSignatureasunsignedInitialStateis slightly confusing and may become brittle ifdepositAndCreateChannelever starts relying onsigsbeing empty or server-only. You could avoid this by:
- Keeping
unsignedInitialStatewithsigs: [](or at most the server sig, depending on the intended contract).- Letting
depositAndCreateChanneland the configuredstateSignerhandle user signing, using only theserverSignatureparameter.This would more closely mirror the RPC path used in the wallet-participant flow and reduce the chance of subtle signature handling changes breaking this test in the future.
132-267: Session‑key participant lifecycle flow is coherent but tightly couples tests via shared mutable stateThe four
itblocks here nicely exercise resize → challenge → checkpoint → close → withdraw over the same channel, and the assertions onlastValidState.version,ChannelStatus.VOID, and balances align with the intended lifecycle.The trade‑off is that
currentStateandcurrentVersionare mutated across tests, so the suite relies on them running in order and all succeeding. For integration tests this is often acceptable, but it does make failures harder to isolate and can hinder parallelization.If you ever want more robust isolation, consider either:
- Combining these into a single “full lifecycle” test, or
- Cloning the fundamental setup into independent flows per
it(at the cost of additional runtime).As a tiny nit, the hard‑coded
toRaw(BigInt(500))in the withdrawal mirrors the earlierresizeAmountof 500 USDC; reusing a shared constant would avoid the magic number duplication.
322-458: Wallet‑participant lifecycle mirrors the session‑key flow; minor nits onlyThe resize, challenge, checkpoint, close, and withdraw tests here mirror the earlier session‑key scenario and provide good coverage that operations still work end‑to‑end when the channel participant is the wallet:
- Resize uses Clearnode RPC and asserts
lastValidState.versionand balance.- Challenge/checkpoint craft states signed by the wallet and
clearnodeSigner, then submit via the client.- Close uses
closeChannelwith a final state and checks forChannelStatus.VOID.- Withdraw asserts the account balance returns to zero.
Two small optional nits:
- As above, the flow depends on
currentState/currentVersionbeing mutated acrossits; acceptable for integration, but worth being aware of for debugging and future parallelization.- There is a bit of duplication between this block and the session‑key block (version bumping, manual state construction). If the flows start to diverge more, a tiny helper for “build signed OPERATE/FINALIZE state from currentState/currentVersion” might reduce boilerplate and keep the tests easier to evolve.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
integration/common/nitroliteClient.ts(2 hunks)integration/tests/backward_compatibility/onchain_ops_with_sk.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- integration/common/nitroliteClient.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:425-427
Timestamp: 2025-11-04T10:39:28.297Z
Learning: In clearnode/custody.go's handleResized function, when a resize event is received for a channel with status ChannelStatusOpen (not ChannelStatusResizing), the code intentionally logs an error but continues processing the resize operation normally. This is the expected behavior.
🧬 Code graph analysis (1)
integration/tests/backward_compatibility/onchain_ops_with_sk.test.ts (9)
integration/common/ws.ts (2)
TestWebSocket(9-139)getCreateChannelPredicate(193-197)integration/common/identity.ts (1)
Identity(7-33)integration/common/blockchainUtils.ts (1)
BlockchainUtils(22-222)integration/common/databaseUtils.ts (1)
DatabaseUtils(7-135)integration/common/setup.ts (2)
CONFIG(4-44)chain(46-46)integration/common/auth.ts (1)
createAuthSessionWithClearnode(11-45)integration/common/nitroliteClient.ts (1)
TestNitroliteClient(28-193)sdk/src/utils/channel.ts (3)
getChannelId(11-25)convertRPCToClientState(69-81)convertRPCToClientChannel(60-67)integration/common/testHelpers.ts (1)
toRaw(21-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Publish (Clearnode)
- GitHub Check: Test (Integration) / Test Integration
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
integration/tests/backward_compatibility/onchain_ops_with_sk.test.ts (1)
276-315: Wallet‑participant scenario correctly exercises backward compatibility with a session‑key signerThis setup correctly reflects the backward‑compat use case:
- The channel is created via Clearnode RPC using
identity.messageWalletSigner, so the participant is the wallet address.TestNitroliteClientis instantiated withidentity.stateSKSigner, matching how newer clients will operate.depositAndCreateChanneluses the RPC-derivedconvertRPCToClientStateandconvertRPCToClientChannel, and the post‑creation assertion confirmsparticipants[0] === walletAddressandChannelStatus.ACTIVE.This is a good targeted scenario for validating the new signer‑resolution logic against legacy wallet‑participant channels.
philanton
force-pushed
the
fix/cerebro-resizing
branch
from
654e2d1 to
701f5b6
Compare
Summary by CodeRabbit
New Features
Bug Fixes
Breaking/SDK Changes
Tests