feat: add XDC network support for savings widget#45
feat: add XDC network support for savings widget#45kadiray34 wants to merge 1 commit intoGoodDollar:mainfrom
Conversation
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
GooddollarSavingsWidget.validateInputyou rely onparseEther(this.inputAmount)for runtime checks, but the regex allows arbitrary decimal precision; consider either tightening validation or wrappingparseEtherin a try/catch to surface a user-friendly error instead of letting it throw on too many decimals. - The JSON-stringification of
options.contractsintocontractsKeyinuseGooddollarSavingsis a bit brittle (property order / non-serializable values); if you expect this to evolve, consider normalizing to a stable structure (e.g. sorted entries of[chainId, staking, gdollar]) instead of relying onJSON.stringify.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `GooddollarSavingsWidget.validateInput` you rely on `parseEther(this.inputAmount)` for runtime checks, but the regex allows arbitrary decimal precision; consider either tightening validation or wrapping `parseEther` in a try/catch to surface a user-friendly error instead of letting it throw on too many decimals.
- The JSON-stringification of `options.contracts` into `contractsKey` in `useGooddollarSavings` is a bit brittle (property order / non-serializable values); if you expect this to evolve, consider normalizing to a stable structure (e.g. sorted entries of `[chainId, staking, gdollar]`) instead of relying on `JSON.stringify`.
## Individual Comments
### Comment 1
<location path="packages/savings-sdk/src/wagmi-sdk.ts" line_range="35-44" />
<code_context>
+ const publicClient = usePublicClient({ chainId }) as PublicClient | undefined
+ const { data: walletClient } = useWalletClient({ chainId })
const [sdk, setSDK] = useState<GooddollarSavingsSDK | null>(null)
const [error, setError] = useState<string | null>(null)
const [loading, setLoading] = useState<boolean>(false)
</code_context>
<issue_to_address>
**issue (bug_risk):** Error state is never cleared on successful SDK initialisation, so a previous error can linger even when things are fixed.
In the effect, `setError` only runs on failure or when `!publicClient` / `isWrongNetwork`. If the hook initially fails (e.g. unsupported chain) and later becomes valid, the SDK is created but the old `error` value remains. Please clear the error either at the start of the effect (after the early returns) or explicitly set it to `null` on success so consumers can trust `error === null` to indicate a healthy state.
</issue_to_address>
### Comment 2
<location path="packages/savings-widget/README.md" line_range="7" />
<code_context>
+The widget supports the **Celo** and **XDC** networks. If the connected wallet is on any other chain, the widget shows a "Wrong network" banner and disables the action button until the user switches to one of the supported networks.
+
## Integratig The Component
Can be used in any website, for a quick setup:
</code_context>
<issue_to_address>
**issue (typo):** Typo in section heading: "Integratig" should be "Integrating".
Please update the heading text to “Integrating The Component.”
```suggestion
## Integrating The Component
```
</issue_to_address>
### Comment 3
<location path="packages/savings-widget/src/GooddollarSavingsWidget.ts" line_range="389" />
<code_context>
- private publicClient: PublicClient | null = null;
- private sdk: GooddollarSavingsSDK | null = null;
- private userAddress: string | null = null;
+ /** Active chain id (from wallet when connected, otherwise the default). */
+ @state()
+ activeChainId: SupportedChainId = SupportedChainId.CELO
</code_context>
<issue_to_address>
**issue (complexity):** Consider deriving `activeChainId` from a centralized connection helper and using a single cached public client to avoid redundant state and scattered chain-handling logic.
You can keep all the new multi‑chain / wrong‑network behavior but collapse several moving parts by centralizing connection/chain state and deriving `activeChainId` instead of storing/syncing it.
### 1. Derive `activeChainId` instead of storing it
You don’t need `@state activeChainId` plus manual syncing in `connectedCallback`, `updated`, and `refreshData`. You can derive it from `walletChainId` + `defaultChainId` and a single connection helper.
```ts
// Remove this:
// @state()
// activeChainId: SupportedChainId = SupportedChainId.CELO
// Replace all direct writes to this.activeChainId with updates to walletChainId/defaultChainId.
// Then add:
private get connection() {
const provider = this.web3Provider;
const hasProvider = !!provider;
const hasAccount = !!this.userAddress;
const walletChainId = this.walletChainId;
const isSupportedChain = isSupportedChainId(walletChainId);
const displayChainId: SupportedChainId =
isSupportedChain
? (walletChainId as SupportedChainId)
: (isSupportedChainId(this.defaultChainId)
? this.defaultChainId
: SupportedChainId.CELO);
const isWrongNetwork = hasProvider && hasAccount && !isSupportedChain;
return { hasProvider, hasAccount, isSupportedChain, isWrongNetwork, displayChainId };
}
private get activeChainId(): SupportedChainId {
return this.connection.displayChainId;
}
private activeChainLabel(): string {
return CHAIN_LABEL[this.activeChainId] ?? "Unknown";
}
```
Usage changes stay localized:
```ts
// render()
const { hasProvider, hasAccount, isWrongNetwork } = this.connection;
const isProviderConnected = hasProvider && hasAccount;
const isConnected = isProviderConnected && !isWrongNetwork;
// refreshData()
const { displayChainId } = this.connection;
// use displayChainId instead of writing this.activeChainId
// getPublicClient()
private getPublicClient(): PublicClient {
const chainId = this.activeChainId;
// ...
}
```
This lets you remove explicit `this.activeChainId = ...` assignments from:
- `connectedCallback`
- `updated` (defaultChainId branch)
- `refreshData` (`!isProviderConnected` and wrong‑network branches)
and rely on `walletChainId` / `defaultChainId` as the only sources of truth for chain selection.
### 2. Collapse public client cache to a single instance
Inside this widget, you only ever use a single “display” chain at any time (now exposed via `activeChainId` getter). You can avoid the `Partial<Record<SupportedChainId, PublicClient>>` cache and just keep one client for the current display chain.
```ts
// Replace:
private publicClients: Partial<Record<SupportedChainId, PublicClient>> = {}
// With:
private displayPublicClient: PublicClient | null = null;
private displayPublicClientChainId: SupportedChainId | null = null;
private getPublicClient(): PublicClient {
const chainId = this.activeChainId;
if (!this.displayPublicClient || this.displayPublicClientChainId !== chainId) {
this.displayPublicClient = createPublicClient({
chain: CHAIN_BY_ID[chainId],
transport: http(),
}) as unknown as PublicClient;
this.displayPublicClientChainId = chainId;
}
return this.displayPublicClient;
}
```
Then update SDK initialization:
```ts
// before
this.sdk = new GooddollarSavingsSDK(
this.getPublicClient(this.activeChainId),
this.walletClient,
);
// after
this.sdk = new GooddollarSavingsSDK(
this.getPublicClient(),
this.walletClient,
);
```
This preserves behavior (including handling default vs wallet chain, wrong network, etc.) while removing one state field and a small in‑component cache, and centralizes connection/chain reasoning into a single helper.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const [sdk, setSDK] = useState<GooddollarSavingsSDK | null>(null) | ||
| const [error, setError] = useState<string | null>(null) | ||
| const [loading, setLoading] = useState<boolean>(false) | ||
|
|
||
| if (!walletClient || !publicClient) { | ||
| return; | ||
| } | ||
| const isWrongNetwork = useMemo( | ||
| () => chainId !== undefined && !isSupportedChainId(chainId), | ||
| [chainId], | ||
| ) | ||
|
|
||
| // Re-initialise the SDK when clients or chain change. Memoise the override |
There was a problem hiding this comment.
issue (bug_risk): Error state is never cleared on successful SDK initialisation, so a previous error can linger even when things are fixed.
In the effect, setError only runs on failure or when !publicClient / isWrongNetwork. If the hook initially fails (e.g. unsupported chain) and later becomes valid, the SDK is created but the old error value remains. Please clear the error either at the start of the effect (after the early returns) or explicitly set it to null on success so consumers can trust error === null to indicate a healthy state.
|
|
||
| The widget supports the **Celo** and **XDC** networks. If the connected wallet is on any other chain, the widget shows a "Wrong network" banner and disables the action button until the user switches to one of the supported networks. | ||
|
|
||
| ## Integratig The Component |
There was a problem hiding this comment.
issue (typo): Typo in section heading: "Integratig" should be "Integrating".
Please update the heading text to “Integrating The Component.”
| ## Integratig The Component | |
| ## Integrating The Component |
| private publicClient: PublicClient | null = null; | ||
| private sdk: GooddollarSavingsSDK | null = null; | ||
| private userAddress: string | null = null; | ||
| /** Active chain id (from wallet when connected, otherwise the default). */ |
There was a problem hiding this comment.
issue (complexity): Consider deriving activeChainId from a centralized connection helper and using a single cached public client to avoid redundant state and scattered chain-handling logic.
You can keep all the new multi‑chain / wrong‑network behavior but collapse several moving parts by centralizing connection/chain state and deriving activeChainId instead of storing/syncing it.
1. Derive activeChainId instead of storing it
You don’t need @state activeChainId plus manual syncing in connectedCallback, updated, and refreshData. You can derive it from walletChainId + defaultChainId and a single connection helper.
// Remove this:
// @state()
// activeChainId: SupportedChainId = SupportedChainId.CELO
// Replace all direct writes to this.activeChainId with updates to walletChainId/defaultChainId.
// Then add:
private get connection() {
const provider = this.web3Provider;
const hasProvider = !!provider;
const hasAccount = !!this.userAddress;
const walletChainId = this.walletChainId;
const isSupportedChain = isSupportedChainId(walletChainId);
const displayChainId: SupportedChainId =
isSupportedChain
? (walletChainId as SupportedChainId)
: (isSupportedChainId(this.defaultChainId)
? this.defaultChainId
: SupportedChainId.CELO);
const isWrongNetwork = hasProvider && hasAccount && !isSupportedChain;
return { hasProvider, hasAccount, isSupportedChain, isWrongNetwork, displayChainId };
}
private get activeChainId(): SupportedChainId {
return this.connection.displayChainId;
}
private activeChainLabel(): string {
return CHAIN_LABEL[this.activeChainId] ?? "Unknown";
}Usage changes stay localized:
// render()
const { hasProvider, hasAccount, isWrongNetwork } = this.connection;
const isProviderConnected = hasProvider && hasAccount;
const isConnected = isProviderConnected && !isWrongNetwork;
// refreshData()
const { displayChainId } = this.connection;
// use displayChainId instead of writing this.activeChainId
// getPublicClient()
private getPublicClient(): PublicClient {
const chainId = this.activeChainId;
// ...
}This lets you remove explicit this.activeChainId = ... assignments from:
connectedCallbackupdated(defaultChainId branch)refreshData(!isProviderConnectedand wrong‑network branches)
and rely on walletChainId / defaultChainId as the only sources of truth for chain selection.
2. Collapse public client cache to a single instance
Inside this widget, you only ever use a single “display” chain at any time (now exposed via activeChainId getter). You can avoid the Partial<Record<SupportedChainId, PublicClient>> cache and just keep one client for the current display chain.
// Replace:
private publicClients: Partial<Record<SupportedChainId, PublicClient>> = {}
// With:
private displayPublicClient: PublicClient | null = null;
private displayPublicClientChainId: SupportedChainId | null = null;
private getPublicClient(): PublicClient {
const chainId = this.activeChainId;
if (!this.displayPublicClient || this.displayPublicClientChainId !== chainId) {
this.displayPublicClient = createPublicClient({
chain: CHAIN_BY_ID[chainId],
transport: http(),
}) as unknown as PublicClient;
this.displayPublicClientChainId = chainId;
}
return this.displayPublicClient;
}Then update SDK initialization:
// before
this.sdk = new GooddollarSavingsSDK(
this.getPublicClient(this.activeChainId),
this.walletClient,
);
// after
this.sdk = new GooddollarSavingsSDK(
this.getPublicClient(),
this.walletClient,
);This preserves behavior (including handling default vs wallet chain, wrong network, etc.) while removing one state field and a small in‑component cache, and centralizes connection/chain reasoning into a single helper.
For the savings widget, XDC network support is added
Summary by Sourcery
Add multi-chain (Celo and XDC) support to the Gooddollar savings SDK and widget, including network-aware UX and configuration.
New Features:
Enhancements:
Documentation: