-
Notifications
You must be signed in to change notification settings - Fork 7
Migrate JWT to SIWE for App registration in e2e test utils #475
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
awisniew207
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
![]()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request migrates authentication from JWT to SIWE (Sign-In with Ethereum) for app registration operations in the e2e test utilities. The change implements EIP-4361 compliant SIWE authentication for interacting with the Vincent Registry API, replacing the previous JWT-based approach.
Changes:
- Implemented new SIWE authentication using EIP-4361 standard with secure nonce generation and message signing
- Updated app registration API functions (registerApp, registerAppVersion, setActiveAppVersion) to use SIWE authentication headers
- Enhanced test validations for serialized permission accounts with proper base64 decoding
- Removed redundant appId parameter from completeAppInstallation request body (already in URL path)
- Added .env.example documentation for environment variables
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/libs/e2e-test-utils/src/setup-dev-env/vincent-registry-api/generateSiweAuth.ts | New SIWE authentication implementation with domain/URI inference, nonce generation, and EIP-4361 message formatting |
| packages/libs/e2e-test-utils/src/setup-dev-env/vincent-registry-api/registerApp.ts | Updated to use SIWE authentication instead of JWT |
| packages/libs/e2e-test-utils/src/setup-dev-env/vincent-registry-api/registerAppVersion.ts | Updated to use SIWE authentication instead of JWT |
| packages/libs/e2e-test-utils/src/setup-dev-env/vincent-registry-api/setActiveAppVersion.ts | Updated to use SIWE authentication instead of JWT |
| packages/libs/e2e-test-utils/src/setup-dev-env/vincent-registry-api/completeAppInstallation.ts | Removed redundant appId from request body |
| packages/libs/e2e-test-utils/test/setupVincentDevEnv.spec.ts | Added non-null assertions for optional agentSmartAccount property and improved base64/JSON validation for serializedPermissionAccount |
| packages/libs/e2e-test-utils/package.json | Version bump to 4.0.1-alpha.0 for pre-release |
| packages/libs/e2e-test-utils/.env.example | New environment variable documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| domain: domainOverride, | ||
| }: { | ||
| appManagerPrivateKey: `0x${string}`; | ||
| vincentApiUrl?: string; |
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The vincentApiUrl parameter is marked as optional but it's required by all callers (registerApp, registerAppVersion, setActiveVersion). This creates a type safety issue where the function could be called without a vincentApiUrl, leading to the fallback localhost defaults. Consider making this parameter required or documenting the intended use case for when it should be omitted.
| vincentApiUrl?: string; | |
| vincentApiUrl: string; |
|
|
||
| // Check if it's production API | ||
| if (urlObj.hostname === 'api.heyvincent.ai') { | ||
| return { |
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The production domain 'vincent-dashboard-20.vercel.app' is hardcoded in the domain inference logic. If this domain changes in the future (e.g., to 'vincent-dashboard-21.vercel.app'), this code will need to be updated. Consider making this configurable through an environment variable or documenting that this is expected to match the production dashboard domain.
| nonce, | ||
| issuedAt, | ||
| expirationTime, | ||
| statement = 'Sign in with Ethereum to authenticate with Vincent Registry API', |
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The chainId is hardcoded to 85452 (Base Sepolia) in the SIWE message creation. However, the codebase appears to support multiple chains including Base mainnet (chain ID 8453). The chain ID in a SIWE message should match the actual chain being used for authentication. Consider making the chainId configurable or deriving it from the vincentRegistryChain configuration to ensure the SIWE message accurately represents the chain the signer is authenticating for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 7 out of 10 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Check if it's production API | ||
| if (urlObj.hostname === 'api.heyvincent.ai') { | ||
| return { | ||
| domain: 'vincent-dashboard-20.vercel.app', |
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The production domain 'vincent-dashboard-20.vercel.app' appears to be hardcoded with a version-specific deployment identifier ('20'). If the dashboard is redeployed with a different identifier, this will break SIWE authentication. Consider whether this domain should be:
- Configurable via environment variable
- Made dynamic to handle different Vercel deployment versions
- Use a stable production domain (e.g., 'dashboard.heyvincent.ai' if available)
| issuedAt, | ||
| expirationTime, | ||
| statement = 'Sign in with Ethereum to authenticate with Vincent Registry API', | ||
| chainId = 85452, |
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The chainId is hardcoded to 85452 (Base Sepolia) in the SIWE message. While this might be the primary test chain, consider making this configurable to support different deployment environments. The chainId in SIWE messages should ideally match the chain where the user's wallet/account is being used, which may vary depending on the environment (e.g., mainnet Base vs Base Sepolia).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 9 out of 12 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "@lit-protocol/vincent-app-sdk": "workspace:*", | ||
| "@lit-protocol/contracts-sdk": "^7.2.3", | ||
| "@lit-protocol/pkp-ethers": "^7.3.1", | ||
| "@lit-protocol/vincent-contracts-sdk": "workspace:*", |
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The package still imports @lit-protocol/contracts-sdk (specifically LitContracts in src/setup-dev-env/wallets/ensureWalletHasCapacityCredit.ts), but it's being removed from direct dependencies. While this dependency is transitively available through @lit-protocol/vincent-contracts-sdk, relying on transitive dependencies is fragile and can break if the parent package removes it. Consider either keeping @lit-protocol/contracts-sdk as a direct dependency or refactoring ensureWalletHasCapacityCredit to not use it.
| "@lit-protocol/vincent-contracts-sdk": "workspace:*", | |
| "@lit-protocol/vincent-contracts-sdk": "workspace:*", | |
| "@lit-protocol/contracts-sdk": "workspace:*", |
| chainId = 85452, | ||
| version = '1', |
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The chainId is hardcoded to 85452 in the SIWE message. According to chainlist.org, this is Base Sepolia's chain ID. However, this value should ideally match the chain where the authentication is happening (Vincent Registry chain). If Vincent Registry can be on different chains (e.g., mainnet Base with chain ID 8453), this hardcoded value could cause authentication issues. Consider making the chainId configurable or deriving it from the vincentRegistryChain parameter that's passed to the functions calling this.
| deprecated: Old versions of tar are not supported, and contain widely publicized security vulnerabilities, which have been fixed in the current version. Please update. Support for old versions may be purchased (at exhorbitant rates) by contacting i@izs.me | ||
|
|
||
| tar@7.5.2: | ||
| resolution: {integrity: sha512-7NyxrTE4Anh8km8iEy7o0QYPs+0JKBTj5ZaqHg6B39erLg0qYXN3BijtShwbsNSvQ+LN75+KV+C4QR/f6Gwnpg==} | ||
| engines: {node: '>=18'} | ||
| deprecated: Old versions of tar are not supported, and contain widely publicized security vulnerabilities, which have been fixed in the current version. Please update. Support for old versions may be purchased (at exhorbitant rates) by contacting i@izs.me |
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The deprecation warnings for tar versions 6.2.1 and 7.5.2 are being added to the lockfile. These warnings indicate that these versions contain widely publicized security vulnerabilities. While this change is just adding the warning metadata (not changing versions), the project should consider updating to newer versions of tar or updating the dependencies that depend on these old versions to address the security vulnerabilities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
No description provided.