Fix/evault idempotency #422
Conversation
WalkthroughReplaces webhook payload w3id source with the request's Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GraphQL
participant Webhook
Note over User,GraphQL: Request carries X-ENAME -> context.eName
User->>GraphQL: storeMetaEnvelope / updateMetaEnvelopeById
activate GraphQL
GraphQL->>GraphQL: build payload (w3id = context.eName)
GraphQL->>Webhook: POST payload (w3id = user's W3ID)
deactivate GraphQL
Webhook-->>User: (async delivery acknowledged)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
infrastructure/evault-core/src/core/protocol/graphql-server.ts (1)
115-124: Remove now-unused getCurrentEvaultW3ID helper.With webhook payloads now reading
context.eName, this private helper is no longer referenced anywhere in the class and just adds noise.Apply this diff to drop the dead code:
- /** - * Gets the current eVault W3ID dynamically from the eVault instance - * @returns string | null - The current eVault W3ID - */ - private getCurrentEvaultW3ID(): string | null { - if (this.evaultInstance && this.evaultInstance.w3id) { - return this.evaultInstance.w3id; - } - return this.evaultW3ID; - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
infrastructure/evault-core/src/core/protocol/graphql-server.spec.ts(1 hunks)infrastructure/evault-core/src/core/protocol/graphql-server.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-16T18:04:28.983Z
Learnt from: coodos
Repo: MetaState-Prototype-Project/prototype PR: 99
File: infrastructure/w3id/tests/w3id.test.ts:48-57
Timestamp: 2025-04-16T18:04:28.983Z
Learning: In test files, suggestions to add defensive coding patterns (like extra null checks) are often unnecessary, as tests are designed to fail explicitly when expectations aren't met. The test in `infrastructure/w3id/tests/w3id.test.ts` validates that `id.logs` is an instance of `IDLogManager` before accessing its repository property.
Applied to files:
infrastructure/evault-core/src/core/protocol/graphql-server.spec.ts
🧬 Code graph analysis (1)
infrastructure/evault-core/src/core/protocol/graphql-server.spec.ts (1)
infrastructure/evault-core/src/test-utils/e2e-setup.ts (6)
E2ETestServer(24-35)ProvisionedEVault(37-40)setupE2ETestServer(50-157)provisionTestEVault(240-272)teardownE2ETestServer(162-235)makeGraphQLRequest(277-304)
⏰ 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). (4)
- GitHub Check: test
- GitHub Check: test-web3-adapter-integration
- GitHub Check: build
- GitHub Check: lint
infrastructure/evault-core/src/core/protocol/graphql-server.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
infrastructure/evault-core/src/core/protocol/graphql-server.spec.ts (1)
78-193: Consider extracting timeout constants for maintainability.The test logic correctly verifies that webhook payloads contain the requesting user's W3ID (from X-ENAME header) rather than the eVault's W3ID. The assertions are thorough and appropriate.
The hardcoded setTimeout delays (lines 106, 170: 3500ms) account for the implementation's 3-second webhook delay. Consider extracting these to named constants for better maintainability:
const WEBHOOK_DELIVERY_TIMEOUT = 3500; // 3s implementation delay + bufferOptional: The webhook call-finding logic (lines 112-115, 173-175) is duplicated. You could extract it to a helper function, though the duplication is acceptable in test code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
infrastructure/evault-core/src/core/protocol/graphql-server.spec.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-16T18:04:28.983Z
Learnt from: coodos
Repo: MetaState-Prototype-Project/prototype PR: 99
File: infrastructure/w3id/tests/w3id.test.ts:48-57
Timestamp: 2025-04-16T18:04:28.983Z
Learning: In test files, suggestions to add defensive coding patterns (like extra null checks) are often unnecessary, as tests are designed to fail explicitly when expectations aren't met. The test in `infrastructure/w3id/tests/w3id.test.ts` validates that `id.logs` is an instance of `IDLogManager` before accessing its repository property.
Applied to files:
infrastructure/evault-core/src/core/protocol/graphql-server.spec.ts
🧬 Code graph analysis (1)
infrastructure/evault-core/src/core/protocol/graphql-server.spec.ts (1)
infrastructure/evault-core/src/test-utils/e2e-setup.ts (5)
E2ETestServer(24-35)ProvisionedEVault(37-40)setupE2ETestServer(50-157)teardownE2ETestServer(162-235)makeGraphQLRequest(277-304)
⏰ 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). (2)
- GitHub Check: build
- GitHub Check: test
🔇 Additional comments (3)
infrastructure/evault-core/src/core/protocol/graphql-server.spec.ts (3)
1-23: LGTM! Clean test setup.The test structure is well-organized with proper imports, original axios function preservation, and clear test suite variables. Storing the original axios implementations before spying (lines 12-14) is excellent defensive practice.
24-76: Excellent spy implementations and test isolation.The lifecycle hooks are properly structured:
- beforeAll sets up the expensive E2E infrastructure once
- beforeEach ensures fresh spy state for each test
- afterAll cleans up comprehensively
The axios spy implementations correctly preserve arguments and context:
- Line 53-61: GET spy uses rest parameters and
applyto forward all arguments- Line 65-75: POST spy explicitly handles the three-parameter signature
The previous concern about argument preservation has been fully addressed.
195-272: LGTM! Proper test isolation for update flow.The test correctly validates that update webhook payloads also contain the user's W3ID from the request context. Good practices observed:
- Line 225:
mockClear()properly isolates create vs update webhook calls- Line 251: Shorter timeout (500ms) is appropriate since updates don't have the setTimeout delay
- Assertions comprehensively verify envelope ID, data, ontology, and W3ID
Description of change
fix the issue with eVault core sending out the same W3ID in webhook payloads
Issue Number
Type of change
How the change has been tested
Change checklist
Summary by CodeRabbit
Bug Fixes
Tests