add wallet-link support to citizen-sdk and react-hooks#38
Open
edehvictor wants to merge 14 commits intoGoodDollar:mainfrom
Open
add wallet-link support to citizen-sdk and react-hooks#38edehvictor wants to merge 14 commits intoGoodDollar:mainfrom
edehvictor wants to merge 14 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
|
Hi @pheobeayo, thanks again for the review. I’ve pushed the requested fixes in Please kindly take another look when you have a chance. |
pheobeayo
reviewed
May 5, 2026
Comment on lines
+124
to
+135
| disabled={actions.loading || !parsedAddress} | ||
| > | ||
| {actions.loading ? "Connecting..." : "Connect Wallet"} | ||
| </button> | ||
| <button | ||
| onClick={handleDisconnect} | ||
| disabled={actions.loading || !parsedAddress} | ||
| > | ||
| {actions.loading | ||
| ? "Disconnecting..." | ||
| : "Disconnect Wallet"} | ||
| </button> |
There was a problem hiding this comment.
Both buttons share the same actions.loading flag, so if a connect is in flight,
the Disconnect button will also read actions.loading as true and show
"Disconnecting..." and vice versa. The label displayed will be incorrect for
whichever action was not triggered.
Consider tracking the last triggered action, e.g.:
const [activeAction, setActiveAction] = useState<"connect" | "disconnect" | null>(null)Then use activeAction === "connect" && actions.loading to gate each label independently.
Contributor
Author
There was a problem hiding this comment.
Fixed in 99c3077: added activeAction tracking so only the triggered action shows its loading label while the shared loading flag is active.
pheobeayo
reviewed
May 5, 2026
Comment on lines
+40
to
+54
| } catch (err) { | ||
| console.error("Connect failed", err) | ||
| } finally { | ||
| connectedStatus.refetch() | ||
| } | ||
| } | ||
|
|
||
| const handleDisconnect = async () => { | ||
| const address = getValidatedAddress() | ||
| if (!address) return | ||
|
|
||
| try { | ||
| await actions.disconnect(address) | ||
| } catch (err) { | ||
| console.error("Disconnect failed", err) |
There was a problem hiding this comment.
The catch here is redundant — actions.connect / actions.disconnect already
catch internally and set actions.error, which is rendered in the UI below in lines 143-146
This console.error will double-log every failure. The catch block can be
simplified to catch { /* error handled by hook */ } or removed entirely if
run() is confirmed to never re-throw.
Contributor
Author
There was a problem hiding this comment.
Fixed in 99c3077: removed the redundant console.error catch blocks and kept the finally refetch, since the hook already captures and renders action errors.
pheobeayo
suggested changes
May 5, 2026
pheobeayo
left a comment
pheobeayo
left a comment
There was a problem hiding this comment.
You have done an excellent job so far. Kudos to you. Kindly fix the two cleanup comments for a better UX experience for users. Well done and Thank you.
Contributor
Author
|
Hi @pheobeayo, thanks for the extra UX cleanup notes. I’ve pushed the fixes in
The demo identity app lint/build checks are passing locally. Please kindly take another look when you have a chance. |
Contributor
Author
|
Thank you @pheobeayo. The feedback was quite helpful. |
L03TJ3
requested changes
May 8, 2026
Collaborator
L03TJ3
left a comment
L03TJ3
left a comment
There was a problem hiding this comment.
So, the issues describes a demo app. it also describes 'non-production ready UI is accepted, only minimal okay'.
The reason why I still ask you to make some updates to the UI is because you add to an existing demo-app which is using the tamagui design system.
its kind of expected to at least follow the pattern of existing apps that gets added to.
So please adjust the widget so that it is aligned with the rest of the demo widgets in demo-identity app and then I can approve (functionally it seemed to work)
Contributor
Author
|
Thanks for the feedback. Gotcha, working on it already. Would push soon. |
Contributor
Author
|
Hi @L03TJ3, thanks for clarifying the UI expectation. I pushed Verified locally:
|
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
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: