YNU-922: deploy stress test v1.3.0#774
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds ChannelHub deployment artifacts for Sepolia and Base, scopes app-session key ownership lookups to specific applications via a new ChangesChannelHub deployments and script enhancements
AppSession key ownership: application scoping
DepositToNode script and batch tooling
Cerebro CLI terminal lifecycle improvements
SDK channel handling and session key lifecycle examples
🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
contracts/deployments/11155111/SessionKeyValidator.sol_SessionKeyValidator/stress-v1_3_0-2026-05-19T12-51-48.json (1)
5-5: ⚡ Quick winNormalize commit hash format across deployment artifacts.
Line 5 uses a full SHA while the rest of this Sepolia stress-v1.3.0 artifact set uses the short SHA (
df4e110a). Standardizing one format avoids brittle metadata joins/comparisons in release tooling.Proposed change
- "commit": "df4e110a38ca3a4b17e46b606cc3233bbb7521c2", + "commit": "df4e110a",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/deployments/11155111/SessionKeyValidator.sol_SessionKeyValidator/stress-v1_3_0-2026-05-19T12-51-48.json` at line 5, Update the "commit" field in the SessionKeyValidator deployment artifact to use the short SHA form used by the other Sepolia stress-v1.3.0 artifacts (replace "df4e110a38ca3a4b17e46b606cc3233bbb7521c2" with "df4e110a"); specifically edit the JSON key "commit" in the SessionKeyValidator stress-v1_3_0 artifact so the value matches the short-hash format used across the release metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contracts/broadcast/DeployChannelHub.s.sol/84532/run-1779195327198.json`:
- Around line 106-107: The first receipt in the JSON has an all-zero blockHash
while its status is 0x1 and blockNumber is populated; replace the placeholder
"0x000...000" value for the receipt's blockHash field with the actual block hash
corresponding to the provided blockNumber so the receipt object (fields
blockHash, blockNumber, status) is consistent and deterministic.
---
Nitpick comments:
In
`@contracts/deployments/11155111/SessionKeyValidator.sol_SessionKeyValidator/stress-v1_3_0-2026-05-19T12-51-48.json`:
- Line 5: Update the "commit" field in the SessionKeyValidator deployment
artifact to use the short SHA form used by the other Sepolia stress-v1.3.0
artifacts (replace "df4e110a38ca3a4b17e46b606cc3233bbb7521c2" with "df4e110a");
specifically edit the JSON key "commit" in the SessionKeyValidator stress-v1_3_0
artifact so the value matches the short-hash format used across the release
metadata.
🪄 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: 3baf5a64-cbe6-418d-9986-129d37dd2009
📒 Files selected for processing (17)
contracts/broadcast/DeployChannelHub.s.sol/11155111/run-1779194593262.jsoncontracts/broadcast/DeployChannelHub.s.sol/11155111/run-latest.jsoncontracts/broadcast/DeployChannelHub.s.sol/84532/run-1779195327198.jsoncontracts/broadcast/DeployChannelHub.s.sol/84532/run-latest.jsoncontracts/deployments/11155111/ChannelEngine.sol_ChannelEngine/stress-v1_3_0-2026-05-19T12-43-13.jsoncontracts/deployments/11155111/ChannelHub.sol_ChannelHub/stress-v1_3_0-2026-05-19T12-43-13.jsoncontracts/deployments/11155111/ECDSAValidator.sol_ECDSAValidator/stress-v1_3_0-2026-05-19T12-43-13.jsoncontracts/deployments/11155111/EscrowDepositEngine.sol_EscrowDepositEngine/stress-v1_3_0-2026-05-19T12-43-13.jsoncontracts/deployments/11155111/EscrowWithdrawalEngine.sol_EscrowWithdrawalEngine/stress-v1_3_0-2026-05-19T12-43-13.jsoncontracts/deployments/11155111/SessionKeyValidator.sol_SessionKeyValidator/stress-v1_3_0-2026-05-19T12-51-48.jsoncontracts/deployments/84532/ChannelEngine.sol_ChannelEngine/stress-v1_3_0-2026-05-19T12-55-27.jsoncontracts/deployments/84532/ChannelHub.sol_ChannelHub/stress-v1_3_0-2026-05-19T12-55-27.jsoncontracts/deployments/84532/ECDSAValidator.sol_ECDSAValidator/stress-v1_3_0-2026-05-19T12-55-27.jsoncontracts/deployments/84532/EscrowDepositEngine.sol_EscrowDepositEngine/stress-v1_3_0-2026-05-19T12-55-27.jsoncontracts/deployments/84532/EscrowWithdrawalEngine.sol_EscrowWithdrawalEngine/stress-v1_3_0-2026-05-19T12-55-27.jsoncontracts/deployments/84532/SessionKeyValidator.sol_SessionKeyValidator/stress-v1_3_0-2026-05-19T12-58-32.jsonnitronode/chart/config/stress-v1/blockchains.yaml
…ss-v1 ethereum sepolia
Resolves the chicken-and-egg failure on app session creation when the session key authorizes by application_id. The previous implementation resolved the application_id via a subquery against the app_sessions row, but during create_app_session that row does not exist yet, so the application-bound branch never matched and recovery failed with "no active session key found". Callers now pass the application_id explicitly: create_app_session reads it from the signed AppDefinitionV1, and post-create handlers read it from the loaded AppSessionV1 record. The session-id branch is preserved for keys bound to specific app sessions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
philanton
force-pushed
the
feat/stress-env-v1-3-0
branch
from
5d83a6e to
9110ba0
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
nitronode/chart/config/stress-v1/blockchains.yaml (1)
4-4: ⚡ Quick winUse EIP-55 checksummed addresses consistently.
Lines 4 and 9 use lowercase addresses while lines 6 and 11 use mixed-case EIP-55 checksummed addresses. Checksumming provides an integrity check against transcription errors and is an Ethereum best practice.
🔧 Apply EIP-55 checksumming
You can generate checksummed addresses using a tool like
eth-checksumor Web3 libraries. For example, using Python:from eth_utils import to_checksum_address # Line 4 print(to_checksum_address("0xf74c93a176794337fb43c951cc0f6cef9a6723f6")) # Line 9 print(to_checksum_address("0xeddf27e378a8b102a98a4b03a3730ef585bfaff5"))Then update the config with the checksummed versions.
Also applies to: 9-9
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nitronode/chart/config/stress-v1/blockchains.yaml` at line 4, Replace any lowercase Ethereum addresses in this YAML with their EIP-55 checksummed equivalents: locate the channel_hub_address entry and the other Ethereum address entries mentioned in the review (the lowercase addresses on the other lines) and update their values to the checksummed form (use a library like eth_utils.to_checksum_address or web3 utilities to generate them) so all addresses in the file consistently use EIP-55 casing.nitronode/store/database/interface.go (1)
222-222: ⚡ Quick winAdd a doc comment for
GetAppSessionKeyOwnerinDatabaseStore.This changed exported interface method should include a short doc comment for consistency and lintability.
As per coding guidelines:
**/*.go: Followgofmtformatting and add doc comments on all exported names in Go code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nitronode/store/database/interface.go` at line 222, Add a Go doc comment for the exported interface method GetAppSessionKeyOwner on DatabaseStore: insert a short comment immediately above the method declaration that begins with the method name and describes what the method does and what it returns (e.g., that it looks up and returns the owner ID or an error given a sessionKey, appSessionId, and applicationId). Ensure the comment follows Go doc style (starts with "GetAppSessionKeyOwner ...") and is placed directly above the GetAppSessionKeyOwner(sessionKey, appSessionId, applicationId string) (string, error) declaration.nitronode/api/app_session_v1/interface.go (1)
65-65: ⚡ Quick winAdd a doc comment for
GetAppSessionKeyOwnerin the exportedStoreinterface.Please add a brief method comment so the changed exported API stays compliant and self-documenting.
As per coding guidelines:
**/*.go: Followgofmtformatting and add doc comments on all exported names in Go code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nitronode/api/app_session_v1/interface.go` at line 65, Add a Go doc comment for the exported method GetAppSessionKeyOwner on the Store interface: place a comment line immediately above the GetAppSessionKeyOwner declaration that begins with "GetAppSessionKeyOwner" and briefly describes what the method does, its purpose, and what the returned string and error represent (e.g., retrieves the owner ID or name for the given sessionKey, appSessionId, and applicationId); ensure the comment follows standard Go doc style and is formatted with gofmt.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@nitronode/api/app_session_v1/interface.go`:
- Line 65: Add a Go doc comment for the exported method GetAppSessionKeyOwner on
the Store interface: place a comment line immediately above the
GetAppSessionKeyOwner declaration that begins with "GetAppSessionKeyOwner" and
briefly describes what the method does, its purpose, and what the returned
string and error represent (e.g., retrieves the owner ID or name for the given
sessionKey, appSessionId, and applicationId); ensure the comment follows
standard Go doc style and is formatted with gofmt.
In `@nitronode/chart/config/stress-v1/blockchains.yaml`:
- Line 4: Replace any lowercase Ethereum addresses in this YAML with their
EIP-55 checksummed equivalents: locate the channel_hub_address entry and the
other Ethereum address entries mentioned in the review (the lowercase addresses
on the other lines) and update their values to the checksummed form (use a
library like eth_utils.to_checksum_address or web3 utilities to generate them)
so all addresses in the file consistently use EIP-55 casing.
In `@nitronode/store/database/interface.go`:
- Line 222: Add a Go doc comment for the exported interface method
GetAppSessionKeyOwner on DatabaseStore: insert a short comment immediately above
the method declaration that begins with the method name and describes what the
method does and what it returns (e.g., that it looks up and returns the owner ID
or an error given a sessionKey, appSessionId, and applicationId). Ensure the
comment follows Go doc style (starts with "GetAppSessionKeyOwner ...") and is
placed directly above the GetAppSessionKeyOwner(sessionKey, appSessionId,
applicationId string) (string, error) declaration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f53e5d6c-1b1e-49b2-b88a-001e2e0257f8
📒 Files selected for processing (26)
contracts/broadcast/DeployChannelHub.s.sol/11155111/run-1779194593262.jsoncontracts/broadcast/DeployChannelHub.s.sol/11155111/run-latest.jsoncontracts/broadcast/DeployChannelHub.s.sol/84532/run-1779195327198.jsoncontracts/broadcast/DeployChannelHub.s.sol/84532/run-latest.jsoncontracts/deployments/11155111/ChannelEngine.sol_ChannelEngine/stress-v1_3_0-2026-05-19T12-43-13.jsoncontracts/deployments/11155111/ChannelHub.sol_ChannelHub/stress-v1_3_0-2026-05-19T12-43-13.jsoncontracts/deployments/11155111/ECDSAValidator.sol_ECDSAValidator/stress-v1_3_0-2026-05-19T12-43-13.jsoncontracts/deployments/11155111/EscrowDepositEngine.sol_EscrowDepositEngine/stress-v1_3_0-2026-05-19T12-43-13.jsoncontracts/deployments/11155111/EscrowWithdrawalEngine.sol_EscrowWithdrawalEngine/stress-v1_3_0-2026-05-19T12-43-13.jsoncontracts/deployments/11155111/SessionKeyValidator.sol_SessionKeyValidator/stress-v1_3_0-2026-05-19T12-51-48.jsoncontracts/deployments/84532/ChannelEngine.sol_ChannelEngine/stress-v1_3_0-2026-05-19T12-55-27.jsoncontracts/deployments/84532/ChannelHub.sol_ChannelHub/stress-v1_3_0-2026-05-19T12-55-27.jsoncontracts/deployments/84532/ECDSAValidator.sol_ECDSAValidator/stress-v1_3_0-2026-05-19T12-55-27.jsoncontracts/deployments/84532/EscrowDepositEngine.sol_EscrowDepositEngine/stress-v1_3_0-2026-05-19T12-55-27.jsoncontracts/deployments/84532/EscrowWithdrawalEngine.sol_EscrowWithdrawalEngine/stress-v1_3_0-2026-05-19T12-55-27.jsoncontracts/deployments/84532/SessionKeyValidator.sol_SessionKeyValidator/stress-v1_3_0-2026-05-19T12-58-32.jsonnitronode/api/app_session_v1/README.mdnitronode/api/app_session_v1/create_app_session.gonitronode/api/app_session_v1/handler.gonitronode/api/app_session_v1/interface.gonitronode/api/app_session_v1/testing.gonitronode/chart/config/stress-v1/blockchains.yamlnitronode/store/database/app_session_key_state.gonitronode/store/database/app_session_key_state_test.gonitronode/store/database/interface.gopkg/blockchain/evm/channel_hub_abi.go
✅ Files skipped from review due to trivial changes (14)
- contracts/deployments/84532/EscrowDepositEngine.sol_EscrowDepositEngine/stress-v1_3_0-2026-05-19T12-55-27.json
- contracts/deployments/84532/ChannelEngine.sol_ChannelEngine/stress-v1_3_0-2026-05-19T12-55-27.json
- contracts/deployments/11155111/SessionKeyValidator.sol_SessionKeyValidator/stress-v1_3_0-2026-05-19T12-51-48.json
- contracts/deployments/84532/ECDSAValidator.sol_ECDSAValidator/stress-v1_3_0-2026-05-19T12-55-27.json
- contracts/deployments/84532/EscrowWithdrawalEngine.sol_EscrowWithdrawalEngine/stress-v1_3_0-2026-05-19T12-55-27.json
- contracts/deployments/11155111/ChannelHub.sol_ChannelHub/stress-v1_3_0-2026-05-19T12-43-13.json
- contracts/deployments/84532/ChannelHub.sol_ChannelHub/stress-v1_3_0-2026-05-19T12-55-27.json
- contracts/deployments/11155111/ECDSAValidator.sol_ECDSAValidator/stress-v1_3_0-2026-05-19T12-43-13.json
- contracts/deployments/11155111/EscrowWithdrawalEngine.sol_EscrowWithdrawalEngine/stress-v1_3_0-2026-05-19T12-43-13.json
- contracts/deployments/84532/SessionKeyValidator.sol_SessionKeyValidator/stress-v1_3_0-2026-05-19T12-58-32.json
- contracts/deployments/11155111/EscrowDepositEngine.sol_EscrowDepositEngine/stress-v1_3_0-2026-05-19T12-43-13.json
- contracts/deployments/11155111/ChannelEngine.sol_ChannelEngine/stress-v1_3_0-2026-05-19T12-43-13.json
- contracts/broadcast/DeployChannelHub.s.sol/11155111/run-1779194593262.json
- contracts/broadcast/DeployChannelHub.s.sol/84532/run-1779195327198.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contracts/script/batchDepositToNode.sh`:
- Around line 175-184: The loop that checks transaction confirmations uses "cast
receipt ... || echo WARNING" which only logs a warning but still allows ok=$((ok
+ 1)) to run; change the logic so the loop sets a failure flag (e.g.
confirmed_all=true; on any failed "cast receipt" set confirmed_all=false) while
iterating hashes from jq, and after the loop only increment ok (ok=$((ok+1)))
and print the "confirmed: ..." message if confirmed_all is true; if false, print
a failure message and do not increment ok (and consider setting a nonzero
overall error indicator).
- Around line 149-162: The awk expression computing raw_amount in
batchDepositToNode.sh is vulnerable to shell injection via unquoted
$human_amount and loses precision; replace that step by validating/quoting
human_amount and invoking a small Python one-liner or heredoc using
decimal.Decimal to compute raw_amount = int(Decimal(human_amount) * (10 **
int(decimals))) to avoid injection and precision loss (use printf/echo to pass
the value, do not eval it in the shell). Also fix the confirmation logic around
cast receipt: only increment ok and treat the token as successful after cast
receipt confirms; on receipt failure, increment fail and append token to
fail_tokens (variables: human_amount, raw_amount, decimals, fail, fail_tokens,
ok and the cast receipt handling) so unconfirmed transactions are not marked
successful.
In `@contracts/script/DepositToNode.s.sol`:
- Around line 35-45: The run function currently broadcasts transactions without
validating inputs; add pre-broadcast guards: require(hub != address(0),
"hub==0"), require(token != address(0), "token==0"), require(amount > 0,
"amount==0"), and (optionally) require(Address.isContract(hub) &&
Address.isContract(token), "not contract") before vm.startBroadcast(); then move
vm.startBroadcast() below those checks so IERC20(token).forceApprove(hub,
amount) and IChannelHub(hub).depositToNode(token, amount) only run after
validation. Ensure you import/use OpenZeppelin's Address.isContract if you add
the contract-check.
🪄 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: a9f5f103-3406-40bf-a61b-46d0c8681527
📒 Files selected for processing (3)
contracts/script/DepositToNode.s.solcontracts/script/batchDepositToNode.mdcontracts/script/batchDepositToNode.sh
…er/close paths Deposit, Withdraw, Transfer, and CloseHomeChannel inspected only state == nil and state.HomeChannelID == nil to decide whether a new channel needs to be created. A previously finalized state still has both fields populated, so subsequent calls reused the closed channel instead of opening a new one. Include state.IsFinal() in the channel-absent predicate so reopens after a Finalize work correctly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…lifecycle app_sessions: rewrite requirements doc, replace WETH/USDC with YELLOW/YUSD, probe apps.v1 group via GetApps and skip RegisterApp + fail-case Step 9 when the registry is disabled, and add ensureChannelOpen() so the example only assumes a minimum off-chain balance instead of a pre-opened channel. channel_session_key: new lifecycle example that registers a channel session key, deposits/withdraws via a session-key-backed client, narrows the asset allow-list across v2/v3, and verifies revocation (v4 with empty assets) blocks every channel operation. Includes checkpointAndWait() that polls GetHomeChannel state_version after each Checkpoint so the next state transition does not race the node's on-chain event ingestion. txSigner remains the wallet's raw signer; only the channel state signer is replaced with ChannelSessionKeySignerV1. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…lifecycle app_sessions: mirror the Go example changes. Rewrite requirements doc, replace WETH/USDC with YELLOW/YUSD, probe apps.v1 via getApps() and skip registerApp + fail-case Step 9 when the registry is disabled, add an ensureChannelOpen() helper that calls acknowledge() on demand, and polyfill globalThis.WebSocket from the ws package because Node <22 does not expose a stable global WebSocket and the SDK dialer uses `new WebSocket(url)` directly. Add ws + @types/ws as deps. channel_session_key: new lifecycle example mirroring the Go counterpart. Registers a channel session key, deposits/withdraws via a session-key- backed client built with ChannelSessionKeyStateSigner, narrows the asset allow-list across v2/v3, and verifies revocation (v4 with empty assets) blocks every channel operation. Includes checkpointAndWait() that polls getHomeChannel state_version after each checkpoint so the next state transition does not race the node's on-chain event ingestion. txSigner remains an EthereumRawSigner over the wallet key; only the state signer is swapped to the session key. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
go-prompt put the tty into raw mode and emitted bracketed-paste, alt-screen
and mouse-tracking escapes that were never disabled on exit. Restoring the
saved termios via term.Restore + stty sane wasn't enough — subsequent
programs in the same tab inherited broken paste behaviour and an
unresponsive Ctrl-C until the user opened a fresh tab.
Changes:
- handleExit() now also emits the matching disable sequences for
bracketed paste, cursor visibility, alt screen and the four mouse
modes, then wipes the leftover ghost prompt + completion-menu rows
that go-prompt leaves on the screen during teardown.
- SIGINT / SIGTERM / SIGHUP handler so abnormal termination (shell
kill, window close) still restores the terminal.
- Removed OptionShowCompletionAtStart so the completion box is not
permanently visible below the prompt.
- Drove "exit" through go-prompt's ExitChecker (instead of the
operator closing exitCh during Execute) so go-prompt's own teardown
runs before main prints the farewell. Eliminates the stray
"cerebro> " line that used to appear between "Exiting..." and the
session-ended log.
- Reordered main: capture farewell, restore terminal, then print —
so the final log lands on a clean row.
Also documented in cerebro/README.md the remaining upstream go-prompt
scrollback-loss bug (c-bata/go-prompt#206) that is triggered whenever the
completion menu renders on macOS Terminal.app / iTerm2, and the plan to
replace go-prompt with chzyer/readline or peterh/liner.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Client.close() only resolved the exit promise; it never invoked rpcClient.close(), which is what actually closes the underlying WebSocket. As a result Node processes that finished their work and called client.close() hung after they were "done" because the live WebSocket kept the event loop alive. Visible in the channel_session_key example: the script reached "=== Example Complete ===" but never returned to the shell. Awaiting this.rpcClient.close() inside Client.close() lets graceful shutdown actually terminate the transport so Node can exit naturally. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
philanton
force-pushed
the
feat/stress-env-v1-3-0
branch
from
6ad48f4 to
2a27193
Compare
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
philanton
changed the title
Flagged by github-code-quality on PR #774 review. The channel_session_key lifecycle example imports ChannelSessionKeyStateSigner for the session-key client and never instantiates ChannelDefaultSigner directly — createSigners() builds the default channel signer internally. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
sdk/go/examples/app_sessions/lifecycle.go (2)
279-279:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPropagate
ctxinstead ofcontext.Background()forSubmitAppSessionKeyStateLine 279 uses
context.Background()forwallet3Client.SubmitAppSessionKeyState, which bypasses the caller’s cancellation/timeouts; pass the existing functionctxthere for consistent lifecycle control.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/go/examples/app_sessions/lifecycle.go` at line 279, Call to wallet3Client.SubmitAppSessionKeyState currently uses context.Background() which ignores the caller's cancellation/timeouts; replace context.Background() with the existing function-scoped ctx when invoking SubmitAppSessionKeyState (keep the same wallet3Client and appSessionKey3State variables) so the operation respects the caller's context lifecycle.
211-211:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle signer.Sign() errors instead of discarding with
_in sdk/go/examples/app_sessions/lifecycle.goMultiple
appSession*Signer.Sign(...)calls ignore the returnederror(e.g., at lines 211, 234, 303-304, 334-335, 370-371, 475-476, 503, 528-529, 559). If signing fails, the code continues and later usessig.String()in client calls, making failures harder to debug.Suggested fix pattern
- appSession1CreateSession1Sig, _ := appSession1Signer.Sign(session1CreateRequest) + appSession1CreateSession1Sig, err := appSession1Signer.Sign(session1CreateRequest) + if err != nil { + log.Fatalf("failed to sign session1 create request: %v", err) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/go/examples/app_sessions/lifecycle.go` at line 211, Several calls to appSessionXSigner.Sign(...) (e.g., appSession1Signer.Sign producing appSession1CreateSession1Sig) ignore the returned error; change each to capture the error (sig, err := appSession1Signer.Sign(...)), check err immediately, and handle it (return the error, log and exit, or propagate) before using sig.String(); update all similar occurrences (appSession1Signer.Sign, appSession2Signer.Sign, etc. that assign variables like appSession1CreateSession1Sig, appSession1ApproveSessionSig, appSession2CreateSessionSig, etc.) so the code does not proceed when signing fails.
🧹 Nitpick comments (1)
contracts/script/DeployChannelHub.s.sol (1)
110-120: 💤 Low valueMinor: Inconsistent label padding in console output.
The label strings have inconsistent character lengths which may cause misaligned output:
"ChannelEngine (new): "vs"ChannelEngine (existing): ""EscrowDepositEngine (new): "vs"EscrowDepositEngine (existing): "This doesn't affect functionality but could make logs harder to read.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/script/DeployChannelHub.s.sol` around lines 110 - 120, The console.log label strings for ChannelEngine, EscrowDepositEngine, and EscrowWithdrawalEngine are padded inconsistently (see the logs using deployChannelEngine, deployEscrowDeposit, deployEscrowWithdrawal and variables channelEngineAddr, escrowDepositAddr, escrowWithdrawalAddr); update the labels so both "(new)" and "(existing)" variants have equal length—either by using a single formatted label per engine and applying a fixed-width pad or by calling a pad helper (e.g., padEnd) to normalize the label strings before passing them to console.log—so the three console.log calls produce aligned, consistent output.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cerebro/main.go`:
- Around line 81-84: The code currently ignores errors from term.GetState,
term.Restore and exec.Command("stty", "sane").Run(); update the initialization
and handleExit closure to capture and handle these errors (e.g., return them,
log via a logger, or at minimum print to stderr) instead of assigning to `_` or
discarding Run()'s error: change the initialState retrieval (term.GetState) to
check and handle the error, have handleExit propagate or log the error returned
by term.Restore(int(os.Stdin.Fd()), initialState), and capture and handle the
error from exec.Command("stty", "sane").Run() so any failure during terminal
teardown is surfaced for diagnosis.
In `@contracts/script/batchDepositToNode.md`:
- Around line 93-95: Add explicit fence language identifiers (e.g., ```text) to
every unlabeled fenced code block in contracts/script/batchDepositToNode.md —
specifically the blocks containing "batchDepositToNode-20260520-143000.log", the
"=== Summary ===" sections (both variants shown), and the
"batchDepositToNode.sh" diagram; update the other occurrences mentioned (around
the ranges corresponding to the other examples) to use ```text so Markdown lint
passes and readability is improved.
In `@contracts/script/batchDepositToNode.sh`:
- Around line 89-95: The cast wallet import call is being silenced and its
failure ignored, so update the import step (the cast wallet import invocation
that sets up TMPKS/TMPPW and precedes SIGNING_ARGS and unset PK) to fail fast:
capture the exit status of the import and exit non‑zero with a clear error
message if it fails (or use a conditional/&& so subsequent lines that set
SIGNING_ARGS and unset PK only run on success), ensuring you still reference
TMPKS and TMPPW in the error context so debugging shows which keystore/password
failed.
In `@contracts/script/DepositToNode.s.sol`:
- Around line 9-11: Add a NatSpec comment for the external interface method
IChannelHub.depositToNode: above the interface declaration include a
method-level comment describing its purpose using `@notice`, document parameters
with `@param` token and `@param` amount, and add `@dev` notes that the function is
payable and any important side effects (e.g., token transfer or contract state
changes) so tooling and readers can understand expected behavior.
In `@sdk/go/examples/app_sessions/lifecycle.go`:
- Around line 73-75: Replace the hard-coded private key literals assigned to
wallet1PrivateKey, wallet2PrivateKey and wallet3PrivateKey with
environment-driven inputs: read each key from an environment variable (e.g.,
WALLET1_PRIVATE_KEY, WALLET2_PRIVATE_KEY, WALLET3_PRIVATE_KEY) and fail fast if
any are missing or empty by returning an error or logging and exiting; update
any tests/examples to document the required env vars instead of embedding
secrets.
In `@sdk/go/examples/channel_session_key/lifecycle.go`:
- Line 61: Remove the hardcoded private key assigned to walletPrivateKey in
lifecycle.go and replace it with a secure retrieval mechanism (e.g., read from
an environment variable or use a secure prompt/secret manager). Locate the
walletPrivateKey variable and change its initialization so it loads from
os.Getenv("WALLET_PRIVATE_KEY") or a provided secret accessor and validate that
the value is present, returning an error if missing; ensure no literal secret
remains in the file or repo.
In `@sdk/ts/examples/app_sessions/lifecycle.ts`:
- Around line 107-109: Replace the hard-coded private key literals
(wallet1PrivateKey, wallet2PrivateKey, wallet3PrivateKey) with values loaded
from environment variables or CLI args (e.g., process.env.WALLET1_PRIVATE_KEY
etc.), validate each is present and well-formed at startup, and throw a clear
error (fail fast) if any are missing or invalid so the example won’t run with
embedded secrets.
In `@sdk/ts/examples/channel_session_key/lifecycle.ts`:
- Line 69: Replace the hardcoded walletPrivateKey value with a runtime
environment variable: read the key from process.env (e.g. WALLET_PRIVATE_KEY),
validate it exists (throw or log a clear error if missing), and cast/assign it
to the same walletPrivateKey constant (keeping the Hex type). Update any
documentation/comments to instruct users to set WALLET_PRIVATE_KEY before
running the example; do not leave a plaintext fallback in lifecycle.ts.
---
Outside diff comments:
In `@sdk/go/examples/app_sessions/lifecycle.go`:
- Line 279: Call to wallet3Client.SubmitAppSessionKeyState currently uses
context.Background() which ignores the caller's cancellation/timeouts; replace
context.Background() with the existing function-scoped ctx when invoking
SubmitAppSessionKeyState (keep the same wallet3Client and appSessionKey3State
variables) so the operation respects the caller's context lifecycle.
- Line 211: Several calls to appSessionXSigner.Sign(...) (e.g.,
appSession1Signer.Sign producing appSession1CreateSession1Sig) ignore the
returned error; change each to capture the error (sig, err :=
appSession1Signer.Sign(...)), check err immediately, and handle it (return the
error, log and exit, or propagate) before using sig.String(); update all similar
occurrences (appSession1Signer.Sign, appSession2Signer.Sign, etc. that assign
variables like appSession1CreateSession1Sig, appSession1ApproveSessionSig,
appSession2CreateSessionSig, etc.) so the code does not proceed when signing
fails.
---
Nitpick comments:
In `@contracts/script/DeployChannelHub.s.sol`:
- Around line 110-120: The console.log label strings for ChannelEngine,
EscrowDepositEngine, and EscrowWithdrawalEngine are padded inconsistently (see
the logs using deployChannelEngine, deployEscrowDeposit, deployEscrowWithdrawal
and variables channelEngineAddr, escrowDepositAddr, escrowWithdrawalAddr);
update the labels so both "(new)" and "(existing)" variants have equal
length—either by using a single formatted label per engine and applying a
fixed-width pad or by calling a pad helper (e.g., padEnd) to normalize the label
strings before passing them to console.log—so the three console.log calls
produce aligned, consistent output.
🪄 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: 75c1b9b4-dd8a-4544-84ef-3dcdc275757c
⛔ Files ignored due to path filters (2)
sdk/ts/examples/app_sessions/package-lock.jsonis excluded by!**/package-lock.jsonsdk/ts/examples/channel_session_key/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (38)
cerebro/README.mdcerebro/main.gocerebro/operator.gocontracts/broadcast/DeployChannelHub.s.sol/11155111/run-1779278401948.jsoncontracts/broadcast/DeployChannelHub.s.sol/11155111/run-1779286321195.jsoncontracts/broadcast/DeployChannelHub.s.sol/11155111/run-latest.jsoncontracts/broadcast/DeployChannelHub.s.sol/84532/run-1779278728832.jsoncontracts/broadcast/DeployChannelHub.s.sol/84532/run-latest.jsoncontracts/deployments/11155111/ChannelEngine.sol_ChannelEngine/stress-v1_3_0-2026-05-20T12-00-01.jsoncontracts/deployments/11155111/ChannelHub.sol_ChannelHub/stress-v1_3_0-2026-05-20T14-12-01.jsoncontracts/deployments/11155111/EscrowDepositEngine.sol_EscrowDepositEngine/stress-v1_3_0-2026-05-20T12-00-01.jsoncontracts/deployments/11155111/EscrowWithdrawalEngine.sol_EscrowWithdrawalEngine/stress-v1_3_0-2026-05-20T12-00-01.jsoncontracts/deployments/84532/ChannelEngine.sol_ChannelEngine/stress-v1_3_0-2026-05-20T12-05-28.jsoncontracts/deployments/84532/ChannelHub.sol_ChannelHub/stress-v1_3_0-2026-05-20T12-05-28.jsoncontracts/deployments/84532/EscrowDepositEngine.sol_EscrowDepositEngine/stress-v1_3_0-2026-05-20T12-05-28.jsoncontracts/deployments/84532/EscrowWithdrawalEngine.sol_EscrowWithdrawalEngine/stress-v1_3_0-2026-05-20T12-05-28.jsoncontracts/script/DeployChannelHub.s.solcontracts/script/DepositToNode.s.solcontracts/script/batchDepositToNode.mdcontracts/script/batchDepositToNode.shnitronode/api/app_session_v1/README.mdnitronode/api/app_session_v1/create_app_session.gonitronode/api/app_session_v1/handler.gonitronode/api/app_session_v1/interface.gonitronode/api/app_session_v1/testing.gonitronode/chart/config/stress-v1/blockchains.yamlnitronode/store/database/app_session_key_state.gonitronode/store/database/app_session_key_state_test.gonitronode/store/database/interface.gosdk/go/channel.gosdk/go/examples/app_sessions/lifecycle.gosdk/go/examples/channel_session_key/lifecycle.gosdk/ts/examples/app_sessions/lifecycle.tssdk/ts/examples/app_sessions/package.jsonsdk/ts/examples/channel_session_key/lifecycle.tssdk/ts/examples/channel_session_key/package.jsonsdk/ts/examples/channel_session_key/tsconfig.jsonsdk/ts/src/client.ts
✅ Files skipped from review due to trivial changes (11)
- contracts/deployments/84532/ChannelHub.sol_ChannelHub/stress-v1_3_0-2026-05-20T12-05-28.json
- contracts/deployments/84532/EscrowWithdrawalEngine.sol_EscrowWithdrawalEngine/stress-v1_3_0-2026-05-20T12-05-28.json
- contracts/deployments/84532/ChannelEngine.sol_ChannelEngine/stress-v1_3_0-2026-05-20T12-05-28.json
- contracts/deployments/11155111/EscrowDepositEngine.sol_EscrowDepositEngine/stress-v1_3_0-2026-05-20T12-00-01.json
- contracts/broadcast/DeployChannelHub.s.sol/11155111/run-1779286321195.json
- contracts/deployments/11155111/ChannelEngine.sol_ChannelEngine/stress-v1_3_0-2026-05-20T12-00-01.json
- contracts/deployments/11155111/ChannelHub.sol_ChannelHub/stress-v1_3_0-2026-05-20T14-12-01.json
- contracts/deployments/84532/EscrowDepositEngine.sol_EscrowDepositEngine/stress-v1_3_0-2026-05-20T12-05-28.json
- contracts/broadcast/DeployChannelHub.s.sol/84532/run-1779278728832.json
- contracts/deployments/11155111/EscrowWithdrawalEngine.sol_EscrowWithdrawalEngine/stress-v1_3_0-2026-05-20T12-00-01.json
- contracts/broadcast/DeployChannelHub.s.sol/11155111/run-1779278401948.json
Project Go rule prohibits ignoring errors with `_`. `term.GetState`, `term.Restore`, and `stty sane` previously swallowed any failure, which could leave the user with a broken shell state and no clue what went wrong. Now log warnings on each failure path, and skip Restore when the initial state was never captured successfully. Flagged by CodeRabbit on PR #774. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CodeRabbit / Betterleaks flagged the committed private-key literals in
all four lifecycle examples as a secret-leak vector. The keys were also
the wrong default: examples should target the public sandbox endpoint,
not the internal stress-test deployment.
- sdk/go/examples/app_sessions/lifecycle.go
- sdk/go/examples/channel_session_key/lifecycle.go
- sdk/ts/examples/app_sessions/lifecycle.ts
- sdk/ts/examples/channel_session_key/lifecycle.ts
Each file now uses placeholder hex literals ("0x7d607..." etc.) the
operator must replace before running, and wsURL points at
wss://nitronode-sandbox.yellow.org/v1/ws.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
2 participants
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
Stress-env v1.3.0 cut: redeploys + supporting SDK / cerebro / examples / nitronode fixes uncovered while running stress traffic against the new ChannelHub on Ethereum Sepolia + Base Sepolia.
Contracts / deployments
feat(contracts/script): add DepositToNode batch top-up scripts— single-chain + multi-chain batch deposit tooling with dry-run + alt signing modes; README updated.feat(contracts/script): take library address params— deploy scripts now accept linked library addresses instead of redeploying them.fix(deployments): redeploy ChH with 1 min activation delay for stress envfix(deployments): redeploy ChH for stress test sepolia with correct default sig valchore(nitronode/chart): drop unused locking_contract_address for stress-v1 ethereum sepoliaNitronode
fix(nitronode/store): pass applicationId into GetAppSessionKeyOwner— fixes a chicken-and-egg failure oncreate_app_sessionwhere the owner-recovery path looked up the application via a subquery on the not-yet-insertedapp_sessionsrow. Callers now pass the application ID directly (from the signedAppDefinitionV1on create, from the loadedAppSessionV1on post-create flows). Updates theDBStorequery,Storeinterfaces (channel + app_session_v1),MockStore, README, and existing tests; adds a regression test that exercises lookup-by-application-id when the app session row does not yet exist.SDK — Go
fix(sdk/go): treat finalized state as channel-absent in create/transfer/close paths—Deposit,Withdraw,Transfer,CloseHomeChannelpreviously checked onlystate == nil || state.HomeChannelID == nilwhen deciding whether a new channel needs to be created. A finalized state still has both fields populated, so subsequent calls reused the closed channel instead of opening a new one. Predicate now also checksstate.IsFinal().SDK — TypeScript
fix(sdk/ts): tear down WebSocket in Client.close()—Client.close()only resolved its internal exit promise; it never invokedrpcClient.close(), so the WebSocket stayed open and Node never exited. Now also awaitsthis.rpcClient.close()so graceful shutdown actually terminates the transport.Examples
feat(sdk/go/examples): expand app_sessions + add channel_session_key lifecyclefeat(sdk/ts/examples): expand app_sessions + add channel_session_key lifecycleBoth:
app_sessions/lifecycle: rewrite requirements doc, swap USDC/WETH → YUSD/YELLOW for the stress env, probeapps.v1viaGetApps/getApps()and skipRegisterApp+ the unregistered-app fail-case (Step 9) when the registry is disabled, addensureChannelOpen()(auto-callsAcknowledgeif the wallet has funds but no acknowledged channel) so the example only assumes a minimum off-chain balance instead of a pre-opened channel.channel_session_key/lifecycle(new): registers a channel session key, deposits/withdraws via a session-key-backed client (ChannelSessionKeySignerV1/ChannelSessionKeyStateSigner), narrows the asset allow-list across v2/v3, verifies revocation (v4 with empty assets) blocks every channel operation. IncludescheckpointAndWait()that pollsGetHomeChannel.StateVersionafter eachCheckpointso the next state transition does not race the node's on-chain event ingestion.globalThis.WebSocketfromwsbecause Node <22 does not expose a stable global WebSocket and the SDK dialer usesnew WebSocket(url)directly.Cerebro
fix(cerebro): clean up REPL exit + restore terminal modes properly— go-prompt left the tty in raw mode and never disabled bracketed-paste / alt-screen / mouse-tracking escapes, so the next program in the same tab inherited broken paste and an unresponsive Ctrl-C.handleExitnow emits the matching disable sequences and wipes the leftover ghost prompt + completion-menu rows. Added SIGINT/SIGTERM/SIGHUP handler so abnormal termination still restores the terminal. Droveexitthrough go-prompt'sExitCheckerso the prompt's own teardown runs before the farewell is printed. Documented the remaining upstreamc-bata/go-promptscrollback-loss bug (#206) incerebro/README.mdalong with the plan to replace go-prompt withchzyer/readlineorpeterh/liner.Test plan
go test ./...cd sdk/ts && npm run test:allcd contracts && forge testgo run ./sdk/go/examples/app_sessionsagainst the stress env end-to-end.go run ./sdk/go/examples/channel_session_keyagainst the stress env end-to-end (deposits, narrowing, revocation steps all behave as expected).npm run lifecyclein bothsdk/ts/examples/app_sessionsandsdk/ts/examples/channel_session_key— both exit cleanly after=== Example Complete ===(no hang).cerebro, typeexit— single clean farewell line, no stray prompts or completion artefacts; subsequent shell commands behave normally in the same tab.Note on history rewrite
The
tempcommit was reworded tochore(nitronode/chart): drop unused locking_contract_address for stress-v1 ethereum sepoliaviagit filter-branch, so all commit SHAs from that point forward have shifted. Reviewers who already pulled the branch will need agit fetch && git reset --hard origin/feat/stress-env-v1-3-0after the force-push.Summary by CodeRabbit
New Features
Bug Fixes
Documentation