Skip to content

feat: add XDC network support for savings widget#45

Open
kadiray34 wants to merge 1 commit intoGoodDollar:mainfrom
Ubeswap:new-savings-contracts
Open

feat: add XDC network support for savings widget#45
kadiray34 wants to merge 1 commit intoGoodDollar:mainfrom
Ubeswap:new-savings-contracts

Conversation

@kadiray34
Copy link
Copy Markdown
Contributor

@kadiray34 kadiray34 commented Apr 28, 2026

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:

  • Enable the savings widget to operate on both Celo and XDC networks with a configurable default chain and visible active network label.
  • Expose supported chain metadata and contract configuration through a new constants module and SDK options, including per-chain contract overrides.
  • Extend the Wagmi React hook to surface network state (current chain, supported chains, and wrong-network detection) alongside the SDK instance.

Enhancements:

  • Handle wallet provider chain/account changes in the widget, showing a wrong-network banner and disabling staking actions when connected to unsupported chains.
  • Refactor the SDK to validate client chain IDs, derive contract addresses per chain, and provide clearer errors for unsupported or mismatched networks.

Documentation:

  • Update the savings widget README to document Celo/XDC support, wrong-network behavior, and the new default-chain-id configuration option.

@kadiray34 kadiray34 requested review from a team and L03TJ3 April 28, 2026 14:38
@github-project-automation github-project-automation Bot moved this to Ready-For-Assignment in GoodBounties Apr 28, 2026
Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 3 issues, and left some high level feedback:

  • 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.
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>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment on lines 35 to +44
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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (typo): Typo in section heading: "Integratig" should be "Integrating".

Please update the heading text to “Integrating The Component.”

Suggested change
## 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). */
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  • 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.

// 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.

@L03TJ3 L03TJ3 moved this from Ready-For-Assignment to In Review in GoodBounties Apr 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Review

Development

Successfully merging this pull request may close these issues.

2 participants