feat: implement encryption and access control features#28
Merged
Patrick-Ehimen merged 2 commits intomainfrom Oct 23, 2025
Merged
feat: implement encryption and access control features#28Patrick-Ehimen merged 2 commits intomainfrom
Patrick-Ehimen merged 2 commits intomainfrom
Conversation
Reviewer's GuideThis PR integrates the Kavach SDK to provide threshold‐based encryption and smart contract–driven access control across both the SDK wrapper and the MCP server. It introduces an EncryptionManager in the SDK layer with new methods and event forwarding, updates type definitions, extends the server service and tools to expose key generation and access control setup, and adds corresponding tests and dependencies while preserving existing functionality. Sequence diagram for encryption key generation via MCP toolsequenceDiagram
participant User
participant MCPServer
participant LighthouseGenerateKeyTool
participant LighthouseService
participant LighthouseAISDK
participant EncryptionManager
User->>MCPServer: Call lighthouse_generate_key tool (threshold, keyCount)
MCPServer->>LighthouseGenerateKeyTool: execute(args)
LighthouseGenerateKeyTool->>LighthouseService: generateEncryptionKey(threshold, keyCount)
LighthouseService->>LighthouseAISDK: generateEncryptionKey(threshold, keyCount)
LighthouseAISDK->>EncryptionManager: generateKey(threshold, keyCount)
EncryptionManager-->>LighthouseAISDK: GeneratedKey
LighthouseAISDK-->>LighthouseService: GeneratedKey
LighthouseService-->>LighthouseGenerateKeyTool: { success, data }
LighthouseGenerateKeyTool-->>MCPServer: ProgressAwareToolResult
MCPServer-->>User: Return key shards
Sequence diagram for access control setup via MCP toolsequenceDiagram
participant User
participant MCPServer
participant LighthouseSetupAccessControlTool
participant LighthouseService
participant LighthouseAISDK
participant EncryptionManager
User->>MCPServer: Call lighthouse_setup_access_control tool (address, cid, conditions, keyShards, authToken)
MCPServer->>LighthouseSetupAccessControlTool: execute(args)
LighthouseSetupAccessControlTool->>LighthouseService: setupAccessControl(config, authToken)
LighthouseService->>LighthouseAISDK: setupAccessControl(config, authToken)
LighthouseAISDK->>EncryptionManager: setupAccessControl(config, authToken)
EncryptionManager-->>LighthouseAISDK: EncryptionResponse
LighthouseAISDK-->>LighthouseService: EncryptionResponse
LighthouseService-->>LighthouseSetupAccessControlTool: { success }
LighthouseSetupAccessControlTool-->>MCPServer: ProgressAwareToolResult
MCPServer-->>User: Return setup status
Class diagram for new EncryptionManager and SDK integrationclassDiagram
class LighthouseAISDK {
- progress: ProgressTracker
- errorHandler: ErrorHandler
- circuitBreaker: CircuitBreaker
- encryption: EncryptionManager
- config: LighthouseConfig
+ generateEncryptionKey(threshold, keyCount): Promise<GeneratedKey>
+ shardEncryptionKey(masterKey, threshold, keyCount): Promise<keyShards: KeyShard[]>
+ setupAccessControl(config, authToken): Promise<EncryptionResponse>
+ recoverEncryptionKey(keyShards): Promise<masterKey: string | null; error: string | null>
+ shareFileAccess(cid, ownerAddress, shareToAddress, authToken): Promise<EncryptionResponse>
+ getEncryptionAuthMessage(address): Promise<message: string | null; error: string | null>
+ getEncryptionJWT(address, signedMessage): Promise<string | null>
+ isEncryptionAvailable(): boolean
+ destroy(): void
}
class EncryptionManager {
+ isAvailable(): boolean
+ generateKey(threshold, keyCount): Promise<GeneratedKey>
+ shardKey(masterKey, threshold, keyCount): Promise<keyShards: KeyShard[]>
+ saveKeyShards(address, cid, authToken, keyShards, sharedTo): Promise<EncryptionResponse>
+ setupAccessControl(config, authToken): Promise<EncryptionResponse>
+ recoverKey(keyShards): Promise<masterKey: string | null; error: string | null>
+ getAuthMessage(address): Promise<message: string | null; error: string | null>
+ getJWT(address, signedMessage): Promise<string | null>
+ shareToAddress(cid, address, shareToAddress, authToken): Promise<EncryptionResponse>
+ destroy(): void
}
LighthouseAISDK --> EncryptionManager
class GeneratedKey {
masterKey: string | null
keyShards: KeyShard[]
}
class KeyShard {
key: string
index: string
}
class AccessControlConfig {
address: string
cid: string
conditions: EnhancedAccessCondition[]
aggregator: string
chainType: ChainType
keyShards: KeyShard[]
decryptionType: DecryptionType
}
class EncryptionResponse {
isSuccess: boolean
error: string | null
keyShards: KeyShard[]
masterKey: string
}
EncryptionManager --> GeneratedKey
GeneratedKey --> KeyShard
EncryptionManager --> EncryptionResponse
EncryptionManager --> AccessControlConfig
class EnhancedAccessCondition
class EVMAccessCondition {
id: number
standardContractType: string
contractAddress: string
chain: string
method: string
parameters: any[]
returnValueTest: ReturnValueTest
inputArrayType: string[]
outputType: string
}
class SolanaAccessCondition {
id: number
contractAddress: string
chain: string
method: string
standardContractType: string
parameters: any[]
pdaInterface: object
returnValueTest: ReturnValueTest
}
EnhancedAccessCondition <|-- EVMAccessCondition
EnhancedAccessCondition <|-- SolanaAccessCondition
class ReturnValueTest {
comparator: string
value: any
}
EVMAccessCondition --> ReturnValueTest
SolanaAccessCondition --> ReturnValueTest
Class diagram for MCP server service and new toolsclassDiagram
class ILighthouseService {
+ generateEncryptionKey(threshold, keyCount): Promise
+ setupAccessControl(config, authToken): Promise
}
class LighthouseService {
+ generateEncryptionKey(threshold, keyCount): Promise
+ setupAccessControl(config, authToken): Promise
}
class MockLighthouseService {
+ generateEncryptionKey(threshold, keyCount): Promise
+ setupAccessControl(config, authToken): Promise
}
ILighthouseService <|.. LighthouseService
ILighthouseService <|.. MockLighthouseService
class LighthouseGenerateKeyTool {
+ execute(args): ProgressAwareToolResult
+ static getDefinition(): MCPToolDefinition
}
class LighthouseSetupAccessControlTool {
+ execute(args): ProgressAwareToolResult
+ static getDefinition(): MCPToolDefinition
}
LighthouseGenerateKeyTool --> ILighthouseService
LighthouseSetupAccessControlTool --> ILighthouseService
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `packages/sdk-wrapper/src/encryption/EncryptionManager.ts:96-100` </location>
<code_context>
+
+ const result = await kavach.shardKey(masterKey, threshold, keyCount);
+
+ if (!result || !result.isShardable || !result.keyShards) {
+ throw new Error("Failed to shard encryption key");
+ }
</code_context>
<issue_to_address>
**suggestion:** Error handling for shardKey may be too generic.
Consider using distinct error messages for unshardable keys and missing shards to improve diagnosability.
```suggestion
const result = await kavach.shardKey(masterKey, threshold, keyCount);
if (!result) {
throw new Error("No result returned from shardKey");
}
if (!result.isShardable) {
throw new Error("Master key is not shardable");
}
if (!result.keyShards) {
throw new Error("Key shards missing from shardKey result");
}
```
</issue_to_address>
### Comment 2
<location> `packages/sdk-wrapper/src/encryption/EncryptionManager.ts:136-140` </location>
<code_context>
+
+ const result = await kavach.saveShards(address, cid, authToken, keyShards, sharedTo);
+
+ if (!result) {
+ throw new Error("Failed to save key shards");
+ }
</code_context>
<issue_to_address>
**suggestion:** Null result checks may not cover all error cases.
If the SDK provides structured error objects, handle those directly to improve error reporting and propagation.
```suggestion
const result = await kavach.saveShards(address, cid, authToken, keyShards, sharedTo);
if (result && result.error) {
// Handle structured error from SDK
throw new Error(`Failed to save key shards: ${result.error.message || JSON.stringify(result.error)}`);
}
if (!result) {
throw new Error("Failed to save key shards: No result returned from SDK");
}
```
</issue_to_address>
### Comment 3
<location> `apps/mcp-server/src/tests/encryption-tools.test.ts:4` </location>
<code_context>
+ this.lighthouseService,
+ this.logger,
+ );
// Register lighthouse_upload_file tool
this.registry.register(
</code_context>
<issue_to_address>
**issue (testing):** Missing encryption and access control tool tests.
Please add integration and edge case tests for `lighthouse_generate_key` and `lighthouse_setup_access_control`, covering invalid parameters, error handling, and success/failure scenarios.
</issue_to_address>
### Comment 4
<location> `packages/sdk-wrapper/src/__tests__/EncryptionManager.test.ts:47-48` </location>
<code_context>
+ expect(result).toEqual(mockResult);
+ });
+
+ it("should generate encryption key with custom parameters", async () => {
+ const mockResult = {
+ masterKey: "mock-master-key",
+ keyShards: [
+ { key: "shard1", index: "index1" },
+ { key: "shard2", index: "index2" },
+ { key: "shard3", index: "index3" },
+ ],
+ };
+ mockKavach.generate.mockResolvedValue(mockResult);
+
+ const result = await encryptionManager.generateKey(2, 3);
+
+ expect(mockKavach.generate).toHaveBeenCalledWith(2, 3);
+ expect(result).toEqual(mockResult);
+ });
+
</code_context>
<issue_to_address>
**suggestion (testing):** No test for edge case: threshold > keyCount.
Add a test to ensure an error is thrown when threshold exceeds keyCount, confirming invalid configurations are handled.
```suggestion
expect(result).toEqual(mockResult);
});
it("should throw an error when threshold exceeds keyCount", async () => {
const threshold = 6;
const keyCount = 5;
const errorMessage = "Threshold cannot be greater than keyCount";
mockKavach.generate.mockImplementationOnce(() => {
throw new Error(errorMessage);
});
await expect(encryptionManager.generateKey(threshold, keyCount)).rejects.toThrow(errorMessage);
expect(mockKavach.generate).toHaveBeenCalledWith(threshold, keyCount);
});
```
</issue_to_address>
### Comment 5
<location> `packages/sdk-wrapper/src/__tests__/EncryptionManager.test.ts:67-78` </location>
<code_context>
+ expect(result).toEqual(mockResult);
+ });
+
+ it("should throw error when Kavach SDK is not available", async () => {
+ const encryptionManagerWithoutKavach = new (class extends EncryptionManager {
+ constructor() {
+ super();
+ (this as any).isKavachAvailable = false;
+ }
+ })();
+
+ await expect(encryptionManagerWithoutKavach.generateKey()).rejects.toThrow(
+ "Kavach SDK not available - encryption features disabled",
+ );
+ });
+
</code_context>
<issue_to_address>
**suggestion (testing):** No test for fallback to local encryption SDK.
Please add a test to confirm that the local encryption SDK is used when Kavach is unavailable, ensuring the fallback logic is covered.
```suggestion
it("should throw error when Kavach SDK is not available", async () => {
const encryptionManagerWithoutKavach = new (class extends EncryptionManager {
constructor() {
super();
(this as any).isKavachAvailable = false;
}
})();
await expect(encryptionManagerWithoutKavach.generateKey()).rejects.toThrow(
"Kavach SDK not available - encryption features disabled",
);
});
it("should fallback to local encryption SDK when Kavach is not available", async () => {
// Mock local encryption SDK
const mockLocalEncryptionSDK = {
generate: jest.fn().mockResolvedValue("localResult"),
};
// Extend EncryptionManager to inject the mock local SDK
const encryptionManagerWithLocalFallback = new (class extends EncryptionManager {
constructor() {
super();
(this as any).isKavachAvailable = false;
(this as any).localEncryptionSDK = mockLocalEncryptionSDK;
}
})();
const result = await encryptionManagerWithLocalFallback.generateKey(5, 7);
expect(mockLocalEncryptionSDK.generate).toHaveBeenCalledWith(5, 7);
expect(result).toEqual("localResult");
});
```
</issue_to_address>
### Comment 6
<location> `packages/sdk-wrapper/src/__tests__/EncryptionManager.test.ts:185` </location>
<code_context>
+ });
+
+ describe("recoverKey", () => {
+ it("should recover master key from shards", async () => {
+ const mockResult = { masterKey: "recovered-key", error: null };
+ mockKavach.recoverKey.mockResolvedValue(mockResult);
+
+ const keyShards: KeyShard[] = [
+ { key: "shard1", index: "index1" },
+ { key: "shard2", index: "index2" },
+ { key: "shard3", index: "index3" },
+ ];
+
+ const result = await encryptionManager.recoverKey(keyShards);
+
+ expect(mockKavach.recoverKey).toHaveBeenCalledWith(keyShards);
+ expect(result).toEqual(mockResult);
+ });
+
</code_context>
<issue_to_address>
**suggestion (testing):** No test for insufficient key shards during recovery.
Please add a test case that checks recovery with insufficient key shards, ensuring it fails and returns the correct error.
```suggestion
describe("recoverKey", () => {
it("should fail to recover master key with insufficient shards", async () => {
const mockErrorResult = { masterKey: null, error: "Insufficient shards" };
mockKavach.recoverKey.mockResolvedValue(mockErrorResult);
const insufficientShards: KeyShard[] = [
{ key: "shard1", index: "index1" },
];
const result = await encryptionManager.recoverKey(insufficientShards);
expect(mockKavach.recoverKey).toHaveBeenCalledWith(insufficientShards);
expect(result).toEqual(mockErrorResult);
});
```
</issue_to_address>
### Comment 7
<location> `packages/sdk-wrapper/src/__tests__/EncryptionManager.test.ts:217-166` </location>
<code_context>
+ });
+
+ describe("shareToAddress", () => {
+ it("should share access to another address", async () => {
+ const mockResult = { isSuccess: true, error: null };
+ mockKavach.shareToAddress.mockResolvedValue(mockResult);
+
+ const result = await encryptionManager.shareToAddress(
+ "QmTest",
+ "0x123",
+ "0x456",
+ "jwt-token",
+ );
+
+ expect(mockKavach.shareToAddress).toHaveBeenCalledWith(
+ "QmTest",
+ "0x123",
+ "0x456",
+ "jwt-token",
+ );
+ expect(result).toEqual({ isSuccess: true, error: null });
+ });
+ });
</code_context>
<issue_to_address>
**suggestion (testing):** No test for sharing access to invalid addresses.
Add a test to cover sharing access to an invalid or malformed address, ensuring proper error handling and reporting.
Suggested implementation:
```typescript
describe("shareToAddress", () => {
```
```typescript
it("should share access to another address", async () => {
const mockResult = { isSuccess: true, error: null };
mockKavach.shareToAddress.mockResolvedValue(mockResult);
const result = await encryptionManager.shareToAddress(
"QmTest",
"0x123",
"0x456",
"jwt-token",
);
expect(mockKavach.shareToAddress).toHaveBeenCalledWith(
"QmTest",
"0x123",
"0x456",
"jwt-token",
);
expect(result).toEqual({ isSuccess: true, error: null });
});
it("should handle error when sharing to an invalid address", async () => {
const mockErrorResult = { isSuccess: false, error: "Invalid address format" };
mockKavach.shareToAddress.mockResolvedValue(mockErrorResult);
const result = await encryptionManager.shareToAddress(
"QmTest",
"0x123",
"invalid-address",
"jwt-token",
);
expect(mockKavach.shareToAddress).toHaveBeenCalledWith(
"QmTest",
"0x123",
"invalid-address",
"jwt-token",
);
expect(result).toEqual({ isSuccess: false, error: "Invalid address format" });
});
```
</issue_to_address>
### Comment 8
<location> `packages/sdk-wrapper/src/__tests__/EncryptionManager.test.ts:239-247` </location>
<code_context>
+ });
+
+ describe("getAuthMessage and getJWT", () => {
+ it("should get auth message for signing", async () => {
+ const mockResult = { message: "Sign this message", error: null };
+ mockKavach.getAuthMessage.mockResolvedValue(mockResult);
+
+ const result = await encryptionManager.getAuthMessage("0x123");
+
+ expect(mockKavach.getAuthMessage).toHaveBeenCalledWith("0x123");
+ expect(result).toEqual(mockResult);
+ });
+
</code_context>
<issue_to_address>
**suggestion (testing):** No test for getAuthMessage with invalid address.
Please add a test case for getAuthMessage using an invalid or empty address to verify proper error handling.
```suggestion
it("should get auth message for signing", async () => {
const mockResult = { message: "Sign this message", error: null };
mockKavach.getAuthMessage.mockResolvedValue(mockResult);
const result = await encryptionManager.getAuthMessage("0x123");
expect(mockKavach.getAuthMessage).toHaveBeenCalledWith("0x123");
expect(result).toEqual(mockResult);
});
it("should handle invalid or empty address in getAuthMessage", async () => {
const mockErrorResult = { message: null, error: "Invalid address" };
mockKavach.getAuthMessage.mockResolvedValue(mockErrorResult);
const result = await encryptionManager.getAuthMessage("");
expect(mockKavach.getAuthMessage).toHaveBeenCalledWith("");
expect(result).toEqual(mockErrorResult);
});
```
</issue_to_address>
### Comment 9
<location> `packages/sdk-wrapper/src/__tests__/EncryptionManager.test.ts:249-257` </location>
<code_context>
+ expect(result).toEqual(mockResult);
+ });
+
+ it("should generate JWT from signed message", async () => {
+ const mockResult = "jwt:token";
+ mockKavach.getJWT.mockResolvedValue(mockResult);
+
+ const result = await encryptionManager.getJWT("0x123", "signed-message");
+
+ expect(mockKavach.getJWT).toHaveBeenCalledWith("0x123", "signed-message");
+ expect(result).toEqual(mockResult);
+ });
+ });
</code_context>
<issue_to_address>
**suggestion (testing):** No test for getJWT with invalid signature.
Add a test for getJWT with an invalid or empty signature to ensure proper error handling.
```suggestion
it("should generate JWT from signed message", async () => {
const mockResult = "jwt:token";
mockKavach.getJWT.mockResolvedValue(mockResult);
const result = await encryptionManager.getJWT("0x123", "signed-message");
expect(mockKavach.getJWT).toHaveBeenCalledWith("0x123", "signed-message");
expect(result).toEqual(mockResult);
});
it("should handle invalid or empty signature when generating JWT", async () => {
const mockError = new Error("Invalid signature");
mockKavach.getJWT.mockRejectedValue(mockError);
await expect(encryptionManager.getJWT("0x123", "")).rejects.toThrow("Invalid signature");
expect(mockKavach.getJWT).toHaveBeenCalledWith("0x123", "");
});
```
</issue_to_address>
### Comment 10
<location> `apps/mcp-server/src/tools/LighthouseGenerateKeyTool.ts:69-73` </location>
<code_context>
if (params.threshold !== undefined) {
if (!Number.isInteger(params.threshold) || params.threshold < 2 || params.threshold > 10) {
return "threshold must be an integer between 2 and 10";
}
}
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge nested if conditions ([`merge-nested-ifs`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/merge-nested-ifs))
```suggestion
if (params.threshold !== undefined && (!Number.isInteger(params.threshold) || params.threshold < 2 || params.threshold > 10)) {
return "threshold must be an integer between 2 and 10";
}
```
<br/><details><summary>Explanation</summary>Reading deeply nested conditional code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two `if` conditions can be combined using
`and` is an easy win.
</details>
</issue_to_address>
### Comment 11
<location> `apps/mcp-server/src/tools/LighthouseGenerateKeyTool.ts:75-79` </location>
<code_context>
if (params.keyCount !== undefined) {
if (!Number.isInteger(params.keyCount) || params.keyCount < 3 || params.keyCount > 15) {
return "keyCount must be an integer between 3 and 15";
}
}
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge nested if conditions ([`merge-nested-ifs`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/merge-nested-ifs))
```suggestion
if (params.keyCount !== undefined && (!Number.isInteger(params.keyCount) || params.keyCount < 3 || params.keyCount > 15)) {
return "keyCount must be an integer between 3 and 15";
}
```
<br/><details><summary>Explanation</summary>Reading deeply nested conditional code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two `if` conditions can be combined using
`and` is an easy win.
</details>
</issue_to_address>
### Comment 12
<location> `packages/sdk-wrapper/src/encryption/EncryptionManager.ts:252-253` </location>
<code_context>
const result = await kavach.getAuthMessage(address);
return result;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Inline variable that is immediately returned ([`inline-immediately-returned-variable`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/inline-immediately-returned-variable))
```suggestion
return await kavach.getAuthMessage(address);
```
<br/><details><summary>Explanation</summary>Something that we often see in people's code is assigning to a result variable
and then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
</details>
</issue_to_address>
### Comment 13
<location> `packages/sdk-wrapper/src/encryption/EncryptionManager.ts:275-276` </location>
<code_context>
const result = await kavach.getJWT(address, signedMessage);
return result;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Inline variable that is immediately returned ([`inline-immediately-returned-variable`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/inline-immediately-returned-variable))
```suggestion
return await kavach.getJWT(address, signedMessage);
```
<br/><details><summary>Explanation</summary>Something that we often see in people's code is assigning to a result variable
and then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+96
to
+100
| const result = await kavach.shardKey(masterKey, threshold, keyCount); | ||
|
|
||
| if (!result || !result.isShardable || !result.keyShards) { | ||
| throw new Error("Failed to shard encryption key"); | ||
| } |
There was a problem hiding this comment.
suggestion: Error handling for shardKey may be too generic.
Consider using distinct error messages for unshardable keys and missing shards to improve diagnosability.
Suggested change
| const result = await kavach.shardKey(masterKey, threshold, keyCount); | |
| if (!result || !result.isShardable || !result.keyShards) { | |
| throw new Error("Failed to shard encryption key"); | |
| } | |
| const result = await kavach.shardKey(masterKey, threshold, keyCount); | |
| if (!result) { | |
| throw new Error("No result returned from shardKey"); | |
| } | |
| if (!result.isShardable) { | |
| throw new Error("Master key is not shardable"); | |
| } | |
| if (!result.keyShards) { | |
| throw new Error("Key shards missing from shardKey result"); | |
| } |
Comment on lines
+136
to
+140
| const result = await kavach.saveShards(address, cid, authToken, keyShards, sharedTo); | ||
|
|
||
| if (!result) { | ||
| throw new Error("Failed to save key shards"); | ||
| } |
There was a problem hiding this comment.
suggestion: Null result checks may not cover all error cases.
If the SDK provides structured error objects, handle those directly to improve error reporting and propagation.
Suggested change
| const result = await kavach.saveShards(address, cid, authToken, keyShards, sharedTo); | |
| if (!result) { | |
| throw new Error("Failed to save key shards"); | |
| } | |
| const result = await kavach.saveShards(address, cid, authToken, keyShards, sharedTo); | |
| if (result && result.error) { | |
| // Handle structured error from SDK | |
| throw new Error(`Failed to save key shards: ${result.error.message || JSON.stringify(result.error)}`); | |
| } | |
| if (!result) { | |
| throw new Error("Failed to save key shards: No result returned from SDK"); | |
| } |
Comment on lines
+47
to
+48
| expect(result).toEqual(mockResult); | ||
| }); |
There was a problem hiding this comment.
suggestion (testing): No test for edge case: threshold > keyCount.
Add a test to ensure an error is thrown when threshold exceeds keyCount, confirming invalid configurations are handled.
Suggested change
| expect(result).toEqual(mockResult); | |
| }); | |
| expect(result).toEqual(mockResult); | |
| }); | |
| it("should throw an error when threshold exceeds keyCount", async () => { | |
| const threshold = 6; | |
| const keyCount = 5; | |
| const errorMessage = "Threshold cannot be greater than keyCount"; | |
| mockKavach.generate.mockImplementationOnce(() => { | |
| throw new Error(errorMessage); | |
| }); | |
| await expect(encryptionManager.generateKey(threshold, keyCount)).rejects.toThrow(errorMessage); | |
| expect(mockKavach.generate).toHaveBeenCalledWith(threshold, keyCount); | |
| }); |
| }); | ||
| }); | ||
|
|
||
| describe("recoverKey", () => { |
There was a problem hiding this comment.
suggestion (testing): No test for insufficient key shards during recovery.
Please add a test case that checks recovery with insufficient key shards, ensuring it fails and returns the correct error.
Suggested change
| describe("recoverKey", () => { | |
| describe("recoverKey", () => { | |
| it("should fail to recover master key with insufficient shards", async () => { | |
| const mockErrorResult = { masterKey: null, error: "Insufficient shards" }; | |
| mockKavach.recoverKey.mockResolvedValue(mockErrorResult); | |
| const insufficientShards: KeyShard[] = [ | |
| { key: "shard1", index: "index1" }, | |
| ]; | |
| const result = await encryptionManager.recoverKey(insufficientShards); | |
| expect(mockKavach.recoverKey).toHaveBeenCalledWith(insufficientShards); | |
| expect(result).toEqual(mockErrorResult); | |
| }); |
| [], | ||
| "ADDRESS", | ||
| ); | ||
| expect(result).toEqual({ isSuccess: true, error: null }); |
There was a problem hiding this comment.
suggestion (testing): No test for sharing access to invalid addresses.
Add a test to cover sharing access to an invalid or malformed address, ensuring proper error handling and reporting.
Suggested implementation:
describe("shareToAddress", () => { it("should share access to another address", async () => {
const mockResult = { isSuccess: true, error: null };
mockKavach.shareToAddress.mockResolvedValue(mockResult);
const result = await encryptionManager.shareToAddress(
"QmTest",
"0x123",
"0x456",
"jwt-token",
);
expect(mockKavach.shareToAddress).toHaveBeenCalledWith(
"QmTest",
"0x123",
"0x456",
"jwt-token",
);
expect(result).toEqual({ isSuccess: true, error: null });
});
it("should handle error when sharing to an invalid address", async () => {
const mockErrorResult = { isSuccess: false, error: "Invalid address format" };
mockKavach.shareToAddress.mockResolvedValue(mockErrorResult);
const result = await encryptionManager.shareToAddress(
"QmTest",
"0x123",
"invalid-address",
"jwt-token",
);
expect(mockKavach.shareToAddress).toHaveBeenCalledWith(
"QmTest",
"0x123",
"invalid-address",
"jwt-token",
);
expect(result).toEqual({ isSuccess: false, error: "Invalid address format" });
});
Comment on lines
+69
to
+73
| if (params.threshold !== undefined) { | ||
| if (!Number.isInteger(params.threshold) || params.threshold < 2 || params.threshold > 10) { | ||
| return "threshold must be an integer between 2 and 10"; | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion (code-quality): Merge nested if conditions (merge-nested-ifs)
Suggested change
| if (params.threshold !== undefined) { | |
| if (!Number.isInteger(params.threshold) || params.threshold < 2 || params.threshold > 10) { | |
| return "threshold must be an integer between 2 and 10"; | |
| } | |
| } | |
| if (params.threshold !== undefined && (!Number.isInteger(params.threshold) || params.threshold < 2 || params.threshold > 10)) { | |
| return "threshold must be an integer between 2 and 10"; | |
| } | |
Explanation
Reading deeply nested conditional code is confusing, since you have to keep track of whichconditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two
if conditions can be combined usingand is an easy win.
Comment on lines
+75
to
+79
| if (params.keyCount !== undefined) { | ||
| if (!Number.isInteger(params.keyCount) || params.keyCount < 3 || params.keyCount > 15) { | ||
| return "keyCount must be an integer between 3 and 15"; | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion (code-quality): Merge nested if conditions (merge-nested-ifs)
Suggested change
| if (params.keyCount !== undefined) { | |
| if (!Number.isInteger(params.keyCount) || params.keyCount < 3 || params.keyCount > 15) { | |
| return "keyCount must be an integer between 3 and 15"; | |
| } | |
| } | |
| if (params.keyCount !== undefined && (!Number.isInteger(params.keyCount) || params.keyCount < 3 || params.keyCount > 15)) { | |
| return "keyCount must be an integer between 3 and 15"; | |
| } | |
Explanation
Reading deeply nested conditional code is confusing, since you have to keep track of whichconditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two
if conditions can be combined usingand is an easy win.
Comment on lines
+252
to
+253
| const result = await kavach.getAuthMessage(address); | ||
| return result; |
There was a problem hiding this comment.
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
Suggested change
| const result = await kavach.getAuthMessage(address); | |
| return result; | |
| return await kavach.getAuthMessage(address); | |
Explanation
Something that we often see in people's code is assigning to a result variableand then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
Comment on lines
+275
to
+276
| const result = await kavach.getJWT(address, signedMessage); | ||
| return result; |
There was a problem hiding this comment.
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
Suggested change
| const result = await kavach.getJWT(address, signedMessage); | |
| return result; | |
| return await kavach.getJWT(address, signedMessage); | |
Explanation
Something that we often see in people's code is assigning to a result variableand then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
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.
📋 Summary
This PR implements comprehensive encryption and access control capabilities to the MCP server and SDK wrapper through Kavach SDK integration.
🎯 What This PR Does
🚀 Key Features Implemented
1. Threshold Cryptography
2. Access Control System
==,>=,<=,!=,>,<)3. New MCP Tools
lighthouse_generate_key: Generate threshold encryption keyslighthouse_setup_access_control: Configure file access conditions4. Enhanced SDK Wrapper
Summary by Sourcery
Enable end-to-end encryption and fine-grained access control by integrating the Kavach SDK: extend the JS SDK with threshold cryptography methods and event forwarding, enhance the MCP server with corresponding service methods and tools, and add comprehensive tests to validate all new workflows
New Features:
Enhancements:
Build:
Tests: