Merged
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
|
Reviewer's GuideImplements account federation for wallets by adding add/remove login signer APIs and redirect flows, refactoring auth commitments from a boolean isSignUp flag to a discriminated union of auth/reauth/add-signer, and wiring configuration updates, witnessing, and tests to support these new flows, along with version/changelog bumps to 3.0.5 across packages. Class diagram for Wallets account federation and auth commitment typesclassDiagram
class WalletsInterface {
<<interface>>
+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 LoginSignerPrepared) Promise~string~
}
class StartAddLoginSignerWithRedirectArgs {
+wallet Address
+kind AuthKind
+target string
}
class AddLoginSignerArgs {
+wallet Address
}
class AddLoginSignerArgsMnemonic {
+kind "mnemonic"
+mnemonic string
}
class AddLoginSignerArgsEmailOtp {
+kind "email-otp"
+email string
}
class AddLoginSignerArgsIdToken {
+kind IdTokenKind
+idToken string
}
class RemoveLoginSignerArgs {
+wallet Address
+signerAddress Address
}
class LoginSignerPrepared {
+signer SignerWitnessable
+extra WitnessExtraSignerKind
}
class AuthCommitments {
+set(commitment AuthCommitment) Promise~void~
+get(id string) Promise~AuthCommitment~
}
class AuthCommitment {
+id string
+kind AuthKind
+verifier string
+challenge string
+target string
}
class AuthCommitmentAuth {
+type "auth"
}
class AuthCommitmentReauth {
+type "reauth"
+signer string
}
class AuthCommitmentAddSigner {
+type "add-signer"
+wallet string
}
class CommitAuthArgsAuth {
+type "auth"
+state string
}
class CommitAuthArgsReauth {
+type "reauth"
+state string
+signer string
}
class CommitAuthArgsAddSigner {
+type "add-signer"
+state string
+wallet string
}
class AuthCodeHandler {
+commitAuth(target string, args CommitAuthArgs) Promise~string~
+completeAuth(commitment AuthCommitment, code string) Promise~IdentitySignerMetadata~
}
class AuthCodePkceHandler {
+commitAuth(target string, args CommitAuthArgs) Promise~string~
}
class ActionsEnum {
<<enumeration>>
+AddLoginSigner
+RemoveLoginSigner
}
class SignaturesModule {
+get(id string) SignatureRequest
}
class SignatureRequest {
+action ActionsEnum
}
class AuthKind {
<<type>>
+values google-pkce, apple, custom-*
}
class IdTokenKind {
<<type>>
+values google-id-token, apple-id-token, custom-*
}
class SignerWitnessable {
+address Address
+witness(stateProvider any, wallet Address, extra WitnessExtraSignerKind) Promise~void~
}
class WitnessExtraSignerKind {
}
WalletsInterface <|.. Wallets
AddLoginSignerArgs o-- AddLoginSignerArgsMnemonic
AddLoginSignerArgs o-- AddLoginSignerArgsEmailOtp
AddLoginSignerArgs o-- AddLoginSignerArgsIdToken
AuthCommitment <|-- AuthCommitmentAuth
AuthCommitment <|-- AuthCommitmentReauth
AuthCommitment <|-- AuthCommitmentAddSigner
CommitAuthArgsAuth <|-- CommitAuthArgsReauth
CommitAuthArgsAuth <|-- CommitAuthArgsAddSigner
AuthCodeHandler <|-- AuthCodePkceHandler
Wallets --> AuthCommitments
Wallets --> AuthCodeHandler
Wallets --> SignaturesModule
AuthCodeHandler --> AuthCommitments
AuthCodePkceHandler --> AuthCommitments
SignatureRequest --> ActionsEnum
AuthCommitment --> AuthKind
AddLoginSignerArgsIdToken --> IdTokenKind
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
@taylanpince is attempting to deploy a commit to the Foundry development Team on Vercel. A member of the Team first needs to authorize it. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces significant enhancements to account management by implementing account federation support. This feature allows users to link various login methods to a single wallet, improving flexibility and user experience. The changes involve a version bump across several packages, a refactoring of authentication commitment types, and the addition of new wallet interface methods and signature request actions to facilitate the management of these federated login signers. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
⛔ Snyk checks have failed. 3 issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
![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 5 issues, and left some high level feedback:
- The change from
isSignUp/signer?to a discriminatedtypeunion onAuthCommitmentmeans existing DB entries created by older versions will not havetypeset and will hit theswitch (commitment.type)incompleteRedirectwithout a matching case—consider adding a backward‑compatibility path (e.g. inferringtypefrom legacy fields) or a migration/guard that handles legacy commitments explicitly. - In
addLoginSignerFromPrepared, you callwitnesson the new signer before the configuration update is signed and applied; this may let the account become discoverable via a credential that is not yet part of the wallet config if the update is rejected or fails—consider deferring the witness call until aftercompleteConfigurationUpdate, or otherwise making the two steps atomic.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The change from `isSignUp`/`signer?` to a discriminated `type` union on `AuthCommitment` means existing DB entries created by older versions will not have `type` set and will hit the `switch (commitment.type)` in `completeRedirect` without a matching case—consider adding a backward‑compatibility path (e.g. inferring `type` from legacy fields) or a migration/guard that handles legacy commitments explicitly.
- In `addLoginSignerFromPrepared`, you call `witness` on the new signer before the configuration update is signed and applied; this may let the account become discoverable via a credential that is not yet part of the wallet config if the update is rejected or fails—consider deferring the witness call until after `completeConfigurationUpdate`, or otherwise making the two steps atomic.
## Individual Comments
### Comment 1
<location path="packages/wallet/wdk/src/sequence/wallets.ts" line_range="1457-1459" />
<code_context>
+ const { loginTopology, modules } = await this.getConfigurationParts(args.wallet)
+
+ const existingSigners = Config.getSigners(loginTopology)
+ const allExistingAddresses = [...existingSigners.signers, ...existingSigners.sapientSigners.map((s) => s.address)]
+
+ if (!allExistingAddresses.some((addr) => Address.isEqual(addr, args.signerAddress))) {
+ throw new Error('signer-not-found')
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid allowing removal of the ZeroAddress sentinel as a login signer.
In `removeLoginSigner`, `allExistingAddresses` includes `Constants.ZeroAddress`, so a call with `signerAddress = ZeroAddress` will pass the `signer-not-found` check, even though you later filter `ZeroAddress` out of `remainingMembers`. Since the rest of the code treats `ZeroAddress` as a sentinel, it shouldn’t be removable. Consider either excluding `ZeroAddress` from `allExistingAddresses` in the presence check or explicitly rejecting attempts to remove it with a dedicated error to keep the sentinel from being treated as a normal signer.
</issue_to_address>
### Comment 2
<location path="packages/wallet/wdk/test/authcode-pkce.test.ts" line_range="91" />
<code_context>
describe('commitAuth', () => {
it('Should create Google PKCE auth commitment and return OAuth URL', async () => {
const target = 'https://example.com/success'
- const isSignUp = true
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding explicit tests that assert on the new `type` discriminator for all flows, including add-signer
The updated tests now assert `type: 'auth'` / `type: 'reauth'` on commitments, which is good, but the new `add-signer` flow isn’t covered, and some tests still emphasize URL/`metadata` instead of the discriminated union shape.
Please add:
- A test for `commitAuth(target, { type: 'add-signer', wallet: '0x...' })` that checks the stored commitment includes `type: 'add-signer'` and the correct `wallet`.
- Optionally, a (parameterized) test that verifies the `type` field for all three modes (`auth`, `reauth`, `add-signer`) to protect the `CommitAuthArgs` union behavior from regressions in the PKCE handler.
Suggested implementation:
```typescript
describe('commitAuth', () => {
it('Should create Google PKCE auth commitment and return OAuth URL', async () => {
const target = 'https://example.com/success'
const result = await handler.commitAuth(target, { type: 'auth' })
// Verify nitroCommitVerifier was called with correct challenge and auth type
expect(handler['nitroCommitVerifier']).toHaveBeenCalledWith(
expect.objectContaining({
challenge: 'mock-challenge-hash',
target,
metadata: expect.objectContaining({
type: 'auth',
}),
}),
)
// Keep existing expectations about the returned OAuth URL, verifier options, etc.
// e.g. expect(result).toBe(expectedUrl)
// expect(...).toHaveBeenCalledWith(...)
})
it('Should create add-signer PKCE auth commitment with correct type and wallet', async () => {
const target = 'https://example.com/success'
const wallet = '0x1234567890abcdef1234567890abcdef12345678'
await handler.commitAuth(target, { type: 'add-signer', wallet })
expect(handler['nitroCommitVerifier']).toHaveBeenCalledWith(
expect.objectContaining({
challenge: 'mock-challenge-hash',
target,
metadata: expect.objectContaining({
type: 'add-signer',
wallet,
}),
}),
)
})
it.each([
{ label: 'auth', mode: { type: 'auth' as const }, expectedType: 'auth' },
{
label: 'reauth',
mode: { type: 'reauth' as const },
expectedType: 'reauth',
},
{
label: 'add-signer',
mode: {
type: 'add-signer' as const,
wallet: '0xabcdefabcdefabcdefabcdefabcdefabcdefabcd',
},
expectedType: 'add-signer',
},
])(
'Should store commitment with correct type discriminator for %s flow',
async ({ mode, expectedType }) => {
const target = 'https://example.com/success'
await handler.commitAuth(target, mode as any)
expect(handler['nitroCommitVerifier']).toHaveBeenCalledWith(
expect.objectContaining({
metadata: expect.objectContaining({
type: expectedType,
}),
}),
)
},
)
```
Depending on the actual implementation, you may need to:
1. Adjust the `expect(handler['nitroCommitVerifier']).toHaveBeenCalledWith(...)` shape if `nitroCommitVerifier` does not receive a single object argument. If it receives positional arguments, update the expectations to match your real signature, e.g.:
- `toHaveBeenCalledWith('mock-challenge-hash', target, expect.objectContaining({ type: 'auth' }))`.
2. Ensure the `metadata` structure truly contains `{ type, wallet? }`. If `type` is stored at a different level, change `metadata: expect.objectContaining({ type: ... })` to match the real data path.
3. If the type of `commitAuth` does not accept `{ type: 'reauth' }` or `{ type: 'add-signer', wallet }` exactly as written, update the `mode` objects in `it.each` to conform to your existing `CommitAuthArgs` union type.
4. If you already have separate tests for `reauth`, you can merge or deduplicate them so that you don’t have overlapping coverage with conflicting expectations.
</issue_to_address>
### Comment 3
<location path="packages/wallet/wdk/test/authcode.test.ts" line_range="248" />
<code_context>
it('Should create auth commitment and return OAuth URL', async () => {
const target = '/test-target'
- const isSignUp = true
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for `type: 'add-signer'` commitments and ensure all `CommitAuthArgs` variants are tested
The tests now cover `type: 'auth'` and `type: 'reauth'` in `AuthCodeHandler.commitAuth`, but not the new `type: 'add-signer'` variant.
Please add a test that calls `commitAuth('/target', { type: 'add-signer', wallet: testWallet })` and asserts that `mockAuthCommitmentsSet` receives a commitment with `type: 'add-signer'` and `wallet: testWallet`.
You could also add a small table-driven test over all three `type` values to verify the expected presence/absence of `signer` and `wallet` fields, matching the discriminated union definition.
</issue_to_address>
### Comment 4
<location path="packages/wallet/wdk/test/identity-auth-dbs.test.ts" line_range="90-85" />
<code_context>
{
"name": "@0xsequence/api",
- "version": "3.0.4",
</code_context>
<issue_to_address>
**suggestion (testing):** AuthCommitments DB tests should validate the new `add-signer` commitment variant
Tests currently cover only the `auth` and `reauth` variants of `AuthCommitment`, but not `add-signer`. Please add a test that writes an `AuthCommitment` with `type: 'add-signer'` and a `wallet` value, then reads it back via `get` and asserts both the `type` and `wallet` fields are preserved. This will confirm the DB schema and serialization handle the new variant correctly.
</issue_to_address>
### Comment 5
<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 (complexity):** Consider extracting shared helpers for handler lookup, topology computation, and module updates to simplify redirect flows and login-signer add/remove logic.
You can reduce complexity without losing any behavior by factoring out some repeated logic and isolating pure computations.
### 1. Reduce duplication in `completeRedirect` (and redirect starters)
The handler lookup + validation is now repeated in multiple places:
- `startSignUpWithRedirect`
- `startAddLoginSignerWithRedirect`
- `completeRedirect` (`add-signer` and `reauth` cases)
Extract a small helper and reuse it:
```ts
private getAuthCodeHandlerForKind(kind: SignupArgs['kind'] | AuthCommitment['kind']): AuthCodeHandler {
const handlerKind = getSignupHandlerKey(kind)
const handler = this.shared.handlers.get(handlerKind)
if (!handler) throw new Error('handler-not-registered')
if (!(handler instanceof AuthCodeHandler)) throw new Error('handler-does-not-support-redirect')
return handler
}
```
Then the call sites become much simpler:
```ts
async startSignUpWithRedirect(args: StartSignUpWithRedirectArgs) {
const handler = this.getAuthCodeHandlerForKind(args.kind)
return handler.commitAuth(args.target, { type: 'auth' })
}
async startAddLoginSignerWithRedirect(args: StartAddLoginSignerWithRedirectArgs) {
const walletEntry = await this.get(args.wallet)
if (!walletEntry) throw new Error('wallet-not-found')
if (walletEntry.status !== 'ready') throw new Error('wallet-not-ready')
const handler = this.getAuthCodeHandlerForKind(args.kind)
return handler.commitAuth(args.target, { type: 'add-signer', wallet: args.wallet })
}
```
And `completeRedirect` becomes more readable:
```ts
async completeRedirect(args: CompleteRedirectArgs): Promise<string> {
const commitment = await this.shared.databases.authCommitments.get(args.state)
if (!commitment) throw new Error('invalid-state')
switch (commitment.type) {
case 'add-signer': {
const handler = this.getAuthCodeHandlerForKind(commitment.kind)
const walletAddress = commitment.wallet as Address.Address
const walletEntry = await this.get(walletAddress)
if (!walletEntry) throw new Error('wallet-not-found')
if (walletEntry.status !== 'ready') throw new Error('wallet-not-ready')
const [signer] = await handler.completeAuth(commitment, args.code)
const signerKind = getSignerKindForSignup(commitment.kind)
await this.addLoginSignerFromPrepared(walletAddress, { signer, extra: { signerKind } })
break
}
case 'auth': {
await this.signUp({
kind: commitment.kind,
commitment,
code: args.code,
noGuard: args.noGuard,
target: commitment.target,
isRedirect: true,
use4337: args.use4337,
})
break
}
case 'reauth': {
const handler = this.getAuthCodeHandlerForKind(commitment.kind)
await handler.completeAuth(commitment, args.code)
break
}
}
if (!commitment.target) throw new Error('invalid-state')
return commitment.target
}
```
If you want to go one step further, you can also split each switch branch into private methods (`handleAddSignerRedirect`, `handleAuthRedirect`, `handleReauthRedirect`) to keep `completeRedirect` very shallow.
---
### 2. Split responsibilities in `addLoginSignerFromPrepared` (and share with `removeLoginSigner`)
`addLoginSignerFromPrepared` currently does all of the following:
- Reads configuration
- Computes next login topology
- Mutates session and recovery modules
- Performs the witness
- Requests configuration update
You can move the pure topology computation to a helper that can also be reused in `removeLoginSigner`:
```ts
private computeNextLoginTopologyWithAddedSigner(
loginTopology: Config.Topology,
newMember: { address: Address.Address; imageHash?: Hex.Hex },
): Config.Topology {
const existingSigners = Config.getSigners(loginTopology)
const existingMembers = [
...existingSigners.signers
.filter((x) => x !== Constants.ZeroAddress)
.map((x) => ({ address: x })),
...existingSigners.sapientSigners.map((x) => ({ address: x.address, imageHash: x.imageHash })),
]
return buildCappedTree([...existingMembers, newMember])
}
```
Then `addLoginSignerFromPrepared` reads more linearly:
```ts
private async addLoginSignerFromPrepared(
wallet: Address.Address,
loginSigner: {
signer: (Signers.Signer | Signers.SapientSigner) & Signers.Witnessable
extra: WitnessExtraSignerKind
},
): Promise<string> {
const newSignerAddress = await loginSigner.signer.address
const { loginTopology, modules } = await this.getConfigurationParts(wallet)
const existingSigners = Config.getSigners(loginTopology)
const allExistingAddresses = [
...existingSigners.signers,
...existingSigners.sapientSigners.map((s) => s.address),
]
if (allExistingAddresses.some((addr) => Address.isEqual(addr, newSignerAddress))) {
throw new Error('signer-already-exists')
}
const newMember = {
address: newSignerAddress,
imageHash: Signers.isSapientSigner(loginSigner.signer)
? await loginSigner.signer.imageHash
: undefined,
}
const nextLoginTopology = this.computeNextLoginTopologyWithAddedSigner(loginTopology, newMember)
await this.addLoginSignerToModules(modules, newSignerAddress, loginSigner.signer)
await loginSigner.signer.witness(this.shared.sequence.stateProvider, wallet, loginSigner.extra)
return this.requestConfigurationUpdate(
wallet,
{ loginTopology: nextLoginTopology, modules },
Actions.AddLoginSigner,
'wallet-webapp',
)
}
```
With a small helper for module updates:
```ts
private async addLoginSignerToModules(
modules: Module[],
signerAddress: Address.Address,
signer: Signers.Signer | Signers.SapientSigner,
): Promise<void> {
if (!Signers.isSapientSigner(signer) && this.shared.modules.sessions.hasSessionModule(modules)) {
await this.shared.modules.sessions.addIdentitySignerToModules(modules, signerAddress)
}
if (this.shared.modules.recovery.hasRecoveryModule(modules)) {
await this.shared.modules.recovery.addRecoverySignerToModules(modules, signerAddress)
}
}
```
Similarly, `removeLoginSigner`’s topology update and module updates could reuse complementary helpers (`computeNextLoginTopologyWithRemovedSigner`, `removeLoginSignerFromModules`), which would make both add/remove flows easier to follow and maintain:
```ts
private computeNextLoginTopologyWithRemovedSigner(
loginTopology: Config.Topology,
signerToRemove: Address.Address,
): Config.Topology {
const existingSigners = Config.getSigners(loginTopology)
const remainingMembers = [
...existingSigners.signers
.filter((x) => x !== Constants.ZeroAddress && !Address.isEqual(x, signerToRemove))
.map((x) => ({ address: x })),
...existingSigners.sapientSigners
.filter((x) => !Address.isEqual(x.address, signerToRemove))
.map((x) => ({ address: x.address, imageHash: x.imageHash })),
]
if (remainingMembers.length < 1) {
throw new Error('cannot-remove-last-login-signer')
}
return buildCappedTree(remainingMembers)
}
```
Using these small helpers keeps the imperative methods focused on the high-level flow (“validate wallet”, “compute next topology”, “update modules”, “witness”, “request config update”) and reduces cognitive load without changing any behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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 introduces account federation capabilities, which is a significant and valuable feature. The refactoring of AuthCommitment from using a boolean isSignUp flag to a discriminated union with type is a great improvement for type safety and code clarity. The new methods for adding and removing login signers are well-implemented and follow the existing patterns in the codebase. The tests have also been updated to cover the new functionality. I've left a couple of minor suggestions to reduce code duplication, but overall this is a solid contribution.
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
3 participants
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 and extend OAuth auth code flows to support signup, reauth, and add-signer flows while bumping package versions to 3.0.5.
New Features:
Enhancements:
Build:
Documentation:
Tests: