Add static authorization methods to Node SDK Client #740
Conversation
🦋 Changeset detectedLatest commit: c8d7dea The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
Warning Rate limit exceeded@rygine has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 31 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes introduce static authorization methods to the Node SDK Client in the Changes
Possibly related PRs
Suggested reviewers
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 (
|
Deploying xmtp-js-docs with
|
| Latest commit: |
c8d7dea
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f173f434.xmtp-js.pages.dev |
| Branch Preview URL: | https://rygine-node-sdk-client-updat.xmtp-js.pages.dev |
| "@xmtp/node-sdk": patch | ||
| --- | ||
|
|
||
| Add static authorization methods to Node SDK Client |
There was a problem hiding this comment.
The random name generator really picked a good one this time. 😬 😆
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
.changeset/clean-students-sell.md (1)
1-5: Consider enhancing the changeset descriptionWhile the description accurately reflects the changes, it could be more detailed to help users understand the impact. Consider adding:
- Mention of the specific methods being added (
isAddressAuthorized,isInstallationAuthorized)- Note about the removal of instance methods
- Any breaking changes or migration steps if applicable
sdks/node-sdk/test/Client.test.ts (1)
238-242: Consider extracting environment configurationThe environment configuration is hardcoded in both tests. Consider:
- Using a shared constant or configuration
- Testing with different environment values
- Adding validation for the environment parameter
+const TEST_ENV = "local"; + it("should check if an address is authorized", async () => { // ... const authorized = await Client.isAddressAuthorized( client.inboxId, user.account.address.toLowerCase(), - { env: "local" }, + { env: TEST_ENV }, ); // ... }); it("should check if an installation is authorized", async () => { // ... const authorized = await Client.isInstallationAuthorized( client.inboxId, client.installationIdBytes, - { env: "local" }, + { env: TEST_ENV }, ); // ... });Also applies to: 249-253
📜 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 (4)
.changeset/clean-students-sell.md(1 hunks)sdks/node-sdk/package.json(1 hunks)sdks/node-sdk/src/Client.ts(2 hunks)sdks/node-sdk/test/Client.test.ts(1 hunks)
🔇 Additional comments (3)
sdks/node-sdk/package.json (1)
56-56: Verify compatibility with node-bindings pre-1.0 version
The dependency update to @xmtp/node-bindings@^0.0.28 uses caret versioning with a pre-1.0 version. Since pre-1.0 versions can include breaking changes even in minor versions, consider:
- Pinning to exact version (
0.0.28) to prevent unexpected updates - Adding a changelog entry or comment documenting the reason for the update
✅ Verification successful
Version update is documented and safe to use
The CHANGELOG.md shows this is a patch update (0.0.29) that simply upgrades to the latest node bindings. Since this is documented and intentional, the caret versioning (^0.0.28) is acceptable as it follows the project's versioning pattern seen throughout the changelog. No breaking changes are indicated for this update.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for breaking changes in node-bindings changelog
gh api repos/xmtp/xmtp-js/releases | jq -r '.[] | select(.tag_name | contains("node-bindings-v0.0.28")) | .body'
Length of output: 114
Script:
#!/bin/bash
# Let's try a different approach to check for any breaking changes or major updates
# First, check if there are any major dependency changes in package-lock.json
fd package-lock.json --exec cat {} | grep -A 5 -B 5 "@xmtp/node-bindings"
# Also check for any migration guides or upgrade notes in documentation
fd "CHANGELOG.md|UPGRADING.md|MIGRATION.md" --type f
Length of output: 751
Script:
#!/bin/bash
# Let's check the node-sdk's changelog for any relevant information about this dependency update
cat sdks/node-sdk/CHANGELOG.md
# Also check the package.json history to see the version progression
git log -p sdks/node-sdk/package.json | grep -A 5 -B 5 "@xmtp/node-bindings"
Length of output: 5780
sdks/node-sdk/src/Client.ts (2)
18-19: LGTM: Clean import additions
The new authorization binding imports are properly organized and clearly aliased.
444-460: Verify the impact of moving to static methods
The change from instance methods to static methods is a breaking change. Let's verify that all usages have been updated.
✅ Verification successful
Let me search for all usages of these methods to ensure they are being called correctly as static methods.
No breaking changes detected in the migration to static methods
All existing usages in the codebase are already using the static method syntax (Client.isAddressAuthorized and Client.isInstallationAuthorized). There are no instances of these methods being called with this. syntax, indicating that the migration to static methods has been properly handled.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instance method calls that need to be updated
rg -l 'isAddressAuthorized|isInstallationAuthorized' --type ts | while read -r file; do
echo "=== Checking $file ==="
# Look for instance method calls (this.*) that need to be updated to static calls (Client.*)
rg 'this\.(isAddressAuthorized|isInstallationAuthorized)' "$file" || true
done
Length of output: 515
Script:
#!/bin/bash
# Search for all usages of these methods
rg -A 2 'isAddressAuthorized|isInstallationAuthorized' --type ts
Length of output: 1453
Summary
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests