Add V3 support to Frames client and validator #731
Conversation
🦋 Changeset detectedLatest commit: 86a8f9f The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThe pull request introduces several updates to the Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (24)
packages/frames-client/vitest.setup.ts (1)
8-11: Consider enhancing the teardown function with error handling and logging.While the function works, it could benefit from some improvements:
- Add error handling for file deletions
- Add logging for cleanup operations
- Make the glob pattern more specific
Consider this enhanced implementation:
export const teardown = async () => { - const files = await glob("**/*.db3*", { cwd: __dirname }); - await Promise.all(files.map((file) => unlink(join(__dirname, file)))); + try { + console.log("Starting test database cleanup..."); + const files = await glob("**/*.db3{,.journal,.wal}", { + cwd: __dirname, + onlyFiles: true + }); + + if (files.length === 0) { + console.log("No database files found to clean up"); + return; + } + + await Promise.all( + files.map(async (file) => { + const fullPath = join(__dirname, file); + try { + await unlink(fullPath); + console.log(`Cleaned up: ${file}`); + } catch (err) { + if (err.code !== 'ENOENT') { + console.warn(`Failed to delete ${file}: ${err.message}`); + } + } + }) + ); + console.log("Database cleanup completed"); + } catch (err) { + console.error("Error during database cleanup:", err); + throw err; + } };This implementation:
- Adds specific file extensions for SQLite databases
- Includes error handling and logging
- Ignores missing files
- Uses
onlyFiles: truefor safetypackages/frames-validator/vitest.setup.ts (1)
1-11: Consider moving database cleanup to a dedicated file.The current file is named generically as
vitest.setup.tsbut only contains database cleanup logic. Consider moving this functionality to a more specific file likedatabase-cleanup.tsto better reflect its purpose and allow for additional test setup logic in the future.packages/frames-validator/package.json (1)
Line range hint
2-2: Bump package version for breaking changesThe PR introduces breaking changes to the
validateFramesPostmethod, making it async and requiring a new environment parameter. This warrants a major version bump according to semver.Apply this diff:
- "version": "0.6.3", + "version": "1.0.0",sdks/browser-sdk/src/Conversations.ts (2)
19-21: Add documentation for the new syncAll method.Please add JSDoc comments to explain:
- The purpose of this method
- How it differs from the existing
sync()method- The expected return type
- Any potential errors that might be thrown
Example documentation:
+/** + * Synchronizes all conversations for the client. + * @returns {Promise<void>} A promise that resolves when synchronization is complete + * @throws {Error} If synchronization fails + */ async syncAll() { return this.#client.sendMessage("syncAllConversations", undefined); }
19-21: Consider adding explicit return type.While TypeScript can infer the return type, it would be more maintainable to explicitly declare it.
- async syncAll() { + async syncAll(): Promise<void> {packages/frames-client/src/types.ts (2)
55-58: Consider adding JSDoc comments to document the V2FramesSigner interface.While the type definition is clean, adding documentation would help developers understand:
- The purpose of each method
- Expected return values
- Any side effects or important notes about the signing process
Example improvement:
+/** + * Signer interface for V2 frames + */ export type V2FramesSigner = { + /** Returns the wallet address associated with this signer */ address: () => Promise<string> | string; + /** Returns the public key bundle used for message encryption */ getPublicKeyBundle: () => Promise<publicKeyProto.PublicKeyBundle>; + /** Signs a message using the V2 signature scheme */ sign: (message: Uint8Array) => Promise<signatureProto.Signature>; };
70-73: Consider enhancing the type guard robustness.While the current implementation works, it could be more robust by:
- Verifying method types
- Adding error handling for edge cases
Consider this enhanced version:
export const isV3FramesSigner = ( signer: FramesSigner, ): signer is V3FramesSigner => { - return "installationId" in signer && "inboxId" in signer; + try { + return ( + typeof signer === "object" && + signer !== null && + "installationId" in signer && + "inboxId" in signer && + typeof signer.installationId === "function" && + typeof signer.inboxId === "function" + ); + } catch { + return false; + } };sdks/browser-sdk/package.json (2)
Line range hint
3-3: Package version should be incrementedGiven the PR introduces breaking changes and new features (V3 support, new signature methods), the package version should be incremented following semantic versioning principles. Consider bumping to at least
0.1.0for breaking changes.- "version": "0.0.5", + "version": "0.1.0",
67-68: Consider adding viem as a peer dependencyThe SDK appears to use viem (present in devDependencies) for wallet interactions. Consider adding it as a peer dependency to make this requirement explicit to consumers and avoid potential version conflicts.
"dependencies": { "@xmtp/proto": "^3.72.3", "@xmtp/wasm-bindings": "^0.0.7", "uuid": "^11.0.3" }, + "peerDependencies": { + "viem": "^2.0.0" + }, "devDependencies": {sdks/browser-sdk/src/WorkerConversations.ts (1)
26-28: Consider adding error handling and documentation.While the implementation is correct, consider these improvements for consistency and maintainability:
- Add error handling to match the pattern used in other methods
- Add JSDoc documentation explaining the purpose and behavior
- Add explicit return type annotation
Here's a suggested implementation:
+ /** + * Synchronizes all conversations across the network. + * @returns {Promise<void>} A promise that resolves when synchronization is complete + * @throws {Error} If synchronization fails + */ - async syncAll() { - return this.#conversations.syncAllConversations(); + async syncAll(): Promise<void> { + try { + return await this.#conversations.syncAllConversations(); + } catch (error) { + throw new Error(`Failed to sync all conversations: ${error}`); + } + }sdks/browser-sdk/src/WorkerClient.ts (1)
141-167: Consider enhancing error handling in verification methodsBoth verification methods follow a good pattern of converting errors to boolean returns. However, consider adding error logging or telemetry for debugging purposes in production.
verifySignedWithInstallationKey( signatureText: string, signatureBytes: Uint8Array, ) { try { this.#client.verifySignedWithInstallationKey( signatureText, signatureBytes, ); return true; - } catch { + } catch (error) { + // Consider adding error logging/telemetry here + console.debug('Installation key verification failed:', error); return false; } } verifySignedWithPublicKey( signatureText: string, signatureBytes: Uint8Array, publicKey: Uint8Array, ) { try { verifySignedWithPublicKey(signatureText, signatureBytes, publicKey); return true; - } catch { + } catch (error) { + // Consider adding error logging/telemetry here + console.debug('Public key verification failed:', error); return false; } }sdks/node-sdk/test/Client.test.ts (1)
200-233: LGTM! Well-structured test coverage for signature verification.The test case thoroughly covers:
- Text and binary input types
- Instance and static verification methods
- Positive and negative cases
- Hex string conversion requirement
Consider adding these test cases to improve coverage:
- Invalid signature format
- Invalid public key
- Empty/null inputs
Example:
it("should handle invalid signatures", async () => { const user = createUser(); const client = await createRegisteredClient(user); const signatureText = "gm1"; // Test invalid signature format expect(() => client.verifySignedWithInstallationKey( signatureText, "invalid-signature" ) ).toThrow(); // Test invalid public key expect( Client.verifySignedWithPublicKey( signatureText, client.signWithInstallationKey(signatureText), new Uint8Array(32) // Invalid public key ) ).toBe(false); // Test empty inputs expect(() => client.signWithInstallationKey("") ).toThrow(); });sdks/browser-sdk/src/Client.ts (1)
318-342: Add JSDoc documentation for new signature methodsWhile the implementation is correct, these new signature verification methods would benefit from JSDoc documentation describing:
- Purpose and use cases
- Parameter requirements
- Return value format
- Possible error conditions
Additionally, consider adding error handling for invalid parameters.
Example documentation format:
/** * Signs data using the installation key * @param signatureText - The text to sign * @returns Promise<Uint8Array> The signature bytes * @throws If the installation is not initialized */ signWithInstallationKey(signatureText: string): Promise<Uint8Array>sdks/browser-sdk/test/Conversations.test.ts (1)
100-100: LGTM! Consider adding a clarifying comment.The change correctly fixes the assertion to expect 0 messages in a newly created DM group, which aligns with the expected behavior that conversations start empty until messages are explicitly sent.
Consider adding a comment to explain this behavior:
+ // DM groups start with 0 messages - messages must be explicitly sent after group creation expect((await group.messages()).length).toBe(0);packages/frames-client/src/client.ts (1)
77-97: Refactor to Reduce Code DuplicationThere is duplicated logic in the
buildSignedFrameActionmethod within the conditional branches. Refactoring can enhance readability and maintainability.Consider refactoring as follows:
const signature = await this.#signer.sign(digest); + let inboxId; + let installationId; + let installationSignature; + let signedPublicKeyBundle; if (isV3FramesSigner(this.#signer)) { + inboxId = await this.#signer.inboxId(); + installationId = await this.#signer.installationId(); + installationSignature = signature; + signedPublicKeyBundle = undefined; } else { + inboxId = ""; + installationId = new Uint8Array(); + installationSignature = new Uint8Array(); + const publicKeyBundle = await this.#signer.getPublicKeyBundle(); + signedPublicKeyBundle = v1ToV2Bundle(publicKeyBundle); } payload = { actionBody, inboxId, installationId, installationSignature, signature: isV3FramesSigner(this.#signer) ? undefined : signature, signedPublicKeyBundle, };This refactoring reduces duplication and clarifies the differences between signer types.
packages/frames-validator/src/validation.ts (2)
31-31: Clarify the necessity ofinstallationIdvariableThe
installationIdis being extracted but is commented as// not necessary. IfinstallationIdis not required, consider removing it from the destructured variables. If it is necessary, please update or remove the comment to reflect its purpose accurately.
46-46: Avoid usingconsole.logfor error informationUsing
console.logmay not be appropriate for logging error information in production code. Consider removing this statement or replacing it with a proper logging mechanism that adheres to your project's logging standards.Apply this diff to remove the
console.logstatement:if (verifiedWalletAddress !== walletAddress) { - console.log(`${verifiedWalletAddress} !== ${walletAddress}`); throw new Error("Invalid wallet address"); }packages/frames-validator/src/validation.test.ts (3)
18-24: OptimizescrambleBytesfunction using built-in methodsThe
scrambleBytesfunction manually reverses the bytes, which can be simplified for better readability and performance by using the built-inreversemethod.Apply this diff to refactor the function:
-function scrambleBytes(bytes: Uint8Array) { - const scrambled = new Uint8Array(bytes.length); - for (let i = 0; i < bytes.length; i++) { - scrambled[i] = bytes[bytes.length - i - 1]; - } - return scrambled; +function scrambleBytes(bytes: Uint8Array) { + return Uint8Array.from(bytes).reverse(); +}
147-148: Use consistent invalid data for clearer test resultsIn the
shouldFailWithWalletAddressMismatchtest for V3, using a hardcoded value like"wrong-inbox-id"may not simulate realistic scenarios. Consider generating a random or format-invalidinboxIdto ensure the test accurately reflects potential mismatches.For example:
-deserialized.inboxId = "wrong-inbox-id"; +deserialized.inboxId = crypto.randomUUID();
150-153: Reuse existing utilities to generate public key bundlesCreating a new
PrivateKeyBundleV2with a throwaway wallet increases test complexity. If possible, reuse existing utilities or mock objects to generate a mismatched public key bundle more efficiently.Consider simplifying as:
-const throwAwayWallet = Wallet.createRandom(); -deserialized.signedPublicKeyBundle = ( - await PrivateKeyBundleV2.generate(throwAwayWallet) -).getPublicKeyBundle(); +deserialized.signedPublicKeyBundle = existingOtherPublicKeyBundle;packages/frames-client/src/client.test.ts (4)
134-175: Consider mocking external dependencies in E2E testsTo make tests more reliable and faster, consider mocking external service calls instead of making actual network requests to external URLs like
"https://fc-polls-five.vercel.app/polls/...". This reduces dependencies on external services, prevents potential failures due to network issues or service outages, and improves test performance.
177-202: Consider mocking external dependencies in testsSimilar to the previous comment, consider mocking external service calls in the
sendsBackButtonPostUrltest to enhance test reliability and execution speed.
7-52: Use 'ethers' utilities to convert Uint8Array to hex and reduce dependenciesSince you're already importing
ethers, you can useethers.utils.hexlifyto convertUint8Arrayto hex strings instead of importinguint8ArrayToHexfrom"uint8array-extras". This reduces external dependencies and simplifies your code.Update the code as follows:
In the imports:
-import { getBytes, Wallet } from "ethers"; +import { getBytes, hexlify, Wallet } from "ethers"; -import { uint8ArrayToHex } from "uint8array-extras";In line 52, replace
uint8ArrayToHexwithhexlify:sign: (digest: Uint8Array) => - client.signWithInstallationKey(uint8ArrayToHex(digest)), + client.signWithInstallationKey(hexlify(digest)),
223-233: Plan to add E2E tests for V3 clients when possibleAs noted in your comment on line 133, E2E tests will be added once Frames are deployed with the new schema. To maintain consistent test coverage between V2 and V3 clients, ensure to include the
"works e2e"test for V3 clients when the deployment is ready.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (30)
packages/frames-client/package.json(2 hunks)packages/frames-client/src/client.test.ts(1 hunks)packages/frames-client/src/client.ts(3 hunks)packages/frames-client/src/index.test.ts(0 hunks)packages/frames-client/src/index.ts(1 hunks)packages/frames-client/src/types.ts(2 hunks)packages/frames-client/src/utils.ts(1 hunks)packages/frames-client/tsconfig.json(1 hunks)packages/frames-client/vitest.config.ts(1 hunks)packages/frames-client/vitest.setup.ts(1 hunks)packages/frames-validator/package.json(2 hunks)packages/frames-validator/src/index.test.ts(0 hunks)packages/frames-validator/src/index.ts(1 hunks)packages/frames-validator/src/validation.test.ts(1 hunks)packages/frames-validator/src/validation.ts(2 hunks)packages/frames-validator/src/validator.ts(2 hunks)packages/frames-validator/tsconfig.json(1 hunks)packages/frames-validator/vitest.config.ts(1 hunks)packages/frames-validator/vitest.setup.ts(1 hunks)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)
💤 Files with no reviewable changes (2)
- packages/frames-client/src/index.test.ts
- packages/frames-validator/src/index.test.ts
✅ Files skipped from review due to trivial changes (3)
- packages/frames-client/tsconfig.json
- packages/frames-validator/src/index.ts
- packages/frames-validator/tsconfig.json
🔇 Additional comments (44)
packages/frames-client/src/index.ts (1)
2-2: LGTM! Verify type guard usage across the codebase.
The addition of isV3FramesSigner export aligns with the V3 support objectives and follows proper barrel file organization.
Let's verify the implementation and usage of this type guard:
✅ Verification successful
Type guard implementation and usage verified successfully
The isV3FramesSigner type guard is:
- Correctly implemented using property checks for
installationIdandinboxId - Properly used in conditional blocks in both client.ts and client.test.ts
- Well integrated with the V2/V3 signer type system
- Used appropriately for runtime type discrimination in the signing flow
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation and usage of isV3FramesSigner
# Check the type guard implementation
echo "Checking type guard implementation:"
ast-grep --pattern 'export function isV3FramesSigner($_): $_ is V3FramesSigner { $$$ }'
# Check usage of the type guard in conditional blocks
echo -e "\nChecking usage in conditionals:"
rg -A 3 'if.*isV3FramesSigner'
Length of output: 919
Script:
#!/bin/bash
# Let's verify the actual implementation of isV3FramesSigner type guard
echo "Checking isV3FramesSigner implementation in types.ts:"
rg -A 10 "export function isV3FramesSigner" packages/frames-client/src/types.ts
# Let's also check the V3FramesSigner type definition
echo -e "\nChecking V3FramesSigner type definition:"
rg -B 5 -A 5 "export (type|interface) V3FramesSigner" packages/frames-client/src/types.ts
Length of output: 816
Script:
#!/bin/bash
# Let's check the full types.ts file to understand the context better
echo "Checking full types.ts content:"
cat packages/frames-client/src/types.ts
# Also check if there are any other files importing V3FramesSigner
echo -e "\nChecking V3FramesSigner imports:"
rg "import.*V3FramesSigner"
Length of output: 2291
packages/frames-client/vitest.setup.ts (2)
1-6: LGTM! Clean and efficient module imports.
The code follows best practices by:
- Using the "node:" prefix for built-in modules
- Importing specific functions instead of entire modules
- Properly setting up
__dirnamefor ESM modules
9-9: Verify the glob pattern matches test database files.
The current glob pattern **/*.db3* might be too broad. Let's verify what database files are actually created during tests.
✅ Verification successful
Let me try a different approach to verify the database files pattern by checking the test files and their setup.
Let me check the content of vitest.setup.ts to understand how database files are used in tests.
Let me check the test files to see how SQLite databases are created and used in tests.
Let me try one final search to find any test files that might be creating temporary databases.
The glob pattern **/*.db3* is appropriate for cleanup purposes
The pattern **/*.db3* in the vitest.setup.ts teardown function is actually well-suited for its purpose:
- It matches both the main SQLite database files (.db3) and their associated journal/WAL files (.db3-journal, *.db3-wal)
- The teardown function's role is to clean up any database files after tests, so a broader pattern is safer to ensure complete cleanup
- No database files are currently present, which is expected as they are temporary test artifacts that get cleaned up
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all database-related files in test directories
# and analyze their naming patterns
echo "Looking for database files in test directories..."
fd -e db3 -e "db3-journal" -e "db3-wal" . packages/frames-client/
Length of output: 172
Script:
#!/bin/bash
# Search for test files that might create or use database files
rg -l "\.db3" packages/frames-client/
Length of output: 78
Script:
#!/bin/bash
# Check the content of vitest.setup.ts
cat packages/frames-client/vitest.setup.ts
Length of output: 442
Script:
#!/bin/bash
# Search for SQLite database creation or usage in test files
rg -l "SQLite|createConnection|Database" packages/frames-client/
Length of output: 66
Script:
#!/bin/bash
# Look for test files and their database-related patterns
fd -e test.ts -e spec.ts . packages/frames-client/ --exec rg -l "db|database|sqlite" {}
Length of output: 91
packages/frames-validator/vitest.setup.ts (3)
1-4: LGTM! Well-structured imports.
The imports are well-organized, using the recommended node: protocol prefix for Node.js built-ins and named imports for clarity.
6-6: LGTM! Proper ESM directory path setup.
Correctly implements __dirname for ESM modules using fileURLToPath and dirname, ensuring cross-platform compatibility.
8-11: Consider adding error handling and safety checks.
While the implementation is functional, consider these improvements for better robustness:
- Add error handling for file operations
- Add a safety check for the number of files
- Consider using a more specific glob pattern
Here's a suggested implementation:
export const teardown = async () => {
- const files = await glob("**/*.db3*", { cwd: __dirname });
- await Promise.all(files.map((file) => unlink(join(__dirname, file))));
+ try {
+ const files = await glob("**/*.db3*", {
+ cwd: __dirname,
+ onlyFiles: true,
+ absolute: false,
+ });
+
+ if (files.length > 100) { // arbitrary limit
+ throw new Error("Too many database files to delete");
+ }
+
+ await Promise.all(
+ files.map(async (file) => {
+ try {
+ await unlink(join(__dirname, file));
+ } catch (err) {
+ console.warn(`Failed to delete ${file}:`, err);
+ }
+ }),
+ );
+ } catch (err) {
+ console.error("Teardown failed:", err);
+ throw err;
+ }
};Also, verify that this cleanup doesn't affect any persistent databases:
✅ Verification successful
Review suggestions are appropriate but can be simplified
The review suggestions are valid but can be adjusted based on the codebase context. The .db3 files are exclusively used for test databases and temporary client storage, with no persistent databases in source control. Here's a more focused suggestion:
export const teardown = async () => {
- const files = await glob("**/*.db3*", { cwd: __dirname });
- await Promise.all(files.map((file) => unlink(join(__dirname, file))));
+ try {
+ const files = await glob("**/*.db3*", { cwd: __dirname });
+ await Promise.all(
+ files.map(async (file) => {
+ try {
+ await unlink(join(__dirname, file));
+ } catch (err) {
+ console.warn(`Failed to delete ${file}:`, err);
+ }
+ }),
+ );
+ } catch (err) {
+ console.error("Teardown failed:", err);
+ }
};The file limit check can be omitted since these are test databases that are created and cleaned up during test runs. Basic error handling is still recommended to ensure test cleanup is robust.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any .db3 files in source control that shouldn't be deleted
git ls-files "**/*.db3*"
Length of output: 26
Script:
#!/bin/bash
# Let's check the test files to understand the database usage context
rg -l "\.db3" --type ts
# Also check for database-related imports and configurations
rg "sqlite|\.db3" --type ts -C 2
Length of output: 3747
packages/frames-client/vitest.config.ts (1)
15-15: LGTM! Verify setup file and consider adding documentation.
The addition of the global setup is appropriate for managing test environment cleanup.
Let's verify the setup file exists and check its implementation:
Consider adding a code comment above the globalSetup configuration to document its purpose, particularly the database cleanup functionality:
test: {
globals: true,
environment: "happy-dom",
testTimeout: 60000,
+ // Configures global test setup for cleaning up SQLite database files after tests
globalSetup: ["./vitest.setup.ts"],
},✅ Verification successful
Setup file is properly implemented for database cleanup
The setup file exists and correctly implements database cleanup by:
- Using
globto find all.db3*files in the test directory - Properly cleaning up SQLite database files using
unlink - Following Node.js best practices with proper path handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and implementation of the setup file
# Check if the setup file exists
if [ -f "packages/frames-client/vitest.setup.ts" ]; then
echo "Setup file exists"
# Show the content to verify the cleanup implementation
cat "packages/frames-client/vitest.setup.ts"
else
echo "Setup file is missing"
fi
Length of output: 539
packages/frames-client/src/utils.ts (2)
4-4: LGTM! Type import aligns with frame action signing refactor.
The import of FrameActionInputs type is consistent with the PR's objective of refactoring the signing process and supporting V3 frames.
Line range hint 1-46: Verify the impact of removed client type checks.
The removal of client-specific type imports and the isReactNativeClient function aligns with the migration to FramesSigner. The remaining utility functions maintain their robustness with proper input validation and error handling.
Let's verify that all client-type checks have been properly migrated to use FramesSigner:
✅ Verification successful
The removal of client type checks is properly handled with the migration to FramesSigner
The verification confirms that:
- No references to
ReactNativeClientexist in the codebase - No instances of
isReactNativeClientchecks remain - The codebase has properly migrated to using
FramesSignertypes (V2FramesSignerandV3FramesSigner) with appropriate type checks throughisV3FramesSigner
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that client type checks have been properly migrated
# Check for any remaining references to ReactNativeClient
rg -l "ReactNativeClient"
# Check for new FramesSigner usage
rg "FramesSigner" -A 5
# Look for any potential missed client type checks
ast-grep --pattern 'isReactNativeClient'
Length of output: 10454
packages/frames-validator/src/validator.ts (3)
6-6: LGTM: Import of XmtpEnv type
The addition of the XmtpEnv type import aligns with the PR objective of supporting V3 frame validation.
40-41: Verify default behavior when env is not provided
The env parameter is correctly marked as optional, but we should verify the default behavior in validateFramesPost when env is undefined.
✅ Verification successful
Optional env parameter is properly handled with backward compatibility
The verification shows that:
- The
validateFramesPostfunction correctly accepts an optionalenvparameter in its implementation invalidation.ts - The function is used both with and without the env parameter in tests and documentation:
- Tests use it with "local" environment:
validateFramesPost(postData, "local") - README example shows usage without env parameter:
validateFramesPost(requestBody)
- Tests use it with "local" environment:
- The validator class properly passes the env parameter through to the validation function
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for validateFramesPost implementation to verify default behavior
ast-grep --pattern 'function validateFramesPost($_: $_, $_?: $_) {
$$$
}'
Length of output: 77
Script:
#!/bin/bash
# Let's try a different pattern to find the validateFramesPost implementation
ast-grep --pattern 'validateFramesPost($$$) {
$$$
}'
# Also search with ripgrep for more context
rg "validateFramesPost" -A 10
Length of output: 7268
Line range hint 46-56: Consider simplifying Promise handling and verify error cases
The implementation correctly handles the new async nature of validateFramesPost, but there are two suggestions:
- Since we're in an async function, the
Promise.resolvecalls are redundant and could be simplified. - The catch block should be more specific about which errors it handles.
Consider this refactor:
try {
const validationResponse = await validateFramesPost(payload, env);
- return await Promise.resolve({
+ return {
isValid: true,
clientProtocol: payload.clientProtocol,
message: validationResponse,
- });
+ };
- } catch {
+ } catch (error) {
+ // Handle specific validation errors
return Promise.resolve({
isValid: false,
+ error: error instanceof Error ? error.message : 'Unknown validation error'
});
}✅ Verification successful
Let me analyze the validateFramesPost error handling based on the search results. I can see that in packages/frames-validator/src/validation.ts, the function throws several specific error types:
- "Invalid wallet address"
- "Invalid inbox ID"
- "Invalid signature"
- "Mismatched URL"
- "Mismatched button index"
- "Mismatched conversation identifier"
- "Mismatched timestamp"
- "Mismatched state"
- "Missing input text"
Let me verify the error handling in the validator.ts file to see the full context.
Let me check the validation.ts file to see how these errors are handled and used.
The Promise.resolve is indeed redundant, but the generic error handling is appropriate
The current catch-all error handling in validator.ts is actually appropriate for this use case because:
- The validateFramesPost function throws specific, well-defined errors that all indicate invalid frame requests
- All error cases should result in the same outcome - marking the validation as invalid (isValid: false)
- Exposing specific error messages to the client could potentially leak sensitive validation details
The Promise.resolve calls are redundant though, since we're in an async function. Here's the suggested refactor:
async validate(
payload: XmtpOpenFramesRequest,
env?: XmtpEnv,
): Promise<ValidationResponse<XmtpValidationResponse, typeof this.protocolIdentifier>> {
try {
const validationResponse = await validateFramesPost(payload, env);
- return await Promise.resolve({
+ return {
isValid: true,
clientProtocol: payload.clientProtocol,
message: validationResponse,
- });
+ };
} catch {
- return Promise.resolve({
+ return {
isValid: false,
- });
+ };
}
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for specific error types thrown by validateFramesPost
rg -A 3 "throw.*Error" --type ts
Length of output: 71527
Script:
#!/bin/bash
# Get the full validator.ts file content
cat packages/frames-validator/src/validator.ts
Length of output: 1719
Script:
#!/bin/bash
# Get the validation.ts file content
cat packages/frames-validator/src/validation.ts
Length of output: 3833
packages/frames-validator/package.json (1)
43-44: Verify dependency versions for compatibility
Let's verify the compatibility and latest versions of the newly added and updated dependencies.
✅ Verification successful
Dependencies are using latest versions and no critical compatibility issues found
The verification shows that both dependencies are using their latest available versions:
@xmtp/[email protected]is the latest release@xmtp/[email protected]is the latest release
While there are some open issues in the repository, none indicate compatibility problems between these specific versions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check latest versions and compatibility of dependencies
# Check latest versions on npm
echo "Checking latest versions:"
for pkg in @xmtp/node-sdk @xmtp/proto; do
echo "$pkg:"
npm view $pkg versions --json | jq -r '.[-3:]'
done
# Check for any reported issues or breaking changes
echo -e "\nChecking for issues:"
gh search issues --repo xmtp/xmtp-js "in:title,body @xmtp/[email protected] OR @xmtp/[email protected]" --json title,url,state
Length of output: 4513
sdks/browser-sdk/src/Conversations.ts (1)
19-21: Verify worker client implementation.
The method looks correct but let's verify that the worker client properly handles the "syncAllConversations" message type.
✅ Verification successful
Based on the results, I can generate the final response as we have found clear evidence of the worker client implementation.
Worker client correctly implements "syncAllConversations" message handling
The implementation is properly wired up across the codebase:
- Message type is defined in
types/clientEvents.ts - Worker client handles the message in
workers/client.tswith appropriate response - The handler correctly delegates to
client.conversations.syncAll()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if WorkerClient implements the handler for "syncAllConversations"
# Search for "syncAllConversations" handler in worker client
ast-grep --pattern 'case "syncAllConversations": {
$$$
}'
# Also check for any direct references to this action
rg "syncAllConversations" -A 5
Length of output: 2105
packages/frames-client/src/types.ts (3)
7-10: LGTM! Clean type-only imports.
The imports are well-structured and follow TypeScript best practices by using type-only imports.
68-68: LGTM! Clean union type definition.
The union type correctly enables support for both V2 and V3 signers as mentioned in the PR objectives.
61-66: Consider aligning signature types between V2 and V3 signers.
The sign method returns different types in V2 and V3:
- V2:
Promise<signatureProto.Signature> - V3:
Promise<Uint8Array> | Uint8Array
This inconsistency could lead to confusion and potential type-casting issues. Consider:
- Using the same return type for both versions if possible
- If different types are necessary, document why they differ
Let's verify if this difference is intentional by checking other files:
packages/frames-client/package.json (2)
83-83: LGTM: uint8array-extras addition
The addition of [email protected] is appropriate for byte manipulation needs in the signing process.
66-66: Verify minimum required proto version for V3 support
The upgrade to @xmtp/proto ^3.72.3 seems related to V3 support. Let's verify this is the minimum required version.
✅ Verification successful
Let me search for more specific V3-related changes in the codebase to verify if this version is sufficient.
Let me check the frames-client package for any V3-specific requirements or changes.
Proto version 3.72.3 is appropriate for V3 support
The codebase shows active V3 implementation in frames-client with V3-specific types, tests, and client code. The proto package upgrade from 3.72.0 to 3.72.3 includes recent bug fixes that are likely important for V3 functionality, particularly around frames actions and member chain IDs. This version is suitable for the V3 support requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the changelog or git history for V3-related changes
gh api repos/xmtp/proto/releases | jq -r '.[] | select(.tag_name | contains("3.72")) | {tag: .tag_name, body: .body}' | grep -i "v3"
Length of output: 1383
Script:
#!/bin/bash
# Search for V3-related changes or requirements in the codebase
rg -i "v3|version 3" --type md
Length of output: 278
Script:
#!/bin/bash
# Search for V3-related changes in frames-client
rg -i "v3|version 3" packages/frames-client/
Length of output: 1365
sdks/browser-sdk/src/WorkerClient.ts (3)
2-2: LGTM: Import addition is appropriate
The new import of verifySignedWithPublicKey from wasm-bindings aligns with the new verification functionality.
46-48: LGTM: Getter implementation follows established patterns
The getter properly exposes the underlying client's installationIdBytes property, maintaining consistency with other getter implementations in the class.
137-139: LGTM: Clean implementation of installation key signing
The method properly delegates to the client implementation, allowing errors to propagate as expected.
sdks/node-sdk/test/Client.test.ts (1)
2-2: LGTM!
The import of uint8ArrayToHex aligns with the PR requirement to convert action frame bytes to hex string before signing.
sdks/browser-sdk/src/Client.ts (3)
33-33: LGTM: Well-implemented installation ID bytes accessor
The addition of the #installationIdBytes private field and its corresponding getter follows TypeScript best practices and maintains consistency with existing patterns.
Also applies to: 111-113
78-78: LGTM: Proper initialization of installation ID bytes
The initialization of #installationIdBytes is correctly placed within the init method alongside related installation ID assignments.
318-342: Verify worker implementation of signature methods
Let's ensure the worker properly implements these new signature-related message handlers.
✅ Verification successful
Worker implementation properly handles signature methods
The worker implementation in sdks/browser-sdk/src/workers/client.ts correctly handles all three signature-related messages:
signWithInstallationKeywith signature text parameterverifySignedWithInstallationKeywith signature text and bytes parametersverifySignedWithPublicKeywith signature text, bytes and public key parameters
Each handler properly forwards the parameters to the corresponding client methods and returns the results via postMessage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for corresponding worker message handlers
rg -t typescript "case \"(signWithInstallationKey|verifySignedWithInstallationKey|verifySignedWithPublicKey)\"" sdks/browser-sdk/src/workers/
Length of output: 176
Script:
#!/bin/bash
# Search for corresponding worker message handlers without file type restriction
rg "case \"(signWithInstallationKey|verifySignedWithInstallationKey|verifySignedWithPublicKey)\"" sdks/browser-sdk/src/workers/
# Also search for these message handlers with a different pattern
rg "(signWithInstallationKey|verifySignedWithInstallationKey|verifySignedWithPublicKey)" sdks/browser-sdk/src/workers/
# List files in workers directory to understand the structure
fd . sdks/browser-sdk/src/workers/
Length of output: 1168
Script:
#!/bin/bash
# Let's check the implementation details of these handlers in the worker
rg -A 10 "case \"signWithInstallationKey\"" sdks/browser-sdk/src/workers/client.ts
rg -A 10 "case \"verifySignedWithInstallationKey\"" sdks/browser-sdk/src/workers/client.ts
rg -A 10 "case \"verifySignedWithPublicKey\"" sdks/browser-sdk/src/workers/client.ts
Length of output: 1154
sdks/browser-sdk/src/types/clientEvents.ts (3)
38-38: LGTM: Installation ID bytes field addition
The addition of installationIdBytes as Uint8Array aligns with the PR objectives and follows proper typing conventions.
262-267: LGTM: Conversations sync action addition
The syncAllConversations action type is properly defined and aligns with the PR objectives for adding Conversations.syncAll functionality.
161-187: Verify hex string conversion requirement
The signature-related action types are well-structured. However, based on the PR objectives, we need to ensure that action frame bytes are converted to hex string before signing.
Let's verify the implementation of this requirement:
✅ Verification successful
Let me search for the actual implementation of the hex conversion to verify if it's being handled correctly.
Hex string conversion is properly implemented
The codebase shows consistent usage of hex string conversion before signing:
- The
uint8ArrayToHexutility fromuint8array-extrasis used in test cases and validation - Both direct hex conversion via
toString("hex")anduint8ArrayToHexare used appropriately when callingsignWithInstallationKey - The implementation is consistent across different packages (frames-client, frames-validator) and SDKs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for signWithInstallationKey implementations to verify hex string conversion
rg -A 5 "signWithInstallationKey" --type ts
Length of output: 3966
Script:
#!/bin/bash
# Search for uint8ArrayToHex implementation and usage
rg -A 3 "uint8ArrayToHex" --type ts
# Also search for any other hex conversion implementations
rg -A 3 "toString\(.*hex" --type ts
Length of output: 2801
sdks/node-sdk/src/Client.ts (5)
20-20: LGTM: Import statement is correctly added
The import of verifySignedWithPublicKeyBinding is properly added and follows good naming conventions.
167-169: LGTM: New getter properly exposes installation ID bytes
The getter follows the established pattern and correctly delegates to the inner client, fulfilling the PR objective of adding installationIdBytes accessors.
413-422: LGTM: Verification method properly handles errors
The method follows good practices by:
- Converting exceptions to boolean results
- Maintaining consistency with other verification methods
- Properly delegating to the inner client
425-439: LGTM: Static verification method properly implemented
The method is well-implemented with:
- Appropriate use of static modifier
- Consistent error handling pattern
- Proper use of the imported binding function
184-190: LGTM: Method signature simplified while maintaining functionality
The method has been correctly simplified to take only the new account address while maintaining proper error handling.
Let's verify that all calls to this method have been updated:
✅ Verification successful
Method signature change is consistent across the codebase
All calls to #addAccountSignatureText have been properly updated to use a single parameter (account address). The method is used consistently in both node-sdk and browser-sdk implementations, with all callers passing only the address parameter.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining calls that might pass two parameters
rg -g '*.ts' -g '*.js' "addAccountSignatureText.*,.*\)"
Length of output: 57
Script:
#!/bin/bash
# Search for all calls to addAccountSignatureText to verify the parameter usage
rg -g '*.ts' -g '*.js' "addAccountSignatureText\(" -A 2
Length of output: 1370
sdks/browser-sdk/src/workers/client.ts (3)
73-73: LGTM: Addition of installationIdBytes accessor
The addition of installationIdBytes to the initialization result aligns with the PR objectives and follows consistent naming conventions.
324-332: LGTM: Addition of syncAllConversations case
The implementation of syncAllConversations follows the established pattern of other sync methods and properly integrates with the worker's error handling.
220-253: Verify hex string conversion for action frame bytes
The implementation of signature methods looks good. However, per the PR objectives, action frame bytes must be converted to hex string before signing with signWithInstallationKey.
Let's verify the hex string conversion requirement:
packages/frames-client/src/client.ts (1)
19-22:
Update Constructor Usage Across Codebase
The FramesClient constructor now requires a FramesSigner instead of a Client or ReactNativeClient. This is a breaking change. Ensure that all instances where FramesClient is instantiated are updated accordingly to prevent runtime errors.
Run the following script to identify and update outdated constructor usages:
packages/frames-validator/src/validation.ts (3)
87-89: Consistent extraction of necessary fields in deserializeProtoMessage
The additional fields installationId, installationSignature, and inboxId are extracted from frameAction. Ensure that all necessary fields are consistently extracted and that any unused fields are omitted to keep the code clean.
16-19: Ensure all callers handle the new asynchronous function signature
The validateFramesPost function is now asynchronous and returns a Promise. Please verify that all callers of this function have been updated to handle the Promise correctly, using await or .then() as appropriate.
Run the following script to identify all usages of validateFramesPost and ensure they handle the asynchronous behavior:
✅ Verification successful
All callers correctly handle the asynchronous function signature
Based on the search results, all usages of validateFramesPost properly handle the asynchronous nature of the function:
- In
validator.ts: Usesawaitwith the function call - In
validation.test.ts: Usesawaitin test assertions and properly tests Promise rejections
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of 'validateFramesPost' in the codebase to ensure they are updated to handle Promises.
# Search for usages of 'validateFramesPost' in JavaScript and TypeScript files
rg 'validateFramesPost\(' -t js -t ts -A 2 -B 2
Length of output: 1915
18-19: Ensure env parameter is properly handled when undefined
The env parameter is optional in validateFramesPost, but it's passed to getInboxIdForAddress(walletAddress, env). Ensure that getInboxIdForAddress can handle an undefined env value or consider providing a default value to prevent potential runtime errors.
Run the following script to check the usage of getInboxIdForAddress and verify if it handles env being undefined:
✅ Verification successful
env parameter is properly handled with default value
The implementation in node-sdk (which is imported by frames-validator) shows that getInboxIdForAddress has a default value of "dev" for the env parameter:
export const getInboxIdForAddress = async (
accountAddress: string,
env: XmtpEnv = "dev",
) => {Additionally, the browser-sdk implementation also handles undefined env by defaulting to "dev":
const host = env ? ApiUrls[env] : ApiUrls.dev;🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if 'getInboxIdForAddress' can handle an undefined 'env' parameter.
# Search for the definition of 'getInboxIdForAddress'
rg 'function getInboxIdForAddress' -A 10
# Look for default parameter handling in its implementation
Length of output: 42
Script:
#!/bin/bash
# Let's try different search patterns and methods to locate the implementation
# Search for any occurrence of getInboxIdForAddress
rg "getInboxIdForAddress"
# Use ast-grep to find function declarations/expressions
ast-grep --pattern 'const getInboxIdForAddress = $_'
# Also search for export statements containing this function
ast-grep --pattern 'export const getInboxIdForAddress = $_'
# Search for type/interface definitions that might show the function signature
rg "getInboxIdForAddress.*:" -A 3
Length of output: 5165
packages/frames-validator/src/validation.test.ts (3)
177-210: Well-structured test cases covering key scenarios
The test suites for both V2 and V3 are comprehensive and effectively cover success and failure scenarios for signature verification and wallet address mismatches. The use of shared utility functions enhances code reuse and maintainability.
97-99: Confirm the scrambling logic for V3 installation signatures
When scrambling the installationSignature in V3 tests, ensure that the scrambling effectively invalidates the signature without causing unforeseen side effects. This will help in accurately testing the failure case for invalid signatures.
Consider verifying the scrambling effect:
✅ Verification successful
Let me analyze the scrambling logic by looking at the implementation first.
Scrambling implementation is appropriate for testing invalid signatures
The scrambleBytes function implements a simple byte reversal which effectively invalidates signatures while maintaining the same byte length. This is a suitable approach for testing signature validation because:
- It deterministically transforms the input by reversing byte order
- Preserves the signature length which is important for ECDSA signatures
- Guarantees the scrambled output will be invalid while still being a valid byte array
The implementation is used consistently for both V3 installation signatures and ECDSA compact signatures in the test file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Ensure scrambling changes the signature
# Print original and scrambled signatures to confirm they differ
echo "Original Signature:"
echo "$(echo "${postData.trustedData.messageBytes}" | base64 -d | hexdump -C)"
echo "Scrambled Signature:"
echo "$(echo "${reserialized}" | hexdump -C)"
Length of output: 592
Script:
#!/bin/bash
# Find the scrambleBytes implementation
ast-grep --pattern 'function scrambleBytes'
# Also check the test file content for context
rg -A 5 "scrambleBytes" packages/frames-validator/src/validation.test.ts
Length of output: 754
55-60: Ensure correct data format for signWithInstallationKey
In the V3 signer implementation, the sign method converts the digest to a hex string before signing. Verify that the signWithInstallationKey method expects a hex string as input. If it expects a Uint8Array, the conversion might be unnecessary and could lead to incorrect signatures.
Run the following script to check the expected input type of signWithInstallationKey:
✅ Verification successful
Based on the search results, I can now generate the final response:
The hex string conversion is correct and necessary
The codebase consistently shows that signWithInstallationKey expects a string parameter across different implementations:
- In
sdks/node-sdk/src/Client.ts:signWithInstallationKey(signatureText: string) - In
sdks/browser-sdk/src/Client.ts:signWithInstallationKey(signatureText: string)
This is further validated by other usage examples in the codebase:
- In
packages/frames-client/src/client.test.ts, it usesuint8ArrayToHex(digest) - In
sdks/node-sdk/test/Client.test.ts, it usesuint8ArrayToHex(signatureText2)
The conversion from Uint8Array to hex string in the sign method is correct and aligned with the API requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the input type for signWithInstallationKey method.
# Search for the method signature in the codebase
rg 'signWithInstallationKey' packages/@xmtp/node-sdk/src/
# Expected: The method signature should indicate the expected input type (hex string or Uint8Array).
Length of output: 126
Script:
#!/bin/bash
# Let's search for signWithInstallationKey across the entire codebase
rg "signWithInstallationKey" -A 2
# Also search for any interface or type definitions related to this method
ast-grep --pattern 'interface $_ {
$$$
signWithInstallationKey($_): $_
$$$
}'
Length of output: 2610
| } | ||
|
|
||
| checkUntrustedData(untrustedData, actionBody); | ||
|
|
||
| return { | ||
| actionBody, | ||
| verifiedWalletAddress, | ||
| verifiedWalletAddress: walletAddress, |
There was a problem hiding this comment.
Ensure walletAddress is securely verified before returning
In the return statement, verifiedWalletAddress is set to walletAddress from untrustedData. While in the V2 frame case the walletAddress is verified, in the non-V2 frame case it may not be. To maintain security, ensure that walletAddress is properly verified before returning it in all cases.
Consider updating the return statement to use a verified wallet address. You might need to retrieve and verify the wallet address in the non-V2 frame case similarly to how it's handled in the V2 frame case.
Deploying xmtp-js-docs with
|
| Latest commit: |
86a8f9f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d604f0e0.xmtp-js.pages.dev |
| Branch Preview URL: | https://rygine-frames-update.xmtp-js.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/frames-validator/tsconfig.json (1)
8-8: Consider adding types for core dependenciesThe
typesarray only includes Vitest-related types. Given that this package is being updated for V3 support with breaking changes, consider adding types for core dependencies to improve type safety.Add relevant types to the array:
- "types": ["vitest", "vitest/globals"] + "types": ["vitest", "vitest/globals", "node", "@xmtp/proto"]packages/frames-client/tsconfig.json (1)
4-8: Consider browser compatibility with ESNextThe configuration is set to use ESNext for module, lib, and target. While this enables the latest JavaScript features, it might affect browser compatibility.
Consider one of these approaches:
- Add a build step that transpiles to a more compatible target for production
- Document the minimum browser versions supported
- Use a more conservative target (like ES2020) if broad browser support is needed
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/frames-client/tsconfig.json(1 hunks)packages/frames-validator/tsconfig.json(1 hunks)
🔇 Additional comments (5)
packages/frames-validator/tsconfig.json (1)
4-7: Verify browser compatibility requirements
The configuration uses "ESNext" for lib, module, and target, which generates the most modern JavaScript features. This might cause compatibility issues in environments that don't support the latest ECMAScript features.
Let's check the package.json to verify the supported environments:
✅ Verification successful
ESNext configuration is appropriate for this package
The aggressive ESNext configuration is justified because:
- The package is a Node.js module (type: "module") with Node >=20 requirement
- It's built for both ESM (
index.js) and CommonJS (index.cjs) formats - No browser-specific code or dependencies are present
- The build setup uses Rollup with proper module resolution
- Dependencies are modern packages like viem, @noble/curves that also target modern environments
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check package.json for browser targets and dependencies
# Check if package.json exists and contains browserslist or engines field
cat packages/frames-validator/package.json | jq '.browserslist, .engines'
# Check if any transpilation/polyfill tools are used
fd -t f "package.json|rollup.config" -x grep -l "babel|core-js|@babel" {} \;
Length of output: 183
Script:
#!/bin/bash
# Let's check if this package is used in browser environments and its build configuration
# Check if the package is imported in any browser-related files
rg -l "frames-validator" | grep -i "browser\|web\|client"
# Check build configuration and dependencies
cat packages/frames-validator/package.json | jq '{type, main, module, exports, dependencies, devDependencies}'
# Check if there are any browser-specific tests or code
fd -t f . packages/frames-validator -x grep -l "browser\|window\|document" {} \;
Length of output: 464
Script:
#!/bin/bash
# Let's try a different approach to check the package configuration and usage
# Check package.json content
cat packages/frames-validator/package.json
# Look for any imports of this package
rg -l "@farcaster/frames-validator"
# Check if there are any rollup/build configs
fd -t f "rollup.config|build.config|vite.config" packages/frames-validator
Length of output: 1911
Script:
#!/bin/bash
# Let's check the rollup config and build output formats
# Check rollup configuration
cat packages/frames-validator/rollup.config.js
# Check if there are any browser-specific imports or code in the source
rg "window\.|document\.|browser" packages/frames-validator/src/
Length of output: 964
packages/frames-client/tsconfig.json (4)
10-10: LGTM: Include paths are properly configured
The include paths appropriately cover source files, build configuration, and test setup files.
2-2: Verify React dependencies are properly handled
The change from react-sdk.json to base.json removes React-specific TypeScript configurations. Ensure this package no longer requires React-specific type definitions.
✅ Verification successful
No React dependencies found - configuration change is appropriate
The package.json inspection shows no React-related dependencies, and there are no React imports in the source code. This confirms that switching from react-sdk.json to base.json is the correct approach since the package doesn't require React-specific TypeScript configurations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for React dependencies and usage in the frames-client package
echo "Checking for React dependencies..."
cat packages/frames-client/package.json | jq '.dependencies,.devDependencies'
echo "Checking for React imports..."
rg -l "from 'react'" packages/frames-client/src/
rg -l "from \"react\"" packages/frames-client/src/
Length of output: 988
8-8: Ensure vitest configuration is complete
The addition of vitest types suggests a transition in testing infrastructure.
✅ Verification successful
Vitest configuration is properly set up and complete
The verification shows a complete Vitest setup with:
- A properly configured
vitest.config.tswith test environment and globals - Multiple test files present in the codebase (
client.test.ts,converters.test.ts,utils.test.ts) - Required Vitest dependency in package.json
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify vitest setup is complete
echo "Checking for vitest config file..."
cat packages/frames-client/vitest.config.ts
echo "Checking for test files..."
fd -e test.ts -e spec.ts . packages/frames-client/src/
echo "Checking for vitest dependencies..."
cat packages/frames-client/package.json | jq '.devDependencies | with_entries(select(.key | contains("vitest")))'
Length of output: 1091
6-6: Verify bundler compatibility
Setting "moduleResolution": "bundler" is a newer TypeScript feature that might require specific bundler configurations.
✅ Verification successful
Bundler configuration is properly set up for moduleResolution: "bundler"
The project is using Rollup as the bundler with appropriate TypeScript integration through @rollup/plugin-typescript. The configuration includes:
- ES module output format (
format: "es") - TypeScript configuration paths support via
rollup-plugin-tsconfig-paths - Proper type declarations generation using
rollup-plugin-dts
All necessary bundler-related dependencies are present in package.json:
rollupand associated plugins- TypeScript bundling support through
@rollup/plugin-typescript
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check bundler configuration
echo "Checking rollup config..."
cat packages/frames-client/rollup.config.js
echo "Checking package.json for bundler-related dependencies..."
cat packages/frames-client/package.json | jq '.devDependencies | keys[] | select(contains("rollup") or contains("webpack") or contains("esbuild"))'
Length of output: 1610
rygine
changed the title
rygine
force-pushed
the
rygine/frames-update
branch
from
0e9be1f to
7543fdd
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (14)
packages/frames-client/src/utils.ts (2)
Line range hint
9-13: Consider adding input validation to concatArraysWhile the function works correctly, it might benefit from input validation to handle edge cases.
Consider this improvement:
export function concatArrays(...arrays: Uint8Array[]): Uint8Array { + if (arrays.length === 0) { + return new Uint8Array(0); + } return new Uint8Array( arrays.reduce<number[]>((acc, arr) => acc.concat(Array.from(arr)), []), ); }
Line range hint
24-41: Enhance error handling in buildOpaqueIdentifierThe error message could be more descriptive to help developers identify which specific inputs are missing.
Consider this improvement:
if (!conversationTopic || !participantAccountAddresses.length) { throw new InvalidArgumentsError( - "Missing conversation topic or participants", + `Invalid inputs: ${!conversationTopic ? 'conversation topic is missing' : '' + }${!participantAccountAddresses.length ? 'participant addresses are empty' : ''}` ); }packages/frames-client/src/types.ts (3)
55-58: Consider consistent Promise patterns across methods.While
address()supports both sync and async returns,getPublicKeyBundle()is Promise-only. Consider allowing synchronous returns for consistency, if applicable.- getPublicKeyBundle: () => Promise<publicKeyProto.PublicKeyBundle>; + getPublicKeyBundle: () => Promise<publicKeyProto.PublicKeyBundle> | publicKeyProto.PublicKeyBundle;
70-73: LGTM! Consider adding runtime type validation.The type guard is well-implemented for TypeScript type checking. Consider adding runtime validation of the actual method implementations for more robust error handling.
export const isV3FramesSigner = ( signer: FramesSigner, ): signer is V3FramesSigner => { - return "installationId" in signer && "inboxId" in signer; + return ( + "installationId" in signer && + "inboxId" in signer && + typeof signer.installationId === "function" && + typeof signer.inboxId === "function" + ); };
55-73: Add JSDoc comments for better documentation.Consider adding JSDoc comments to document:
- The differences between V2 and V3 signers
- The purpose and usage of each method
- The reason for different sign method return types
+/** + * V2 Frame signer interface supporting legacy signature format. + */ export type V2FramesSigner = { address: () => Promise<string> | string; getPublicKeyBundle: () => Promise<publicKeyProto.PublicKeyBundle>; sign: (message: Uint8Array) => Promise<signatureProto.Signature>; }; +/** + * V3 Frame signer interface with enhanced capabilities. + * Supports installation and inbox identification. + */ export type V3FramesSigner = { installationId: () => Promise<Uint8Array> | Uint8Array; inboxId: () => Promise<string> | string; address: () => Promise<string> | string; sign: (message: Uint8Array) => Promise<Uint8Array> | Uint8Array; };packages/frames-client/src/client.ts (1)
77-97: Consider enhancing type safety for payload constructionWhile the V2/V3 branching logic is correct, we can improve type safety by creating specific payload types.
Consider creating separate interfaces for V2 and V3 payloads and using type guards:
interface V2FrameActionPayload { actionBody: Uint8Array; inboxId: string; installationId: Uint8Array; installationSignature: Uint8Array; signature: Uint8Array; signedPublicKeyBundle: frames.SignedPublicKeyBundle; } interface V3FrameActionPayload { actionBody: Uint8Array; inboxId: string; installationId: Uint8Array; installationSignature: Uint8Array; signature: undefined; signedPublicKeyBundle: undefined; } type FrameActionPayload = V2FrameActionPayload | V3FrameActionPayload;packages/frames-validator/src/validation.ts (2)
47-47: Enhance error messages with more contextThe error messages could be more descriptive to help with debugging. Consider adding more context about the validation step that failed.
-throw new Error("Invalid wallet address"); +throw new Error("Frame validation failed: Provided wallet address does not match the verified address"); -throw new Error("Invalid inbox ID"); +throw new Error("Frame validation failed: Provided inbox ID does not match the address's inbox ID"); -throw new Error("Invalid signature"); +throw new Error("Frame validation failed: Installation signature verification failed");Also applies to: 53-53, 66-66
31-31: Remove redundant commentThe comment
// not necessaryseems out of place. If theinstallationIdis truly not necessary, consider documenting why or remove the comment.packages/frames-validator/src/validation.test.ts (3)
18-24: Consider using crypto.getRandomValues for stronger signature invalidationWhile reversing bytes works for testing invalid signatures, using cryptographically secure random bytes would provide stronger test coverage against signature validation.
function scrambleBytes(bytes: Uint8Array) { - const scrambled = new Uint8Array(bytes.length); - for (let i = 0; i < bytes.length; i++) { - scrambled[i] = bytes[bytes.length - i - 1]; - } - return scrambled; + const scrambled = new Uint8Array(bytes.length); + getRandomValues(scrambled); + return scrambled; }
110-121: Extract common message serialization logicThe FrameAction encoding logic is duplicated. Consider extracting it into a helper function.
function serializeFrameAction(deserialized: any, isV3: boolean) { return frames.FrameAction.encode({ actionBody: deserialized.actionBodyBytes, signature: isV3 ? undefined : deserialized.signature, signedPublicKeyBundle: isV3 ? undefined : deserialized.signedPublicKeyBundle, installationSignature: isV3 ? deserialized.installationSignature : new Uint8Array(), installationId: isV3 ? deserialized.installationId : new Uint8Array(), inboxId: isV3 ? deserialized.inboxId : "", }).finish(); }Also applies to: 155-166
177-211: Consider adding edge case testsThe test suite has good coverage of main scenarios, but could benefit from additional edge cases:
- Empty/null frame URL
- Invalid button index
- Empty participant addresses
- Malformed message bytes
Would you like me to help generate these additional test cases?
packages/frames-client/src/client.test.ts (3)
19-33: Consider adding error handling in V2 setupThe setup function should handle potential client creation failures.
const getV2Setup = async () => { + try { const client = await Client.create(Wallet.createRandom(), { env: "local" }); const signer: V2FramesSigner = { address: () => client.address, getPublicKeyBundle: () => client.keystore.getPublicKeyBundle(), sign: (digest: Uint8Array) => client.keystore.signDigest({ digest, identityKey: true, prekeyIndex: undefined, }), }; const framesClient = new FramesClient(signer); return { signer, framesClient }; + } catch (error) { + throw new Error(`Failed to setup V2 client: ${error}`); + } };
35-56: Consider adding error handling in V3 setupThe setup function should handle potential client creation failures and validate the encryption key.
const getV3Setup = async () => { + try { const encryptionKey = getRandomValues(new Uint8Array(32)); + if (encryptionKey.length !== 32) { + throw new Error('Invalid encryption key length'); + } const wallet = Wallet.createRandom(); const client = await V3Client.create( { getAddress: () => wallet.address, signMessage: async (message: string) => getBytes(await wallet.signMessage(message)), }, encryptionKey, { env: "local" }, ); const signer: V3FramesSigner = { address: () => client.accountAddress, installationId: () => client.installationIdBytes, inboxId: () => client.inboxId, sign: (digest: Uint8Array) => client.signWithInstallationKey(uint8ArrayToHex(digest)), }; const framesClient = new FramesClient(signer); return { signer, framesClient }; + } catch (error) { + throw new Error(`Failed to setup V3 client: ${error}`); + } };
204-235: Enhance test coverageWhile the basic test cases are covered, consider adding these scenarios:
- Error cases for both V2 and V3
- Edge cases around signature validation
- Network failure scenarios
Add these test cases to both V2 and V3 describe blocks:
it("should handle network failures gracefully", async () => { const { framesClient } = await getV2Setup(); // or getV3Setup() framesClient.proxy.post = jest.fn().mockRejectedValue(new Error("Network error")); await expect(framesClient.signFrameAction({ frameUrl: "https://example.com", buttonIndex: 1, // ... other required params })).rejects.toThrow("Network error"); }); it("should handle invalid frame URLs", async () => { const { framesClient } = await getV2Setup(); // or getV3Setup() await expect(framesClient.signFrameAction({ frameUrl: "invalid-url", buttonIndex: 1, // ... other required params })).rejects.toThrow(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (19)
packages/frames-client/package.json(1 hunks)packages/frames-client/src/client.test.ts(1 hunks)packages/frames-client/src/client.ts(3 hunks)packages/frames-client/src/index.test.ts(0 hunks)packages/frames-client/src/index.ts(1 hunks)packages/frames-client/src/types.ts(2 hunks)packages/frames-client/src/utils.ts(1 hunks)packages/frames-client/tsconfig.json(1 hunks)packages/frames-client/vitest.config.ts(1 hunks)packages/frames-client/vitest.setup.ts(1 hunks)packages/frames-validator/package.json(2 hunks)packages/frames-validator/src/index.test.ts(0 hunks)packages/frames-validator/src/index.ts(1 hunks)packages/frames-validator/src/validation.test.ts(1 hunks)packages/frames-validator/src/validation.ts(2 hunks)packages/frames-validator/src/validator.ts(2 hunks)packages/frames-validator/tsconfig.json(1 hunks)packages/frames-validator/vitest.config.ts(1 hunks)packages/frames-validator/vitest.setup.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/frames-client/src/index.test.ts
- packages/frames-validator/src/index.test.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/frames-client/package.json
- packages/frames-client/src/index.ts
- packages/frames-client/tsconfig.json
- packages/frames-client/vitest.config.ts
- packages/frames-client/vitest.setup.ts
- packages/frames-validator/package.json
- packages/frames-validator/src/index.ts
- packages/frames-validator/src/validator.ts
- packages/frames-validator/tsconfig.json
- packages/frames-validator/vitest.config.ts
- packages/frames-validator/vitest.setup.ts
🔇 Additional comments (14)
packages/frames-client/src/utils.ts (2)
1-5: LGTM! Import changes align with new architecture
The removal of React Native specific imports and the focus on FrameActionInputs type aligns well with the new architecture using FramesSigner. This change simplifies the codebase by removing platform-specific dependencies.
Line range hint 24-41: Verify compatibility with V3 frames
The buildOpaqueIdentifier function handles sensitive cryptographic operations. Let's ensure it's being used correctly with V3 frames.
✅ Verification successful
V3 frame compatibility is maintained in the cryptographic utilities
The code analysis reveals that buildOpaqueIdentifier is version-agnostic and works correctly with both V2 and V3 frames:
- The function accepts a generic
FrameActionInputstype and handles two distinct cases:- Group-based identification (using
groupIdandgroupSecret) - Conversation-based identification (using
conversationTopicandparticipantAccountAddresses)
- Group-based identification (using
- The V3 frame support is properly integrated through the
V3FramesSignertype, which is correctly handled in the client implementation - The cryptographic operations (sha256, base64 encoding) are fundamental operations that remain consistent across versions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of buildOpaqueIdentifier in V3-related code
rg -A 5 "buildOpaqueIdentifier.*V3"
# Search for test cases covering V3 scenarios
rg -A 10 "describe.*V3.*buildOpaqueIdentifier"
Length of output: 86
Script:
#!/bin/bash
# Let's search for buildOpaqueIdentifier usage more broadly
rg -A 5 "buildOpaqueIdentifier"
# Search for V3 frame-related files and their content
fd -e ts -e js ".*[vV]3.*frame.*"
# Search for test files containing V3 frame tests
fd -e test.ts -e spec.ts | xargs rg "V3.*frame"
Length of output: 2727
Script:
#!/bin/bash
# Let's check the FrameActionInputs type definition to understand the input structure
ast-grep --pattern 'type FrameActionInputs = {
$$$
}'
# Let's also check for any version-specific types or interfaces
rg -A 10 "type.*V3|interface.*V3"
# Check the frames.proto file for message definitions
fd -e proto | xargs rg "message.*Frame"
Length of output: 2877
packages/frames-client/src/types.ts (3)
7-10: LGTM! Clean type-only imports.
The imports correctly use TypeScript's type-only import syntax, which is optimal for type definitions.
68-68: LGTM! Clean union type definition.
The union type correctly allows for handling both V2 and V3 signers polymorphically.
61-66: Verify the sign method return type difference between V2 and V3.
The sign method returns different types in V2 (signatureProto.Signature) vs V3 (Uint8Array). Please verify if this difference is intentional and document the reason if it is.
✅ Verification successful
Let me search for more context about the V2 and V3 implementations to understand this design choice better.
Let me search for any migration guides or documentation that might explain this change, and also look for actual usage of these signers.
Let me check the actual implementation of V3 frame actions to understand how these signatures are used.
Based on the evidence from the code, I can now provide a final response:
The sign method return type difference between V2 and V3 is intentional and correct.
The code shows that V2 and V3 use different signature schemes:
- V2 uses
signatureProto.Signaturewith additional metadata like public key bundles - V3 uses raw
Uint8Arraysignatures as part of a simpler signing scheme where the signature is used directly in theinstallationSignaturefield
This is clearly demonstrated in the client implementation where:
- V3 payloads use the signature directly without additional wrapping
- V2 payloads include the signature as part of a more complex bundle with public keys
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for sign method usage to understand the implications
rg -A 5 "sign.*message.*Uint8Array.*" --type ts
Length of output: 1234
Script:
#!/bin/bash
# Search for V2 and V3 signer implementations and usages
rg -A 10 "class.*implements.*FramesSigner" --type ts
# Also search for signature proto usage
rg "signatureProto.Signature" --type ts
Length of output: 198
Script:
#!/bin/bash
# Search for documentation about V3 changes
fd README.md | xargs rg -A 5 "V3.*Frame"
# Look for actual usage of sign method
rg -A 5 "\.sign\(" --type ts
# Check for any migration guides
fd MIGRATION --type f
Length of output: 7267
Script:
#!/bin/bash
# Search for V3 frame action implementations
rg -B 5 -A 10 "V3.*Frame.*Action" --type ts
# Look for signature handling in the client
ast-grep --pattern 'if (isV3FramesSigner($_)) {
$$$
}'
Length of output: 3903
packages/frames-client/src/client.ts (5)
2-2: LGTM: Import statements are properly structured
The new imports support V3 functionality and follow TypeScript best practices.
Also applies to: 8-12
16-22: LGTM: Well-structured class initialization
The changes properly encapsulate the signer and proxy using private fields, and the constructor correctly handles the new FramesSigner parameter with appropriate default proxy initialization.
24-26: LGTM: Clean getter implementation
The proxy getter provides appropriate access to the private field while maintaining encapsulation.
53-53: Error handling suggestion from previous review still applies
The async call to this.#signer.address() should include error handling as suggested in the previous review.
77-97: Error handling suggestion from previous review still applies
The multiple async operations in the buildSignedFrameAction method should include error handling as suggested in the previous review.
packages/frames-validator/src/validation.ts (3)
1-2: LGTM: Function signature changes align with V3 support requirements
The changes to make validateFramesPost async and add the env parameter are well-structured and necessary for V3 frame validation support.
Also applies to: 16-19
38-67: Wallet address verification is now secure
The implementation now properly verifies the wallet address in both V2 and V3 cases:
- V2: Through signature and public key bundle verification
- V3: Through inbox ID verification against the network
This addresses the security concern raised in the previous review.
16-19: Verify usage of the new async validateFramesPost
Since this is a breaking change, let's verify that all callers have been updated to handle the async nature of the function.
✅ Verification successful
Let me gather more context to verify the async usage properly.
All usages of validateFramesPost properly handle async behavior
Based on the search results, all calls to validateFramesPost are properly awaited:
- In
validator.ts:const validationResponse = await validateFramesPost(payload, env); - In
validation.test.ts:const validated = await validateFramesPost(postData, "local");await expect(() => validateFramesPost(postData, "local")).rejects.toThrow();
The README.md example was a code snippet showing the return value usage, not the actual function call.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all calls to validateFramesPost to ensure they're properly awaited
rg -l "validateFramesPost\(" | while read -r file; do
echo "=== Checking $file ==="
# Look for calls without await
rg -A 2 "validateFramesPost\(" "$file" | grep -v "await.*validateFramesPost"
done
Length of output: 1401
Script:
#!/bin/bash
# Search for actual validateFramesPost calls with context
rg -B 2 -A 2 "validateFramesPost\(" --type ts
# Also check test files specifically as they often contain usage examples
rg -B 2 -A 2 "validateFramesPost\(" --type test
Length of output: 1992
packages/frames-validator/src/validation.test.ts (1)
42-63: LGTM! V3 setup follows best practices
The V3 setup implementation correctly:
- Uses secure random encryption keys
- Properly implements wallet signing interface
- Correctly sets up the V3FramesSigner with all required methods
| } | ||
| } else { | ||
| // make sure inbox IDs match | ||
| const addressInboxId = await getInboxIdForAddress(walletAddress, env); |
There was a problem hiding this comment.
The wallet address is untrusted data. You can make it say anything.
Having it match the inbox ID doesn't actually tell us that the signer is a member of an inbox with that wallet address.
Summary
This PR adds support for V3 to the Frames client and validator.
Frames client
@xmtp/protoFramesSignertypeBREAKING CHANGE
This update requires passing a
FramesSignerwhen instantiating a newFramesClient. This makes it easier to handle both V2 and V3.Frames validator
BREAKING CHANGE
validateFramesPostis now async to support fetching the inbox ID of an account address when validating V3 frames. It also accepts a second parameter to specify the environment to use when looking up inbox IDs. In production, this parameter must be set toproduction.Notes
Since the
Client.signWithInstallationKeymethods only allow signing astring, the action frame bytes to be signed must be converted to a hex string first. We must make sure all SDKs are doing the conversion the same.Summary by CodeRabbit
Release Notes
New Features
@xmtp/frames-clientand@xmtp/frames-validatorpackages.@xmtp/frames-clientand@xmtp/frames-validatorpackages, including V3 support.Improvements
FramesClientclass.Tests
Documentation