Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 1:1 messaging to Browser SDK #703

Merged
merged 11 commits into from
Nov 1, 2024
Merged

Add 1:1 messaging to Browser SDK #703

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
6f9ac70
Upgrade WASM bindings
rygine Nov 1, 2024
77f4e99
applySignaturesRequests => applySignatures
rygine Nov 1, 2024
48368e6
Add 1:1 messaging
rygine Nov 1, 2024
ad0fa8e
Add list options for messages and groups
rygine Nov 1, 2024
c8a526f
Add getRevokeInstallationsSignatureText to client
rygine Nov 1, 2024
0f64130
Use dbPath option on client create
rygine Nov 1, 2024
e4fc2f5
Update browser SDK tests
rygine Nov 1, 2024
c8e14a7
Update Node SDK test
rygine Nov 1, 2024
8fe8d12
Add comment about dbPath validation
rygine Nov 1, 2024
fe97cc5
Fix toSafeListConversationsOptions
rygine Nov 1, 2024
04a8244
Add break to last case
rygine Nov 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion sdks/browser-sdk/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@
"@xmtp/content-type-primitives": "^1.0.1",
"@xmtp/content-type-text": "^1.0.0",
"@xmtp/proto": "^3.70.0",
"@xmtp/wasm-bindings": "^0.0.1",
"@xmtp/wasm-bindings": "^0.0.2",
"uuid": "^10.0.0"
},
"devDependencies": {
Expand Down
14 changes: 10 additions & 4 deletions sdks/browser-sdk/src/Client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,12 +95,16 @@ export class Client extends ClientWorkerClass {
return this.sendMessage("getRevokeWalletSignatureText", { accountAddress });
}

async getRevokeInstallationsSignatureText() {
return this.sendMessage("getRevokeInstallationsSignatureText", undefined);
}

async addSignature(type: WasmSignatureRequestType, bytes: Uint8Array) {
return this.sendMessage("addSignature", { type, bytes });
}

async applySignaturesRequests() {
return this.sendMessage("applySignaturesRequests", undefined);
async applySignatures() {
return this.sendMessage("applySignatures", undefined);
Comment on lines +106 to +107
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Inconsistency found in method naming between Client and WorkerClient

The verification revealed an inconsistency in the implementation:

  • WorkerClient.ts and Client.ts in browser-sdk call applySignatureRequests() internally
  • But the method is exposed as applySignatures() in the public API

This creates a mismatch between the internal implementation and public interface. The internal method should be renamed to match the public API.

Locations needing attention:

  • sdks/browser-sdk/src/WorkerClient.ts: Change applySignatureRequests() to applySignatures()
  • sdks/node-sdk/src/Client.ts: Change applySignatureRequests() to applySignatures()
🔗 Analysis chain

Verify the method rename across the codebase.

The rename from applySignaturesRequests to applySignatures improves clarity. However, we should ensure all call sites are updated.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining references to the old method name
# Test: Search for old method name. Expect: No results
rg "applySignaturesRequests" --type ts

# Test: Verify new method name usage for consistency
rg "applySignatures" -A 2 --type ts

Length of output: 2912

}

async registerIdentity() {
Expand All @@ -119,8 +123,10 @@ export class Client extends ClientWorkerClass {
return this.sendMessage("findInboxIdByAddress", { address });
}

async inboxState(refreshFromNetwork: boolean) {
return this.sendMessage("inboxState", { refreshFromNetwork });
async inboxState(refreshFromNetwork?: boolean) {
return this.sendMessage("inboxState", {
refreshFromNetwork: refreshFromNetwork ?? false,
});
}

async getLatestInboxState(inboxId: string) {
Expand Down
6 changes: 6 additions & 0 deletions sdks/browser-sdk/src/Conversation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,4 +282,10 @@ export class Conversation {
state,
});
}

async dmPeerInboxId() {
return this.#client.sendMessage("getDmPeerInboxId", {
id: this.#id,
});
}
Comment on lines +286 to +290
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Handler implementation verified but needs async support

The client worker implements the "getDmPeerInboxId" case handler, but there's a mismatch in the implementation:

  • The Conversation.ts calls sendMessage with an async expectation
  • The worker's handler calls group.dmPeerInboxId() synchronously without await

This could lead to timing issues in message handling.

Locations needing attention:

  • sdks/browser-sdk/src/workers/client.ts: The getDmPeerInboxId case handler should be marked as async and await the result
🔗 Analysis chain

Verify client message handler implementation.

Let's ensure the client properly handles the "getDmPeerInboxId" action.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if the client implements the getDmPeerInboxId message handler

# Test: Search for getDmPeerInboxId handler implementation
rg -A 5 "getDmPeerInboxId.*case"

Length of output: 34

Script:

#!/bin/bash
# Let's try a broader search for getDmPeerInboxId implementation
rg -i "getDmPeerInboxId"

# Also search for message handler patterns in client implementation
rg -A 10 "sendMessage.*case|handleMessage.*case|onMessage.*case"

# Look for client class implementation
ast-grep --pattern 'class $_ {
  $$$
  sendMessage($$$) {
    $$$
  }
  $$$
}'

Length of output: 416

Script:

#!/bin/bash
# Let's check the implementation in the client worker
cat sdks/browser-sdk/src/workers/client.ts

Length of output: 18834

}
30 changes: 30 additions & 0 deletions sdks/browser-sdk/src/Conversations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,12 @@ export class Conversations {
});
}

async getDmByInboxId(inboxId: string) {
return this.#client.sendMessage("getDmByInboxId", {
inboxId,
});
}

async list(options?: SafeListConversationsOptions) {
const conversations = await this.#client.sendMessage("getConversations", {
options,
Expand All @@ -39,6 +45,22 @@ export class Conversations {
);
}

async listGroups(
options?: Omit<SafeListConversationsOptions, "conversation_type">,
) {
return this.#client.sendMessage("getGroups", {
options,
});
}

async listDms(
options?: Omit<SafeListConversationsOptions, "conversation_type">,
) {
return this.#client.sendMessage("getDms", {
options,
});
}

async newGroup(accountAddresses: string[], options?: SafeCreateGroupOptions) {
const conversation = await this.#client.sendMessage("newGroup", {
accountAddresses,
Expand All @@ -47,4 +69,12 @@ export class Conversations {

return new Conversation(this.#client, conversation.id, conversation);
}

async newDm(accountAddress: string) {
const conversation = await this.#client.sendMessage("newDm", {
accountAddress,
});

return new Conversation(this.#client, conversation.id, conversation);
}
Comment on lines +73 to +79
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add return type, input validation, and documentation.

Consider the following improvements:

  1. Add explicit return type
  2. Validate the accountAddress parameter
  3. Add JSDoc documentation
+  /**
+   * Creates a new direct message conversation with the specified account.
+   * @param accountAddress The Ethereum address of the account to start a DM with
+   * @returns Promise<Conversation> The newly created DM conversation
+   * @throws {Error} If the account address is invalid
+   */
-  async newDm(accountAddress: string) {
+  async newDm(accountAddress: string): Promise<Conversation> {
+    if (!accountAddress || !/^0x[a-fA-F0-9]{40}$/.test(accountAddress)) {
+      throw new Error('Invalid Ethereum address');
+    }
+
     const conversation = await this.#client.sendMessage("newDm", {
       accountAddress,
     });

     return new Conversation(this.#client, conversation.id, conversation);
   }
📝 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 newDm(accountAddress: string) {
const conversation = await this.#client.sendMessage("newDm", {
accountAddress,
});
return new Conversation(this.#client, conversation.id, conversation);
}
/**
* Creates a new direct message conversation with the specified account.
* @param accountAddress The Ethereum address of the account to start a DM with
* @returns Promise<Conversation> The newly created DM conversation
* @throws {Error} If the account address is invalid
*/
async newDm(accountAddress: string): Promise<Conversation> {
if (!accountAddress || !/^0x[a-fA-F0-9]{40}$/.test(accountAddress)) {
throw new Error('Invalid Ethereum address');
}
const conversation = await this.#client.sendMessage("newDm", {
accountAddress,
});
return new Conversation(this.#client, conversation.id, conversation);
}

}
2 changes: 1 addition & 1 deletion sdks/browser-sdk/src/WorkerClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ export class WorkerClient {
return this.#client.addSignature(type, bytes);
}

async applySignaturesRequests() {
async applySignatures() {
return this.#client.applySignatureRequests();
}
Comment on lines +87 to 89
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

⚠️ Potential issue

Method naming inconsistency found across SDKs

The verification reveals that applySignatureRequests is still being used in the Node.js SDK's Client implementation while the Browser SDK has partially migrated to applySignatures. This creates an inconsistency between SDKs:

  • sdks/node-sdk/src/Client.ts: Uses applySignatureRequests
  • sdks/browser-sdk/src/WorkerClient.ts: Calls applySignatureRequests internally
  • Both SDKs expose applySignatures as the public API (based on test files)

The change should be applied consistently across all SDK implementations to maintain API parity.

🔗 Analysis chain

Breaking Change: Method rename requires attention.

The renaming of applySignaturesRequests to applySignatures is a breaking change that could affect existing consumers. Additionally, there's an inconsistency between the public method name applySignatures and the internal call to applySignatureRequests.

Let's verify the impact and consistency:

Consider either:

  1. Updating the internal call to match the new method name for consistency
  2. Adding a code comment explaining why the internal method name differs

Here's the suggested fix for consistency:

  async applySignatures() {
-   return this.#client.applySignatureRequests();
+   return this.#client.applySignatures();
  }
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining references to the old method name and verify consistency

# Test 1: Check for any remaining references to old method name
echo "Checking for references to 'applySignaturesRequests'..."
rg -i "applySignaturesRequests"

# Test 2: Check for all references to both old and new internal method name
echo "Checking for references to internal method 'applySignatureRequests'..."
rg "applySignatureRequests"

# Test 3: Check for all references to new method name
echo "Checking for references to new method 'applySignatures'..."
rg "applySignatures"

Length of output: 1735


Expand Down
4 changes: 4 additions & 0 deletions sdks/browser-sdk/src/WorkerConversation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,4 +166,8 @@ export class WorkerConversation {
updateConsentState(state: WasmConsentState) {
this.#group.update_consent_state(state);
}

dmPeerInboxId() {
return this.#group.dm_peer_inbox_id();
}
}
32 changes: 32 additions & 0 deletions sdks/browser-sdk/src/WorkerConversations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,18 +43,50 @@ export class WorkerConversations {
}
}

getDmByInboxId(inboxId: string) {
try {
const group = this.#conversations.find_dm_by_target_inbox_id(inboxId);
return new WorkerConversation(this.#client, group);
} catch {
return undefined;
}
}

async list(options?: SafeListConversationsOptions) {
const groups = (await this.#conversations.list(
options ? fromSafeListConversationsOptions(options) : undefined,
)) as WasmGroup[];
return groups.map((group) => new WorkerConversation(this.#client, group));
}

async listGroups(
options?: Omit<SafeListConversationsOptions, "conversation_type">,
) {
const groups = (await this.#conversations.list_groups(
options ? fromSafeListConversationsOptions(options) : undefined,
)) as WasmGroup[];
return groups.map((group) => new WorkerConversation(this.#client, group));
}

async listDms(
options?: Omit<SafeListConversationsOptions, "conversation_type">,
) {
const groups = (await this.#conversations.list_dms(
options ? fromSafeListConversationsOptions(options) : undefined,
)) as WasmGroup[];
return groups.map((group) => new WorkerConversation(this.#client, group));
}
Comment on lines +71 to +78
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider refactoring to reduce code duplication with listGroups.

The implementation is nearly identical to listGroups. Consider extracting the common logic:

+ private async listByType(
+   type: 'groups' | 'dms',
+   options?: Omit<SafeListConversationsOptions, "conversation_type">,
+ ): Promise<WorkerConversation[]> {
+   const method = type === 'groups' ? 'list_groups' : 'list_dms';
+   const groups = (await this.#conversations[method](
+     options ? fromSafeListConversationsOptions(options) : undefined,
+   )) as WasmGroup[];
+   return groups.map((group) => new WorkerConversation(this.#client, group));
+ }

+ /**
+  * Lists all DM conversations.
+  * @param options - Filtering options for listing DMs (excluding conversation_type)
+  * @returns Promise<WorkerConversation[]> - Array of DM conversations
+  */
  async listDms(
    options?: Omit<SafeListConversationsOptions, "conversation_type">,
  ): Promise<WorkerConversation[]> {
-   const groups = (await this.#conversations.list_dms(
-     options ? fromSafeListConversationsOptions(options) : undefined,
-   )) as WasmGroup[];
-   return groups.map((group) => new WorkerConversation(this.#client, group));
+   return this.listByType('dms', options);
  }

Committable suggestion skipped: line range outside the PR's diff.


async newGroup(accountAddresses: string[], options?: SafeCreateGroupOptions) {
const group = await this.#conversations.create_group(
accountAddresses,
options ? fromSafeCreateGroupOptions(options) : undefined,
);
return new WorkerConversation(this.#client, group);
}

async newDm(accountAddress: string) {
const group = await this.#conversations.create_dm(accountAddress);
return new WorkerConversation(this.#client, group);
}
Comment on lines +88 to +91
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add error handling and input validation.

The implementation should be more robust with these improvements:

+ /**
+  * Creates a new DM conversation with the specified account.
+  * @param accountAddress - The Ethereum address of the account to start a DM with
+  * @returns Promise<WorkerConversation> - The newly created DM conversation
+  * @throws Error if the account address is invalid or DM creation fails
+  */
- async newDm(accountAddress: string) {
+ async newDm(accountAddress: string): Promise<WorkerConversation> {
+   if (!accountAddress?.trim()) {
+     throw new Error("Account address is required");
+   }
+   try {
      const group = await this.#conversations.create_dm(accountAddress);
      return new WorkerConversation(this.#client, group);
+   } catch (error) {
+     throw new Error(`Failed to create DM: ${error.message}`);
+   }
  }
📝 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 newDm(accountAddress: string) {
const group = await this.#conversations.create_dm(accountAddress);
return new WorkerConversation(this.#client, group);
}
/**
* Creates a new DM conversation with the specified account.
* @param accountAddress - The Ethereum address of the account to start a DM with
* @returns Promise<WorkerConversation> - The newly created DM conversation
* @throws Error if the account address is invalid or DM creation fails
*/
async newDm(accountAddress: string): Promise<WorkerConversation> {
if (!accountAddress?.trim()) {
throw new Error("Account address is required");
}
try {
const group = await this.#conversations.create_dm(accountAddress);
return new WorkerConversation(this.#client, group);
} catch (error) {
throw new Error(`Failed to create DM: ${error.message}`);
}
}

}
42 changes: 41 additions & 1 deletion sdks/browser-sdk/src/types/clientEvents.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ export type ClientEvents =
};
}
| {
action: "applySignaturesRequests";
action: "applySignatures";
id: string;
result: undefined;
data: undefined;
Expand Down Expand Up @@ -164,6 +164,14 @@ export type ClientEvents =
id: string;
};
}
| {
action: "getDmByInboxId";
id: string;
result: SafeConversation | undefined;
data: {
inboxId: string;
};
}
| {
action: "getConversations";
id: string;
Expand All @@ -172,6 +180,22 @@ export type ClientEvents =
options?: SafeListConversationsOptions;
};
}
| {
action: "getGroups";
id: string;
result: SafeConversation[];
data: {
options?: Omit<SafeListConversationsOptions, "conversation_type">;
};
}
| {
action: "getDms";
id: string;
result: SafeConversation[];
data: {
options?: Omit<SafeListConversationsOptions, "conversation_type">;
};
}
| {
action: "newGroup";
id: string;
Expand All @@ -181,6 +205,14 @@ export type ClientEvents =
options?: SafeCreateGroupOptions;
};
}
| {
action: "newDm";
id: string;
result: SafeConversation;
data: {
accountAddress: string;
};
}
| {
action: "syncConversations";
id: string;
Expand Down Expand Up @@ -399,6 +431,14 @@ export type ClientEvents =
id: string;
state: WasmConsentState;
};
}
| {
action: "getDmPeerInboxId";
id: string;
result: string;
data: {
id: string;
};
};

export type ClientEventsActions = ClientEvents["action"];
Expand Down
12 changes: 12 additions & 0 deletions sdks/browser-sdk/src/utils/conversions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@ import {
WasmListMessagesOptions,
type WasmConsentEntityType,
type WasmConsentState,
type WasmConversationType,
type WasmDeliveryStatus,
type WasmDirection,
type WasmGroupMembershipState,
type WasmGroupMessageKind,
type WasmGroupPermissionsOptions,
type WasmInboxState,
Expand Down Expand Up @@ -141,6 +144,7 @@ export const toSafeMessage = (message: WasmMessage): SafeMessage => ({

export type SafeListMessagesOptions = {
delivery_status?: WasmDeliveryStatus;
direction?: WasmDirection;
limit?: bigint;
sent_after_ns?: bigint;
sent_before_ns?: bigint;
Expand All @@ -150,6 +154,7 @@ export const toSafeListMessagesOptions = (
options: WasmListMessagesOptions,
): SafeListMessagesOptions => ({
delivery_status: options.delivery_status,
direction: options.direction,
limit: options.limit,
sent_after_ns: options.sent_after_ns,
sent_before_ns: options.sent_before_ns,
Expand All @@ -163,9 +168,12 @@ export const fromSafeListMessagesOptions = (
options.sent_after_ns,
options.limit,
options.delivery_status,
options.direction,
);

export type SafeListConversationsOptions = {
allowed_states?: WasmGroupMembershipState[];
conversation_type?: WasmConversationType;
Comment on lines +175 to +176
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix asymmetric conversion in ListConversationsOptions.

The conversion functions for SafeListConversationsOptions are asymmetric:

  • fromSafeListConversationsOptions handles the new properties
  • toSafeListConversationsOptions doesn't map them, which could lead to data loss

Update toSafeListConversationsOptions to include the new properties:

 export const toSafeListConversationsOptions = (
   options: WasmListConversationsOptions,
 ): SafeListConversationsOptions => ({
+  allowed_states: options.allowed_states,
+  conversation_type: options.conversation_type,
   created_after_ns: options.created_after_ns,
   created_before_ns: options.created_before_ns,
   limit: options.limit,
 });

Also applies to: 194-195

created_after_ns?: bigint;
created_before_ns?: bigint;
limit?: bigint;
Expand All @@ -174,6 +182,8 @@ export type SafeListConversationsOptions = {
export const toSafeListConversationsOptions = (
options: WasmListConversationsOptions,
): SafeListConversationsOptions => ({
allowed_states: options.allowed_states,
conversation_type: options.conversation_type,
created_after_ns: options.created_after_ns,
created_before_ns: options.created_before_ns,
limit: options.limit,
Expand All @@ -183,6 +193,8 @@ export const fromSafeListConversationsOptions = (
options: SafeListConversationsOptions,
): WasmListConversationsOptions =>
new WasmListConversationsOptions(
options.allowed_states,
options.conversation_type,
options.created_after_ns,
options.created_before_ns,
options.limit,
Expand Down
7 changes: 6 additions & 1 deletion sdks/browser-sdk/src/utils/createClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,12 @@ export const createClient = async (
await init();

const host = options?.apiUrl ?? ApiUrls[options?.env ?? "dev"];
const dbPath = `xmtp-${options?.env ?? "dev"}-${accountAddress}.db3`;
// TODO: add db path validation
// - must end with .db3
// - must not contain invalid characters
// - must not start with a dot
const dbPath =
options?.dbPath ?? `xmtp-${options?.env ?? "dev"}-${accountAddress}.db3`;
Comment on lines +21 to +22
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add validation for user-provided dbPath.

The dbPath option should be validated to ensure it meets security requirements and has the correct extension.

Add validation before using the dbPath:

   const dbPath =
     options?.dbPath ?? `xmtp-${options?.env ?? "dev"}-${accountAddress}.db3`;
+  if (!dbPath.endsWith('.db3')) {
+    throw new Error('Database path must end with .db3 extension');
+  }
+  if (dbPath.includes('../') || dbPath.includes('..\\')) {
+    throw new Error('Database path cannot contain parent directory references');
+  }
📝 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
const dbPath =
options?.dbPath ?? `xmtp-${options?.env ?? "dev"}-${accountAddress}.db3`;
const dbPath =
options?.dbPath ?? `xmtp-${options?.env ?? "dev"}-${accountAddress}.db3`;
if (!dbPath.endsWith('.db3')) {
throw new Error('Database path must end with .db3 extension');
}
if (dbPath.includes('../') || dbPath.includes('..\\')) {
throw new Error('Database path cannot contain parent directory references');
}


const inboxId =
(await getInboxIdForAddress(host, accountAddress)) ||
Expand Down
Loading
Loading