MF3-M01, M02, M03, L09: feat(app-sessions): remove rebalance feature#806
MF3-M01, M02, M03, L09: feat(app-sessions): remove rebalance feature#806philanton wants to merge 1 commit into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR removes the ChangesRebalance Feature Complete Removal
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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 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}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/api.yaml (1)
287-301:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDocument the legacy transaction-type fallback.
The PR notes say historical ledger rows with value
42now render as"unknown", but the canonicaltransaction_typeenum no longer admits either"unknown"or the deprecated legacy value. That makesget_transactionsresponses invalid for strict clients generated or validated from this schema when they encounter old rebalance rows. Please document the legacy response value here, or split request/response enums so the contract matches runtime behavior. As per coding guidelines,docs/api.yaml: API definition and canonical list of all v1 RPC methods, types, and request/response schemas is located indocs/api.yaml.🤖 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 `@docs/api.yaml` around lines 287 - 301, The API schema currently lists the canonical transaction_type enum but omits the historical fallback used at runtime, causing strict clients to fail for old ledger rows; update docs/api.yaml so the transaction_type used in get_transactions responses includes the legacy fallback (add "unknown" to the transaction_type enum used for responses) or alternatively split the type into request vs response variants and add "unknown" to the response enum only; update the transaction_type definition and any referenced get_transactions response schema to reflect this change and add a brief description noting that legacy numeric value 42 maps to "unknown".
🤖 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.
Outside diff comments:
In `@docs/api.yaml`:
- Around line 287-301: The API schema currently lists the canonical
transaction_type enum but omits the historical fallback used at runtime, causing
strict clients to fail for old ledger rows; update docs/api.yaml so the
transaction_type used in get_transactions responses includes the legacy fallback
(add "unknown" to the transaction_type enum used for responses) or alternatively
split the type into request vs response variants and add "unknown" to the
response enum only; update the transaction_type definition and any referenced
get_transactions response schema to reflect this change and add a brief
description noting that legacy numeric value 42 maps to "unknown".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d404b6a5-9002-44f9-a39a-bc0cb8da48da
⛔ Files ignored due to path filters (1)
sdk/ts/test/unit/__snapshots__/public-api-drift.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (43)
docs/api.yamldocs/data_models.mmdllms-full.txtnitronode/README.mdnitronode/api/app_session_v1/README.mdnitronode/api/app_session_v1/create_app_session_test.gonitronode/api/app_session_v1/handler.gonitronode/api/app_session_v1/rebalance_app_sessions.gonitronode/api/app_session_v1/rebalance_app_sessions_test.gonitronode/api/app_session_v1/submit_app_state_test.gonitronode/api/app_session_v1/submit_deposit_state_test.gonitronode/api/app_session_v1/submit_session_key_state_test.gonitronode/api/rpc_router.gonitronode/chart/config/prod-v1/nitronode.yaml.gotmplnitronode/chart/config/sandbox-v1/nitronode.yaml.gotmplnitronode/chart/config/stress-v1/nitronode.yaml.gotmplnitronode/main.gonitronode/metrics/exporter.gonitronode/runtime.gopkg/app/app_session_v1.gopkg/app/app_session_v1_test.gopkg/core/types.gopkg/rpc/api.gopkg/rpc/client.gopkg/rpc/methods.goplayground/mockups/history/history-tab.mdplayground/src/components/HistoryTab.tsxsdk/go/README.mdsdk/go/app_session.gosdk/go/client_test.gosdk/go/examples/app_sessions/lifecycle.gosdk/mcp/src/index.tssdk/ts/README.mdsdk/ts/examples/app_sessions/README.mdsdk/ts/examples/app_sessions/lifecycle.tssdk/ts/src/app/packing.tssdk/ts/src/app/types.tssdk/ts/src/client.tssdk/ts/src/core/types.tssdk/ts/src/rpc/api.tssdk/ts/src/rpc/client.tssdk/ts/src/rpc/methods.tssdk/ts/test/unit/rpc-drift.test.ts
💤 Files with no reviewable changes (30)
- sdk/ts/src/rpc/methods.ts
- sdk/ts/src/core/types.ts
- nitronode/api/app_session_v1/rebalance_app_sessions.go
- nitronode/api/app_session_v1/submit_session_key_state_test.go
- playground/src/components/HistoryTab.tsx
- sdk/ts/examples/app_sessions/lifecycle.ts
- nitronode/chart/config/prod-v1/nitronode.yaml.gotmpl
- pkg/rpc/api.go
- pkg/rpc/client.go
- nitronode/README.md
- nitronode/api/app_session_v1/rebalance_app_sessions_test.go
- sdk/ts/src/client.ts
- sdk/go/app_session.go
- sdk/ts/src/rpc/client.ts
- sdk/ts/src/rpc/api.ts
- sdk/ts/test/unit/rpc-drift.test.ts
- nitronode/runtime.go
- pkg/rpc/methods.go
- llms-full.txt
- sdk/go/README.md
- nitronode/chart/config/sandbox-v1/nitronode.yaml.gotmpl
- sdk/ts/src/app/types.ts
- nitronode/api/app_session_v1/submit_deposit_state_test.go
- pkg/app/app_session_v1_test.go
- sdk/ts/README.md
- sdk/go/client_test.go
- docs/data_models.mmd
- sdk/go/examples/app_sessions/lifecycle.go
- nitronode/chart/config/stress-v1/nitronode.yaml.gotmpl
- pkg/app/app_session_v1.go
nksazonov
changed the title
nksazonov
changed the title
Remove the app-session rebalancing feature end to end across the nitronode broker, shared pkg/ packages, Go and TypeScript SDKs, MCP server, protocol docs, and playground. - nitronode: delete rebalance_app_sessions handler + tests, drop the MaxSignedUpdates/MaxRebalanceSignedUpdates config and route gate - pkg/rpc: remove rebalance_app_sessions method, request/response types, and client wrapper - pkg/app: remove AppStateUpdateIntentRebalance, AppSessionVersionV1, and the GenerateRebalanceBatchIDV1/GenerateRebalanceTransactionIDV1 helpers - pkg/core: remove TransactionTypeRebalance (value 42) - sdk/go, sdk/ts: drop client methods, types, packing helpers, enum members, examples, and READMEs; regenerate drift snapshots - docs/api.yaml, data_models.mmd, llms-full.txt, MCP categorizer, playground history view: remove all rebalance references The intent (4) and transaction type (42) enum values were terminal, so removal shifts no other wire values. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
nksazonov
force-pushed
the
feat/remove-rebalancing
branch
from
8679048 to
4b8a96a
Compare
nksazonov
left a comment
nksazonov
left a comment
There was a problem hiding this comment.
Clean approach and thorough removal across all layers. However, the NewHandler signature change was partially applied — 5 NewHandler call sites and 1 struct literal still reference the removed maxSignedUpdates parameter, causing go test ./nitronode/api/app_session_v1/... to fail to compile. These must be fixed before this can land.
[Bug — compile failure] create_app_session_test.go lines 1198, 1339, 1413
TestCreateAppSession_TotalWeightsOver255, TestCreateAppSession_TotalWeightsWrapToZero, and TestCreateAppSession_QuorumExceedsTotalWeights_Rejected all pass five integer arguments to NewHandler — 32, 1024, 256, 16, 100 — but the signature now takes four (maxParticipants, maxSessionData, maxSessionKeyIDs, maxSessionKeysPerUser). The maxSignedUpdates arg (16) was not removed here during the bulk update. Compiler error: too many arguments in call to NewHandler.
Suggestion: Remove the 16 from each call so they read 32, 1024, 256, 100.
[Bug — compile failure] submit_app_state_test.go lines 2160, 2272
TestSubmitAppState_VerifyQuorumWeightOver255 and TestSubmitAppState_VerifyQuorumWeightWrapToZero have the same stale five-argument NewHandler call. Same compiler error.
Suggestion: Remove the 16 from each call so they read 32, 1024, 256, 100.
[Bug — compile failure] submit_deposit_state_test.go line 1023
TestSubmitDepositState_VerifyQuorumWeightOver255 initialises Handler via a struct literal rather than NewHandler, so it was not caught by the bulk update. maxSignedUpdates no longer exists as a field on Handler. Compiler error: unknown field maxSignedUpdates in struct literal of type Handler.
Suggestion: Remove the maxSignedUpdates: 16, line from the struct literal.
[Doc] nitronode/README.md line 34
The app_session_v1 entry in the API groups section still lists "Rebalancing" as a capability. Line 9 was correctly cleaned up, but this one was missed.
Suggestion: Change "Advanced application session management (Creation, Deposits, Rebalancing)" to remove "Rebalancing".
2 participants
Summary
Removes the app-session rebalancing feature end to end. The endpoint was already disabled in every deployed environment (
NITRONODE_MAX_SIGNED_UPDATES=0, route gated on>= 2), so this is effectively dead-code cleanup at runtime plus full removal of the API surface, SDK methods, types, and docs.Changes by layer
nitronode
rebalance_app_sessions.gohandler + its 1358-line testMaxSignedUpdates/MaxRebalanceSignedUpdatesconfig and the route gate; removeNITRONODE_MAX_SIGNED_UPDATESfrom the 3 helm configsNewHandlersignature + 35 call sitespkg/
rpc: removerebalance_app_sessionsmethod const, request/response types, client wrapperapp: removeAppStateUpdateIntentRebalance,AppSessionVersionV1,GenerateRebalanceBatchIDV1,GenerateRebalanceTransactionIDV1core: removeTransactionTypeRebalance(42)SDKs
sdk/go,sdk/ts: drop client methods, types, packing helpers, enum members, examples, READMEsrpc-driftandpublic-api-driftsnapshotsDocs / misc
docs/api.yaml,docs/data_models.mmd,llms-full.txt, MCP categorizer regex, playground history viewNotes
4) and transaction-type (42) enum values were terminal, so removal shifts no other wire values.TransactionTypeRebalance=42is removed entirely — pre-existing ledger rows tagged42(if any) now render asunknown. Chosen deliberately over keeping a display-only mapping.Verification
go vet ./...clean ·go test ./...all passtsc+ ts-compat typecheck OK🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Breaking Changes
rebalance_app_sessionsRPC endpoint for atomic multi-session rebalancingDocumentation