chore: fixed broken link (0xsequence#985)#411
Merged
Dargon789 merged 0 commit intoDargon789:mainfrom Mar 28, 2026
Merged
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
|
|
@dependabot[bot] is attempting to deploy a commit to the Foundry development Team on Vercel. A member of the Team first needs to authorize it. |
Reviewer's GuideImplements account login-signer federation/defederation flows in the wallet WDK, refactors OAuth auth commitments from a boolean isSignUp flag to a discriminated union type, wires these flows into redirect handling, and bumps package versions and dependencies for a 3.0.5 release along with a README link fix. Sequence diagram for add-login-signer redirect flowsequenceDiagram
actor User
participant App
participant Wallets
participant AuthCodeHandler
participant AuthCommitments
participant OAuthProvider
participant SignaturesModule
User->>App: Initiate add login signer (select provider)
App->>Wallets: startAddLoginSignerWithRedirect(args: StartAddLoginSignerWithRedirectArgs)
Wallets->>Wallets: get(wallet)
Wallets->>Wallets: validate wallet-ready
Wallets->>Wallets: getSignupHandlerKey(kind)
Wallets->>AuthCodeHandler: commitAuth(target, { type: add-signer, wallet })
AuthCodeHandler->>AuthCommitments: set({ id, kind, target, type: add-signer, wallet })
AuthCodeHandler-->>Wallets: oauthUrl
Wallets-->>App: oauthUrl
App->>User: Redirect browser to oauthUrl
User->>OAuthProvider: Authenticate and authorize
OAuthProvider-->>App: Redirect back with code, state
App->>Wallets: completeRedirect({ code, state })
Wallets->>AuthCommitments: get(state)
AuthCommitments-->>Wallets: commitment{ type: add-signer, wallet, kind }
Wallets->>Wallets: get(wallet)
Wallets->>Wallets: validate wallet-ready
Wallets->>AuthCodeHandler: completeAuth(commitment, code)
AuthCodeHandler-->>Wallets: [signer, metadata]
Wallets->>Wallets: getSignerKindForSignup(commitment.kind)
Wallets->>Wallets: addLoginSignerFromPrepared(wallet, { signer, extra })
Wallets->>SignaturesModule: requestConfigurationUpdate(..., Actions.AddLoginSigner)
SignaturesModule-->>Wallets: requestId
Wallets-->>App: requestId
App->>User: Show pending signature request
Sequence diagram for remove-login-signer flowsequenceDiagram
actor User
participant App
participant Wallets
participant ConfigModule
participant SessionsModule
participant RecoveryModule
participant SignaturesModule
User->>App: Request to unlink login method
App->>Wallets: removeLoginSigner({ wallet, signerAddress })
Wallets->>Wallets: get(wallet)
Wallets->>Wallets: validate wallet-ready
Wallets->>ConfigModule: getConfigurationParts(wallet)
ConfigModule-->>Wallets: { loginTopology, modules }
Wallets->>ConfigModule: Config.getSigners(loginTopology)
Wallets->>Wallets: verify signerAddress exists
Wallets->>Wallets: compute remainingMembers
Wallets->>Wallets: ensure remainingMembers.length >= 1
Wallets->>Wallets: buildCappedTree(remainingMembers) = nextLoginTopology
Wallets->>SessionsModule: hasSessionModule(modules)
alt hasSessionModule
Wallets->>SessionsModule: removeIdentitySignerFromModules(modules, signerAddress)
end
Wallets->>RecoveryModule: hasRecoveryModule(modules)
alt hasRecoveryModule
Wallets->>RecoveryModule: removeRecoverySignerFromModules(modules, signerAddress)
end
Wallets->>SignaturesModule: requestConfigurationUpdate(wallet, { loginTopology: nextLoginTopology, modules }, Actions.RemoveLoginSigner, wallet-webapp)
SignaturesModule-->>Wallets: requestId
Wallets-->>App: requestId
App->>User: Show pending signature request
User->>SignaturesModule: Sign configuration update
App->>Wallets: completeRemoveLoginSigner(requestId)
Wallets->>SignaturesModule: get(requestId)
SignaturesModule-->>Wallets: request{ action: RemoveLoginSigner }
Wallets->>Wallets: completeConfigurationUpdate(requestId)
Updated class diagram for wallet auth and login signer federation typesclassDiagram
class WalletsInterface {
+addLoginSigner(args: AddLoginSignerArgs) Promise~string~
+completeAddLoginSigner(requestId: string) Promise~void~
+startAddLoginSignerWithRedirect(args: StartAddLoginSignerWithRedirectArgs) Promise~string~
+removeLoginSigner(args: RemoveLoginSignerArgs) Promise~string~
+completeRemoveLoginSigner(requestId: string) Promise~void~
}
class Wallets {
+addLoginSigner(args: AddLoginSignerArgs) Promise~string~
+completeAddLoginSigner(requestId: string) Promise~void~
+startAddLoginSignerWithRedirect(args: StartAddLoginSignerWithRedirectArgs) Promise~string~
+removeLoginSigner(args: RemoveLoginSignerArgs) Promise~string~
+completeRemoveLoginSigner(requestId: string) Promise~void~
-addLoginSignerFromPrepared(wallet: Address, loginSigner: PreparedLoginSigner) Promise~string~
}
class PreparedLoginSigner {
+signer SignerWitnessable
+extra WitnessExtraSignerKind
}
class StartAddLoginSignerWithRedirectArgs {
+wallet Address
+kind string
+target string
}
class AddLoginSignerArgs {
+wallet Address
+kind string
+mnemonic string
+email string
+idToken string
}
class RemoveLoginSignerArgs {
+wallet Address
+signerAddress Address
}
class AuthCommitment {
+id string
+kind string
+verifier string
+challenge string
+target string
+type string
+signer string
+wallet string
}
class CommitAuthArgs {
+type string
+state string
+signer string
+wallet string
}
class AuthCommitments {
+set(commitment: AuthCommitment) Promise~void~
+get(id: string) Promise~AuthCommitment~
}
class AuthCodeHandler {
+commitAuth(target: string, args: CommitAuthArgs) Promise~string~
+completeAuth(commitment: AuthCommitment, code: string) Promise~IdentitySigner, map~
}
class AuthCodePkceHandler {
+commitAuth(target: string, args: CommitAuthArgs) Promise~string~
}
class Actions {
<<enumeration>>
+AddLoginSigner
+RemoveLoginSigner
}
class SignaturesModule {
+get(requestId: string) Promise~SignatureRequest~
}
class SignatureRequest {
+action Actions
}
WalletsInterface <|.. Wallets
AuthCodeHandler <|-- AuthCodePkceHandler
AuthCommitments --> AuthCommitment
AuthCodeHandler --> AuthCommitments
AuthCodeHandler --> CommitAuthArgs
Wallets --> AuthCodeHandler
Wallets --> Actions
Wallets --> SignaturesModule
SignaturesModule --> SignatureRequest
Wallets --> StartAddLoginSignerWithRedirectArgs
Wallets --> AddLoginSignerArgs
Wallets --> RemoveLoginSignerArgs
Wallets --> PreparedLoginSigner
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 6 issues, and left some high level feedback:
- In
removeLoginSigner, attempting to removeConstants.ZeroAddresswill pass thesigner-not-foundcheck but be filtered out when buildingremainingMembers, so the configuration won’t actually change; consider either disallowing ZeroAddress up front or aligning the existence check and removal logic so the requested signer is actually removed or a specific error is thrown. - In
addLoginSignerFromPrepared, the signer is witnessed (and thus the wallet becomes discoverable via the new credential) before the configuration update request is created; ifrequestConfigurationUpdatefails, discovery and configuration can diverge—consider deferringwitnessuntil after the config update has been successfully created or adding rollback/compensation handling.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `removeLoginSigner`, attempting to remove `Constants.ZeroAddress` will pass the `signer-not-found` check but be filtered out when building `remainingMembers`, so the configuration won’t actually change; consider either disallowing ZeroAddress up front or aligning the existence check and removal logic so the requested signer is actually removed or a specific error is thrown.
- In `addLoginSignerFromPrepared`, the signer is witnessed (and thus the wallet becomes discoverable via the new credential) before the configuration update request is created; if `requestConfigurationUpdate` fails, discovery and configuration can diverge—consider deferring `witness` until after the config update has been successfully created or adding rollback/compensation handling.
## Individual Comments
### Comment 1
<location path="packages/wallet/wdk/src/sequence/wallets.ts" line_range="926" />
<code_context>
+ return handler.commitAuth(args.target, { type: 'add-signer', wallet: args.wallet })
}
async completeRedirect(args: CompleteRedirectArgs): Promise<string> {
</code_context>
<issue_to_address>
**issue:** Handle legacy `AuthCommitment` records that still use `isSignUp` instead of `type` to avoid breaking existing redirects.
Existing `AuthCommitment` records in IndexedDB use the old `{ isSignUp: boolean, signer?: string }` shape and won’t have `type` set. In those cases `commitment.type` will be `undefined`, no `switch` case will match, and we’ll fall through to the `target` redirect without performing sign-up/add-signer/reauth. That’s a regression for users upgrading with pending auth commitments.
Please add a fallback for legacy records (e.g., derive `type` from `isSignUp`/`signer`, or at least throw a clear error) so they either continue to work or fail explicitly instead of silently doing nothing.
</issue_to_address>
### Comment 2
<location path="packages/wallet/wdk/test/authcode-pkce.test.ts" line_range="94" />
<code_context>
- const isSignUp = true
- const result = await handler.commitAuth(target, isSignUp)
+ const result = await handler.commitAuth(target, { type: 'auth' })
// Verify nitroCommitVerifier was called with correct challenge
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests covering the new `type: 'add-signer'` path and `wallet` field for PKCE auth commitments
The new `type: 'add-signer'` path in `AuthCodePkceHandler.commitAuth` isn’t covered. Please add a test that calls `commitAuth(target, { type: 'add-signer', wallet: <address>, state?: <custom> })`, asserts `commitments.set` receives `{ type: 'add-signer', wallet: <address> }` with no `signer` field, and checks that `state` is preserved when provided and randomly generated when omitted. This will help guard the add-signer redirect flow against regressions.
Suggested implementation:
```typescript
describe('commitAuth', () => {
it('should create add-signer PKCE commitment with explicit state', async () => {
const target = 'https://example.com/success'
const wallet = '0x1111111111111111111111111111111111111111'
const state = 'custom-state'
const result = await handler.commitAuth(target, {
type: 'add-signer',
wallet,
state,
})
// commitment should be stored without signer field
expect(handler['commitments'].set).toHaveBeenCalledWith(state, {
type: 'add-signer',
wallet,
})
// returned state/URL should preserve the provided state
if ('state' in result) {
expect(result.state).toBe(state)
}
if ('url' in result && typeof result.url === 'string') {
expect(result.url).toContain(`state=${encodeURIComponent(state)}`)
}
})
it('should create add-signer PKCE commitment with generated state when omitted', async () => {
const target = 'https://example.com/success'
const wallet = '0x2222222222222222222222222222222222222222'
const result = await handler.commitAuth(target, {
type: 'add-signer',
wallet,
})
// inspect last call to commitments.set to get generated state and stored commitment
const setMock = handler['commitments'].set as jest.Mock
const [[generatedState, storedCommitment]] = setMock.mock.calls.slice(-1)
expect(typeof generatedState).toBe('string')
expect(generatedState.length).toBeGreaterThan(0)
// commitment should have no signer field
expect(storedCommitment).toEqual({
type: 'add-signer',
wallet,
})
// returned state/URL should contain the generated state
if ('state' in result) {
expect(result.state).toBe(generatedState)
}
if ('url' in result && typeof result.url === 'string') {
expect(result.url).toContain(`state=${encodeURIComponent(generatedState)}`)
}
})
```
These edits assume:
1. `handler` is in scope in this `describe('commitAuth', ...)` block and is the same instance used in the existing PKCE tests.
2. `handler['commitments'].set` is a Jest mock/spied function. If it is not yet mocked, you will need to:
- Either construct `handler` with a mocked `commitments` implementation,
- Or wrap `jest.spyOn(handler['commitments'], 'set')` in a `beforeEach` for this `describe` and cast it to `jest.Mock` where used.
3. The `commitAuth` return type either exposes a `state` field, a URL with `state` as a query parameter, or both. If the actual shape differs, adjust the `result.state` / `result.url` assertions to match your existing `auth` flow test style (e.g. if you already parse `new URL(result.url).searchParams.get('state')` in the other test, mirror that here).
</issue_to_address>
### Comment 3
<location path="packages/wallet/wdk/test/authcode.test.ts" line_range="251" />
<code_context>
- const signer = testWallet
- const result = await authCodeHandler.commitAuth(target, isSignUp, undefined, signer)
+ const result = await authCodeHandler.commitAuth(target, { type: 'auth' })
// Verify commitment was saved
</code_context>
<issue_to_address>
**suggestion (testing):** Cover `AuthCodeHandler.commitAuth` for the `add-signer` commitment type as well
The core `AuthCodeHandler.commitAuth` now has three branches: `auth`, `reauth` (with `signer`), and `add-signer` (with `wallet`). The updated tests cover `auth` and `reauth`, but not `type: 'add-signer'`.
Please add a test that calls `commitAuth('/target', { type: 'add-signer', wallet: testWallet })` and asserts that `authCommitments.set` receives `{ type: 'add-signer', wallet: testWallet }` with no `signer` field. This ensures the DB representation for `add-signer` commitments matches the new union type.
</issue_to_address>
### Comment 4
<location path="packages/wallet/wdk/test/identity-auth-dbs.test.ts" line_range="69" />
<code_context>
target,
metadata: {},
- isSignUp,
+ type: 'auth',
})
</code_context>
<issue_to_address>
**suggestion (testing):** Extend DB tests to include the new `add-signer` auth commitment variant
The DB tests only cover `type: 'auth'` and `type: 'reauth'` commitments; they don’t exercise the new `type: 'add-signer'` variant that stores a `wallet` instead of a `signer`.
Please add a test that writes an `AuthCommitment` with `type: 'add-signer'` and a `wallet` address, then reads it back and asserts the type, `wallet` value, and absence of `signer`, to verify the IndexedDB layer handles this variant correctly.
</issue_to_address>
### Comment 5
<location path="packages/wallet/wdk/test/wallets.test.ts" line_range="163" />
<code_context>
expect(url).toBe('https://accounts.google.com/o/oauth2/v2/auth?state=test-state')
- expect(commitAuthSpy).toHaveBeenCalledWith('/auth/return', true)
+ expect(commitAuthSpy).toHaveBeenCalledWith('/auth/return', { type: 'auth' })
})
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for the new add/remove login signer flows and `completeRedirect` branches
The tests only verify the updated `commitAuth` signature, but the new `Wallets` behavior remains untested, including:
- `startAddLoginSignerWithRedirect` (wallet existence/readiness, handler lookup, `{ type: 'add-signer', wallet }` commitment).
- `addLoginSigner` / `addLoginSignerFromPrepared` (argument conversion, duplicate signers, topology update, module integration, witnessing, config update).
- `removeLoginSigner` / `completeRemoveLoginSigner` (missing signer, last-signer protection, module updates, request-action validation).
- `completeRedirect` branching on `commitment.type` (`add-signer` / `auth` / `reauth`) and related error paths.
Please add focused unit tests that:
- Cover the happy path for adding a login signer via redirect with mocked handlers/config.
- Assert key error cases (`wallet-not-found`, `wallet-not-ready`, `signer-already-exists`, `signer-not-found`, `cannot-remove-last-login-signer`, `invalid-request-action`).
- Verify that `completeRedirect` with an `add-signer` commitment calls `addLoginSignerFromPrepared` with the expected signer and `signerKind`.
Even partial coverage here would substantially increase confidence in the new federation/defederation flow.
Suggested implementation:
```typescript
expect(url).toBe('https://accounts.google.com/o/oauth2/v2/auth?state=test-state')
expect(commitAuthSpy).toHaveBeenCalledWith('/auth/return', { type: 'auth' })
})
describe('login signer redirect flows', () => {
it('starts add-login-signer with redirect and commits add-signer auth', async () => {
// Arrange
const commitAuth = vi.fn()
const startAuthWithRedirect = vi.fn().mockResolvedValue(
'https://accounts.google.com/o/oauth2/v2/auth?state=test-state',
)
const wallets = new Wallets({
// These options should match the existing test helpers/config used above
commitAuth,
authHandlers: {
google: {
startAuthWithRedirect,
},
},
} as any)
const walletId = 'wallet-1'
// Ensure the wallet exists and is ready. Use whatever helper the rest of the test
// file already uses (e.g. createWalletForTest / seedWallet / etc.).
await wallets.createWallet({
id: walletId,
status: 'ready',
loginSigners: [],
} as any)
// Act
const url = await wallets.startAddLoginSignerWithRedirect(walletId, {
provider: 'google',
signerKind: 'login',
redirectPath: '/auth/return',
} as any)
// Assert
expect(startAuthWithRedirect).toHaveBeenCalledTimes(1)
expect(url).toContain('https://accounts.google.com/o/oauth2/v2/auth')
expect(commitAuth).toHaveBeenCalledWith('/auth/return', {
type: 'add-signer',
wallet: walletId,
signerKind: 'login',
provider: 'google',
})
})
it('fails startAddLoginSignerWithRedirect when wallet is not found or not ready', async () => {
const commitAuth = vi.fn()
const startAuthWithRedirect = vi.fn()
const wallets = new Wallets({
commitAuth,
authHandlers: {
google: {
startAuthWithRedirect,
},
},
} as any)
await expect(
wallets.startAddLoginSignerWithRedirect('missing-wallet', {
provider: 'google',
signerKind: 'login',
redirectPath: '/auth/return',
} as any),
).rejects.toMatchObject({ code: 'wallet-not-found' })
const walletId = 'wallet-not-ready'
await wallets.createWallet({
id: walletId,
status: 'pending',
loginSigners: [],
} as any)
await expect(
wallets.startAddLoginSignerWithRedirect(walletId, {
provider: 'google',
signerKind: 'login',
redirectPath: '/auth/return',
} as any),
).rejects.toMatchObject({ code: 'wallet-not-ready' })
})
it('prevents adding duplicate login signers and updates topology on addLoginSignerFromPrepared', async () => {
const commitAuth = vi.fn()
const wallets = new Wallets({
commitAuth,
} as any)
const walletId = 'wallet-with-signer'
const existingSigner = {
id: 'signer-1',
kind: 'login',
provider: 'google',
subject: 'user-123',
}
await wallets.createWallet({
id: walletId,
status: 'ready',
loginSigners: [existingSigner],
} as any)
const prepared = {
walletId,
signer: {
id: 'signer-1',
kind: 'login',
provider: 'google',
subject: 'user-123',
},
}
await expect(wallets.addLoginSignerFromPrepared(prepared as any)).rejects.toMatchObject({
code: 'signer-already-exists',
})
const newPrepared = {
walletId,
signer: {
id: 'signer-2',
kind: 'login',
provider: 'google',
subject: 'user-456',
},
}
const updated = await wallets.addLoginSignerFromPrepared(newPrepared as any)
expect(updated.loginSigners).toEqual(
expect.arrayContaining([
expect.objectContaining({ id: 'signer-1' }),
expect.objectContaining({ id: 'signer-2' }),
]),
)
})
it('prevents removing missing or last login signer', async () => {
const commitAuth = vi.fn()
const wallets = new Wallets({
commitAuth,
} as any)
const walletId = 'wallet-remove-signer'
const signer = {
id: 'signer-1',
kind: 'login',
provider: 'google',
subject: 'user-123',
}
await wallets.createWallet({
id: walletId,
status: 'ready',
loginSigners: [signer],
} as any)
await expect(
wallets.removeLoginSigner(walletId, 'non-existent-signer'),
).rejects.toMatchObject({ code: 'signer-not-found' })
await expect(wallets.removeLoginSigner(walletId, signer.id)).rejects.toMatchObject({
code: 'cannot-remove-last-login-signer',
})
})
it('completes add-signer redirect and calls addLoginSignerFromPrepared with signer and kind', async () => {
const commitAuth = vi.fn()
const addLoginSignerFromPrepared = vi.fn().mockResolvedValue({
id: 'wallet-1',
loginSigners: [],
})
const wallets = new Wallets({
commitAuth,
addLoginSignerFromPrepared,
} as any)
const walletId = 'wallet-1'
const signer = {
id: 'signer-2',
kind: 'login',
provider: 'google',
subject: 'user-456',
}
const commitment = {
type: 'add-signer' as const,
wallet: walletId,
signerKind: 'login' as const,
signer,
}
const state = 'test-state'
;(wallets as any)._redirectCommitments = new Map([[state, commitment]])
await wallets.completeRedirect({
state,
provider: 'google',
code: 'oauth-code',
idToken: 'id-token',
} as any)
expect(addLoginSignerFromPrepared).toHaveBeenCalledWith({
walletId,
signer,
signerKind: 'login',
provider: 'google',
})
})
it('branches in completeRedirect based on commitment type and validates request action', async () => {
const commitAuth = vi.fn()
const addLoginSignerFromPrepared = vi.fn()
const completeAuth = vi.fn()
const completeReauth = vi.fn()
const wallets = new Wallets({
commitAuth,
addLoginSignerFromPrepared,
completeAuth,
completeReauth,
} as any)
const stateAdd = 'state-add'
const stateAuth = 'state-auth'
const stateReauth = 'state-reauth'
const stateInvalid = 'state-invalid'
const signer = { id: 'signer-1', kind: 'login' }
;(wallets as any)._redirectCommitments = new Map([
[
stateAdd,
{
type: 'add-signer',
wallet: 'wallet-1',
signerKind: 'login',
signer,
},
],
[
stateAuth,
{
type: 'auth',
requestAction: 'login',
},
],
[
stateReauth,
{
type: 'reauth',
},
],
[
stateInvalid,
{
type: 'auth',
requestAction: 'unknown-action',
},
],
])
await wallets.completeRedirect({
state: stateAdd,
provider: 'google',
code: 'c',
} as any)
expect(addLoginSignerFromPrepared).toHaveBeenCalled()
await wallets.completeRedirect({
state: stateAuth,
provider: 'google',
code: 'c',
} as any)
expect(completeAuth).toHaveBeenCalled()
await wallets.completeRedirect({
state: stateReauth,
provider: 'google',
code: 'c',
} as any)
expect(completeReauth).toHaveBeenCalled()
await expect(
wallets.completeRedirect({
state: stateInvalid,
provider: 'google',
code: 'c',
} as any),
).rejects.toMatchObject({ code: 'invalid-request-action' })
})
})
it('Should reject google-id-token signup when Google is configured for redirect auth', async () => {
```
These tests assume several APIs and helpers which you may need to align with the actual implementation:
1. **Wallet construction and helpers**
- Replace `new Wallets({ ... } as any)` with the real factory/helper used elsewhere in `wallets.test.ts` (e.g. `createWalletsForTest`), and pass the correct options shape.
- Replace direct calls to `wallets.createWallet(...)` with the existing helper or repository pattern used in the rest of the tests to create/seed wallets.
2. **Method signatures**
- Adjust the signatures for:
- `startAddLoginSignerWithRedirect(walletId, options)`
- `addLoginSignerFromPrepared(prepared)`
- `removeLoginSigner(walletId, signerId)`
- `completeRedirect(params)`
- Ensure option/parameter names (`provider`, `signerKind`, `redirectPath`, `walletId`, `signer`) match the actual implementation.
3. **Error objects**
- The tests currently assert `rejects.toMatchObject({ code: 'wallet-not-found' })` etc. Update the `code` strings and the error shape to match the real error types thrown by the wallet module.
4. **Internal commitment storage**
- The tests access `(wallets as any)._redirectCommitments` as a `Map`. If commitments are stored differently (e.g. via a separate store, repo, or module), replace this with the appropriate setup, or use the real API to seed commitments for `completeRedirect`.
5. **Branch callbacks**
- The `completeRedirect` branching test passes `completeAuth` and `completeReauth` on the `Wallets` constructor; if the actual module wires these differently (e.g. via separate services, injected modules, or methods on `wallets`), update the tests to spy on/capture the actual call sites.
Once these are aligned with your existing helpers and module wiring, the tests will provide coverage for:
- `startAddLoginSignerWithRedirect` happy path and error paths.
- `addLoginSignerFromPrepared` duplicate-signer protection and topology update.
- `removeLoginSigner` missing/last-signer protection.
- `completeRedirect` branching on `commitment.type` (`add-signer` / `auth` / `reauth`) and invalid `requestAction`.
</issue_to_address>
### Comment 6
<location path="README.md" line_range="35" />
<code_context>
- > **Note:** Tests require [anvil](https://github.com/foundry-rs/foundry/tree/master/anvil) and [forge](https://github.com/foundry-rs/foundry) to be installed. You can run a local anvil instance using `pnpm run test:anvil`.
+ > **Note:** Tests require [anvil](https://github.com/foundry-rs/foundry/tree/master/crates/anvil) and [forge](https://github.com/foundry-rs/foundry) to be installed. You can run a local anvil instance using `pnpm run test:anvil`.
- Linting and formatting is enforced via git hooks
</code_context>
<issue_to_address>
**suggestion (typo):** Consider changing the verb to match the compound subject: "Linting and formatting are enforced via git hooks."
Since the subject is plural ("Linting and formatting"), the verb should be plural as well: "are enforced."
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| return handler.commitAuth(args.target, { type: 'add-signer', wallet: args.wallet }) | ||
| } | ||
|
|
||
| async completeRedirect(args: CompleteRedirectArgs): Promise<string> { |
There was a problem hiding this comment.
issue: Handle legacy AuthCommitment records that still use isSignUp instead of type to avoid breaking existing redirects.
Existing AuthCommitment records in IndexedDB use the old { isSignUp: boolean, signer?: string } shape and won’t have type set. In those cases commitment.type will be undefined, no switch case will match, and we’ll fall through to the target redirect without performing sign-up/add-signer/reauth. That’s a regression for users upgrading with pending auth commitments.
Please add a fallback for legacy records (e.g., derive type from isSignUp/signer, or at least throw a clear error) so they either continue to work or fail explicitly instead of silently doing nothing.
| const isSignUp = true | ||
|
|
||
| const result = await handler.commitAuth(target, isSignUp) | ||
| const result = await handler.commitAuth(target, { type: 'auth' }) |
There was a problem hiding this comment.
suggestion (testing): Add tests covering the new type: 'add-signer' path and wallet field for PKCE auth commitments
The new type: 'add-signer' path in AuthCodePkceHandler.commitAuth isn’t covered. Please add a test that calls commitAuth(target, { type: 'add-signer', wallet: <address>, state?: <custom> }), asserts commitments.set receives { type: 'add-signer', wallet: <address> } with no signer field, and checks that state is preserved when provided and randomly generated when omitted. This will help guard the add-signer redirect flow against regressions.
Suggested implementation:
describe('commitAuth', () => {
it('should create add-signer PKCE commitment with explicit state', async () => {
const target = 'https://example.com/success'
const wallet = '0x1111111111111111111111111111111111111111'
const state = 'custom-state'
const result = await handler.commitAuth(target, {
type: 'add-signer',
wallet,
state,
})
// commitment should be stored without signer field
expect(handler['commitments'].set).toHaveBeenCalledWith(state, {
type: 'add-signer',
wallet,
})
// returned state/URL should preserve the provided state
if ('state' in result) {
expect(result.state).toBe(state)
}
if ('url' in result && typeof result.url === 'string') {
expect(result.url).toContain(`state=${encodeURIComponent(state)}`)
}
})
it('should create add-signer PKCE commitment with generated state when omitted', async () => {
const target = 'https://example.com/success'
const wallet = '0x2222222222222222222222222222222222222222'
const result = await handler.commitAuth(target, {
type: 'add-signer',
wallet,
})
// inspect last call to commitments.set to get generated state and stored commitment
const setMock = handler['commitments'].set as jest.Mock
const [[generatedState, storedCommitment]] = setMock.mock.calls.slice(-1)
expect(typeof generatedState).toBe('string')
expect(generatedState.length).toBeGreaterThan(0)
// commitment should have no signer field
expect(storedCommitment).toEqual({
type: 'add-signer',
wallet,
})
// returned state/URL should contain the generated state
if ('state' in result) {
expect(result.state).toBe(generatedState)
}
if ('url' in result && typeof result.url === 'string') {
expect(result.url).toContain(`state=${encodeURIComponent(generatedState)}`)
}
})These edits assume:
handleris in scope in thisdescribe('commitAuth', ...)block and is the same instance used in the existing PKCE tests.handler['commitments'].setis a Jest mock/spied function. If it is not yet mocked, you will need to:- Either construct
handlerwith a mockedcommitmentsimplementation, - Or wrap
jest.spyOn(handler['commitments'], 'set')in abeforeEachfor thisdescribeand cast it tojest.Mockwhere used.
- Either construct
- The
commitAuthreturn type either exposes astatefield, a URL withstateas a query parameter, or both. If the actual shape differs, adjust theresult.state/result.urlassertions to match your existingauthflow test style (e.g. if you already parsenew URL(result.url).searchParams.get('state')in the other test, mirror that here).
| const signer = testWallet | ||
|
|
||
| const result = await authCodeHandler.commitAuth(target, isSignUp, undefined, signer) | ||
| const result = await authCodeHandler.commitAuth(target, { type: 'auth' }) |
There was a problem hiding this comment.
suggestion (testing): Cover AuthCodeHandler.commitAuth for the add-signer commitment type as well
The core AuthCodeHandler.commitAuth now has three branches: auth, reauth (with signer), and add-signer (with wallet). The updated tests cover auth and reauth, but not type: 'add-signer'.
Please add a test that calls commitAuth('/target', { type: 'add-signer', wallet: testWallet }) and asserts that authCommitments.set receives { type: 'add-signer', wallet: testWallet } with no signer field. This ensures the DB representation for add-signer commitments matches the new union type.
| }, | ||
| target: 'apple-redirect-url', | ||
| isSignUp: false, | ||
| type: 'auth', |
There was a problem hiding this comment.
suggestion (testing): Extend DB tests to include the new add-signer auth commitment variant
The DB tests only cover type: 'auth' and type: 'reauth' commitments; they don’t exercise the new type: 'add-signer' variant that stores a wallet instead of a signer.
Please add a test that writes an AuthCommitment with type: 'add-signer' and a wallet address, then reads it back and asserts the type, wallet value, and absence of signer, to verify the IndexedDB layer handles this variant correctly.
|
|
||
| expect(url).toBe('https://accounts.google.com/o/oauth2/v2/auth?state=test-state') | ||
| expect(commitAuthSpy).toHaveBeenCalledWith('/auth/return', true) | ||
| expect(commitAuthSpy).toHaveBeenCalledWith('/auth/return', { type: 'auth' }) |
There was a problem hiding this comment.
suggestion (testing): Add tests for the new add/remove login signer flows and completeRedirect branches
The tests only verify the updated commitAuth signature, but the new Wallets behavior remains untested, including:
startAddLoginSignerWithRedirect(wallet existence/readiness, handler lookup,{ type: 'add-signer', wallet }commitment).addLoginSigner/addLoginSignerFromPrepared(argument conversion, duplicate signers, topology update, module integration, witnessing, config update).removeLoginSigner/completeRemoveLoginSigner(missing signer, last-signer protection, module updates, request-action validation).completeRedirectbranching oncommitment.type(add-signer/auth/reauth) and related error paths.
Please add focused unit tests that:
- Cover the happy path for adding a login signer via redirect with mocked handlers/config.
- Assert key error cases (
wallet-not-found,wallet-not-ready,signer-already-exists,signer-not-found,cannot-remove-last-login-signer,invalid-request-action). - Verify that
completeRedirectwith anadd-signercommitment callsaddLoginSignerFromPreparedwith the expected signer andsignerKind.
Even partial coverage here would substantially increase confidence in the new federation/defederation flow.
Suggested implementation:
expect(url).toBe('https://accounts.google.com/o/oauth2/v2/auth?state=test-state')
expect(commitAuthSpy).toHaveBeenCalledWith('/auth/return', { type: 'auth' })
})
describe('login signer redirect flows', () => {
it('starts add-login-signer with redirect and commits add-signer auth', async () => {
// Arrange
const commitAuth = vi.fn()
const startAuthWithRedirect = vi.fn().mockResolvedValue(
'https://accounts.google.com/o/oauth2/v2/auth?state=test-state',
)
const wallets = new Wallets({
// These options should match the existing test helpers/config used above
commitAuth,
authHandlers: {
google: {
startAuthWithRedirect,
},
},
} as any)
const walletId = 'wallet-1'
// Ensure the wallet exists and is ready. Use whatever helper the rest of the test
// file already uses (e.g. createWalletForTest / seedWallet / etc.).
await wallets.createWallet({
id: walletId,
status: 'ready',
loginSigners: [],
} as any)
// Act
const url = await wallets.startAddLoginSignerWithRedirect(walletId, {
provider: 'google',
signerKind: 'login',
redirectPath: '/auth/return',
} as any)
// Assert
expect(startAuthWithRedirect).toHaveBeenCalledTimes(1)
expect(url).toContain('https://accounts.google.com/o/oauth2/v2/auth')
expect(commitAuth).toHaveBeenCalledWith('/auth/return', {
type: 'add-signer',
wallet: walletId,
signerKind: 'login',
provider: 'google',
})
})
it('fails startAddLoginSignerWithRedirect when wallet is not found or not ready', async () => {
const commitAuth = vi.fn()
const startAuthWithRedirect = vi.fn()
const wallets = new Wallets({
commitAuth,
authHandlers: {
google: {
startAuthWithRedirect,
},
},
} as any)
await expect(
wallets.startAddLoginSignerWithRedirect('missing-wallet', {
provider: 'google',
signerKind: 'login',
redirectPath: '/auth/return',
} as any),
).rejects.toMatchObject({ code: 'wallet-not-found' })
const walletId = 'wallet-not-ready'
await wallets.createWallet({
id: walletId,
status: 'pending',
loginSigners: [],
} as any)
await expect(
wallets.startAddLoginSignerWithRedirect(walletId, {
provider: 'google',
signerKind: 'login',
redirectPath: '/auth/return',
} as any),
).rejects.toMatchObject({ code: 'wallet-not-ready' })
})
it('prevents adding duplicate login signers and updates topology on addLoginSignerFromPrepared', async () => {
const commitAuth = vi.fn()
const wallets = new Wallets({
commitAuth,
} as any)
const walletId = 'wallet-with-signer'
const existingSigner = {
id: 'signer-1',
kind: 'login',
provider: 'google',
subject: 'user-123',
}
await wallets.createWallet({
id: walletId,
status: 'ready',
loginSigners: [existingSigner],
} as any)
const prepared = {
walletId,
signer: {
id: 'signer-1',
kind: 'login',
provider: 'google',
subject: 'user-123',
},
}
await expect(wallets.addLoginSignerFromPrepared(prepared as any)).rejects.toMatchObject({
code: 'signer-already-exists',
})
const newPrepared = {
walletId,
signer: {
id: 'signer-2',
kind: 'login',
provider: 'google',
subject: 'user-456',
},
}
const updated = await wallets.addLoginSignerFromPrepared(newPrepared as any)
expect(updated.loginSigners).toEqual(
expect.arrayContaining([
expect.objectContaining({ id: 'signer-1' }),
expect.objectContaining({ id: 'signer-2' }),
]),
)
})
it('prevents removing missing or last login signer', async () => {
const commitAuth = vi.fn()
const wallets = new Wallets({
commitAuth,
} as any)
const walletId = 'wallet-remove-signer'
const signer = {
id: 'signer-1',
kind: 'login',
provider: 'google',
subject: 'user-123',
}
await wallets.createWallet({
id: walletId,
status: 'ready',
loginSigners: [signer],
} as any)
await expect(
wallets.removeLoginSigner(walletId, 'non-existent-signer'),
).rejects.toMatchObject({ code: 'signer-not-found' })
await expect(wallets.removeLoginSigner(walletId, signer.id)).rejects.toMatchObject({
code: 'cannot-remove-last-login-signer',
})
})
it('completes add-signer redirect and calls addLoginSignerFromPrepared with signer and kind', async () => {
const commitAuth = vi.fn()
const addLoginSignerFromPrepared = vi.fn().mockResolvedValue({
id: 'wallet-1',
loginSigners: [],
})
const wallets = new Wallets({
commitAuth,
addLoginSignerFromPrepared,
} as any)
const walletId = 'wallet-1'
const signer = {
id: 'signer-2',
kind: 'login',
provider: 'google',
subject: 'user-456',
}
const commitment = {
type: 'add-signer' as const,
wallet: walletId,
signerKind: 'login' as const,
signer,
}
const state = 'test-state'
;(wallets as any)._redirectCommitments = new Map([[state, commitment]])
await wallets.completeRedirect({
state,
provider: 'google',
code: 'oauth-code',
idToken: 'id-token',
} as any)
expect(addLoginSignerFromPrepared).toHaveBeenCalledWith({
walletId,
signer,
signerKind: 'login',
provider: 'google',
})
})
it('branches in completeRedirect based on commitment type and validates request action', async () => {
const commitAuth = vi.fn()
const addLoginSignerFromPrepared = vi.fn()
const completeAuth = vi.fn()
const completeReauth = vi.fn()
const wallets = new Wallets({
commitAuth,
addLoginSignerFromPrepared,
completeAuth,
completeReauth,
} as any)
const stateAdd = 'state-add'
const stateAuth = 'state-auth'
const stateReauth = 'state-reauth'
const stateInvalid = 'state-invalid'
const signer = { id: 'signer-1', kind: 'login' }
;(wallets as any)._redirectCommitments = new Map([
[
stateAdd,
{
type: 'add-signer',
wallet: 'wallet-1',
signerKind: 'login',
signer,
},
],
[
stateAuth,
{
type: 'auth',
requestAction: 'login',
},
],
[
stateReauth,
{
type: 'reauth',
},
],
[
stateInvalid,
{
type: 'auth',
requestAction: 'unknown-action',
},
],
])
await wallets.completeRedirect({
state: stateAdd,
provider: 'google',
code: 'c',
} as any)
expect(addLoginSignerFromPrepared).toHaveBeenCalled()
await wallets.completeRedirect({
state: stateAuth,
provider: 'google',
code: 'c',
} as any)
expect(completeAuth).toHaveBeenCalled()
await wallets.completeRedirect({
state: stateReauth,
provider: 'google',
code: 'c',
} as any)
expect(completeReauth).toHaveBeenCalled()
await expect(
wallets.completeRedirect({
state: stateInvalid,
provider: 'google',
code: 'c',
} as any),
).rejects.toMatchObject({ code: 'invalid-request-action' })
})
})
it('Should reject google-id-token signup when Google is configured for redirect auth', async () => {These tests assume several APIs and helpers which you may need to align with the actual implementation:
-
Wallet construction and helpers
- Replace
new Wallets({ ... } as any)with the real factory/helper used elsewhere inwallets.test.ts(e.g.createWalletsForTest), and pass the correct options shape. - Replace direct calls to
wallets.createWallet(...)with the existing helper or repository pattern used in the rest of the tests to create/seed wallets.
- Replace
-
Method signatures
- Adjust the signatures for:
startAddLoginSignerWithRedirect(walletId, options)addLoginSignerFromPrepared(prepared)removeLoginSigner(walletId, signerId)completeRedirect(params)
- Ensure option/parameter names (
provider,signerKind,redirectPath,walletId,signer) match the actual implementation.
- Adjust the signatures for:
-
Error objects
- The tests currently assert
rejects.toMatchObject({ code: 'wallet-not-found' })etc. Update thecodestrings and the error shape to match the real error types thrown by the wallet module.
- The tests currently assert
-
Internal commitment storage
- The tests access
(wallets as any)._redirectCommitmentsas aMap. If commitments are stored differently (e.g. via a separate store, repo, or module), replace this with the appropriate setup, or use the real API to seed commitments forcompleteRedirect.
- The tests access
-
Branch callbacks
- The
completeRedirectbranching test passescompleteAuthandcompleteReauthon theWalletsconstructor; if the actual module wires these differently (e.g. via separate services, injected modules, or methods onwallets), update the tests to spy on/capture the actual call sites.
- The
Once these are aligned with your existing helpers and module wiring, the tests will provide coverage for:
startAddLoginSignerWithRedirecthappy path and error paths.addLoginSignerFromPreparedduplicate-signer protection and topology update.removeLoginSignermissing/last-signer protection.completeRedirectbranching oncommitment.type(add-signer/auth/reauth) and invalidrequestAction.
| > **Note:** Tests require [anvil](https://github.com/foundry-rs/foundry/tree/master/anvil) and [forge](https://github.com/foundry-rs/foundry) to be installed. You can run a local anvil instance using `pnpm run test:anvil`. | ||
| > **Note:** Tests require [anvil](https://github.com/foundry-rs/foundry/tree/master/crates/anvil) and [forge](https://github.com/foundry-rs/foundry) to be installed. You can run a local anvil instance using `pnpm run test:anvil`. | ||
|
|
||
| - Linting and formatting is enforced via git hooks |
There was a problem hiding this comment.
suggestion (typo): Consider changing the verb to match the compound subject: "Linting and formatting are enforced via git hooks."
Since the subject is plural ("Linting and formatting"), the verb should be plural as well: "are enforced."
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements account federation support across the @0xsequence ecosystem, updating multiple packages to version 3.0.5. Core changes include refactoring authentication commitments to a discriminated union and adding wallet methods for managing federated login signers. Dependency updates include Next.js 15.5.14. Feedback was provided to address a security vulnerability in the basic-ftp dependency by upgrading to version 5.2.0 or later.
| basic-ftp@5.0.5: | ||
| resolution: {integrity: sha512-4Bcg1P8xhUuqcii/S0Z9wiHIrQVPMermM1any+MX5GeGD7faD3/msQUDGLol9wOcz4/jbg/WJnGqoJF6LiBdtg==} | ||
| engines: {node: '>=10.0.0'} | ||
| deprecated: Security vulnerability fixed in 5.2.0, please upgrade |
There was a problem hiding this comment.
The basic-ftp dependency (version 5.0.5) is marked as deprecated in the lock file due to a security vulnerability, with a recommendation to upgrade to 5.2.0 or newer. It is important to address security vulnerabilities in dependencies to maintain the integrity and security of the project. Please consider updating basic-ftp to a secure version in the relevant package.json file and regenerating the lock file.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add account federation capabilities to wallets by supporting adding and removing login signers, and extend OAuth auth code flows to distinguish between auth, reauth, and add-signer flows.
New Features:
Enhancements:
Build:
Documentation:
Tests: