Fix/group charter issues #426
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis PR refactors W3ID resolution to use vault.ename directly instead of key service lookups, updates vault access guard with new creation pathways, enhances Cerberus detection with multi-fallback strategies and AI-driven charter summarization, renames signing service classes for clarity, introduces eVault auto-provisioning in GroupController, and refactors webhook handling with partial-update semantics and improved Firestore watcher state tracking. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ScanLogic
participant Vault
participant KeyService as Key Service<br/>(old)
rect rgb(200, 220, 240)
Note over Client,KeyService: OLD: W3ID via Key Service
Client->>ScanLogic: handleAuth()
ScanLogic->>KeyService: getPublicKey()
KeyService-->>ScanLogic: w3id
ScanLogic->>ScanLogic: add w3id to authPayload
ScanLogic-->>Client: authPayload with w3id
end
rect rgb(220, 240, 220)
Note over Client,Vault: NEW: Direct vault.ename
Client->>ScanLogic: handleAuth()
ScanLogic->>Vault: check vault.ename
Vault-->>ScanLogic: ename value
ScanLogic->>ScanLogic: ename ? proceed : skip
ScanLogic-->>Client: authPayload (no w3id field)
end
sequenceDiagram
participant Webhook as Webhook<br/>Request
participant Controller as Webhook<br/>Controller
participant Cerberus as Cerberus<br/>Service
participant OpenAI as OpenAI<br/>Service
participant DB as Database
rect rgb(240, 220, 200)
Note over Webhook,DB: Charter Created Workflow
Webhook->>Controller: POST webhook (charter event)
Controller->>Cerberus: isCerberusEnabled(charter)
Cerberus-->>Controller: false (not enabled)
Controller->>Controller: wait 10s
Controller->>Cerberus: sendAvailabilityNotification()
Cerberus-->>Controller: notification sent
Controller-->>Webhook: 200 OK
end
rect rgb(220, 240, 220)
Note over Webhook,DB: Charter Created + Cerberus Enabled
Webhook->>Controller: POST webhook (charter event)
Controller->>Cerberus: isCerberusEnabled(charter)
Cerberus-->>Controller: true
Controller->>Controller: wait 10s
Controller->>OpenAI: summarizeCharter(charterText)
OpenAI->>OpenAI: GPT-4 summarization
OpenAI-->>Controller: CharterChangeSummary
Controller->>Cerberus: analyzeCharterViolations()
Cerberus-->>Controller: analysis result
Controller->>DB: createSystemMessage(welcomeMsg)
DB-->>Controller: created
Controller-->>Webhook: 200 OK
end
sequenceDiagram
participant Client
participant GroupController as GroupController
participant GroupService
participant Provisioner as spinUpEVault
participant DB as Database
rect rgb(240, 240, 220)
Note over Client,DB: Charter Add to Charterless Group
Client->>GroupController: PATCH /group/:id<br/>(add charter)
alt group.ename exists
GroupController->>GroupService: updateGroup(id, {charter})
GroupService->>DB: update only charter
else group lacks both charter and ename
GroupController->>Provisioner: spinUpEVault(registryUrl,<br/>provisionerUrl, demoCode)
Provisioner-->>GroupController: {w3id}
GroupController->>GroupService: updateGroup(id,<br/>{charter, ename: w3id})
GroupService->>DB: update charter + ename
end
GroupService-->>GroupController: updated
GroupController-->>Client: 200 OK
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
platforms/blabsy/src/lib/context/chat-context.tsx (2)
35-35: Critical: Type definition missing description parameter.The type definition for
createNewChatdoesn't include thedescriptionparameter that was added to the implementation (lines 180-184). This creates a TypeScript type mismatch that will prevent callers from passing the description parameter.Apply this diff to fix the type definition:
- createNewChat: (participants: string[], name?: string) => Promise<string>; + createNewChat: (participants: string[], name?: string, description?: string) => Promise<string>;
180-201: Update the ChatContext interface type to match the createNewChat implementation signature.The type definition at line 35 is missing the
descriptionparameter that was added to the implementation:Current interface (line 35):
createNewChat: (participants: string[], name?: string) => Promise<string>;Should be:
createNewChat: (participants: string[], name?: string, description?: string) => Promise<string>;The implementation now accepts
descriptionas an optional parameter (lines 180-183), but the interface contract hasn't been updated. This creates a type mismatch and prevents TypeScript consumers from passing the description parameter.
🧹 Nitpick comments (11)
infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts (1)
656-677: Simplify redundant error handling.Since
w3idResultis now directly assigned fromvault.enameat line 667, the try-catch block (lines 657-677) and the fallback at line 676 are redundant. The code will either succeed withvault.enameor fail ifvaultis not available (already handled by the check at lines 651-654).Apply this diff to simplify the code:
- let voterPublicKey: string; - try { - const { created } = await globalState.keyService.ensureKey( - vault.ename, - "signing", - ); - console.log( - "Key generation result for blind voting:", - created ? "key-generated" : "key-exists", - ); - - const w3idResult = vault.ename; - if (!w3idResult) { - throw new Error("Failed to get W3ID"); - } - voterPublicKey = w3idResult; - - console.log("🔑 Voter W3ID retrieved:", voterPublicKey); - } catch (error) { - console.error("Failed to get W3ID using KeyManager:", error); - voterPublicKey = vault.ename || "unknown_public_key"; - } + // Ensure signing key exists for this vault + const { created } = await globalState.keyService.ensureKey( + vault.ename, + "signing", + ); + console.log( + "Key generation result for blind voting:", + created ? "key-generated" : "key-exists", + ); + + const voterPublicKey = vault.ename; + if (!voterPublicKey) { + throw new Error("Failed to get W3ID: vault.ename is not available"); + } + console.log("🔑 Voter W3ID retrieved:", voterPublicKey);platforms/blabsy-w3ds-auth-api/src/web3adapter/watchers/firestoreWatcher.ts (1)
382-448: LGTM! Good refactor to extract change processing logic.The
processChanges()method effectively centralizes the document change processing logic, reducing duplication and improving maintainability. The implementation correctly handles:
- Duplicate detection via
processedIdsandprocessingIds- Lock checking via
adapter.lockedIds- Parallel processing with appropriate error handling
- Cleanup of tracking sets on removal or error
platforms/cerberus/src/web3adapter/watchers/subscriber.ts (1)
210-233: Rich message/envelope logging is useful; consider gating and sensitivityThe new logs around message payloads and the envelope response are great for debugging system-message handling. Two things to keep in mind:
- This logs
data.sender,data.group, and a text excerpt; if those can contain user or group PII, it may be worth routing through your structured logger (with levels) and/or gating by environment.- If message volume grows, these console logs could become noisy in production.
Functionality-wise, the change is safe since it only runs after the existing system‑message check.
platforms/group-charter-manager-api/src/controllers/GroupController.ts (1)
4-8: Duplication of demo verification code confirmed across multiple platform servicesThe verification confirms the original concern: the demo code
d66b7138-538a-465f-a6ce-f6985854c3f4is hard-coded in at least four locations:
platforms/group-charter-manager-api/src/controllers/GroupController.ts:138(under review)platforms/dreamsync-api/src/services/PlatformEVaultService.ts:97platforms/cerberus/src/services/PlatformEVaultService.ts:97infrastructure/web3-adapter/src/index.ts:23, 104While a centralized
DEMO_CODE_W3DSenvironment variable pattern exists in test utilities and some core services, it is not consistently applied across all platform controllers. Extracting the constant to a shared location (either within web3-adapter as an export or via a consistently-applied environment variable across all platforms) would reduce maintenance burden and drift risk.platforms/group-charter-manager-api/src/controllers/CharterSigningController.ts (1)
57-107: SSE status polling remains correct but keeps a long‑lived intervalThe SSE endpoint correctly:
- Validates
sessionId,- Sends an initial status event,
- Polls
getSessionStatusevery second and emits terminal events forcompleted,expired,security_violation, or missing sessions,- Clears the interval on completion or client disconnect.
This is structurally sound for low/moderate traffic; if this endpoint sees heavy use, you may later want to consider event‑driven notifications instead of 1s polling.
platforms/group-charter-manager-api/src/services/SigningSessionService.ts (2)
38-79: Session creation logic is sound but sessions are never pruned
createSessioncorrectly:
- Generates a
sessionId,- Builds a QR payload with a redirect URI,
- Stores a
pendingsession,- Schedules a timeout to flip stale
pendingsessions toexpired.However, completed/expired sessions remain in
this.sessionsindefinitely. Under sustained usage this map can grow without bound.Consider pruning entries (e.g., deleting sessions once they become
expired/completed, or running a periodic cleanup) to keep memory usage bounded.
97-195: Signed payload processing and security check are consistent with new flow
processSignedPayload:
- Loads the session via
getSession,- Rejects missing or non‑pending sessions,
- Ensures
signature,publicKey(now effectivelyw3id), andmessageare present,- Verifies the provided identifier (after stripping
@) matchesuser.ename(also normalized),- Marks sessions with mismatched identifiers as
security_violationand returns a structured error,- Records signatures via
CharterSignatureService.recordSignature,- Marks successful sessions
completedand returns aSigningResult.This matches how the controller uses it and how SSE interprets statuses. Aside from naming (
publicKeyvs the w3id semantics), behavior looks correct.If you want to reduce confusion now that
w3idis passed in, consider renamingpublicKey→w3id(and the correspondingSigningResultfield) in a follow‑up refactor.platforms/cerberus/src/controllers/WebhookController.ts (2)
190-237: New-group creation byenamewith charter-trigger looks reasonable but is synchronousThe new flow when
localIdis missing:
- Optionally finds an existing group by
enameand just stores the mapping if found.- Otherwise creates a new group with normalized admins/participants and charter, then:
- Stores mapping,
- If
group.charterexists, awaitscerberusTriggerService.processCharterChange(...).Functionally this is solid and avoids duplicate groups when
enameis reused. The only concern is thatprocessCharterChangenow includes a 10‑second delay and OpenAI calls, so the webhook request will block on that work.Consider running
processCharterChangeasynchronously (e.g., withoutawait, or via a background job/queue) to keep webhook latency low while still triggering Cerberus behavior.
373-404: Charter signature creation, mapping, and activation analysisFor
charter_signatures:
- A new charter signature is created via
charterSignatureService.createCharterSignatureusing normalized group/user IDs and payload fields.- The local/global mapping is stored and the signature ID is locked.
analyzeCharterActivation(group.id, this.messageService)is invoked to recompute charter activation and emit any status messages.This is a good encapsulation of signature creation and downstream charter-state handling.
Please confirm that
CharterSignatureServiceexposes acreateCharterSignature({ data: ... })method with the same data shape, and thatanalyzeCharterActivationis idempotent and safe to call on every new signature (it may call OpenAI and update group state).platforms/cerberus/src/services/CerberusTriggerService.ts (2)
52-109: Expanded Cerberus enablement detection is permissive but coherent
isCerberusEnablednow:
- Looks for explicit “watchdog name” in markdown (multi‑line
Watchdog Name:\n**cerberus**).- Falls back to same‑line
watchdog name: cerberus.- Fallback 1: simple substring
'watchdog name: cerberus'.- Fallback 2: “Automated Watchdog Policy” section mentioning Cerberus within ~500 chars.
- Fallback 3: presence of both
watchdogandcerberusanywhere.Given the goal of not missing legitimate Cerberus configurations, these fallbacks make sense, but Fallback 3 in particular will treat any charter that casually mentions both terms as “enabled”. If stricter semantics are desired, you might later tighten or remove that last fallback.
156-265: Charter-change processing adds rich flows but introduces deliberate delaysThe new
processCharterChangebehavior:
- Determines
changeType(created/updated/removed).- Checks
isCerberusEnabled(groupId):
- If not enabled:
- For
created, waits 10s then sends a system message explaining how to enable Cerberus via “Watchdog Name: Cerberus”.- For other change types, simply returns.
- If enabled:
- Waits 10s.
- For
created:
- Dynamically imports
CharterSignatureServiceandOpenAIService.- Summarizes the charter, runs
analyzeCharterActivation, and posts a detailed welcome message.- For
updated/removed:
- Posts a concise change message explaining implications.
- For
updatedwith botholdCharterandnewCharter, still callshandleCharterTextChangefor deeper invalidation and analysis.Functionally this is a solid step up in UX and safety. The main trade‑off is that callers that
await processCharterChange(e.g., the webhook controller) will now be blocked for at least 10 seconds plus OpenAI latency.If webhook or HTTP response latency is a concern, consider not awaiting
processCharterChangefrom request handlers (or moving the delay/AI work to a background job/queue), while keeping this richer charter‑change logic intact.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts(3 hunks)infrastructure/evault-core/src/core/db/db.service.ts(1 hunks)infrastructure/evault-core/src/core/protocol/vault-access-guard.ts(1 hunks)infrastructure/web3-adapter/src/index.ts(3 hunks)platforms/blabsy-w3ds-auth-api/src/web3adapter/watchers/firestoreWatcher.ts(6 hunks)platforms/blabsy/src/lib/context/chat-context.tsx(1 hunks)platforms/cerberus/src/controllers/WebhookController.ts(8 hunks)platforms/cerberus/src/database/entities/Group.ts(1 hunks)platforms/cerberus/src/services/CerberusTriggerService.ts(2 hunks)platforms/cerberus/src/services/OpenAIService.ts(1 hunks)platforms/cerberus/src/web3adapter/mappings/group.mapping.json(1 hunks)platforms/cerberus/src/web3adapter/mappings/message.mapping.json(1 hunks)platforms/cerberus/src/web3adapter/watchers/subscriber.ts(1 hunks)platforms/group-charter-manager-api/src/controllers/CharterSigningController.ts(7 hunks)platforms/group-charter-manager-api/src/controllers/GroupController.ts(2 hunks)platforms/group-charter-manager-api/src/services/CharterSignatureService.ts(4 hunks)platforms/group-charter-manager-api/src/services/SigningSessionService.ts(11 hunks)platforms/group-charter-manager-api/src/web3adapter/watchers/subscriber.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
platforms/group-charter-manager-api/src/controllers/GroupController.ts (1)
infrastructure/web3-adapter/src/index.ts (2)
spinUpEVault(18-64)spinUpEVault(415-432)
platforms/group-charter-manager-api/src/services/SigningSessionService.ts (1)
platforms/group-charter-manager-api/src/services/CharterSignatureService.ts (1)
CharterSignatureService(7-142)
platforms/cerberus/src/services/CerberusTriggerService.ts (2)
platforms/cerberus/src/services/CharterSignatureService.ts (1)
CharterSignatureService(8-287)platforms/cerberus/src/services/OpenAIService.ts (1)
OpenAIService(18-296)
platforms/group-charter-manager-api/src/controllers/CharterSigningController.ts (1)
platforms/group-charter-manager-api/src/services/SigningSessionService.ts (1)
SigningSessionService(34-204)
infrastructure/web3-adapter/src/index.ts (4)
infrastructure/web3-adapter/src/mapper/mapper.types.ts (1)
IMapping(3-42)infrastructure/web3-adapter/src/db/mapping.db.ts (1)
MappingDatabase(6-186)infrastructure/web3-adapter/src/evault/evault.ts (1)
EVaultClient(131-618)infrastructure/web3-adapter/src/mapper/mapper.ts (2)
toGlobal(238-356)fromGlobal(123-208)
⏰ 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). (5)
- GitHub Check: build
- GitHub Check: test-web3-adapter-integration
- GitHub Check: test
- GitHub Check: lint
- GitHub Check: test
🔇 Additional comments (21)
platforms/blabsy/src/lib/context/chat-context.tsx (1)
120-133: Excellent null-safe sorting logic.The enhanced sorting logic properly handles edge cases where
updatedAtorcreatedAtmight be null/undefined, with appropriate fallbacks at each level. This defensive approach prevents runtime errors and ensures deterministic behavior.infrastructure/eid-wallet/src/routes/(app)/scan-qr/scanLogic.ts (1)
526-529: LGTM: Simplified W3ID resolution in signing flow.The change to use
vault.enamedirectly is consistent with the authentication flow. Note that despite the AI summary, thew3idfield is still included insignedPayloadat line 564, which maintains backward compatibility.Also applies to: 564-566
infrastructure/evault-core/src/core/db/db.service.ts (1)
555-555: Remove theparsedfield fromupdateMetaEnvelopeByIdreturn; do not add tostoreMetaEnvelope.The
StoreMetaEnvelopeResulttype definition explicitly definesmetaEnvelopewith onlyid,ontology, andaclfields—noparsed. TheupdateMetaEnvelopeByIdmethod at line 555 violates this type contract by addingparsed: meta.payload. Both methods should match the type definition; the fix is to removeparsedfromupdateMetaEnvelopeById, not add it tostoreMetaEnvelope.Likely an incorrect or invalid review comment.
platforms/blabsy-w3ds-auth-api/src/web3adapter/watchers/firestoreWatcher.ts (4)
21-22: LGTM! State tracking fields added.The new fields
watcherStartTimeandfirstSnapshotReceivedproperly support the first-snapshot filtering logic and are correctly reset instart(),reconnect(), andhandleError().
62-64: LGTM! State correctly reset on start.The watcher state is properly initialized before setting up the Firestore listener.
240-241: LGTM! Watcher state correctly reset on reconnection.The state is properly reset in both
reconnect()andhandleError()paths, ensuring that first-snapshot filtering works correctly after re-establishing the listener.Also applies to: 359-360
450-454: LGTM! Clean delegation to centralized change processing.The refactored
processSnapshot()method now properly delegates toprocessChanges(), maintaining a single responsibility and improving code clarity.platforms/cerberus/src/database/entities/Group.ts (1)
37-38: ChangingisCharterActivedefault tofalseonly affects new groupsThis will keep existing rows as-is and only make newly created groups start with an inactive charter. Please double-check that:
- Any flow that should auto-activate a charter explicitly sets
isCharterActive = true.- If you intend existing groups to become inactive until re‑evaluated, a DB migration or backfill is added elsewhere.
platforms/cerberus/src/web3adapter/mappings/message.mapping.json (1)
6-13: AddingisSystemMessagemapping looks consistentMapping
isSystemMessage→isSystemMessagematches howPostgresSubscriber.handleChangeinspects/logs this flag for messages and keeps the schema symmetrical between local and global views.platforms/cerberus/src/web3adapter/mappings/group.mapping.json (1)
6-18:ename→eNamemapping matches GroupManifest schemaRemapping the local
enamefield to the globaleNamekey aligns this mapping with theGroupManifestshape in the web3-adapter and should make identity fields consistent across components.platforms/group-charter-manager-api/src/services/CharterSignatureService.ts (1)
26-41: No functional changes in this diff blockThe changes here appear to be whitespace/formatting only; the signature recording and lookup logic is unchanged.
infrastructure/web3-adapter/src/index.ts (1)
18-63: EVault spin-up helper is cohesive and error handling is reasonableThe standalone
spinUpEVaultfunction cleanly encapsulates entropy + provision calls, basic validation of the success flag, and Axios-aware error messages. This keeps the Web3Adapter and controller code simpler while centralizing the provisioning behavior.platforms/cerberus/src/services/OpenAIService.ts (1)
95-147:summarizeCharterimplementation is consistent and safely falls backThe new
summarizeChartermethod matches the existing OpenAI patterns here: clear instructions, low temperature, and a robust fallbackCharterChangeSummaryif the response is missing or not valid JSON. The return shape aligns with theCharterChangeSummaryinterface.platforms/group-charter-manager-api/src/web3adapter/watchers/subscriber.ts (1)
222-235: No-op whitespace change inhandleChangeThe added blank line does not affect behavior; existing delayed
handleChangeflow remains intact.platforms/group-charter-manager-api/src/controllers/CharterSigningController.ts (3)
2-22: Service rename toSigningSessionServicelooks consistentImport, field type, constructor wiring, and
ensureServiceall consistently useSigningSessionService; error messaging is updated and clear.
37-49: Improved validation message for session creationThe 400 response now clearly lists the required fields (
groupId,charterData,userId), which should make client debugging easier. The rest of the flow (userId fromreq.user, delegation tocreateSession) is unchanged.
170-189: Session lookup endpoint is straightforward
getSigningSessionvalidatessessionId, returns 404 when missing, and otherwise returns the session object fromSigningSessionService. Behavior is consistent with the in‑memory session model.platforms/group-charter-manager-api/src/services/SigningSessionService.ts (2)
4-36: Session and result typing matches controller usage
SigningSessionandSigningResultinterfaces align with how the controller and SSE endpoints consume session fields and status values (pending,completed,expired,security_violation). Typing ofsessions: Map<string, SigningSession>and the injectedCharterSignatureServiceis consistent.
197-203:getSessionStatuswrapper is minimal and appropriate
getSessionStatussimply delegates togetSession, preserving the expiration behavior there. No issues here.platforms/cerberus/src/controllers/WebhookController.ts (2)
106-138: Improved group logging and admin parsing
- Additional logs for global/local IDs will help trace group webhook flows.
- Admin extraction now:
- Starts from
local.data.admins(default[]),- Filters out
null/undefined,- Normalizes relation strings to bare IDs.
This is a clear improvement over raw usage of admin refs.
257-269: System message detection handles both flag and text prefix
isSystemMessagenow considers:
local.data.isSystemMessage === true, ortextstarting with'$$system-message$$'.Combined with the later logic that allows
senderto be null only for system messages, this should correctly distinguish system vs non‑system messages coming from web3.
platforms/blabsy-w3ds-auth-api/src/web3adapter/watchers/firestoreWatcher.ts
Show resolved
Hide resolved
platforms/group-charter-manager-api/src/controllers/CharterSigningController.ts
Show resolved
Hide resolved
Description of change
Issue Number
Type of change
How the change has been tested
Change checklist
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Other Changes