add wallet-link support to citizen-sdk and react-hooks#38
Open
edehvictor wants to merge 10 commits intoGoodDollar:mainfrom
Open
add wallet-link support to citizen-sdk and react-hooks#38edehvictor wants to merge 10 commits intoGoodDollar:mainfrom
edehvictor wants to merge 10 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 4 issues, and left some high level feedback:
- The demo
WalletLinkWidgetcomponent appears out of sync with theuseWalletLink/useConnectedStatusAPI (it referencesconnectedStatus.status,connectedStatus.allChainStatuses, etc., which are not returned by the hook), so it likely won’t compile or behave as intended; align the widget with the actual hook return shape (statuses,loading,error,refetch). useConnectedStatus’suseEffectomitspublicClientsfrom the dependency array, so updating the set of clients will not trigger a refetch; consider addingpublicClients(or a stable memoized representation of it) to the dependencies to keep results consistent with the provided clients.- In
WalletLinkWidgetyou passtargetAddress as AddressintouseWalletLinkbefore validating it withisAddress, which means the hook can run RPCs against an invalid address; consider tracking a separate validated address or only passing a value intowatchAccountafter it has been validated.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The demo `WalletLinkWidget` component appears out of sync with the `useWalletLink`/`useConnectedStatus` API (it references `connectedStatus.status`, `connectedStatus.allChainStatuses`, etc., which are not returned by the hook), so it likely won’t compile or behave as intended; align the widget with the actual hook return shape (`statuses`, `loading`, `error`, `refetch`).
- `useConnectedStatus`’s `useEffect` omits `publicClients` from the dependency array, so updating the set of clients will not trigger a refetch; consider adding `publicClients` (or a stable memoized representation of it) to the dependencies to keep results consistent with the provided clients.
- In `WalletLinkWidget` you pass `targetAddress as Address` into `useWalletLink` before validating it with `isAddress`, which means the hook can run RPCs against an invalid address; consider tracking a separate validated address or only passing a value into `watchAccount` after it has been validated.
## Individual Comments
### Comment 1
<location path="packages/react-hooks/src/citizen-sdk/wagmi-wallet-link-sdk.ts" line_range="26-35" />
<code_context>
+ sdk: IdentitySDK | null,
</code_context>
<issue_to_address>
**issue (bug_risk):** publicClients is used in the effect but missing from the dependency array, which can cause stale reads when the clients change.
If callers swap out `publicClients` (for example after reconnecting or changing transports), this hook will still use the old instance. Please include `publicClients` in the dependency array (or depend on a memoized `publicClients` from the caller) so the effect tracks the current value.
</issue_to_address>
### Comment 2
<location path="apps/demo-identity-app/src/components/WalletLinkWidget.tsx" line_range="97-106" />
<code_context>
+ <p style={{ color: "green" }}>Tx Hash: {connectAccount.txHash || disconnectAccount.txHash}</p>
+ )}
+
+ <div style={{ background: "#f5f5f5", padding: "10px", marginTop: "1rem", fontSize: "14px" }}>
+ <h4>Status for {targetAddress || "..."}</h4>
+ {connectedStatus.loading ? (
+ <p>Loading status...</p>
+ ) : (
+ <>
+ <p><strong>Connected (Current Chain):</strong> {connectedStatus.status?.isConnected ? "✅ Yes" : "❌ No"}</p>
+ <p><strong>Root Identity:</strong> {connectedStatus.status?.root || "None"}</p>
+
+ <details style={{ marginTop: "10px" }}>
+ <summary>Multi-Chain Statuses</summary>
+ <ul>
+ {connectedStatus.allChainStatuses.map(chain => (
+ <li key={chain.chainId}>
+ {chain.chainName}: {chain.isConnected ? `✅ (Root: ${chain.root})` : "❌"}
</code_context>
<issue_to_address>
**issue (bug_risk):** The widget references `status` and `allChainStatuses` properties that do not exist on `UseConnectedStatusReturn`.
The `useConnectedStatus` hook returns `{ statuses, loading, error, refetch }`, but the widget reads `connectedStatus.status` and `connectedStatus.allChainStatuses`, which will be `undefined` and can break the UI at runtime. You likely want to derive a “current chain” status from `statuses` (e.g. `statuses.find(...)`) for the primary display, and use `statuses` directly for the multi-chain list. Updating the widget to match `UseConnectedStatusReturn` will prevent these runtime errors.
</issue_to_address>
### Comment 3
<location path="apps/demo-identity-app/src/components/WalletLinkWidget.tsx" line_range="5-7" />
<code_context>
+
+export const WalletLinkWidget = () => {
+ const [targetAddress, setTargetAddress] = useState<string>("");
+ const { connectAccount, disconnectAccount, connectedStatus } = useWalletLink("development", targetAddress as Address);
+
+ const handleConnect = async () => {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Casting `targetAddress` to `Address` before validation can cause unnecessary RPC errors while the user types.
`targetAddress as Address` is passed directly into `useWalletLink` → `useConnectedStatus` → `sdk.checkConnectedStatus`, so RPC calls are made even when the user has typed an invalid/partial address. Instead, only pass a validated address into the hook, e.g.:
```ts
const parsedAddress = isAddress(targetAddress) ? (targetAddress as Address) : undefined
const { connectAccount, disconnectAccount, connectedStatus } = useWalletLink("development", parsedAddress)
```
This prevents RPC calls until the input is a valid address.
```suggestion
export const WalletLinkWidget = () => {
const [targetAddress, setTargetAddress] = useState<string>("");
const parsedAddress = isAddress(targetAddress) ? (targetAddress as Address) : undefined;
const { connectAccount, disconnectAccount, connectedStatus } = useWalletLink("development", parsedAddress);
```
</issue_to_address>
### Comment 4
<location path="packages/react-hooks/src/citizen-sdk/wagmi-wallet-link-sdk.ts" line_range="32" />
<code_context>
+ 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
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the `useWalletLinkAction` control flow by keeping only the security message in React state, moving the resolver to a ref, and extracting the option-wrapping into a helper function.
The main complexity cost here comes from the generic `useWalletLinkAction` + promise-in-state pattern. You can keep all existing behavior while making the control flow much easier to follow by:
1. **Moving the resolver out of React state**
2. **Storing only the security message in state**
3. **Extracting the option-wrapping into a small helper**
That keeps `useWalletLinkAction`, `useConnectAccount`, and `useDisconnectAccount` APIs intact, but makes the implementation less “clever” and easier to reason about.
### 1. Store only message in state, resolver in a ref
Instead of:
```ts
const [pendingSecurityConfirm, setPendingSecurityConfirm] = useState<{
message: string
resolve: (confirmed: boolean) => void
} | null>(null)
const confirmSecurity = useCallback(
(confirmed: boolean) => {
pendingSecurityConfirm?.resolve(confirmed)
setPendingSecurityConfirm(null)
},
[pendingSecurityConfirm],
)
```
you can separate UI state (message) from control flow (resolver) using a ref:
```ts
const [pendingSecurityMessage, setPendingSecurityMessage] = useState<string | null>(null)
const securityResolveRef = useRef<((confirmed: boolean) => void) | null>(null)
const confirmSecurity = useCallback((confirmed: boolean) => {
securityResolveRef.current?.(confirmed)
securityResolveRef.current = null
setPendingSecurityMessage(null)
}, [])
```
Then in the `run` function, instead of putting `resolve` into state:
```ts
onSecurityMessage:
options?.onSecurityMessage ??
(options?.skipSecurityMessage
? undefined
: (message) =>
new Promise((resolve) => {
setPendingSecurityConfirm({ message, resolve })
})),
```
update to:
```ts
onSecurityMessage:
options?.onSecurityMessage ??
(options?.skipSecurityMessage
? undefined
: (message) =>
new Promise<boolean>((resolve) => {
securityResolveRef.current = resolve
setPendingSecurityMessage(message)
})),
```
And adjust the return type accordingly:
```ts
interface UseWalletLinkActionReturn {
// ...
pendingSecurityConfirm: { message: string } | null
confirmSecurity: (confirmed: boolean) => void
}
```
With this, `useConnectAccount` / `useDisconnectAccount` no longer need to strip `resolve`, and their implementation becomes simpler:
```ts
export const useConnectAccount = (sdk: IdentitySDK | null): UseConnectAccountReturn => {
const base = useWalletLinkAction(sdk, (s, account, options) =>
s.connectAccount(account, options),
)
return {
...base,
connect: base.run,
pendingSecurityConfirm: base.pendingSecurityConfirm,
}
}
```
### 2. Extract a small helper for options wrapping
To reduce density in `run`, you can extract option-wrapping logic into a helper without changing behavior:
```ts
function createWalletLinkOptions(
options: WalletLinkOptions | undefined,
deps: {
setTxHash: (hash: `0x${string}`) => void
setPendingSecurityMessage: (message: string) => void
securityResolveRef: React.MutableRefObject<((confirmed: boolean) => void) | null>
},
): WalletLinkOptions | undefined {
if (!options && !deps) return options
const { setTxHash, setPendingSecurityMessage, securityResolveRef } = deps
return {
...options,
onHash: (hash) => {
setTxHash(hash)
options?.onHash?.(hash)
},
onSecurityMessage:
options?.onSecurityMessage ??
(options?.skipSecurityMessage
? undefined
: (message) =>
new Promise<boolean>((resolve) => {
securityResolveRef.current = resolve
setPendingSecurityMessage(message)
})),
}
}
```
Then `run` becomes easier to scan:
```ts
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,
createWalletLinkOptions(options, {
setTxHash,
setPendingSecurityMessage,
securityResolveRef,
}),
)
} catch (err: any) {
setError(err instanceof Error ? err.message : String(err))
} finally {
setLoading(false)
}
},
[sdk, action],
)
```
This keeps the generic hook and all current behavior, but:
- Removes the non-obvious “promise resolver in state” pattern.
- Makes it clear that `confirmSecurity` resolves an internal promise.
- Reduces nesting and cognitive load in `run`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
edehvictor
changed the title
11 tasks
L03TJ3
requested changes
Apr 3, 2026
Contributor
Author
|
Hey @L03TJ3, I'm doing the necessary fixes from your feedback. Thanks for the feedback. Once I've done, I'd reply to every comment you made with the changes. |
Contributor
Author
|
Gm @L03TJ3, I addressed the maintainer and Sourcery review feedback and pushed the updates. I also synced the branch with the latest |
L03TJ3
requested changes
Apr 7, 2026
Comment on lines
+99
to
+133
| export interface UseConnectAccountReturn | ||
| extends Omit<UseWalletLinkActionReturn, "run" | "pendingSecurityConfirm"> { | ||
| connect: UseWalletLinkActionReturn["run"] | ||
| pendingSecurityConfirm: { message: string } | null | ||
| } | ||
|
|
||
| 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 Omit<UseWalletLinkActionReturn, "run" | "pendingSecurityConfirm"> { | ||
| disconnect: UseWalletLinkActionReturn["run"] | ||
| pendingSecurityConfirm: { message: string } | null | ||
| } | ||
|
|
||
| export const useDisconnectAccount = ( | ||
| sdk: IdentitySDK | null, | ||
| ): UseDisconnectAccountReturn => { | ||
| const base = useWalletLinkAction(sdk, (s, account, options) => | ||
| s.disconnectAccount(account, options), | ||
| ) | ||
| return { | ||
| ...base, | ||
| disconnect: base.run, | ||
| } | ||
| } |
Collaborator
There was a problem hiding this comment.
Connect/disconnect should be able to be handled with a single hook
Contributor
Author
There was a problem hiding this comment.
Done. useWalletLinkActions now handles both connect and disconnect with shared state (loading/error/txHash/security prompt), and useWalletLink returns that single hook for both actions so consumers can manage both flows from one hook.
Collaborator
There was a problem hiding this comment.
That's not what I meant, what I meant instead, and I could have been clearer, apologies...
we have starting from the core-sdk all the way up the pipeline, nearly identical methods > that produce two different hooks > that produce separate handlers in useWalletLinkActions.
there is no reason to have this. The only difference between the two is 'functionName' = "connectAccount" | "disconnectAccount"
Something like:
type WalletLinkAction = "connectAccount" | "disconnectAccount"
async updateConnectionStatus(
action: WalletLinkAction,
account: Address,
options?: WalletLinkOptions,
): Promise<any> {
try {
await this.runSecurityCheck(action, options)
return this.submitAndWait(
{
address: this.contract.contractAddress,
abi: identityV2ABI,
functionName: action,
args: [account],
},
options?.onHash,
)
} catch (error: any) {
const message = error instanceof Error ? error.message : String(error)
console.error("setAccountLink Error:", error)
throw new Error(`Failed to ${action} account: ${message}`)
}
}
That should make handling it in the hooks also de-duplicated and a whole lot simpler.
all someone has to pass down is: 'connectAccount' or 'disconnectAccount' (and the other props)
Contributor
Author
There was a problem hiding this comment.
Updated accordingly. I de-duplicated the flow from the core SDK upward: IdentitySDK now has a shared updateConnectionStatus(action, account, options) path, and the React side now uses a single useWalletLinkActions hook with one run(action, ...) pipeline plus connect / disconnect helpers. I also updated useWalletLink and the demo/README to use the shared actions object instead of separate duplicated handlers.
Contributor
Author
|
Hi @L03TJ3 , I’ve fixed all the review items and pushed the updates. The maintainer threads are resolved and replies posted. The wallet-link changes are updated and tests adjusted accordingly |
Contributor
Author
|
Gm @L03TJ3, can I kindly get a review on this PR. |
L03TJ3
requested changes
Apr 17, 2026
Contributor
Author
|
Thanks for your feedback @L03TJ3. I've ve made the necessary fix. Kindly review. |
Contributor
Author
|
Gm @L03TJ3, please can I get a review. |
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.
Summary
This PR reintroduces wallet-link support on top of the latest
main.It includes:
@goodsdks/citizen-sdk@goodsdks/react-hooksChanges
mainTesting
mainfeat/wallet-link-supportNotes
This branch was previously merged by mistake and then reverted from
main.This PR is the reopened version requested by the maintainer, with the latest upstream changes pulled into the branch.
Summary by Sourcery
Add multi-wallet link functionality to the Identity SDK and React hooks, including security prompts, connection status checks, and demo integration.
New Features:
Enhancements:
Tests: