feat(rpc): createCommunity include:["started"] fast path for community-list started lookups#176
Open
Rinse12 wants to merge 1 commit into
Open
feat(rpc): createCommunity include:["started"] fast path for community-list started lookups#176Rinse12 wants to merge 1 commit into
Rinse12 wants to merge 1 commit into
Conversation
A thin RPC client that only needs each community's `started` flag (e.g. a
CLI `community list`) previously instantiated every community via the full
subscribe -> update() -> wait-for-first-update -> stop() cycle, which is O(N)
blocking round-trips that also serialize each full community record.
Add an optional `include` field-selection arg to createCommunity. When
`include: ["started"]`, the RPC server short-circuits to an in-memory
started lookup (findStartedCommunity) and returns { address, started } in a
single request/response - no DB load, no instantiation, no subscription.
The public createCommunity fast-paths on this and falls back to the full
cycle against an old daemon (version skew). `pkc.communities` is unchanged
(stays string[]). `include` is also honored on communityUpdateSubscribe.
Alternative to #141.
📝 WalkthroughWalkthroughThis PR adds an optional Changesinclude:['started'] fast path
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant PKCWithRpcClient
participant PKCRpcClient
participant RpcServer
Caller->>PKCWithRpcClient: createCommunity({ address, include: ["started"] })
PKCWithRpcClient->>PKCRpcClient: createCommunity({ address, include: ["started"] })
PKCRpcClient->>RpcServer: RPC createCommunity(include=["started"])
RpcServer->>RpcServer: lookup started registry (in-memory)
RpcServer-->>PKCRpcClient: { address, started }
PKCRpcClient-->>PKCWithRpcClient: RpcCommunityStartedResult
PKCWithRpcClient->>PKCWithRpcClient: set community.started
PKCWithRpcClient-->>Caller: RpcLocalCommunity (started only, no subscription)
alt fast path fails
PKCWithRpcClient->>PKCRpcClient: createCommunity() (full path)
PKCRpcClient->>RpcServer: RPC createCommunity + communityUpdateSubscribe
RpcServer-->>PKCRpcClient: update notification
PKCRpcClient-->>PKCWithRpcClient: full community record
PKCWithRpcClient-->>Caller: RpcLocalCommunity (fully subscribed)
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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. Comment |
![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
🤖 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 `@src/rpc/src/index.ts`:
- Around line 629-639: The fast-path in createCommunity is returning a derived
address from parsedIdentifier instead of the canonical community.address. Update
the include/startured-only branch to use the actual community record returned by
findStartedCommunity (or otherwise fetch the started community object) and
return its immutable address field directly, rather than rebuilding it from
params. Keep the existing parseRpcCommunityIdentifierParam and _getPKCInstance
flow, but ensure the result object is populated from the canonical
community.address.
- Around line 1164-1174: The fast-path in this subscription handler is deriving
`address` from `parsedArgs.name`/`parsedArgs.publicKey`, which can diverge from
the canonical community address. Update this branch to use the actual found
community’s immutable `address` (and `started`) from the result of
`findStartedCommunity(pkc, parsedArgs)`, and avoid any fallback-derived address.
Prefer extracting a shared private helper such as
`_resolveStartedOnlyResult(lookup)` that both this path and `createCommunity`’s
fast path can call so the `sendEvent("update", ...)` payload always uses the
canonical address.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2ca5143b-fc22-424e-8066-68a40e6869db
📒 Files selected for processing (10)
src/clients/rpc-client/pkc-rpc-client.tssrc/clients/rpc-client/rpc-schema-util.tssrc/clients/rpc-client/schema.tssrc/clients/rpc-client/types.tssrc/community/schema.tssrc/community/types.tssrc/pkc/pkc-with-rpc-client.tssrc/rpc/src/index.tssrc/rpc/test/node/rpc.listeners.test.tstest/node/community/include-started.community.test.ts
Comment on lines
+629
to
+639
| async createCommunity(params: any): Promise<RpcInternalCommunityRecordBeforeFirstUpdateType | RpcCommunityStartedResult> { | ||
| // Fast path: a caller that only wants cheap in-memory fields (currently just `started`) | ||
| // passes `include`. We read the started flag from the in-memory registry — no DB load, | ||
| // no community instantiation, no subscription. See pkc-js issue #175 (alternative to #141). | ||
| const includeFields = params[0]?.include; | ||
| if (Array.isArray(includeFields) && includeFields.length > 0 && includeFields.every((f: unknown) => f === "started")) { | ||
| const parsedIdentifier = parseRpcCommunityIdentifierParam(params[0]); | ||
| const pkcInstance = await this._getPKCInstance(); | ||
| const started = Boolean(findStartedCommunity(pkcInstance, parsedIdentifier)); | ||
| return { address: parsedIdentifier.name ?? parsedIdentifier.publicKey!, started }; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fast-path result uses a derived address instead of the canonical community.address.
findStartedCommunity's result is discarded except for its truthiness; address is rebuilt from the request identifier (parsedIdentifier.name ?? parsedIdentifier.publicKey) instead of the actual community's immutable .address. If the identifier used for lookup ever differs from the community's current canonical address, the fast path returns a stale/wrong address to the client.
As per coding guidelines: "author.address and community.address are immutable — never override or fall back to a derived address."
🛠️ Proposed fix using the canonical address
const includeFields = params[0]?.include;
if (Array.isArray(includeFields) && includeFields.length > 0 && includeFields.every((f: unknown) => f === "started")) {
const parsedIdentifier = parseRpcCommunityIdentifierParam(params[0]);
const pkcInstance = await this._getPKCInstance();
- const started = Boolean(findStartedCommunity(pkcInstance, parsedIdentifier));
- return { address: parsedIdentifier.name ?? parsedIdentifier.publicKey!, started };
+ const startedCommunity = findStartedCommunity(pkcInstance, parsedIdentifier);
+ return {
+ address: startedCommunity?.address ?? parsedIdentifier.name ?? parsedIdentifier.publicKey!,
+ started: Boolean(startedCommunity)
+ };
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async createCommunity(params: any): Promise<RpcInternalCommunityRecordBeforeFirstUpdateType | RpcCommunityStartedResult> { | |
| // Fast path: a caller that only wants cheap in-memory fields (currently just `started`) | |
| // passes `include`. We read the started flag from the in-memory registry — no DB load, | |
| // no community instantiation, no subscription. See pkc-js issue #175 (alternative to #141). | |
| const includeFields = params[0]?.include; | |
| if (Array.isArray(includeFields) && includeFields.length > 0 && includeFields.every((f: unknown) => f === "started")) { | |
| const parsedIdentifier = parseRpcCommunityIdentifierParam(params[0]); | |
| const pkcInstance = await this._getPKCInstance(); | |
| const started = Boolean(findStartedCommunity(pkcInstance, parsedIdentifier)); | |
| return { address: parsedIdentifier.name ?? parsedIdentifier.publicKey!, started }; | |
| } | |
| async createCommunity(params: any): Promise<RpcInternalCommunityRecordBeforeFirstUpdateType | RpcCommunityStartedResult> { | |
| // Fast path: a caller that only wants cheap in-memory fields (currently just `started`) | |
| // passes `include`. We read the started flag from the in-memory registry — no DB load, | |
| // no community instantiation, no subscription. See pkc-js issue `#175` (alternative to `#141`). | |
| const includeFields = params[0]?.include; | |
| if (Array.isArray(includeFields) && includeFields.length > 0 && includeFields.every((f: unknown) => f === "started")) { | |
| const parsedIdentifier = parseRpcCommunityIdentifierParam(params[0]); | |
| const pkcInstance = await this._getPKCInstance(); | |
| const startedCommunity = findStartedCommunity(pkcInstance, parsedIdentifier); | |
| return { | |
| address: startedCommunity?.address ?? parsedIdentifier.name ?? parsedIdentifier.publicKey!, | |
| started: Boolean(startedCommunity) | |
| }; | |
| } |
🤖 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 `@src/rpc/src/index.ts` around lines 629 - 639, The fast-path in
createCommunity is returning a derived address from parsedIdentifier instead of
the canonical community.address. Update the include/startured-only branch to use
the actual community record returned by findStartedCommunity (or otherwise fetch
the started community object) and return its immutable address field directly,
rather than rebuilding it from params. Keep the existing
parseRpcCommunityIdentifierParam and _getPKCInstance flow, but ensure the result
object is populated from the canonical community.address.
Source: Coding guidelines
Comment on lines
+1164
to
+1174
|
|
||
| // Fast path: a subscriber that only wants the cheap in-memory `started` flag passes `include`. | ||
| // Emit an immediate started-only "update" and skip creating/binding a real community — no DB | ||
| // load, no update wait. See pkc-js issue #175 (alternative to #141). | ||
| if (Array.isArray(parsedArgs.include) && parsedArgs.include.length > 0 && parsedArgs.include.every((f) => f === "started")) { | ||
| const started = Boolean(findStartedCommunity(pkc, parsedArgs)); | ||
| sendEvent("update", { address: parsedArgs.name ?? parsedArgs.publicKey!, started }); | ||
| this.subscriptionCleanups[connectionId][subscriptionId] = async () => {}; // nothing created — no-op cleanup | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Same derived-address issue as createCommunity's fast path (Lines 629-639).
Consider extracting a shared private helper (e.g. _resolveStartedOnlyResult(lookup)) that both call sites use to compute { address, started } from the actual found community, so this can't diverge again.
As per coding guidelines: "author.address and community.address are immutable — never override or fall back to a derived address."
🛠️ Proposed fix using the canonical address
if (Array.isArray(parsedArgs.include) && parsedArgs.include.length > 0 && parsedArgs.include.every((f) => f === "started")) {
- const started = Boolean(findStartedCommunity(pkc, parsedArgs));
- sendEvent("update", { address: parsedArgs.name ?? parsedArgs.publicKey!, started });
+ const startedCommunity = findStartedCommunity(pkc, parsedArgs);
+ sendEvent("update", {
+ address: startedCommunity?.address ?? parsedArgs.name ?? parsedArgs.publicKey!,
+ started: Boolean(startedCommunity)
+ });
this.subscriptionCleanups[connectionId][subscriptionId] = async () => {}; // nothing created — no-op cleanup
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Fast path: a subscriber that only wants the cheap in-memory `started` flag passes `include`. | |
| // Emit an immediate started-only "update" and skip creating/binding a real community — no DB | |
| // load, no update wait. See pkc-js issue #175 (alternative to #141). | |
| if (Array.isArray(parsedArgs.include) && parsedArgs.include.length > 0 && parsedArgs.include.every((f) => f === "started")) { | |
| const started = Boolean(findStartedCommunity(pkc, parsedArgs)); | |
| sendEvent("update", { address: parsedArgs.name ?? parsedArgs.publicKey!, started }); | |
| this.subscriptionCleanups[connectionId][subscriptionId] = async () => {}; // nothing created — no-op cleanup | |
| return; | |
| } | |
| if (Array.isArray(parsedArgs.include) && parsedArgs.include.length > 0 && parsedArgs.include.every((f) => f === "started")) { | |
| const startedCommunity = findStartedCommunity(pkc, parsedArgs); | |
| sendEvent("update", { | |
| address: startedCommunity?.address ?? parsedArgs.name ?? parsedArgs.publicKey!, | |
| started: Boolean(startedCommunity) | |
| }); | |
| this.subscriptionCleanups[connectionId][subscriptionId] = async () => {}; // nothing created — no-op cleanup | |
| return; | |
| } |
🤖 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 `@src/rpc/src/index.ts` around lines 1164 - 1174, The fast-path in this
subscription handler is deriving `address` from
`parsedArgs.name`/`parsedArgs.publicKey`, which can diverge from the canonical
community address. Update this branch to use the actual found community’s
immutable `address` (and `started`) from the result of
`findStartedCommunity(pkc, parsedArgs)`, and avoid any fallback-derived address.
Prefer extracting a shared private helper such as
`_resolveStartedOnlyResult(lookup)` that both this path and `createCommunity`’s
fast path can call so the `sendEvent("update", ...)` payload always uses the
canonical address.
Source: Coding guidelines
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.
Closes #175. Alternative to #141.
Problem
A thin RPC client that only needs each community's
startedflag (e.g. the CLIcommunity list) previously instantiated every community via the fullsubscribe -> update() -> wait-for-first-update -> stop()cycle. That's O(N) blocking round-trips that also serialize each full community record. Measured ~10-20s for 17 communities.The
startedflag needs zero DB load — the daemon already holds it in memory (findStartedCommunity).Change
Add an optional
includefield-selection arg tocreateCommunity:createCommunity({ address, include: ["started"] })takes a fast path: a singlecreateCommunityRPC request/response where the server readsstartedfrom its in-memory registry and returns{ address, started }— no DB load, no instantiation, no subscription.includekey, which the client catches and retries the normal way.includeis also honored oncommunityUpdateSubscribe(emits an immediate started-only update).pkc.communitiesis unchanged (staysstring[]) — this is the key difference from feat(rpc): carry per-community state in the communities subscription (pkc.communities -> {address, started}[]) so RPC clients don't instantiate every community #141, which changed its element type.startedis the only cheap in-memory field today; theincludeenum is extensible for future cheap fields.Tests
test/node/community/include-started.community.test.ts(RPC-only): assertsstartedtrue/false across start/stop and that the fast path opens no update subscription (spiescommunityUpdateSubscribe), unlike the full path.startedresults; the production win is expected larger since it also skips full-record serialization.Verification
npm run buildclean;npx tsc --project test/tsconfig.json --noEmitclean.started-communitiesand the touchedrpc.listenersserver test pass (no regression).Summary by CodeRabbit
New Features
Bug Fixes