Refactor signing in browser and node SDKs #718
Conversation
🦋 Changeset detectedLatest commit: a1a16a0 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 changes across multiple files in both the browser and node SDKs. Key modifications include updating the changeset configuration to ignore a specific package, enhancing the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
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: 6
🧹 Outside diff range and nitpick comments (33)
.changeset/config.json (1)
15-15: LGTM! Consider adding a comment to document the ignore reason.The addition of "@xmtp/react-vite-browser-sdk-example" to the ignore list is appropriate as example packages typically don't need versioning.
Consider adding a comment to document why this package is ignored:
- "ignore": ["@xmtp/react-vite-browser-sdk-example"] + // Ignore example packages as they don't need versioning + "ignore": ["@xmtp/react-vite-browser-sdk-example"]sdks/node-sdk/vitest.setup.ts (1)
8-11: Consider enhancing test cleanup strategyGiven the PR's introduction of new Client features (account management, installations), the test cleanup strategy should be more comprehensive:
- Consider implementing a test context manager to track created databases
- Add cleanup for both account and installation-specific databases
- Implement proper cleanup between test cases to prevent interference
Would you like assistance in implementing a more robust test cleanup strategy that handles these scenarios?
sdks/browser-sdk/src/utils/signer.ts (2)
6-12: Consider enhancing documentation for smart contract wallet fields.The
Signertype is well-designed with a clear separation between core and smart contract wallet functionality. However, the documentation could be more detailed.Consider expanding the comment to explain why these fields are specific to smart contract wallets:
- // these fields indicate that the signer is a smart contract wallet + /** + * The following fields are specific to smart contract wallets: + * - getBlockNumber: Required for tracking on-chain state + * - getChainId: Required for network identification and signature validation + */
14-19: Consider strengthening the type guard implementation.While the current implementation is clean, it might give false positives if properties exist but are undefined.
Consider this more robust implementation:
export const isSmartContractSigner = ( signer: Signer, ): signer is SmartContractSigner => - "getBlockNumber" in signer && "getChainId" in signer; + typeof signer.getBlockNumber === 'function' && + typeof signer.getChainId === 'function';This change ensures the properties are actually callable functions rather than just existing properties.
sdks/node-sdk/src/helpers/signer.ts (3)
1-4: LGTM! Consider adding JSDoc comments.The type definitions are well-structured and handle both synchronous and asynchronous cases appropriately. The return types are suitable for their respective blockchain operations.
Consider adding JSDoc comments to document the purpose and usage of each type:
+/** Signs a message and returns the signature as a byte array */ export type SignMessage = (message: string) => Promise<Uint8Array> | Uint8Array; +/** Returns the account address associated with the signer */ export type GetAddress = () => Promise<string> | string; +/** Returns the current chain ID */ export type GetChainId = () => bigint; +/** Returns the current block number */ export type GetBlockNumber = () => bigint;
6-12: LGTM! Consider adding method constraints.The
Signertype is well-structured with a clear separation between basic and smart contract wallet functionality.Consider adding constraints to prevent invalid combinations:
export type Signer = { getAddress: GetAddress; signMessage: SignMessage; // these fields indicate that the signer is a smart contract wallet - getBlockNumber?: GetBlockNumber; - getChainId?: GetChainId; + getBlockNumber?: GetBlockNumber & (GetChainId extends undefined ? never : GetBlockNumber); + getChainId?: GetChainId & (GetBlockNumber extends undefined ? never : GetChainId); };This ensures that either both smart contract methods are present or neither is present, preventing partial implementations.
14-19: LGTM! Consider adding runtime validation.The
SmartContractSignertype and type guard are well-implemented using TypeScript's type system features.Consider enhancing the type guard with runtime validation:
export const isSmartContractSigner = ( signer: Signer, -): signer is SmartContractSigner => - "getBlockNumber" in signer && "getChainId" in signer; +): signer is SmartContractSigner => { + if (!("getBlockNumber" in signer && "getChainId" in signer)) { + return false; + } + return typeof signer.getBlockNumber === 'function' && + typeof signer.getChainId === 'function'; +};This ensures that the methods not only exist but are also functions, providing stronger runtime guarantees.
sdks/browser-sdk/src/types/options.ts (1)
51-54: Enhance documentation for the disableAutoRegister optionThe implementation looks good and aligns with the PR objectives. However, consider enhancing the documentation to be more explicit about the default behavior and implications.
Consider updating the JSDoc comment to be more descriptive:
/** - * Disable automatic registration when creating a client + * Disable automatic registration when creating a client. + * By default (false), clients are automatically registered upon creation. + * When set to true, you must manually register the client using Client.register(). */ disableAutoRegister?: boolean;sdks/browser-sdk/test/helpers.ts (2)
31-40: Add error handling for signature failuresWhile the implementation is clean, consider adding error handling for potential signature failures. Users might reject signing requests or the wallet connection might fail.
export const createSigner = (user: User): Signer => { return { getAddress: () => user.account.address, signMessage: async (message: string) => { + try { const signature = await user.wallet.signMessage({ message, }); return toBytes(signature); + } catch (error) { + throw new Error(`Failed to sign message: ${error.message}`); + } }, }; };
61-68: Reduce duplication in environment option handlingThe environment option spreading logic is duplicated between
createClientandcreateRegisteredClient. Consider extracting this common logic.+const getDefaultOptions = (options?: ClientOptions) => ({ + ...options, + env: options?.env ?? "local", +}); + export const createClient = async (user: User, options?: ClientOptions) => { - const opts = { - ...options, - env: options?.env ?? "local", - }; + const opts = getDefaultOptions(options); return Client.create(createSigner(user), testEncryptionKey, { ...opts, disableAutoRegister: true, dbPath: `./test-${user.uuid}.db3`, }); }; export const createRegisteredClient = async ( user: User, options?: ClientOptions, ) => { - const opts = { - ...options, - env: options?.env ?? "local", - }; + const opts = getDefaultOptions(options); return Client.create(createSigner(user), testEncryptionKey, { ...opts, dbPath: `./test-${user.uuid}.db3`, }); };sdks/browser-sdk/src/WorkerClient.ts (3)
Line range hint
49-84: Consider enhancing error handling to provide more context.Currently, all methods silently return
undefinedon error. Consider either:
- Logging errors for debugging
- Propagating specific error types to callers
- Adding error context for better troubleshooting
Example implementation:
async createInboxSignatureText() { try { return await this.#client.createInboxSignatureText(); } catch (error) { - return undefined; + console.error('Failed to create inbox signature text:', error); + throw new Error('Failed to create inbox signature text: ' + error.message); } }
Line range hint
57-66: Align parameter naming between Worker and WASM client layers.The Worker layer uses
accountAddresswhile the WASM client uses wallet terminology (e.g.,addWalletSignatureText). Consider aligning these terms for better maintainability.Example alignment:
async addAccountSignatureText(accountAddress: string) { try { - return await this.#client.addWalletSignatureText( + return await this.#client.addAccountSignatureText( this.#accountAddress, accountAddress, ); } catch { return undefined; } }
Found an outdated method call that needs updating
The example code in
examples/react-vite-browser-sdk/src/createClient.tsis still using the old method namegetCreateInboxSignatureText(), while the SDK has been updated to usecreateInboxSignatureText().
examples/react-vite-browser-sdk/src/createClient.ts: UpdategetCreateInboxSignatureText()tocreateInboxSignatureText()🔗 Analysis chain
Line range hint
49-84: Verify all consumers are updated to use new method names.Since these methods are part of the public API and have been renamed, we should ensure all consumers are updated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for old method names to ensure they're not being used rg -l "getCreateInboxSignatureText|getAddWalletSignatureText|getRevokeWalletSignatureText|getRevokeInstallationsSignatureText" \ --type ts \ --type tsxLength of output: 177
Script:
#!/bin/bash # Search for old method names to ensure they're not being used, fixing the file type rg -l "getCreateInboxSignatureText|getAddWalletSignatureText|getRevokeWalletSignatureText|getRevokeInstallationsSignatureText" \ --type typescript # Also search for current method names to understand usage rg -l "createInboxSignatureText|addWalletSignatureText|revokeWalletSignatureText|revokeInstallationsSignatureText" \ --type typescript # Look for any class/interface that might be extending/implementing WorkerClient ast-grep --pattern 'class $_ extends WorkerClient' ast-grep --pattern 'interface $_ extends WorkerClient'Length of output: 461
Script:
#!/bin/bash # Search for old method names without file type restriction rg "getCreateInboxSignatureText|getAddWalletSignatureText|getRevokeWalletSignatureText|getRevokeInstallationsSignatureText" # Search for current method names and their usages with context rg -A 2 "createInboxSignatureText|addWalletSignatureText|revokeWalletSignatureText|revokeInstallationsSignatureText" # Find files that import or use WorkerClient rg "WorkerClient"Length of output: 5961
sdks/node-sdk/test/Client.test.ts (3)
38-45: Consider parameterizing the network value.While the test effectively covers the new static
canMessagemethod, consider parameterizing the "local" network value to make the test more maintainable and reusable across different environments.- const canMessage = await Client.canMessage([user.account.address], "local"); + const network = process.env.XMTP_NETWORK || "local"; + const canMessage = await Client.canMessage([user.account.address], network);
Line range hint
108-129: Consider adding edge case tests for account management.While the basic functionality is well tested, consider adding the following test cases to improve coverage:
- Attempting to add an already associated account
- Attempting to remove a non-existent account
- Attempting to add/remove accounts with invalid addresses
Example test case:
it("should handle duplicate account addition gracefully", async () => { const user = createUser(); const user2 = createUser(); const client = await createRegisteredClient(user); const signer2 = createSigner(user2); await client.addAccount(signer2); // Attempt to add the same account again await expect(client.addAccount(signer2)).rejects.toThrow(); });
Line range hint
1-200: Add test coverage for automatic registration features.The test suite is missing coverage for some key features introduced in the PR:
- Automatic client registration upon creation
- The
disableAutoRegisteroption inClientOptions- Manual registration using
Client.registerExample test cases to add:
it("should auto-register client by default", async () => { const user = createUser(); const client = await Client.create(createSigner(user)); expect(client.isRegistered).toBe(true); }); it("should not auto-register when disabled", async () => { const user = createUser(); const client = await Client.create(createSigner(user), { disableAutoRegister: true }); expect(client.isRegistered).toBe(false); // Test manual registration await client.register(); expect(client.isRegistered).toBe(true); });sdks/browser-sdk/src/types/clientEvents.ts (1)
Update needed in example code to match new signature method names
The type changes are implemented consistently across the SDKs, but there's an outdated reference in the example code:
examples/react-vite-browser-sdk/src/createClient.tsstill uses the old method namegetCreateInboxSignatureTextinstead of the newcreateInboxSignatureTextThe browser and node SDKs have been properly updated with the new method names, showing consistent implementation of:
createInboxSignatureTextaddAccountSignatureTextremoveAccountSignatureTextrevokeInstallationsSignatureText🔗 Analysis chain
Line range hint
46-72: LGTM! The renaming changes improve consistency.The changes align well with the PR objectives by:
- Removing redundant "get" prefixes
- Standardizing terminology from "Wallet" to "Account"
- Using more descriptive parameter names (e.g.,
newAccountAddress)Let's verify the consistency of these naming changes across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that old action names are completely removed and new ones are consistently used # Check for any remaining old action names echo "Checking for old action names..." rg -l "getCreateInboxSignatureText|getAddWalletSignatureText|getRevokeWalletSignatureText|getRevokeInstallationsSignatureText" # Check usage of new action names echo "Checking usage of new action names..." rg -l "createInboxSignatureText|addAccountSignatureText|removeAccountSignatureText|revokeInstallationsSignatureText"Length of output: 637
Script:
#!/bin/bash # Let's check the content of the example file that still contains old names echo "Content of example file with old names:" rg -A 2 -B 2 "getCreateInboxSignatureText|getAddWalletSignatureText|getRevokeWalletSignatureText|getRevokeInstallationsSignatureText" examples/react-vite-browser-sdk/src/createClient.ts # Let's also verify the implementation in Client.ts files to ensure consistency echo -e "\nRelevant sections from browser-sdk Client.ts:" rg -A 2 -B 2 "createInboxSignatureText|addAccountSignatureText|removeAccountSignatureText|revokeInstallationsSignatureText" sdks/browser-sdk/src/Client.ts echo -e "\nRelevant sections from node-sdk Client.ts:" rg -A 2 -B 2 "createInboxSignatureText|addAccountSignatureText|removeAccountSignatureText|revokeInstallationsSignatureText" sdks/node-sdk/src/Client.tsLength of output: 3458
sdks/browser-sdk/src/workers/client.ts (2)
Line range hint
76-115: Add JSDoc comments to document the signature actions.Consider adding JSDoc comments to document the purpose and parameters of each signature action. This would improve code maintainability and help developers understand when to use each type of signature.
Example for the first action:
+ /** + * Generates the signature text for creating a new inbox. + * This is part of the client initialization process. + * @returns {Promise<string>} The text to be signed + */ case "createInboxSignatureText": { const result = await client.createInboxSignatureText(); postMessage({
Update required in example code for signature action rename
The signature action renaming is incomplete. While the main SDK code has been updated, there's still a reference to the old method name in the example code:
examples/react-vite-browser-sdk/src/createClient.tsstill usesgetCreateInboxSignatureTextinstead of the newcreateInboxSignatureText🔗 Analysis chain
Line range hint
76-115: Verify client-side code updates for renamed signature actions.The renaming of signature-related actions improves clarity and consistency. However, we should ensure that all client-side code has been updated to use these new action names:
createInboxSignatureTextaddAccountSignatureTextremoveAccountSignatureTextrevokeInstallationsSignatureText🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any remaining references to old action names # Expected: No matches should be found for old names echo "Searching for old action names..." rg -l "getCreateInboxSignatureText|getAddWalletSignatureText|getRevokeWalletSignatureText|getRevokeInstallationsSignatureText" \ --type ts \ --type tsxLength of output: 254
Script:
#!/bin/bash # Description: Search for any remaining references to old action names # Expected: No matches should be found for old names echo "Searching for old action names..." rg -l "getCreateInboxSignatureText|getAddWalletSignatureText|getRevokeWalletSignatureText|getRevokeInstallationsSignatureText" # Also search for the new action names to verify they are properly used echo -e "\nSearching for new action names..." rg -l "createInboxSignatureText|addAccountSignatureText|removeAccountSignatureText|revokeInstallationsSignatureText"Length of output: 637
Script:
#!/bin/bash # Let's check the specific content in the example file that still uses old names echo "Content of file still using old names:" rg "getCreateInboxSignatureText|getAddWalletSignatureText|getRevokeWalletSignatureText|getRevokeInstallationsSignatureText" -A 2 -B 2 examples/react-vite-browser-sdk/src/createClient.tsLength of output: 487
sdks/browser-sdk/test/Client.test.ts (4)
17-17: Ensure consistent address casing in expectationsTo prevent potential case sensitivity issues, consider normalizing addresses by converting them to lowercase when performing comparisons.
Apply this diff to adjust the assertion:
- expect(client.accountAddress).toBe(user.account.address); + expect(client.accountAddress.toLowerCase()).toBe(user.account.address.toLowerCase());
87-87: Ensure added account's functionality is testedWhile the test confirms that the account addresses include the newly added account, consider adding assertions to verify that the new account (
user2) can perform expected operations, such as sending messages.For example, you could attempt to initiate a conversation or send a message using the added account to confirm its proper integration.
104-105: Consider verifying the effects of account removalTo fully validate the removal of the wallet association, consider adding a test that ensures
user2can no longer interact with the client afterremoveAccountis called.For instance, try to perform an action with
user2that should fail or expect a specific error or result indicating the account has been removed.
130-130: Enhance test coverage after revoking installationsTo comprehensively verify the effect of
revokeInstallations(), consider adding assertions to confirm that prior installations (clientandclient2) are no longer able to access or interact with the inbox or perform client operations.For example, you could attempt to perform client operations using
clientorclient2and assert that they fail as expected due to the revocation.sdks/browser-sdk/src/Client.ts (4)
35-35: StoringSignerinstance may have security implicationsStoring the
Signerinstance (#signer) as a private field might lead to security concerns if theSignercontains sensitive information. Consider whether it's necessary to store it long-term, or if you can limit its scope to the methods that require it.
127-146: Consider handling exceptions during signing and message sendingIn the
#addSignaturemethod, both the signing process and sending messages may fail. It's important to handle exceptions to provide meaningful error messages and to avoid unhandled promise rejections.Consider wrapping the signing and sending in try-catch blocks:
try { const signature = await signer.signMessage(signatureText); if (isSmartContractSigner(signer)) { await this.sendMessage("addScwSignature", { type: signatureType, bytes: signature, chainId: await signer.getChainId(), blockNumber: await signer.getBlockNumber(), }); } else { await this.sendMessage("addSignature", { type: signatureType, bytes: signature, }); } } catch (error) { throw new Error(`Failed to add signature: ${error.message}`); }
170-192: Handle potential errors when adding a new accountIn the
addAccountmethod, if any of the steps fail, such as obtaining the signature text or applying signatures, the errors might not be properly handled. Consider adding error handling to inform users of failures.
235-250: EnsureClient.canMessagecleans up resources to prevent memory leaksIn the static
Client.canMessagemethod, an instance ofClientis created but not explicitly disposed of after use. Ensure any resources or listeners are properly cleaned up.sdks/node-sdk/src/Client.ts (6)
82-85: Ensure default 'disableAutoRegister' behavior is clearly documentedThe
disableAutoRegisteroption defaults tofalse, causing clients to automatically register upon creation. Please ensure this default behavior is prominently documented to prevent any unintended usage by clients unaware of the automatic registration process.
Line range hint
170-176: Consider logging exceptions in '#createInboxSignatureText' for better debuggingThe method catches all exceptions and returns
null, potentially obscuring underlying errors. Including logging within thecatchblock can aid in debugging by providing visibility into any issues encountered during execution.Apply this diff to log exceptions:
async #createInboxSignatureText() { try { const signatureText = await this.#innerClient.createInboxSignatureText(); return signatureText; } catch (error) { + console.error('Error creating inbox signature text:', error); return null; } }
Line range hint
180-192: Consider logging exceptions in signature text methods for better error trackingMethods like
#addAccountSignatureText,#removeAccountSignatureText, and#revokeInstallationsSignatureTextsilently returnnullon failure. Logging the caught exceptions can provide valuable insights during debugging and error monitoring.For example, modify
#addAccountSignatureText:async #addAccountSignatureText( existingAccountAddress: string, newAccountAddress: string, ) { try { const signatureText = await this.#innerClient.addWalletSignatureText( existingAccountAddress, newAccountAddress, ); return signatureText; } catch (error) { + console.error('Error generating add account signature text:', error); return null; } }
260-262: Enhance error message in 'addAccount' method for clarityIncluding the
newAccountAddressin the error message provides more context, aiding in troubleshooting when the signature text cannot be generated.Apply this diff to enhance the error message:
if (!signatureText) { - throw new Error("Unable to generate add account signature text"); + throw new Error(`Unable to generate add account signature text for account ${await newAccountSigner.getAddress()}`); }
283-285: Enhance error message in 'removeAccount' method for clarityIncluding the
accountAddressin the error message offers better context for debugging when the signature text generation fails.Apply this diff:
if (!signatureText) { - throw new Error("Unable to generate remove account signature text"); + throw new Error(`Unable to generate remove account signature text for account ${accountAddress}`); }
299-301: Enhance error message in 'revokeInstallations' method for clarityProviding additional context in the error message can assist in identifying issues during the revocation process.
Apply this diff:
if (!signatureText) { - throw new Error("Unable to generate revoke installations signature text"); + throw new Error("Unable to generate signature text for revoking installations"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
.changeset/config.json(1 hunks)sdks/browser-sdk/src/Client.ts(6 hunks)sdks/browser-sdk/src/WorkerClient.ts(2 hunks)sdks/browser-sdk/src/index.ts(1 hunks)sdks/browser-sdk/src/types/clientEvents.ts(1 hunks)sdks/browser-sdk/src/types/options.ts(1 hunks)sdks/browser-sdk/src/utils/signer.ts(1 hunks)sdks/browser-sdk/src/workers/client.ts(3 hunks)sdks/browser-sdk/test/Client.test.ts(5 hunks)sdks/browser-sdk/test/Utils.test.ts(1 hunks)sdks/browser-sdk/test/helpers.ts(3 hunks)sdks/node-sdk/src/Client.ts(10 hunks)sdks/node-sdk/src/helpers/signer.ts(1 hunks)sdks/node-sdk/src/index.ts(1 hunks)sdks/node-sdk/test/Client.test.ts(5 hunks)sdks/node-sdk/test/helpers.ts(2 hunks)sdks/node-sdk/vitest.setup.ts(1 hunks)
🔇 Additional comments (20)
sdks/node-sdk/vitest.setup.ts (1)
9-10:
Consider adding safeguards for database cleanup
The broader glob pattern "**/*.db3*" could potentially delete non-test databases. Consider these improvements:
- Add a prefix/suffix pattern to identify test databases (e.g., "test-.db3" or ".test.db3")
- Restrict cleanup to a specific test directory to prevent accidental deletion of databases outside the test scope
Here's a suggested implementation:
- const files = await glob("**/*.db3*", { cwd: __dirname });
- await Promise.all(files.map((file) => unlink(join(__dirname, file))));
+ const testDbDir = join(__dirname, 'test-dbs');
+ const files = await glob("*.test.db3*", { cwd: testDbDir });
+ await Promise.all(files.map((file) => unlink(join(testDbDir, file))));Let's verify if there are any database files that might be affected:
sdks/browser-sdk/src/utils/signer.ts (1)
1-4: LGTM! Well-structured type definitions.
The type definitions are well-designed with the following good practices:
- Proper handling of both async and sync operations for signing and address retrieval
- Correct use of
bigintfor blockchain-related numbers - Clear and descriptive type names
sdks/browser-sdk/test/Utils.test.ts (1)
17-20: LGTM! Property rename is correctly implemented.
The change from client.address to client.accountAddress aligns with the PR objective of ensuring consistency between browser and node SDKs.
Let's verify that this property rename has been consistently applied across all test files:
✅ Verification successful
Property rename has been consistently applied across test files
The verification results show that:
- No instances of
client.addressremain in the test files client.accountAddressis used consistently in bothUtils.test.tsandClient.test.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of client.address in test files
# and ensure client.accountAddress is used consistently.
# Search for any remaining instances of client.address
echo "Checking for remaining instances of client.address:"
rg "client\.address" "sdks/browser-sdk/test"
# Search for new instances of client.accountAddress to verify consistent usage
echo -e "\nVerifying usage of client.accountAddress:"
rg "client\.accountAddress" "sdks/browser-sdk/test"
Length of output: 472
sdks/browser-sdk/src/index.ts (1)
37-41: LGTM! The new signer exports align with the PR objectives.
The exports support the new signing architecture and enable smart contract signing capabilities.
Let's verify the documentation and implementation:
✅ Verification successful
Let me try to find the correct path for the signer file first.
The signer exports are well-documented and correctly implemented
The verification confirms that:
- The
Signerinterface is properly defined with essential methods (getAddressandsignMessage) SmartContractSignerextendsSignerwith additional required fields (getBlockNumberandgetChainId)isSmartContractSignerfunction correctly type guards between regular and smart contract signers- All types are well-documented with clear comments explaining the smart contract wallet indicators
The exports in the index file correctly reflect these implementations and align with the PR objectives.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check documentation and implementation of signer types
# Check for documentation in the source file
echo "Checking signer documentation..."
rg -A 10 "interface Signer" "./utils/signer"
# Verify smart contract signer implementation
echo "Checking smart contract signer implementation..."
ast-grep --pattern 'function isSmartContractSigner($_) {
$$$
}'
Length of output: 349
Script:
#!/bin/bash
# Find the signer file
echo "Looking for signer file..."
fd "signer.ts"
# Once we find it, let's check its content
echo "Checking file content..."
fd "signer.ts" --exec cat {}
Length of output: 1674
sdks/node-sdk/src/index.ts (1)
45-49: LGTM! New exports align with the signing refactor.
The addition of Signer and SmartContractSigner types along with the isSmartContractSigner function supports the PR's objective of streamlining the signing process.
sdks/browser-sdk/test/helpers.ts (2)
12-12: LGTM: Import of Signer type
The addition of the Signer type import aligns with the PR's objective of introducing the Signer interface.
43-43: LGTM: Type export placement
The User type export is correctly placed and maintains its functionality.
sdks/browser-sdk/src/WorkerClient.ts (1)
Line range hint 49-84: LGTM! Method names are now more intuitive and aligned with domain terminology.
The renaming of these methods improves clarity and maintains consistency with the broader SDK changes.
sdks/node-sdk/test/Client.test.ts (1)
1-11: LGTM! Import changes align with the new signing architecture.
The simplified imports and removal of signature-related imports reflect the architectural changes to handle signatures through the new Signer interface.
sdks/node-sdk/test/helpers.ts (4)
33-42: createSigner Function Implementation Approved
The createSigner function correctly encapsulates the signing logic by returning a Signer object with getAddress and signMessage methods. This enhances code modularity and readability.
45-46: User Type Export Relocated Appropriately
Moving the User type export improves code organization and clarity. This change helps in maintaining a logical structure within the file.
52-54: Confirm the Use of disableAutoRegister: true in createClient
In the createClient function, disableAutoRegister is explicitly set to true. Verify that this aligns with the intended test behavior, ensuring that clients created with this function should indeed avoid automatic registration.
63-70: createRegisteredClient Function Updated Correctly
The createRegisteredClient function now utilizes createSigner(user) without setting disableAutoRegister, allowing automatic registration as expected. This aligns with the purpose of creating a registered client.
sdks/browser-sdk/test/Client.test.ts (2)
1-9: Imports and helper functions are correctly updated
The import statements and helper functions are appropriately updated to reflect the refactored codebase.
39-46: New test case for static method Client.canMessage
The added test case effectively verifies the functionality of Client.canMessage without requiring a client instance.
sdks/browser-sdk/src/Client.ts (1)
86-87: Possibility of leaking encryptionKey if exceptions occur
In the Client.create method, if client.init() or client.register() throw exceptions, the encryptionKey may be left in memory without the client being usable. Consider whether you need to securely handle or clear the encryptionKey in such scenarios.
sdks/node-sdk/src/Client.ts (4)
109-112: Verify backward compatibility of 'Client.create' method signature change
The create method now requires a signer as the first parameter instead of the accountAddress. This alteration may impact clients using the previous method signature. Ensure that this change is communicated clearly, and consider updating documentation and versioning to reflect any breaking changes.
329-331: Ensure that the dummy signer in 'static canMessage' method is appropriate
The signMessage function returns an empty Uint8Array, which may not be valid if any signing operations are inadvertently triggered. Verify that the client does not perform any signing that would fail due to the dummy signer, to prevent unexpected errors.
317-338: Verify the use of zero address and potential database conflicts in 'static canMessage' method
The method uses the zero address (0x0000000000000000000000000000000000000000) and constructs a database path based on it. This could lead to conflicts if multiple instances are created simultaneously or if the database is shared unintentionally. Ensure that using the zero address does not cause side effects and consider using a unique, temporary address or in-memory database if persistence is not required.
Run the following script to check for other usages of the zero address and assess potential conflicts:
#!/bin/bash
# Description: Search for usages of the zero address and potential dbPath conflicts.
# Find all instances of the zero address
rg -A 2 $'"0x0000000000000000000000000000000000000000"'
# Search for client instantiations using the zero address
rg -A 5 $'createClient\(.*"0x0000000000000000000000000000000000000000"'99-102: Assess potential impact of 'Client' constructor signature change
The Client constructor now includes a signer parameter, which may affect external code if the constructor is used directly. Verify that all client instantiations occur through the Client.create method to prevent any breaking changes.
Run the following script to identify any direct usages of the Client constructor:
Deploying xmtp-js-docs with
|
| Latest commit: |
a1a16a0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0a6269ab.xmtp-js.pages.dev |
| Branch Preview URL: | https://rygine-update-signing-api.xmtp-js.pages.dev |
Summary
This PR brings back the
Signerinterface to simplify the signing process.Client.createnow requires passing in aSigner.Changes
addressproperty ofClienttoaccountAddressin the browser SDK to match the node SDKdisableAutoRegistertoClientOptionsto allow disabling of client registration after creationClient.registermethod for registering a clientClient.addAccountmethod for adding another account to an installationClient.removeAccountmethod for removing an account from an installationClient.revokeInstallationsmethod for revoking all other installationsOther changes
Client.canMessagefor checking if an address is on the network without a client@xmtp/react-vite-browser-sdk-exampleexample for releasesSummary by CodeRabbit
New Features
addAccount,removeAccount, andrevokeInstallations.canMessagefor checking messaging capabilities without an instance of the client.disableAutoRegisteradded to client options.signerobject for signing messages.Bug Fixes
Documentation
Chores