feat: add wallet-link support to citizen-sdk and react-hooks (#33)#36
Open
edehvictor wants to merge 3 commits intoGoodDollar:mainfrom
Open
feat: add wallet-link support to citizen-sdk and react-hooks (#33)#36edehvictor wants to merge 3 commits intoGoodDollar:mainfrom
edehvictor wants to merge 3 commits intoGoodDollar:mainfrom
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 7 issues, and left some high level feedback:
- The wallet-link security flow in
IdentitySDKnow usesrunSecurityCheckwith console messaging only, but the new tests still expectwalletClient.signMessageto be called; consider aligning the implementation, comments, and tests so they reflect the same security-confirmation mechanism. - In
WalletLinkWidget,targetAddressis cast directly fromstringtoAddresswithout validation; consider validating the input (e.g., via viem’s address utilities) before callinguseWalletLinkand the connect/disconnect methods to avoid passing malformed addresses into the SDK. - The
useWalletLinkhook setssdkErrorto "Wallet or Public client not initialized" wheneverpublicClientorwalletClientis unavailable, which may surface as a user-facing error during normal wallet-connection flows; consider deferring this error until after an explicit initialization attempt or distinguishing between "not yet connected" and real error states.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The wallet-link security flow in `IdentitySDK` now uses `runSecurityCheck` with console messaging only, but the new tests still expect `walletClient.signMessage` to be called; consider aligning the implementation, comments, and tests so they reflect the same security-confirmation mechanism.
- In `WalletLinkWidget`, `targetAddress` is cast directly from `string` to `Address` without validation; consider validating the input (e.g., via viem’s address utilities) before calling `useWalletLink` and the connect/disconnect methods to avoid passing malformed addresses into the SDK.
- The `useWalletLink` hook sets `sdkError` to "Wallet or Public client not initialized" whenever `publicClient` or `walletClient` is unavailable, which may surface as a user-facing error during normal wallet-connection flows; consider deferring this error until after an explicit initialization attempt or distinguishing between "not yet connected" and real error states.
## Individual Comments
### Comment 1
<location path="packages/citizen-sdk/src/sdks/viem-identity-sdk.ts" line_range="269-278" />
<code_context>
+ async checkConnectedStatusAllChains(
</code_context>
<issue_to_address>
**suggestion (performance):** Consider reusing public clients for each chain instead of recreating them per call.
This creates a new `publicClient` for every chain on each `checkConnectedStatusAllChains` call, which can be expensive in UIs that poll frequently. Consider caching per-chain clients (e.g., in a module-level map keyed by `chainId`/`env` or on the `IdentitySDK` instance) and reusing them across calls.
Suggested implementation:
```typescript
/**
* Checks connection status for `account` across **all** supported chains
* simultaneously. Each chain uses its own read-only public client so no
* network switch is required.
*
* @param account - The wallet address to check.
* @returns An array of per-chain statuses (one entry per supported chain).
*/
async checkConnectedStatusAllChains(
account: Address,
): Promise<ChainConnectedStatus[]> {
const entries = Object.values(chainConfigs)
const settled = await Promise.allSettled(
entries.map(async (config) => {
const contracts = config.contracts[this.env]
if (!contracts) {
return {
```
To actually reuse public clients instead of recreating them on each `checkConnectedStatusAllChains` call, you’ll need to:
1. **Add a per-instance cache on the SDK class (recommended over module-level):**
- In the `ViemIdentitySDK` class definition, add a private map:
```ts
private publicClientCache: Map<string, PublicClient> = new Map()
```
- Ensure `PublicClient` is imported from `viem` (or your local wrapper) at the top of the file:
```ts
import type { PublicClient } from 'viem'
```
2. **Add a helper to get or create cached public clients:**
- Inside `ViemIdentitySDK`, implement something like:
```ts
private getOrCreatePublicClient(config: ChainConfig): PublicClient {
const key = `${config.id}:${this.env}` // or config.chain.id / chainId depending on your config shape
const cached = this.publicClientCache.get(key)
if (cached) return cached
const publicClient = createPublicClient({
chain: config.chain, // adapt to your existing code
transport: http(config.rpc), // adapt: reuse whatever transport/options you use today
})
this.publicClientCache.set(key, publicClient)
return publicClient
}
```
- Reuse the same `createPublicClient` / transport options you currently use in `checkConnectedStatusAllChains` (or elsewhere in the SDK) to avoid behavior changes.
3. **Replace per-call client creation with the cached helper call:**
- Inside the `entries.map(async (config) => { ... })` block of `checkConnectedStatusAllChains`, find the existing line where a `publicClient` is constructed, e.g.:
```ts
const publicClient = createPublicClient({ /* ... */ })
```
- Replace it with:
```ts
const publicClient = this.getOrCreatePublicClient(config)
```
- Keep the rest of the logic (reads, contract calls, error handling) unchanged.
This will ensure that each `(chain, env)` pair reuses the same `PublicClient` instance across repeated `checkConnectedStatusAllChains` calls, avoiding the overhead of recreating them in polling-heavy UIs. Adjust the `ChainConfig` type, cache key, and `createPublicClient` options to match the existing conventions in this file.
</issue_to_address>
### Comment 2
<location path="packages/citizen-sdk/src/sdks/viem-identity-sdk.ts" line_range="246-248" />
<code_context>
+ isConnected: root !== zeroAddress,
+ root,
+ }
+ } catch (error: any) {
+ console.error("getConnectedAccounts Error:", error)
+ throw new Error(`Failed to get connected accounts: ${error.message}`)
+ }
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Guard against non-`Error` throwables when building error messages.
Since `error` is typed as `any`, accessing `error.message` assumes it’s an `Error` instance and can itself throw or yield `undefined` for non-Error values. Consider normalizing the message first, e.g. `const msg = error instanceof Error ? error.message : String(error);` and then using `msg` in the thrown error. Please apply the same pattern in the other similar catch blocks (`connectAccount`, `disconnectAccount`).
Suggested implementation:
```typescript
return {
isConnected: root !== zeroAddress,
root,
}
} catch (error: any) {
const message = error instanceof Error ? error.message : String(error)
console.error("getConnectedAccounts Error:", error)
throw new Error(`Failed to get connected accounts: ${message}`)
}
/**
```
You should apply the same error-normalization pattern to the `connectAccount` and `disconnectAccount` methods in this file:
1. Locate their `catch (error: any)` blocks.
2. Replace direct `error.message` usage with:
- `const message = error instanceof Error ? error.message : String(error)`
- Use `message` in any thrown `Error` instances or logs rather than `error.message`.
Example pattern to use in those blocks:
```ts
} catch (error: any) {
const message = error instanceof Error ? error.message : String(error)
console.error("connectAccount Error:", error)
throw new Error(`Failed to connect account: ${message}`)
}
```
and similarly for `disconnectAccount`.
</issue_to_address>
### Comment 3
<location path="packages/react-hooks/src/citizen-sdk/wagmi-identity-sdk.ts" line_range="82-85" />
<code_context>
+ resolve: (confirmed: boolean) => void
+ } | null>(null)
+
+ const reset = useCallback(() => {
+ setError(null)
+ setTxHash(null)
+ setPendingSecurityConfirm(null)
+ }, [])
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider resetting `loading` in the wallet-link hooks' `reset` helpers.
`reset` currently clears error/tx/security state but not `loading`. If callers treat `reset` as a full state reset, the hook can remain `loading`, which may be surprising and cause inconsistent UI. Consider also setting `loading` to `false` here, depending on the intended contract of `reset`.
Suggested implementation:
```typescript
const [pendingSecurityConfirm, setPendingSecurityConfirm] = useState<{
message: string
resolve: (confirmed: boolean) => void
} | null>(null)
const reset = useCallback(() => {
setLoading(false)
setError(null)
setTxHash(null)
setPendingSecurityConfirm(null)
}, [])
```
If there are other wallet-link or IdentitySDK-related hooks in this file (or nearby files) that expose a `reset` helper with similar semantics, consider updating those `reset` callbacks to also call `setLoading(false)` so that all `reset` helpers behave consistently as a full state reset.
</issue_to_address>
### Comment 4
<location path="packages/citizen-sdk/test/wallet-link.test.ts" line_range="76-80" />
<code_context>
+
+
+ describe("Write Paths: connectAccount & disconnectAccount", () => {
+ it("connectAccount triggers security message and submits transaction", async () => {
+ await sdk.connectAccount(MOCK_CHILD_ACCOUNT)
+
+
+ expect(walletClient.signMessage).toHaveBeenCalledWith({
+ account: MOCK_ROOT_ACCOUNT,
+ message: WALLET_LINK_SECURITY_MESSAGES.connect,
</code_context>
<issue_to_address>
**issue (testing):** Test expectations for security flow are out of sync with the current implementation
The `connectAccount` test expects `walletClient.signMessage` to be called with `WALLET_LINK_SECURITY_MESSAGES.connect`, but the implementation now uses `runSecurityCheck`, which calls `options.onSecurityMessage` or logs to `console.info` and never calls `signMessage`. This makes the test inconsistent with the code and gives misleading coverage. Please update the test to assert the real `runSecurityCheck` behavior (e.g., that `options.onSecurityMessage` is awaited when provided, and/or that `submitAndWait` is only called after a successful confirmation) instead of checking for `signMessage`.
</issue_to_address>
### Comment 5
<location path="packages/citizen-sdk/test/wallet-link.test.ts" line_range="95-86" />
<code_context>
+ )
+ })
+
+ it("disconnectAccount suppresses security message when skipSecurityMessage=true", async () => {
+
+ await sdk.disconnectAccount(MOCK_CHILD_ACCOUNT, { skipSecurityMessage: true })
+
+ expect(walletClient.signMessage).not.toHaveBeenCalled()
+
+ expect(sdk.submitAndWait).toHaveBeenCalledWith(
+ expect.objectContaining({
+ functionName: "disconnectAccount",
</code_context>
<issue_to_address>
**suggestion (testing):** Cover additional branches of the security check (skip, custom callback, and rejection cases)
This test currently exercises `skipSecurityMessage` indirectly via `walletClient.signMessage`, which is no longer part of the flow, and it misses key `runSecurityCheck` branches:
- `skipSecurityMessage: true`: `onSecurityMessage` must not be called, and the tx should proceed.
- Custom `onSecurityMessage`: it should receive the correct message and its boolean return should control whether `submitAndWait` runs.
- `onSecurityMessage` resolving to `false`: `connectAccount` / `disconnectAccount` should throw the cancellation error from `runSecurityCheck` and must not call `submitAndWait`.
Consider updating this test (and adding one or two more) to mock `runSecurityCheck` / `options.onSecurityMessage` directly and assert these branches, rather than asserting on `walletClient.signMessage`.
Suggested implementation:
```typescript
expect(sdk.submitAndWait).toHaveBeenCalledWith(
expect.objectContaining({
functionName: "connectAccount",
args: [MOCK_CHILD_ACCOUNT],
}),
undefined
)
})
it("disconnectAccount suppresses security message when skipSecurityMessage=true", async () => {
const onSecurityMessage = vi.fn()
await sdk.disconnectAccount(MOCK_CHILD_ACCOUNT, {
skipSecurityMessage: true,
onSecurityMessage,
})
// When skipSecurityMessage is true, the security callback must not be invoked
expect(onSecurityMessage).not.toHaveBeenCalled()
// And the transaction should still be submitted
expect(sdk.submitAndWait).toHaveBeenCalledWith(
expect.objectContaining({
functionName: "disconnectAccount",
args: [MOCK_CHILD_ACCOUNT],
}),
undefined
)
})
it("disconnectAccount calls onSecurityMessage and proceeds when it resolves to true", async () => {
const onSecurityMessage = vi.fn().mockResolvedValue(true)
await sdk.disconnectAccount(MOCK_CHILD_ACCOUNT, { onSecurityMessage })
// Custom onSecurityMessage should be invoked with a security message
expect(onSecurityMessage).toHaveBeenCalledTimes(1)
expect(onSecurityMessage).toHaveBeenCalledWith(
expect.stringMatching(/disconnect/i)
)
// Since onSecurityMessage resolved to true, the transaction should be submitted
expect(sdk.submitAndWait).toHaveBeenCalledWith(
expect.objectContaining({
functionName: "disconnectAccount",
args: [MOCK_CHILD_ACCOUNT],
}),
undefined
)
})
it("disconnectAccount throws and does not submit when onSecurityMessage resolves to false", async () => {
const onSecurityMessage = vi.fn().mockResolvedValue(false)
await expect(
sdk.disconnectAccount(MOCK_CHILD_ACCOUNT, { onSecurityMessage })
).rejects.toMatchObject({
name: expect.stringMatching(/SecurityCheckCancelledError|Error/i),
})
// Custom onSecurityMessage should be invoked with a security message
expect(onSecurityMessage).toHaveBeenCalledTimes(1)
expect(onSecurityMessage).toHaveBeenCalledWith(
expect.stringMatching(/disconnect/i)
)
// Since onSecurityMessage resolved to false, the transaction must not be submitted
expect(sdk.submitAndWait).not.toHaveBeenCalledWith(
expect.objectContaining({
functionName: "disconnectAccount",
args: [MOCK_CHILD_ACCOUNT],
}),
undefined
)
})
})
})
```
Depending on how `runSecurityCheck` is structured in your codebase, you may also want to:
1. Explicitly mock `runSecurityCheck` (e.g., via `vi.mock` / `jest.mock`) in this test file and assert that it is:
- Not called when `skipSecurityMessage: true`.
- Called with `{ onSecurityMessage }` when a custom callback is provided.
2. Align the `expect.stringMatching(/disconnect/i)` matcher with the actual message shape that `runSecurityCheck` passes to `onSecurityMessage`. If the message is not a simple string (e.g., an object `{ title, body }`), update the expectations accordingly, for example:
- `expect(onSecurityMessage).toHaveBeenCalledWith(expect.objectContaining({ title: expect.stringContaining("disconnect") }))`.
3. If `disconnectAccount`/`connectAccount` use a custom error type for cancellations (e.g., `SecurityCheckCancelledError`), replace the generic `name` matcher with a stricter expectation on the specific error class or error code used in your implementation.
</issue_to_address>
### Comment 6
<location path="packages/citizen-sdk/test/wallet-link.test.ts" line_range="42-51" />
<code_context>
+ describe("Read Paths: connectedAccounts & checkConnectedStatusAllChains", () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for `checkConnectedStatusAllChains` and error-handling in read paths
The read-path tests cover `getConnectedAccounts` and `isAccountConnected`, but `checkConnectedStatusAllChains` and its error handling aren’t exercised. Given it fans out across chain configs and uses `Promise.allSettled`, please add tests that:
- Mock multiple chain entries (with and without `contracts[this.env]`) and assert the resulting `ChainConnectedStatus[]` sets `isConnected`, `root`, and `error` correctly.
- Simulate a rejected `readContract` for one chain and verify the `error` field is populated from the rejection reason.
- Optionally, have `publicClient.readContract` throw in `getConnectedAccounts` to assert the error is wrapped as `Failed to get connected accounts: ...`.
This will more thoroughly validate the multi-chain status and error-handling behavior.
Suggested implementation:
```typescript
describe("Read Paths: connectedAccounts & checkConnectedStatusAllChains", () => {
```
Please add the following tests inside the existing
```ts
describe("Read Paths: connectedAccounts & checkConnectedStatusAllChains", () => {
// ... here ...
})
```
block, after the existing happy-path test for `getConnectedAccounts`:
```ts
it("checkConnectedStatusAllChains should return status entries per chain and set isConnected/root correctly", async () => {
// First chain resolves with a root (connected)
// Second chain resolves with no root (not connected)
publicClient.readContract
.mockResolvedValueOnce(MOCK_ROOT_ACCOUNT)
.mockResolvedValueOnce(null)
const result = await sdk.checkConnectedStatusAllChains(MOCK_CHILD_ACCOUNT)
// We don't assert on exact chain IDs here because that depends on the internal chain config;
// instead we assert that we get at least one connected and one non-connected entry.
expect(Array.isArray(result)).toBe(true)
expect(result.length).toBeGreaterThanOrEqual(2)
const connectedEntry = result.find((entry) => entry.isConnected)
const disconnectedEntry = result.find((entry) => !entry.isConnected)
expect(connectedEntry).toEqual(
expect.objectContaining({
isConnected: true,
root: MOCK_ROOT_ACCOUNT,
error: null,
}),
)
expect(disconnectedEntry).toEqual(
expect.objectContaining({
isConnected: false,
root: null,
error: null,
}),
)
})
it("checkConnectedStatusAllChains should surface readContract rejections in the error field", async () => {
const readError = new Error("read failed")
// First chain succeeds, second chain rejects
publicClient.readContract
.mockResolvedValueOnce(MOCK_ROOT_ACCOUNT)
.mockRejectedValueOnce(readError)
const result = await sdk.checkConnectedStatusAllChains(MOCK_CHILD_ACCOUNT)
expect(Array.isArray(result)).toBe(true)
expect(result.length).toBeGreaterThanOrEqual(2)
const erroredEntry = result.find((entry) => entry.error)
expect(erroredEntry).toEqual(
expect.objectContaining({
isConnected: false,
root: null,
// Implementation may store the error as string or as Error; accept both
error: expect.stringContaining("read failed"),
}),
)
})
it("checkConnectedStatusAllChains should handle chains without contracts for the current env", async () => {
// This test assumes the implementation skips or marks chains that do not have contracts[this.env].
// We simulate this by having readContract only called for chains that *do* have contracts.
publicClient.readContract.mockResolvedValue(MOCK_ROOT_ACCOUNT)
const result = await sdk.checkConnectedStatusAllChains(MOCK_CHILD_ACCOUNT)
// Expect at least one entry that is marked as misconfigured / not connected
const misconfiguredEntry = result.find(
(entry) => entry.error && entry.error.toString().toLowerCase().includes("contract"),
)
expect(misconfiguredEntry).toEqual(
expect.objectContaining({
isConnected: false,
root: null,
error: expect.any(String),
}),
)
})
it("getConnectedAccounts should wrap readContract errors with a helpful message", async () => {
const underlyingError = new Error("boom")
publicClient.readContract.mockRejectedValueOnce(underlyingError)
await expect(sdk.getConnectedAccounts(MOCK_CHILD_ACCOUNT)).rejects.toThrow(
"Failed to get connected accounts: boom",
)
})
```
You may need to adjust expectations to match the exact implementation details:
1. **Shape of `ChainConnectedStatus`**
- If your type uses different property names than `isConnected`, `root`, and `error`, update the `expect.objectContaining` calls accordingly.
- If `error` is stored as an `Error` rather than a string, change:
```ts
error: expect.stringContaining("read failed")
```
to something like:
```ts
error: expect.objectContaining({ message: "read failed" })
```
2. **Number and identity of chains**
- If `checkConnectedStatusAllChains` returns a fixed set of chain IDs, you can tighten the tests by asserting on `chainId` (e.g., `expect(result).toContainEqual(expect.objectContaining({ chainId: 1, ... }))`).
- If the method filters chains without `contracts[this.env]` entirely (instead of returning an entry with an error), update the third test to assert the absence of such chains and verify `publicClient.readContract` calls only match configured chains.
3. **Error messages**
- If your implementation uses a different error prefix for `getConnectedAccounts`, update the final `toThrow` message to the exact string used in `getConnectedAccounts` (e.g., `"Failed to get connected accounts"` or similar).
</issue_to_address>
### Comment 7
<location path="packages/react-hooks/src/citizen-sdk/wagmi-identity-sdk.ts" line_range="71" />
<code_context>
+ reset: () => void
+}
+
+export const useConnectAccount = (
+ sdk: IdentitySDK | null,
+): UseConnectAccountReturn => {
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the shared wallet-link action logic into a generic hook and reusing the existing `useIdentitySDK` to avoid duplicating SDK initialization and action handling.
You can fold most of this duplication away without changing behavior by:
1. Extracting a generic wallet-link action hook.
2. Reusing `useIdentitySDK` inside `useWalletLink` instead of re‑implementing SDK init.
### 1) Factor out shared connect/disconnect logic
`useConnectAccount` and `useDisconnectAccount` are structurally identical. You can extract a generic hook that takes the SDK method as a parameter:
```ts
type WalletLinkAction =
(sdk: IdentitySDK, account: Address, options?: WalletLinkOptions) => Promise<void>
interface UseWalletLinkActionReturn {
run: (account: Address, options?: WalletLinkOptions) => Promise<void>
loading: boolean
error: string | null
txHash: `0x${string}` | null
pendingSecurityConfirm: { message: string } | null
confirmSecurity: (confirmed: boolean) => void
reset: () => void
}
const useWalletLinkAction = (
sdk: IdentitySDK | null,
action: WalletLinkAction,
): UseWalletLinkActionReturn => {
const [loading, setLoading] = useState(false)
const [error, setError] = useState<string | null>(null)
const [txHash, setTxHash] = useState<`0x${string}` | null>(null)
const [pendingSecurityConfirm, setPendingSecurityConfirm] = useState<{
message: string
resolve: (confirmed: boolean) => void
} | null>(null)
const reset = useCallback(() => {
setError(null)
setTxHash(null)
setPendingSecurityConfirm(null)
}, [])
const confirmSecurity = useCallback(
(confirmed: boolean) => {
pendingSecurityConfirm?.resolve(confirmed)
setPendingSecurityConfirm(null)
},
[pendingSecurityConfirm],
)
const run = useCallback(
async (account: Address, options?: WalletLinkOptions) => {
if (!sdk) {
setError("IdentitySDK not initialized")
return
}
setLoading(true)
setError(null)
setTxHash(null)
try {
await action(sdk, account, {
...options,
onHash: (hash) => {
setTxHash(hash)
options?.onHash?.(hash)
},
onSecurityMessage:
options?.onSecurityMessage ??
(options?.skipSecurityMessage
? undefined
: (message) =>
new Promise((resolve) => {
setPendingSecurityConfirm({ message, resolve })
})),
})
} catch (err: any) {
setError(err instanceof Error ? err.message : String(err))
} finally {
setLoading(false)
}
},
[sdk, action],
)
return {
run,
loading,
error,
txHash,
pendingSecurityConfirm: pendingSecurityConfirm
? { message: pendingSecurityConfirm.message }
: null,
confirmSecurity,
reset,
}
}
```
Then your public hooks become thin wrappers:
```ts
export interface UseConnectAccountReturn extends UseWalletLinkActionReturn {
connect: UseWalletLinkActionReturn["run"]
}
export const useConnectAccount = (sdk: IdentitySDK | null): UseConnectAccountReturn => {
const base = useWalletLinkAction(sdk, (s, account, options) =>
s.connectAccount(account, options),
)
return { ...base, connect: base.run }
}
export interface UseDisconnectAccountReturn extends UseWalletLinkActionReturn {
disconnect: UseWalletLinkActionReturn["run"]
}
export const useDisconnectAccount = (sdk: IdentitySDK | null): UseDisconnectAccountReturn => {
const base = useWalletLinkAction(sdk, (s, account, options) =>
s.disconnectAccount(account, options),
)
return { ...base, disconnect: base.run }
}
```
This keeps all semantics (security confirmation, `onHash` delegation, error handling) but centralizes them in one place.
### 2) Reuse `useIdentitySDK` inside `useWalletLink`
`useWalletLink` re‑implements SDK initialization instead of reusing `useIdentitySDK`. You can compose them to avoid two divergent init paths:
```ts
export const useWalletLink = (
env: contractEnv = "production",
watchAccount?: Address,
): UseWalletLinkReturn => {
const { sdk, loading, error } = useIdentitySDK(env)
const connectAccount = useConnectAccount(sdk)
const disconnectAccount = useDisconnectAccount(sdk)
const connectedStatus = useConnectedStatus(sdk, watchAccount)
return {
sdk,
sdkLoading: loading,
sdkError: error,
connectAccount,
disconnectAccount,
connectedStatus,
}
}
```
If you need slightly different error messages or behaviors, you can still layer those on top (e.g., map `error` to `sdkError` with custom messaging) while keeping initialization logic in a single hook.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+76
to
+80
| it("connectAccount triggers security message and submits transaction", async () => { | ||
| await sdk.connectAccount(MOCK_CHILD_ACCOUNT) | ||
|
|
||
|
|
||
| expect(walletClient.signMessage).toHaveBeenCalledWith({ |
Contributor
There was a problem hiding this comment.
issue (testing): Test expectations for security flow are out of sync with the current implementation
The connectAccount test expects walletClient.signMessage to be called with WALLET_LINK_SECURITY_MESSAGES.connect, but the implementation now uses runSecurityCheck, which calls options.onSecurityMessage or logs to console.info and never calls signMessage. This makes the test inconsistent with the code and gives misleading coverage. Please update the test to assert the real runSecurityCheck behavior (e.g., that options.onSecurityMessage is awaited when provided, and/or that submitAndWait is only called after a successful confirmation) instead of checking for signMessage.
Comment on lines
+42
to
+51
| describe("Read Paths: connectedAccounts & checkConnectedStatusAllChains", () => { | ||
| it("getConnectedAccounts should return isConnected=true and root for a child account", async () => { | ||
| publicClient.readContract.mockResolvedValueOnce(MOCK_ROOT_ACCOUNT) | ||
|
|
||
| const status = await sdk.getConnectedAccounts(MOCK_CHILD_ACCOUNT) | ||
|
|
||
| expect(publicClient.readContract).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| functionName: "connectedAccounts", | ||
| args: [MOCK_CHILD_ACCOUNT], |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add tests for checkConnectedStatusAllChains and error-handling in read paths
The read-path tests cover getConnectedAccounts and isAccountConnected, but checkConnectedStatusAllChains and its error handling aren’t exercised. Given it fans out across chain configs and uses Promise.allSettled, please add tests that:
- Mock multiple chain entries (with and without
contracts[this.env]) and assert the resultingChainConnectedStatus[]setsisConnected,root, anderrorcorrectly. - Simulate a rejected
readContractfor one chain and verify theerrorfield is populated from the rejection reason. - Optionally, have
publicClient.readContractthrow ingetConnectedAccountsto assert the error is wrapped asFailed to get connected accounts: ....
This will more thoroughly validate the multi-chain status and error-handling behavior.
Suggested implementation:
describe("Read Paths: connectedAccounts & checkConnectedStatusAllChains", () => {Please add the following tests inside the existing
describe("Read Paths: connectedAccounts & checkConnectedStatusAllChains", () => {
// ... here ...
})block, after the existing happy-path test for getConnectedAccounts:
it("checkConnectedStatusAllChains should return status entries per chain and set isConnected/root correctly", async () => {
// First chain resolves with a root (connected)
// Second chain resolves with no root (not connected)
publicClient.readContract
.mockResolvedValueOnce(MOCK_ROOT_ACCOUNT)
.mockResolvedValueOnce(null)
const result = await sdk.checkConnectedStatusAllChains(MOCK_CHILD_ACCOUNT)
// We don't assert on exact chain IDs here because that depends on the internal chain config;
// instead we assert that we get at least one connected and one non-connected entry.
expect(Array.isArray(result)).toBe(true)
expect(result.length).toBeGreaterThanOrEqual(2)
const connectedEntry = result.find((entry) => entry.isConnected)
const disconnectedEntry = result.find((entry) => !entry.isConnected)
expect(connectedEntry).toEqual(
expect.objectContaining({
isConnected: true,
root: MOCK_ROOT_ACCOUNT,
error: null,
}),
)
expect(disconnectedEntry).toEqual(
expect.objectContaining({
isConnected: false,
root: null,
error: null,
}),
)
})
it("checkConnectedStatusAllChains should surface readContract rejections in the error field", async () => {
const readError = new Error("read failed")
// First chain succeeds, second chain rejects
publicClient.readContract
.mockResolvedValueOnce(MOCK_ROOT_ACCOUNT)
.mockRejectedValueOnce(readError)
const result = await sdk.checkConnectedStatusAllChains(MOCK_CHILD_ACCOUNT)
expect(Array.isArray(result)).toBe(true)
expect(result.length).toBeGreaterThanOrEqual(2)
const erroredEntry = result.find((entry) => entry.error)
expect(erroredEntry).toEqual(
expect.objectContaining({
isConnected: false,
root: null,
// Implementation may store the error as string or as Error; accept both
error: expect.stringContaining("read failed"),
}),
)
})
it("checkConnectedStatusAllChains should handle chains without contracts for the current env", async () => {
// This test assumes the implementation skips or marks chains that do not have contracts[this.env].
// We simulate this by having readContract only called for chains that *do* have contracts.
publicClient.readContract.mockResolvedValue(MOCK_ROOT_ACCOUNT)
const result = await sdk.checkConnectedStatusAllChains(MOCK_CHILD_ACCOUNT)
// Expect at least one entry that is marked as misconfigured / not connected
const misconfiguredEntry = result.find(
(entry) => entry.error && entry.error.toString().toLowerCase().includes("contract"),
)
expect(misconfiguredEntry).toEqual(
expect.objectContaining({
isConnected: false,
root: null,
error: expect.any(String),
}),
)
})
it("getConnectedAccounts should wrap readContract errors with a helpful message", async () => {
const underlyingError = new Error("boom")
publicClient.readContract.mockRejectedValueOnce(underlyingError)
await expect(sdk.getConnectedAccounts(MOCK_CHILD_ACCOUNT)).rejects.toThrow(
"Failed to get connected accounts: boom",
)
})You may need to adjust expectations to match the exact implementation details:
-
Shape of
ChainConnectedStatus- If your type uses different property names than
isConnected,root, anderror, update theexpect.objectContainingcalls accordingly. - If
erroris stored as anErrorrather than a string, change:to something like:error: expect.stringContaining("read failed")
error: expect.objectContaining({ message: "read failed" })
- If your type uses different property names than
-
Number and identity of chains
- If
checkConnectedStatusAllChainsreturns a fixed set of chain IDs, you can tighten the tests by asserting onchainId(e.g.,expect(result).toContainEqual(expect.objectContaining({ chainId: 1, ... }))). - If the method filters chains without
contracts[this.env]entirely (instead of returning an entry with an error), update the third test to assert the absence of such chains and verifypublicClient.readContractcalls only match configured chains.
- If
-
Error messages
- If your implementation uses a different error prefix for
getConnectedAccounts, update the finaltoThrowmessage to the exact string used ingetConnectedAccounts(e.g.,"Failed to get connected accounts"or similar).
- If your implementation uses a different error prefix for
L03TJ3
requested changes
Mar 20, 2026
Collaborator
L03TJ3
left a comment
L03TJ3
left a comment
There was a problem hiding this comment.
Besides the comments, README is also not updated to reflect these new changes.
Make sure all comments are accounted for and responded too if fixed and answered potential questions
Collaborator
There was a problem hiding this comment.
removed completely valid comments. revert. and why?
| const cached = this.publicClientCache.get(key) | ||
| if (cached) return cached | ||
|
|
||
| const publicClient = createPublicClient({ |
Collaborator
There was a problem hiding this comment.
The core SDK's are intended to be 'headless'. meaning we don't enforce anything related to wallet-connection policies. thats why we work with a passed down publicClient, provided by the app that integrates this flow.
This is now overriding and ignoring app-level transport config (custom headers, observability, retry policy, fallback,
authenticated RPCs)
Behavior should be aligned with all the other flows we have in the same sdk
| claimGasBuffer: 150000n, | ||
| fvDefaultChain: SupportedChains.XDC, | ||
| contracts: { | ||
| // Production identity contract sourced from reference-assets connect-a-wallet example |
Collaborator
There was a problem hiding this comment.
If unsure, the expected way to contribute is to ask questions, you can always put a PR in draft and communicate with maintainers and reviewers.
We have deployed contracts for XDC: https://github.com/GoodDollar/GoodProtocol/blob/259ca3702afd2601ef4e908963c270282b8aa08e/releases/deployment.json#L671
| * @param account - The account address. | ||
| * @returns The identity expiry data. | ||
| */ | ||
| async getConnectedAccounts(account: Address): Promise<ConnectedAccountStatus> { |
Collaborator
There was a problem hiding this comment.
We have three different methods for the same purpose:
- isConnected (just a two line wrapper for getConnectedAccounts). this tiny wrappers don't have any use. perfectly to have only one
-getConnectedAccounts (for a single chain) - checkConnectedStatusAllChains
- getConnectedAccounts (single chain)
This can be brought together in one method. add an optional paramater for chainId so that someone can choose with one method to either do all chains or one chain
Collaborator
There was a problem hiding this comment.
This should update through the checkConnectedStatusAllChains.
- rename to checkConnectedStatus
- add optional param chainId
- filter chainConfigs when chainId is provided.
- I believe the rest of the logic can stay the same
- remove getConnectedAccounts and isConnected
| } | ||
| } | ||
|
|
||
| async connectAccount( |
Collaborator
There was a problem hiding this comment.
What did you implement for the requested flow support: Native/custodial wallet flow is supported without requiring explicit user signature prompts for transaction signing.
|
|
||
|
|
||
|
|
||
| type WalletLinkAction = (sdk: IdentitySDK, account: Address, options?: WalletLinkOptions) => Promise<void> |
Collaborator
There was a problem hiding this comment.
These new additions should be in separate 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
2 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.
Description
This PR implements the Wallet-Link (Connect-a-Wallet) functionality for the GoodDollar Identity protocol. It enables whitelisted root identities to link and unlink secondary wallets across Celo, Fuse, and XDC networks.
Motivation and Context
This change is part of the Identity V4 upgrade to support multi-wallet management within the SDK. It provides developers with a high-level API and React hooks to handle the complex security confirmation and cross-chain status resolution logic required by the protocol.
Changes:
citizen-sdk: ImplementedconnectAccount,disconnectAccount, and status resolution methods (getConnectedAccounts,isAccountConnected,checkConnectedStatusAllChains).react-hooks: Developed theuseWalletLinkhook to manage transaction loading, error states, and the security notice confirmation flow.demo-identity-app: Integrated a newWalletLinkWidgetfor live demonstration.main.tsxin the demo app to use relative imports (./config), resolving build failures in Windows development environments.Fixes #33
How Has This Been Tested?
packages/citizen-sdk/test/wallet-link.test.tsusing Vitest. Tests cover both write-paths (connect/disconnect) and root resolution logic using mocked Viem clients.WalletLinkWidgetin the demo app. Confirmed it triggers the security signature request, correctly displays the multi-chain connection status, and handles contract reverts (e.g., "not whitelisted").yarn turbo run buildandyarn lintacross the monorepo with zero errors.Checklist:
Screen.Recording.2026-03-12.115954.mp4
Summary by Sourcery
Add wallet linking capabilities to the citizen SDK and react hooks, and expose them via a demo wallet-link widget in the identity demo app.
New Features:
Bug Fixes:
Enhancements:
Tests: