add wallet-link support to citizen-sdk and react-hooks#38
add wallet-link support to citizen-sdk and react-hooks#38edehvictor wants to merge 9 commits intoGoodDollar:mainfrom
Conversation
![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 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
| if (!publicClient || !walletClient) { | ||
| setSdk(null) | ||
| setError("Wallet or Public client not initialized") | ||
| setError(null) |
There was a problem hiding this comment.
I think this has happened before.
Why are you removing things in files that you have not made changes too?
and why are you removing error messages? what is the purpose of changing it to null here?
There was a problem hiding this comment.
Addressed. I restored the previous behavior here and put back the original Wallet or Public client not initialized error so this file keeps its prior semantics.
| * This SDK is headless: it never creates its own public clients internally. | ||
| * For the SDK's current chain `this.publicClient` is reused (preserving the | ||
| * app's transport config — custom headers, retry policy, authenticated RPCs, etc.). |
There was a problem hiding this comment.
I might have made a mistake in my last review. (we keep it headless don't enforce or overwrite app-level). but now reading the comment I understand the intention better.
We do support fallbacks to local read-only public clients in other flows as well.
So I think we need to make a slight adjustment.
While it's good to have the passed-down client as optional, it should always be the leading one.
We could do a fallback on a local rpc-client. but for this we don't need new methods.
you can look at the flow (readChainEntitlement).
And replicate the flow using existing methods to use getRpcFallbackClient
because it make sense we want to support easily reading all chains status.
There was a problem hiding this comment.
Updated accordingly. publicClients still take priority when passed in, the active chain still uses this.publicClient, and for the other supported chains checkConnectedStatus now falls back through getRpcFallbackClient, following the same pattern as readChainEntitlement.
| import { useWalletLink } from "@goodsdks/react-hooks"; | ||
| import { Address, isAddress } from "viem"; | ||
|
|
||
| export const WalletLinkWidget = () => { |
There was a problem hiding this comment.
since we we use wagmi here.
Either here as demo, or at least in the wagmi README
add a section that explains how to handle switchChain if someone wants to connect a wallet on a chain they are currrently not connected to
There was a problem hiding this comment.
Added. I documented the switchChain flow in the React hooks README, including the need to wait for Wagmi to finish switching and for useWalletLink to reinitialize on the new chain before calling connect or disconnect.
|
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. |
|
Gm @L03TJ3, I addressed the maintainer and Sourcery review feedback and pushed the updates. I also synced the branch with the latest |
|
|
||
| #### `useConnectAccount(sdk)` | ||
|
|
||
| Manages the connect-account flow including the security confirmation prompt. |
There was a problem hiding this comment.
This should reference back to the earlier section in the readme, so that it is clear this hook consumes the identity-sdk
There was a problem hiding this comment.
Updated. The wallet-link section now explicitly references the Identity & Claim section/useIdentitySDK so it is clear these hooks consume the identity SDK.
| 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, | ||
| } | ||
| } |
There was a problem hiding this comment.
Connect/disconnect should be able to be handled with a single hook
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.
| ) | ||
| }) | ||
|
|
||
| it("should return a headless error entry when checking all chains without providing publicClients", async () => { |
There was a problem hiding this comment.
Fixed. The headless test now stubs getRpcFallbackClient to use the mock public client and asserts the fallback flow without expecting the old 'No public client provided' error.
|
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 |
2 participants
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: