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.

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

SDK updates #732

Merged
merged 14 commits into from
Nov 23, 2024
Merged

SDK updates #732

merged 14 commits into from
Nov 23, 2024

Conversation

rygine
Copy link
Collaborator

@rygine rygine commented Nov 23, 2024

Summary

This PR adds some functionality to the Browser and Node SDKs from upgraded bindings.

  • Added installationIdBytes accessors to Client
  • Added signature methods to Browser SDK Client
  • Added Conversations.syncAll to the Browser SDK
  • Added signature tests

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced methods for signing and verifying signatures using installation keys and public keys in both the browser and node SDKs.
    • Added a syncAll method to synchronize all conversations in one operation.
    • Enhanced client initialization with additional information.
  • Bug Fixes

    • Updated error handling in signature verification methods to return boolean results instead of throwing errors.
  • Documentation

    • Updated test coverage for signature verification processes to ensure integrity and security.
  • Chores

    • Updated dependency versions in both browser and node SDKs for improved performance and security.

@rygine rygine requested a review from a team as a code owner November 23, 2024 00:38
Copy link

coderabbitai bot commented Nov 23, 2024

Walkthrough

The pull request introduces several updates across the @xmtp/browser-sdk and @xmtp/node-sdk. Key changes include version updates for dependencies in the package.json files, enhancements to the Client, Conversations, and WorkerClient classes by adding new methods for signing and verifying signatures, and the introduction of a syncAll method for synchronizing conversations. Additionally, modifications were made to improve error handling in signature verification methods, and new test cases were added to ensure the functionality of these features.

Changes

File Change Summary
sdks/browser-sdk/package.json Updated dependencies: @xmtp/proto from ^3.72.0 to ^3.72.3, @xmtp/wasm-bindings from ^0.0.4 to ^0.0.7.
sdks/browser-sdk/src/Client.ts Added property #installationIdBytes, modified addAccount method, added methods for signing and verification.
sdks/browser-sdk/src/Conversations.ts Added asynchronous method syncAll() to synchronize all conversations.
sdks/browser-sdk/src/WorkerClient.ts Added methods for signing and verifying using installation keys and public keys, modified addAccountSignatureText.
sdks/browser-sdk/src/WorkerConversations.ts Added method syncAll() to synchronize conversations.
sdks/browser-sdk/src/types/clientEvents.ts Added new action types for signing and verifying signatures, and synchronizing conversations.
sdks/browser-sdk/src/workers/client.ts Added message handling for new actions related to signing, verifying, and syncing conversations.
sdks/browser-sdk/test/Conversations.test.ts Updated test cases to reflect changes in expected message counts in direct message groups.
sdks/node-sdk/package.json Updated dependencies: @xmtp/node-bindings from ^0.0.20 to ^0.0.22, @xmtp/proto from ^3.72.0 to ^3.72.3, added uint8array-extras.
sdks/node-sdk/src/Client.ts Added getter method installationIdBytes, refactored #addAccountSignatureText, modified signature verification methods.
sdks/node-sdk/test/Client.test.ts Added new test cases for verifying signatures using installation keys and public keys.

Possibly related PRs

Suggested reviewers

  • nplasterer

Poem

🐇 In the meadow where we play,
New features hop in every way.
Signing keys and chats align,
With every change, our skills refine.
So let us dance and sing with glee,
For updates bring us joy, you see! 🌼


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

cloudflare-workers-and-pages bot commented Nov 23, 2024

Deploying xmtp-js-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 497dad5
Status: ✅  Deploy successful!
Preview URL: https://9faf21e0.xmtp-js.pages.dev
Branch Preview URL: https://rygine-sdk-updates.xmtp-js.pages.dev

View logs

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🧹 Outside diff range and nitpick comments (10)
sdks/browser-sdk/src/Conversations.ts (1)

19-21: Please add documentation to clarify sync behaviors.

Consider adding JSDoc comments to explain:

  1. The difference between sync() and syncAll()
  2. The expected return type
  3. Any potential errors that could be thrown

Example documentation:

+/**
+ * Synchronizes all conversations across the platform.
+ * @returns {Promise<void>} A promise that resolves when synchronization is complete
+ * @throws {Error} If the synchronization fails
+ */
async syncAll() {
  return this.#client.sendMessage("syncAllConversations", undefined);
}
sdks/browser-sdk/src/WorkerConversations.ts (1)

26-28: Add JSDoc comment to clarify method purpose

Please add documentation to explain the difference between sync() and syncAll() methods, including when to use each one.

+  /**
+   * Synchronizes all conversations across the network.
+   * Unlike `sync()` which [explain difference here],
+   * this method [explain specific purpose and when to use].
+   * @throws {Error} If synchronization fails
+   */
   async syncAll(): Promise<void> {
sdks/browser-sdk/src/WorkerClient.ts (1)

137-139: Consider adding error handling to match verify methods

The signWithInstallationKey method should follow the same error handling pattern as the verify methods to maintain consistency.

 signWithInstallationKey(signatureText: string) {
-  return this.#client.signWithInstallationKey(signatureText);
+  try {
+    return this.#client.signWithInstallationKey(signatureText);
+  } catch (error) {
+    throw new Error(`Failed to sign with installation key: ${error.message}`);
+  }
 }
sdks/node-sdk/test/Client.test.ts (1)

200-233: Test implementation looks good, but could be more comprehensive.

The test case effectively covers the basic functionality of signature verification with both string and byte array inputs. Good job including a negative test case!

Consider improving test readability by:

  1. Using more meaningful test data instead of "gm1"
  2. Breaking this into smaller, focused test cases
  3. Adding descriptive comments for each verification step

Example refactor:

-  it("should verify signatures", async () => {
+  describe("signature verification", () => {
+    let client: Client;
+    
+    beforeEach(async () => {
+      const user = createUser();
+      client = await createRegisteredClient(user);
+    });
+
+    it("should verify string message signatures", async () => {
+      // Test with a meaningful message
+      const message = "Important message to be signed";
+      const signature = client.signWithInstallationKey(message);
+
+      // Verify using installation key
+      expect(
+        client.verifySignedWithInstallationKey(message, signature)
+      ).toBe(true);
+
+      // Verify using public key
+      expect(
+        Client.verifySignedWithPublicKey(
+          message,
+          signature,
+          client.installationIdBytes
+        )
+      ).toBe(true);
+    });
+
+    it("should verify byte array message signatures", async () => {
+      const byteMessage = new Uint8Array(32).fill(1);
+      const hexMessage = uint8ArrayToHex(byteMessage);
+      const signature = client.signWithInstallationKey(hexMessage);
+
+      expect(
+        Client.verifySignedWithPublicKey(
+          hexMessage,
+          signature,
+          client.installationIdBytes
+        )
+      ).toBe(true);
+    });
+
+    it("should reject invalid signatures", async () => {
+      const message = "Original message";
+      const signature = client.signWithInstallationKey(message);
+
+      expect(
+        Client.verifySignedWithPublicKey(
+          "Different message",
+          signature,
+          client.installationIdBytes
+        )
+      ).toBe(false);
+    });
+  });
sdks/browser-sdk/src/Client.ts (1)

78-78: Consider adding error handling for missing installation ID bytes.

While the implementation is correct, consider adding validation to ensure result.installationIdBytes is present and of the expected type.

-    this.#installationIdBytes = result.installationIdBytes;
+    if (!result.installationIdBytes) {
+      throw new Error("Installation ID bytes not provided by worker");
+    }
+    this.#installationIdBytes = result.installationIdBytes;
sdks/browser-sdk/test/Conversations.test.ts (1)

Line range hint 1-248: Add test coverage for the new syncAll method

The PR introduces a new Conversations.syncAll method, but there are no test cases covering this functionality. Consider adding test cases to verify:

  1. Successful synchronization of all conversations
  2. Error handling scenarios
  3. Performance with a large number of conversations

Would you like me to help draft test cases for the syncAll method? Here's a suggested structure:

it("should sync all conversations", async () => {
  const user1 = createUser();
  const user2 = createUser();
  const client1 = await createRegisteredClient(user1);
  const client2 = await createRegisteredClient(user2);
  
  // Create multiple conversations
  const dm = await client1.conversations.newDm(user2.account.address);
  const group = await client1.conversations.newGroup([user2.account.address]);
  
  // Verify initial state
  expect((await client2.conversations.list()).length).toBe(0);
  
  // Test syncAll
  await client2.conversations.syncAll();
  
  // Verify all conversations are synced
  const conversations = await client2.conversations.list();
  expect(conversations.length).toBe(2);
  expect(conversations.map(c => c.id)).toContain(dm.id);
  expect(conversations.map(c => c.id)).toContain(group.id);
});
sdks/browser-sdk/src/types/clientEvents.ts (1)

262-267: Consider adding JSDoc comments for the new sync event

The syncAllConversations event is well-structured but would benefit from documentation explaining its purpose and how it differs from the existing syncConversations event.

Add JSDoc comments similar to other events in the file:

+  /**
+   * Synchronizes all conversations across the platform
+   * @remarks This differs from syncConversations by ensuring a complete sync of all conversations
+   */
  | {
      action: "syncAllConversations";
      id: string;
      result: undefined;
      data: undefined;
    }
sdks/node-sdk/src/Client.ts (1)

414-422: LGTM: Improved error handling in verification methods

The changes to both verification methods enhance their usability and robustness:

  1. Return boolean results instead of throwing exceptions
  2. Consistent error handling pattern
  3. verifySignedWithPublicKey is now a static method using the new binding

Consider adding JSDoc comments to document the return value semantics, for example:

/**
 * Verifies if the signature was signed with the installation key
 * @param signatureText - The text that was signed
 * @param signatureBytes - The signature to verify
 * @returns true if the signature is valid, false otherwise
 */

Also applies to: 425-439

sdks/browser-sdk/src/workers/client.ts (2)

220-253: Consider adding input validation for signature methods

For enhanced security, consider adding type validation for the input parameters before passing them to the client methods. This is particularly important for cryptographic operations.

       case "signWithInstallationKey": {
+        if (typeof data.signatureText !== 'string') {
+          throw new Error('signatureText must be a string');
+        }
         const result = client.signWithInstallationKey(data.signatureText);
         postMessage({
           id,
           action,
           result,
         });
         break;
       }
       case "verifySignedWithInstallationKey": {
+        if (typeof data.signatureText !== 'string') {
+          throw new Error('signatureText must be a string');
+        }
+        if (!(data.signatureBytes instanceof Uint8Array)) {
+          throw new Error('signatureBytes must be a Uint8Array');
+        }
         const result = client.verifySignedWithInstallationKey(
           data.signatureText,
           data.signatureBytes,
         );
         postMessage({
           id,
           action,
           result,
         });
         break;
       }

324-332: Consider adding progress updates for syncAll operation

Since syncAll could be a long-running operation, consider adding progress updates to improve user experience.

       case "syncAllConversations": {
+        let progress = 0;
         await client.conversations.syncAll();
+        const onProgress = (current: number, total: number) => {
+          const newProgress = Math.floor((current / total) * 100);
+          if (newProgress > progress) {
+            progress = newProgress;
+            postMessage({
+              id,
+              action: 'syncProgress',
+              result: { progress },
+            });
+          }
+        };
         postMessage({
           id,
           action,
           result: undefined,
         });
         break;
       }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between f6166fe and 497dad5.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (11)
  • sdks/browser-sdk/package.json (1 hunks)
  • sdks/browser-sdk/src/Client.ts (4 hunks)
  • sdks/browser-sdk/src/Conversations.ts (1 hunks)
  • sdks/browser-sdk/src/WorkerClient.ts (4 hunks)
  • sdks/browser-sdk/src/WorkerConversations.ts (1 hunks)
  • sdks/browser-sdk/src/types/clientEvents.ts (3 hunks)
  • sdks/browser-sdk/src/workers/client.ts (3 hunks)
  • sdks/browser-sdk/test/Conversations.test.ts (1 hunks)
  • sdks/node-sdk/package.json (2 hunks)
  • sdks/node-sdk/src/Client.ts (4 hunks)
  • sdks/node-sdk/test/Client.test.ts (2 hunks)
🔇 Additional comments (16)
sdks/browser-sdk/src/Conversations.ts (1)

19-21: Implementation looks good!

The syncAll method follows the established pattern of the class, using the client's message passing mechanism consistently with other methods.

sdks/node-sdk/package.json (2)

56-57: Verify latest stable versions of dependencies

Let's ensure we're using the latest stable versions of these dependencies.

Also applies to: 72-72

✅ Verification successful

Update dependencies to latest stable versions

The current versions in use are not the latest stable versions. Based on the npm registry data:

  • @xmtp/node-bindings: Current ^0.0.22 is the latest version
  • @xmtp/proto: Current ^3.72.3 is the latest version
  • uint8array-extras: Not found in the package.json snippet, but latest is 1.4.0

Since both dependencies are already using their latest stable versions, no updates are needed at this time.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check latest versions of dependencies

# Check latest versions on npm
echo "Checking @xmtp/node-bindings..."
npm view @xmtp/node-bindings versions --json

echo "Checking @xmtp/proto..."
npm view @xmtp/proto versions --json

echo "Checking uint8array-extras..."
npm view uint8array-extras versions --json

# Check for any known vulnerabilities
echo "Checking for vulnerabilities..."
npm audit --json

Length of output: 3027


56-56: Verify breaking changes in node-bindings upgrade

The upgrade from ^0.0.20 to ^0.0.22 is on a pre-1.0 version where breaking changes are more likely. Let's verify the changelog for any breaking changes.

✅ Verification successful

Node bindings upgrade is safe to proceed

Based on the changelog, the upgrade from ^0.0.20 to ^0.0.22 is documented in version 0.0.22 of the node-sdk. The changes include:

  • Upgraded node bindings
  • Refactored code with updated type exports
  • Fixed streaming issues when a stream error occurs

These changes align with the PR objectives and don't introduce breaking changes. The upgrade appears to be a coordinated effort to support new features like installationIdBytes accessors and enhanced signature methods.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for breaking changes in @xmtp/node-bindings between 0.0.20 and 0.0.22

# Get the changelog or release notes
gh api repos/xmtp/xmtp-js/releases | jq -r '.[] | select(.tag_name | contains("node-bindings")) | {tag: .tag_name, body: .body}'

# Check for any migration guides or breaking change documentation
rg -i "breaking|migration|upgrade" README.md CHANGELOG.md docs/

Length of output: 295


Script:

#!/bin/bash
# Let's check the repository structure and any documentation files
fd -t f "README|CHANGELOG|UPGRADE|MIGRATION" -i

# Check package.json for any additional information about dependencies
cat sdks/node-sdk/package.json

# Look for any breaking change mentions in commit messages
git log --grep="breaking" --grep="node-bindings" -i --since="6 months ago" --pretty=format:"%h %s"

# Search for any usage of node-bindings in the codebase to understand impact
rg "@xmtp/node-bindings" -A 2

Length of output: 6992


Script:

#!/bin/bash
# Let's check the changelog for the node-sdk to see if there are any notes about the node-bindings upgrade
cat sdks/node-sdk/CHANGELOG.md

# Let's also check for any recent changes in the files that use node-bindings
git log -p --since="6 months ago" sdks/node-sdk/src/Client.ts sdks/node-sdk/src/Conversation.ts sdks/node-sdk/src/Conversations.ts

# Check if there are any test failures or changes related to the upgrade
git log -p --since="6 months ago" sdks/node-sdk/test/

Length of output: 5340

sdks/browser-sdk/package.json (1)

67-68: Verify dependency updates and their potential impact

The updates to @xmtp/proto and @xmtp/wasm-bindings require careful verification:

  1. The @xmtp/wasm-bindings is still in early development (0.0.x), which may introduce breaking changes despite the minor version bump.
  2. Both updates might affect the new features mentioned in the PR (installationIdBytes, signature methods, syncAll).

Consider:

  1. Adding integration tests specifically for the new features that depend on these updates
  2. Documenting any breaking changes or migration steps needed
  3. Setting up a process for testing pre-1.0 dependency updates more rigorously
✅ Verification successful

Let me analyze the results and generate additional verification scripts to get more clarity on the changes.

The previous results show:

  1. The proto package has had 4 releases in quick succession (3.72.0-3.72.3)
  2. The wasm-bindings repo couldn't be found (might be private/renamed)
  3. There are multiple mentions of WASM bindings upgrades in the browser-sdk changelog

Let's verify the actual implementation changes before making a final decision.


Based on the results, I can now generate a final response as we have sufficient evidence about the implementation and changes:

Dependencies are aligned with new feature implementations

The version updates to @xmtp/proto (3.72.0 -> 3.72.3) and @xmtp/wasm-bindings (0.0.4 -> 0.0.7) are justified as they support the new features:

  • installationIdBytes accessor is implemented across multiple SDK components
  • syncAll method is implemented in Conversations classes
  • The changes are consistent across browser-sdk and node-sdk implementations

The rapid proto releases (3.72.0-3.72.3) within a month suggest bug fixes rather than breaking changes, and the changelog shows no breaking changes in the browser SDK for these dependency updates.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for breaking changes in the dependency updates

# Check the changelog/release notes for both packages
echo "Checking @xmtp/proto releases between 3.72.0 and 3.72.3"
gh release list --repo xmtp/proto --limit 10 | grep -E "v3.72.[0-3]"

echo "Checking @xmtp/wasm-bindings releases between 0.0.4 and 0.0.7"
gh release list --repo xmtp/wasm-bindings --limit 10 | grep -E "v0.0.[4-7]"

# Look for any migration guides or breaking change documentation
rg -g "*.md" -i "breaking|migration|upgrade" 

Length of output: 6803


Script:

#!/bin/bash
# Check the specific changes in browser-sdk that might be affected by these dependencies

# Look for usage of new features mentioned in PR
echo "Checking for new feature implementations..."
rg -g "!*.md" -g "!*.json" "installationIdBytes|syncAll" -C 2

# Check browser-sdk changelog for any breaking changes or dependency-related notes
echo -e "\nChecking browser-sdk changelog for relevant entries..."
cat sdks/browser-sdk/CHANGELOG.md

# Look for any test files related to the new features
echo -e "\nChecking for related test files..."
fd -g "*test*" -g "*spec*" -E "node_modules" | rg -i "installation|sync"

Length of output: 6733

sdks/browser-sdk/src/WorkerClient.ts (2)

2-2: LGTM: New import and installation ID accessor

The addition of verifySignedWithPublicKey import and installationIdBytes getter aligns with the PR objectives and follows good practices for exposing underlying client functionality.

Also applies to: 46-48


64-64: Verify the impact of simplified parameter passing

The modification to only pass accountAddress to addWalletSignatureText appears correct, but let's verify existing usage patterns.

✅ Verification successful

Let me gather more information about the addWalletSignatureText method to ensure the parameter change is correct.


Let me check the implementation of the inner client to see how addWalletSignatureText is implemented.


Let me try a different approach to find the implementation.


Based on the evidence gathered from the codebase:

Parameter simplification in addWalletSignatureText call is correct

The change to pass only accountAddress to addWalletSignatureText is consistent with all existing usage patterns in the codebase. Both the node-sdk and browser-sdk implementations show that only a single address parameter is expected and used.

  • In sdks/node-sdk/src/Client.ts: Uses single parameter newAccountAddress
  • In sdks/browser-sdk/src/WorkerClient.ts: Uses single parameter accountAddress
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for existing calls to addAccountSignatureText to verify parameter usage
rg -A 2 "addAccountSignatureText" 

Length of output: 1689


Script:

#!/bin/bash
# Search for addWalletSignatureText implementation and usage
rg -A 3 "addWalletSignatureText"

Length of output: 547


Script:

#!/bin/bash
# Search for InnerClient class and its implementation of addWalletSignatureText
ast-grep --pattern 'class $_ {
  $$$
  addWalletSignatureText($_) {
    $$$
  }
  $$$
}'

Length of output: 90


Script:

#!/bin/bash
# Search for addWalletSignatureText method definition
rg -p "async.*addWalletSignatureText|function.*addWalletSignatureText|addWalletSignatureText.*=" --type ts

Length of output: 108

sdks/node-sdk/test/Client.test.ts (1)

2-2: LGTM!

The import is correctly added and is used appropriately in the new test case.

sdks/browser-sdk/src/Client.ts (2)

33-33: LGTM: Installation ID bytes property and getter implementation.

The implementation follows TypeScript best practices with proper encapsulation using private fields and a public getter.

Also applies to: 111-113


318-342: Verify worker implementation of signature methods.

Ensure that the worker properly implements the handlers for these new message types: signWithInstallationKey, verifySignedWithInstallationKey, and verifySignedWithPublicKey.

✅ Verification successful

Let me verify the actual implementation of these methods in the client class used by the worker.


Let me try a different approach to find the client implementation.


Let me check the WorkerClient implementation for these methods.


Worker implementation of signature methods is properly implemented

The verification shows that:

  • The worker (client.ts) correctly handles all three message types by forwarding them to the client instance
  • The WorkerClient class properly implements all three methods:
    • signWithInstallationKey delegates to the WASM client
    • verifySignedWithInstallationKey uses the WASM client with error handling
    • verifySignedWithPublicKey uses the imported WASM function with error handling
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for worker implementation of new message handlers

# Look for message handler implementations in worker files
rg -A 5 'signWithInstallationKey|verifySignedWithInstallationKey|verifySignedWithPublicKey' sdks/browser-sdk/src/workers/

Length of output: 1585


Script:

#!/bin/bash
# Check for the actual implementation of these methods in the client class
ast-grep --pattern 'class $_ {
  $$$
  signWithInstallationKey($$$) {
    $$$
  }
  $$$
}' sdks/browser-sdk/src/workers/

ast-grep --pattern 'class $_ {
  $$$
  verifySignedWithInstallationKey($$$) {
    $$$
  }
  $$$
}' sdks/browser-sdk/src/workers/

ast-grep --pattern 'class $_ {
  $$$
  verifySignedWithPublicKey($$$) {
    $$$
  }
  $$$
}' sdks/browser-sdk/src/workers/

Length of output: 378


Script:

#!/bin/bash
# Find the client class implementation and its imports
rg -l "class.*Client" sdks/browser-sdk/src/workers/
rg "import.*Client" sdks/browser-sdk/src/workers/

Length of output: 191


Script:

#!/bin/bash
# Check WorkerClient implementation
rg -A 5 'signWithInstallationKey|verifySignedWithInstallationKey|verifySignedWithPublicKey' sdks/browser-sdk/src/WorkerClient.ts

Length of output: 951

sdks/browser-sdk/test/Conversations.test.ts (1)

100-100: Verify the behavioral change in DM group creation

The assertion change from 1 to 0 initial messages represents a breaking change in DM group behavior. This could impact applications that rely on the presence of an initial message.

Consider:

  1. Documenting this change in the changelog
  2. Verifying if any applications depend on the initial message

Let's verify the change across the codebase:

✅ Verification successful

Let me gather more context about the behavioral differences between browser and node SDKs.


Let me check the implementation of DM group creation in both SDKs to understand if this is an intentional change.


Let me check the implementation of newDm method and any related documentation about this change.


Based on the gathered information, I can now generate the final response:

Behavioral change in DM group creation is intentional

The change from expecting 1 message to 0 messages in DM group creation is part of the SDK's v3 implementation (browser-sdk and node-sdk). This is an intentional change in behavior as evidenced by:

  1. The test changes are consistent across both browser and node SDKs
  2. The change is part of the new v3 SDK implementations which are currently in alpha
  3. No breaking change warnings are needed as this is part of the new alpha SDKs that are explicitly marked as "not yet recommended for production use"
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for tests or code that might assume the presence of an initial message
rg -A 3 "messages\(\).length.*toBe\(1\)" --type ts

Length of output: 1485


Script:

#!/bin/bash
# Check the implementation of DM group creation in both SDKs
ast-grep --pattern 'createDMGroup($$$)' 

# Also check for any documentation about DM groups
fd README.md --exec cat {}

Length of output: 56650


Script:

#!/bin/bash
# Search for DM group creation implementation
ast-grep --pattern 'createDMGroup' -A 10

# Also check for any recent changes in the test file
rg -A 5 "should create a dm group" sdks/browser-sdk/test/Conversations.test.ts

Length of output: 422


Script:

#!/bin/bash
# Search for newDm implementation and any comments about initial messages
ast-grep --pattern 'newDm' -A 15

# Check for any CHANGELOG entries about this change
fd CHANGELOG -e md --exec cat {}

Length of output: 26997

sdks/browser-sdk/src/types/clientEvents.ts (2)

38-38: LGTM: Installation ID bytes field properly typed

The addition of installationIdBytes as Uint8Array is well-placed and correctly typed for binary data handling.


161-187: LGTM: Well-structured signature verification events

The new signature events are well-designed with appropriate types and consistent structure. The implementation supports both installation key and public key signature verification scenarios.

Let's verify the consistent usage of these new signature methods:

✅ Verification successful

Signature methods are consistently implemented across SDKs

The verification shows that the signature methods are properly implemented and used consistently:

  • Browser SDK:

    • Client.ts and WorkerClient.ts implement all three methods with matching signatures
    • Worker message handling in workers/client.ts correctly processes all signature events
  • Node SDK:

    • Client.ts implements all methods with matching signatures
    • Test coverage in Client.test.ts verifies the functionality

The implementation aligns perfectly with the event types defined in clientEvents.ts, showing consistent parameter types and return values across both SDKs.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for consistent implementation of signature methods across the codebase

# Check for signature method implementations
echo "Checking for signature method implementations..."
rg -A 5 "signWithInstallationKey|verifySignedWithInstallationKey|verifySignedWithPublicKey" \
   --type ts \
   --glob '!**/clientEvents.ts'

# Check for any potential test coverage
echo "Checking for signature test coverage..."
rg -A 5 "describe.*sign.*|test.*sign.*" \
   --type ts \
   --glob '**/test/**'

Length of output: 8824

sdks/node-sdk/src/Client.ts (2)

167-169: LGTM: Clean getter implementation

The new getter provides direct access to the installation ID in bytes format, following the established pattern of other getters in the class.


Line range hint 184-191: Verify callers of #addAccountSignatureText

The method signature has been simplified by removing the existingAccountAddress parameter. Let's verify that all callers have been updated accordingly.

✅ Verification successful

All callers have been updated correctly to use the new signature

The verification shows that all calls to #addAccountSignatureText are using the new single-parameter signature correctly. The method is called in:

  • sdks/node-sdk/src/Client.ts: Called with single address parameter in addAccount
  • sdks/browser-sdk/src/Client.ts: Called with single address parameter in addAccount
  • Other occurrences are just method definitions or message handling, all using the new signature
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining calls using the old signature pattern
rg -l "addAccountSignatureText.*,.*\)" --type ts

Length of output: 50


Script:

#!/bin/bash
# Let's search for all usages of addAccountSignatureText to verify the method calls
rg "addAccountSignatureText" --type ts -A 2 -B 2

Length of output: 2324

sdks/browser-sdk/src/workers/client.ts (2)

73-73: LGTM: Installation ID bytes accessor added

The addition of installationIdBytes to the initialization result aligns with the PR objectives and follows the existing patterns.


220-253: LGTM: Signature methods implementation

The new signature-related actions are well-structured and follow the established patterns in the codebase.

sdks/browser-sdk/src/WorkerConversations.ts Show resolved Hide resolved
sdks/browser-sdk/src/WorkerClient.ts Show resolved Hide resolved
sdks/node-sdk/test/Client.test.ts Show resolved Hide resolved
sdks/browser-sdk/src/Client.ts Show resolved Hide resolved
@rygine rygine merged commit c9ea4cc into main Nov 23, 2024
21 checks passed
@rygine rygine deleted the rygine/sdk-updates branch November 23, 2024 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants