Open
Conversation
added 14 commits
March 9, 2026 19:44
…actApproval option, expanded tests
![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 7 issues, and left some high level feedback:
- In the demo
ReserveSwapflow, the network/env selection and chain detection are hard-coded aroundchain?.id === 50; consider centralizing this by reusing theCELO_CHAIN_ID/XDC_CHAIN_IDandGoodReserveSDK.isChainEnvSupportedhelpers so network handling is consistent with the SDK and easier to extend. - The
ReserveSwapcomponent has grown quite large and mixes state management, formatting utilities, and UI; extracting reusable hooks (e.g. for balances/quotes) and formatting helpers into separate modules would make the swap logic easier to follow and maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the demo `ReserveSwap` flow, the network/env selection and chain detection are hard-coded around `chain?.id === 50`; consider centralizing this by reusing the `CELO_CHAIN_ID`/`XDC_CHAIN_ID` and `GoodReserveSDK.isChainEnvSupported` helpers so network handling is consistent with the SDK and easier to extend.
- The `ReserveSwap` component has grown quite large and mixes state management, formatting utilities, and UI; extracting reusable hooks (e.g. for balances/quotes) and formatting helpers into separate modules would make the swap logic easier to follow and maintain.
## Individual Comments
### Comment 1
<location path="packages/react-hooks/src/good-reserve/wagmi-reserve-sdk.ts" line_range="22-24" />
<code_context>
+ * if (error) return <p>{error}</p>
+ * const quote = await sdk.getBuyQuote(cUSD, parseUnits("10", 18))
+ */
+export function useGoodReserve(env: ReserveEnv = "production", options?: GoodReserveSDKOptions) {
+ const publicClient = usePublicClient() as PublicClient | undefined
+ const { data: walletClient } = useWalletClient()
+
+ // Pull out the primitive value so useMemo tracks it directly instead of
</code_context>
<issue_to_address>
**suggestion:** The hook returns an error when `publicClient` is not ready instead of a loading state, which can cause confusing UX.
When `publicClient` is initially `undefined` (e.g. during Wagmi/AppKit boot), the hook returns `{ sdk: null, loading: false, error: "Public client not initialized" }`. Once `publicClient` becomes available, the hook recomputes, but any UI that rendered this will briefly show an error even though things are just initializing.
Instead, consider treating `!publicClient` as a loading state and returning `{ sdk: null, loading: true, error: null }` in that case to avoid transient error flashes and keep `loading` semantically accurate.
Suggested implementation:
```typescript
return useMemo(() => {
if (!publicClient) {
return { sdk: null, loading: true, error: null }
}
try {
const sdk = new GoodReserveSDK(publicClient, walletClient ?? undefined, env, { exactApproval })
```
1. If other parts of the codebase or documentation rely on the "Public client not initialized" error string, you may want to remove or update those references.
2. Any UI using this hook should be prepared to handle `loading: true` with `sdk: null` as the "initializing" state instead of showing an error message.
</issue_to_address>
### Comment 2
<location path="packages/good-reserve/test/reserve-sdk.test.ts" line_range="208-217" />
<code_context>
+ describe("buy", () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for `minReturn < 0n` validation in `buy` and `sell`
Currently the tests only cover the `amountIn <= 0n` / `gdAmount <= 0n` validations. Please also add cases where `buy` and `sell` are called with a negative `minReturn` (e.g. `-1n`) and assert they throw with `"minReturn cannot be negative"`, so these slippage checks remain enforced during future refactors.
</issue_to_address>
### Comment 3
<location path="packages/good-reserve/test/reserve-sdk.test.ts" line_range="221-230" />
<code_context>
+ it("approves then calls swapIn on Mento broker", async () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Cover the `ensureAllowance` path where no approval is needed
Current `buy`/`sell` tests only cover the case where allowance is insufficient and an approval is sent before `swapIn`. Please also add a test where `allowance >= amount` so no approval is performed. For example, mock `readContract` for `allowance` to return a large value and assert that `simulateContract` is called once with `functionName: "swapIn"` and never with `"approve"`, so we lock in the no-approval path and prevent regressions.
Suggested implementation:
```typescript
const sdk = new GoodReserveSDK(publicClient, wc)
const result = await sdk.buy(CELO_PROD_STABLE, 100n, 90n)
expect(result.hash).toBe(MOCK_TX_HASH)
// Should fire twice: once for the approval, once for swapIn
})
it("does not approve when allowance is already sufficient", async () => {
const mentoReadContract = makeMentoReadContract(CELO_PROD_STABLE, CELO_PROD_GD)
const simulateContract = vi.fn().mockResolvedValue({ request: {} })
const readContract = vi.fn(async (args: any) => {
if (args.functionName === "allowance") {
// Large allowance so no approval should be sent
return 10_000n
}
// Delegate all other calls to the existing mento mock
return (mentoReadContract as any)(args)
})
const wc = makeMockWallet()
const publicClient = makeMockClient({ readContract, simulateContract } as any)
const sdk = new GoodReserveSDK(publicClient, wc)
const result = await sdk.buy(CELO_PROD_STABLE, 100n, 90n)
expect(result.hash).toBe(MOCK_TX_HASH)
expect(simulateContract).toHaveBeenCalledTimes(1)
expect(simulateContract).toHaveBeenCalledWith(
expect.objectContaining({ functionName: "swapIn" }),
)
// Ensure we never try to approve when allowance is already high
for (const [args] of simulateContract.mock.calls as any[]) {
expect(args.functionName).not.toBe("approve")
}
```
1. Make sure the braces and parentheses around the existing `"approves then calls swapIn on Mento broker"` test are correctly matched so that the new `it("does not approve when allowance is already sufficient", ...)` is a sibling test in the same `describe` block, not nested.
2. If `makeMentoReadContract` expects a different call signature or non-`any` args, adjust the `readContract` mock typing to match your existing conventions (e.g. use your `PublicClient["readContract"]` arg type instead of `any`).
3. If your test suite uses a different pattern to inspect `simulateContract` calls (e.g. `simulateContract.mock.calls.map(([args]) => args.functionName)`), you can refactor the loop accordingly while preserving the core assertions: exactly one `swapIn` call and no `approve` calls.
</issue_to_address>
### Comment 4
<location path="packages/good-reserve/test/reserve-sdk.test.ts" line_range="301-310" />
<code_context>
+ describe("getTransactionHistory", () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Assert timestamp enrichment and event ordering in transaction history
Current tests cover event decoding but don’t exercise timestamp enrichment or ordering.
Please add a test that:
- Mocks `getContractEvents` to return multiple events with different `blockNumber`s and `getBlock` to return distinct timestamps per block.
- Asserts that the resulting `ReserveEvent[]` is sorted by `block` and that each event has the correct `timestamp`.
This will validate the chunked `getBlock` behavior and ensure consumers can rely on chronological, timestamped history.
Suggested implementation:
```typescript
it("returns empty array when no events found", async () => {
const rc = makeMentoReadContract(CELO_PROD_STABLE, CELO_PROD_GD)
const sdk = new GoodReserveSDK(makeMockClient({ readContract: rc } as any))
const result = await sdk.getTransactionHistory(
"0x0000000000000000000000000000000000000001",
)
expect(result).toEqual([])
})
it("enriches events with timestamps and sorts chronologically", async () => {
const getContractEvents = vi.fn().mockResolvedValue([
{
blockNumber: 12n,
transactionHash:
"0x0000000000000000000000000000000000000000000000000000000000000003",
logIndex: 0,
args: {} as any,
},
{
blockNumber: 10n,
transactionHash:
"0x0000000000000000000000000000000000000000000000000000000000000001",
logIndex: 0,
args: {} as any,
},
{
blockNumber: 11n,
transactionHash:
"0x0000000000000000000000000000000000000000000000000000000000000002",
logIndex: 0,
args: {} as any,
},
])
const getBlock = vi.fn(
async ({ blockNumber }: { blockNumber: bigint }) =>
({
number: blockNumber,
timestamp:
blockNumber === 10n
? 1000n
: blockNumber === 11n
? 2000n
: 3000n,
}) as any,
)
const client = makeMockClient({
getContractEvents,
getBlock,
} as any)
const sdk = new GoodReserveSDK(client)
const result = await sdk.getTransactionHistory(
"0x0000000000000000000000000000000000000001",
)
// Ensure events are sorted by block number (ascending)
expect(result.map((e) => e.block)).toEqual([10n, 11n, 12n])
// Ensure timestamps are enriched from getBlock and aligned with blockNumber
expect(result.map((e) => e.timestamp)).toEqual([1000n, 2000n, 3000n])
})
it("decodes XDC broker Swap events correctly", async () => {
```
Depending on the existing `GoodReserveSDK.getTransactionHistory` implementation and the `makeMockClient` helper, you may need to:
1. Extend the mocked event objects to include any additional fields the decoder relies on (e.g. `address`, `topics`, `data`, `removed`, `blockHash`, `transactionIndex`), mirroring the shape used in the other `getTransactionHistory` tests (like the "decodes XDC broker Swap events correctly" case).
2. Adjust the `getBlock` mock signature if the real client calls it differently (e.g. `getBlock({ blockNumber })` vs `getBlock(blockNumber)`); align the mock’s parameters to match the existing usage in other tests or in the SDK.
3. If `ReserveEvent` uses a different property name than `block` or `timestamp`, update the assertions accordingly to the actual shape (e.g. `e.blockNumber` or `e.blockTimestamp`).
</issue_to_address>
### Comment 5
<location path="packages/good-reserve/test/reserve-sdk.test.ts" line_range="115-124" />
<code_context>
+
+describe("GoodReserveSDK", () => {
+ // ── constructor ──────────────────────────────────────────────────────────────
+ describe("constructor", () => {
+ it("throws on unsupported chain", () => {
+ const client = makeMockClient({ chain: { id: 1 } } as any)
+ expect(() => new GoodReserveSDK(client)).toThrow("Unsupported chain id")
+ })
+
+ it("accepts a valid Celo public client", () => {
+ expect(() => new GoodReserveSDK(makeMockClient())).not.toThrow()
+ })
+
+ it("accepts a valid XDC public client", () => {
+ const client = makeMockClient({ chain: { id: XDC_CHAIN_ID } } as any)
+ expect(() =>
</code_context>
<issue_to_address>
**suggestion (testing):** Add a constructor test for chains marked as `unavailable` in `RESERVE_CONTRACT_ADDRESSES`
You already cover unsupported chains and supported Celo/XDC cases, and `isChainEnvSupported` is tested. To fully exercise the `RESERVE_CONTRACT_ADDRESSES` behavior, please add a constructor test for a `mode: "unavailable"` chain/env (e.g. `XDC_CHAIN_ID` + `"production"`) that asserts `GoodReserveSDK` throws with the configured `reason` string. This keeps the SDK error surface in sync with the contract map and catches future config changes that might otherwise alter the UX silently.
Suggested implementation:
```typescript
// ── constructor ──────────────────────────────────────────────────────────────
describe("constructor", () => {
it("throws on unsupported chain", () => {
const client = makeMockClient({ chain: { id: 1 } } as any)
expect(() => new GoodReserveSDK(client)).toThrow("Unsupported chain id")
})
it("accepts a valid Celo public client", () => {
expect(() => new GoodReserveSDK(makeMockClient())).not.toThrow()
})
it("accepts a valid XDC public client", () => {
const client = makeMockClient({ chain: { id: XDC_CHAIN_ID } } as any)
expect(() =>
new GoodReserveSDK(client, undefined, "development"),
).not.toThrow()
})
it("throws with configured reason on unavailable chain/env", () => {
const client = makeMockClient({ chain: { id: XDC_CHAIN_ID } } as any)
const reason =
RESERVE_CONTRACT_ADDRESSES[XDC_CHAIN_ID].production.reason
expect(() =>
new GoodReserveSDK(client, undefined, "production"),
).toThrow(reason)
})
})
```
1. Ensure `RESERVE_CONTRACT_ADDRESSES` is imported into `packages/good-reserve/test/reserve-sdk.test.ts` from the same module where `XDC_CHAIN_ID` (and `GoodReserveSDK`) are imported. For example, extend the existing import:
- `import { GoodReserveSDK, XDC_CHAIN_ID } from "../src/..."` to
- `import { GoodReserveSDK, XDC_CHAIN_ID, RESERVE_CONTRACT_ADDRESSES } from "../src/..."`.
2. Confirm that `RESERVE_CONTRACT_ADDRESSES[XDC_CHAIN_ID].production` is indeed the entry with `mode: "unavailable"` and a `reason` string. If the unavailable env is under a different key (e.g. `"mainnet"`), update `"production"` in both the property access and the constructor call to match the actual env key.
</issue_to_address>
### Comment 6
<location path="packages/good-reserve/test/reserve-sdk.test.ts" line_range="93-102" />
<code_context>
+const MOCK_STABLE =
+ "0x00000000000000000000000000000000000000a4" as `0x${string}`
+
+/** Wires up a test SDK forced into exchange-helper mode. */
+const makeExchangeHelperSdk = (
+ readContract: ReturnType<typeof vi.fn>,
+ overridePublicClient?: PublicClient,
+) => {
+ const client = overridePublicClient ?? makeMockClient({ readContract } as any)
+ const sdk = new GoodReserveSDK(client)
+ const anySDK = sdk as any
+ anySDK.contracts = {
+ mode: "exchange-helper",
+ exchangeHelper: MOCK_EXCHANGE_HELPER,
+ buyGDFactory: MOCK_BUY_GD_FACTORY,
+ goodDollar: MOCK_GD,
+ stableToken: MOCK_STABLE,
+ }
+ return sdk
+}
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add write-path tests for exchange-helper `buy` and `sell` to complement quote/history coverage
`makeExchangeHelperSdk` nicely covers quote/history behavior in `exchange-helper` mode, but the `buy`/`sell` paths aren’t exercised.
Please add tests that:
- Invoke `sdk.buy` and `sdk.sell` with `mode: "exchange-helper"`.
- Assert `simulateContract` is called with `address: exchangeHelper`, the correct `functionName` ("buy" / "sell"), and properly ordered `args`.
- Optionally verify the approval targets `exchangeHelper` as spender.
This will keep behavior consistent with broker mode and help catch regressions when the exchange-helper route is enabled on-chain.
Suggested implementation:
```typescript
const CELO_PROD_STABLE = RESERVE_CONTRACT_ADDRESSES.production.celo.stableToken
const CELO_PROD_GD = RESERVE_CONTRACT_ADDRESSES.production.celo.goodDollar
describe("GoodReserveSDK (exchange-helper mode)", () => {
it("calls exchangeHelper.buy via simulateContract with correctly ordered args", async () => {
const readContract = vi.fn()
const simulateContract = vi.fn().mockResolvedValue({
// viem's simulateContract returns an object with a `request` that is passed
// to writeContract; we only need it to be truthy for the SDK to proceed.
request: {} as any,
})
const client = makeMockClient({
readContract,
simulateContract,
} as any)
const sdk = makeExchangeHelperSdk(readContract, client)
const amountIn = 100n
const minReturn = 90n
const account = "0x00000000000000000000000000000000000000b1" as const
await sdk.buy({
mode: "exchange-helper",
fromToken: MOCK_STABLE,
toToken: MOCK_GD,
amount: amountIn,
minReturn,
account,
} as any)
expect(simulateContract).toHaveBeenCalled()
const simulateCall = simulateContract.mock.calls[0]?.[0]
expect(simulateCall).toMatchObject({
address: MOCK_EXCHANGE_HELPER,
functionName: "buy",
})
// Ensure argument ordering matches the exchange-helper buy route expectations.
// Adjust these expectations if the underlying ExchangeHelper ABI differs.
expect(simulateCall.args).toEqual([
MOCK_STABLE,
MOCK_GD,
amountIn,
minReturn,
account,
])
})
it("calls exchangeHelper.sell via simulateContract with correctly ordered args", async () => {
const readContract = vi.fn()
const simulateContract = vi.fn().mockResolvedValue({
request: {} as any,
})
const client = makeMockClient({
readContract,
simulateContract,
} as any)
const sdk = makeExchangeHelperSdk(readContract, client)
const amountIn = 100n
const minReturn = 90n
const account = "0x00000000000000000000000000000000000000b2" as const
await sdk.sell({
mode: "exchange-helper",
fromToken: MOCK_GD,
toToken: MOCK_STABLE,
amount: amountIn,
minReturn,
account,
} as any)
expect(simulateContract).toHaveBeenCalled()
const simulateCall = simulateContract.mock.calls[0]?.[0]
expect(simulateCall).toMatchObject({
address: MOCK_EXCHANGE_HELPER,
functionName: "sell",
})
// Ensure argument ordering matches the exchange-helper sell route expectations.
// Adjust these expectations if the underlying ExchangeHelper ABI differs.
expect(simulateCall.args).toEqual([
MOCK_GD,
MOCK_STABLE,
amountIn,
minReturn,
account,
])
})
it("approves exchangeHelper as spender in exchange-helper mode", async () => {
const readContract = vi.fn()
const simulateContract = vi.fn().mockResolvedValue({
request: {} as any,
})
const client = makeMockClient({
readContract,
simulateContract,
} as any)
const sdk = makeExchangeHelperSdk(readContract, client)
const amountIn = 100n
const minReturn = 90n
const account = "0x00000000000000000000000000000000000000b3" as const
await sdk.buy({
mode: "exchange-helper",
fromToken: MOCK_STABLE,
toToken: MOCK_GD,
amount: amountIn,
minReturn,
account,
} as any)
// Look for an approval targeting the exchangeHelper as spender.
const approvalCall = readContract.mock.calls
.map((c) => c[0])
.find(
(call) =>
call?.functionName === "allowance" &&
// typical ERC20 allowance(owner, spender) shape
call?.args?.[1] === MOCK_EXCHANGE_HELPER,
)
expect(approvalCall).toBeDefined()
})
})
```
These tests assume the following, which you should verify and adjust as needed:
1. `makeMockClient` accepts a partial client object with `readContract` and `simulateContract` that matches how other tests inject mocks.
2. `GoodReserveSDK.buy` / `sell` signatures accept an object with `{ mode, fromToken, toToken, amount, minReturn, account }`. If your API differs (e.g. different property names or positional args), update the `sdk.buy` / `sdk.sell` call sites in the tests accordingly.
3. The ExchangeHelper ABI for `buy` / `sell` matches the `args` expectations used here. If the actual ABI takes different parameters or order, update the `expect(simulateCall.args).toEqual([...])` assertions to match the real shape (you can mirror whatever arguments the production code passes into `simulateContract` for these methods).
4. If approvals are performed via a helper that uses `simulateContract` or `writeContract` instead of `readContract.allowance`, update the final test to search the appropriate mock (`simulateContract` / `writeContract`) for a call whose `spender` / `args[1]` is `MOCK_EXCHANGE_HELPER`.
5. If there is an existing `describe` block for `GoodReserveSDK` tests you want these to live in, you may want to move the `describe("GoodReserveSDK (exchange-helper mode)", ...)` block into that section for consistency.
</issue_to_address>
### Comment 7
<location path="apps/demo-reserve-swap/src/components/ReserveSwap.tsx" line_range="107" />
<code_context>
+ return normalized.length > 0 ? normalized : "0"
+}
+
+export function ReserveSwap() {
+ const { address, chain } = useAccount()
+ const publicClient = usePublicClient()
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the quote/derived-value logic and transaction lifecycle handling from `ReserveSwap` into dedicated hooks to separate concerns and simplify the component.
You’ve already pulled out some helpers (`formatUsd`, `trimDecimalString`, etc.), but `ReserveSwap` still mixes a lot of responsibility in one component. Two concrete refactors will reduce complexity without changing behavior:
---
### 1. Extract quote + derived values into a hook
The component currently handles:
- fetching the quote (`fetchQuote`)
- computing `impliedPrice`, `lowLiquidityWarning`
- computing `inputStableEquivalent`, `outputStableEquivalent`
All of this is wired directly to React state in `ReserveSwap`.
You can move this into a dedicated hook that owns quote state and derived values. The component then just consumes the hook’s result and renders.
**New hook (in e.g. `useReserveSwapQuote.ts`):**
```ts
import { useState, useEffect, useCallback } from "react"
import { formatUnits, parseUnits } from "viem"
import type { GoodReserveSdk } from "@goodsdks/react-hooks" // adjust import
import { mapFriendlyError, formatTokenAmount } from "./reserveFormatters" // existing helpers
type Direction = "buy" | "sell"
export function useReserveSwapQuote(params: {
sdk: GoodReserveSdk | null
direction: Direction
amountIn: string
stableToken: `0x${string}`
stableDecimals: number
gdDecimals: number
decimalsLoading: boolean
}) {
const {
sdk,
direction,
amountIn,
stableToken,
stableDecimals,
gdDecimals,
decimalsLoading,
} = params
const [quote, setQuote] = useState<bigint | null>(null)
const [loading, setLoading] = useState(false)
const [error, setError] = useState<string | null>(null)
const fetchQuote = useCallback(async () => {
if (!sdk || decimalsLoading || !amountIn || isNaN(Number(amountIn))) return
setLoading(true)
setError(null)
try {
if (direction === "buy") {
const parsed = parseUnits(amountIn, stableDecimals)
const result = await sdk.getBuyQuote(stableToken, parsed)
setQuote(result)
} else {
const parsed = parseUnits(amountIn, gdDecimals)
const result = await sdk.getSellQuote(parsed, stableToken)
setQuote(result)
}
} catch (err) {
setQuote(null)
setError(mapFriendlyError(err, "Failed to fetch quote"))
} finally {
setLoading(false)
}
}, [sdk, decimalsLoading, amountIn, direction, stableToken, stableDecimals, gdDecimals])
useEffect(() => {
const timer = setTimeout(fetchQuote, 400)
return () => clearTimeout(timer)
}, [fetchQuote])
// derived values moved here
let impliedPrice: string | null = null
let impliedPriceNumber: number | null = null
if (quote !== null && amountIn && !isNaN(Number(amountIn))) {
try {
const inputParsed = parseUnits(
amountIn,
direction === "buy" ? stableDecimals : gdDecimals,
)
if (inputParsed > 0n && quote > 0n) {
const stableAmount = direction === "buy" ? inputParsed : quote
const gdAmount = direction === "buy" ? quote : inputParsed
const scaled =
(stableAmount * 10n ** BigInt(gdDecimals) * 10n ** 9n) /
(gdAmount * 10n ** BigInt(stableDecimals))
impliedPrice = formatUnits(scaled, 9)
const parsed = Number(impliedPrice)
impliedPriceNumber = Number.isFinite(parsed) ? parsed : null
}
} catch {
impliedPrice = null
impliedPriceNumber = null
}
}
let lowLiquidityWarning: string | null = null
if (direction === "buy" && quote !== null && amountIn && !isNaN(Number(amountIn))) {
try {
const parsedInput = parseUnits(amountIn, stableDecimals)
const smallInputThreshold = parseUnits("10", stableDecimals)
const highOutputThreshold = parseUnits("1000000", gdDecimals)
if (parsedInput <= smallInputThreshold && quote >= highOutputThreshold) {
lowLiquidityWarning =
"High mint volume due to current reserve building phase. Price rises with more collateral added."
}
} catch {
lowLiquidityWarning = null
}
}
return {
quote,
quoteLoading: loading,
quoteError: error,
impliedPrice,
impliedPriceNumber,
lowLiquidityWarning,
refetchQuote: fetchQuote,
}
}
```
**Usage in `ReserveSwap` (simplified):**
```tsx
const {
quote,
quoteLoading,
quoteError,
impliedPrice,
impliedPriceNumber,
lowLiquidityWarning,
refetchQuote,
} = useReserveSwapQuote({
sdk,
direction,
amountIn,
stableToken,
stableDecimals,
gdDecimals,
decimalsLoading,
})
// handleRefresh now uses refetchQuote instead of local fetchQuote:
const handleRefresh = useCallback(async () => {
await loadReserveContext()
await refetchQuote()
await loadBalances()
}, [loadReserveContext, refetchQuote, loadBalances])
```
This removes the quote/derived-value block from the main component and keeps all behavior the same.
---
### 2. Extract transaction handling into `useReserveSwapTx`
`handleExecute` currently coordinates:
- slippage calculation
- parsing amounts
- calling `sdk.buy` / `sdk.sell`
- updating status/result/error
You can move that into a small hook and expose `{ execute, status, result, error }`.
**New hook (e.g. `useReserveSwapTx.ts`):**
```ts
import { useState, useCallback } from "react"
import { parseUnits } from "viem"
import { mapFriendlyError } from "./reserveFormatters"
type Direction = "buy" | "sell"
export function useReserveSwapTx(
sdk: any | null, // type your SDK
{
direction,
stableToken,
stableDecimals,
gdDecimals,
}: {
direction: Direction
stableToken: `0x${string}`
stableDecimals: number
gdDecimals: number
},
) {
const [status, setStatus] = useState<"idle" | "pending" | "done" | "error">("idle")
const [result, setResult] = useState<string | null>(null)
const [error, setError] = useState<string | null>(null)
const execute = useCallback(
async (amountIn: string, quote: bigint | null) => {
if (!sdk || !amountIn || quote === null) return
setStatus("pending")
setResult(null)
setError(null)
try {
const minReturn = (quote * 95n) / 100n
if (direction === "buy") {
const parsed = parseUnits(amountIn, stableDecimals)
const res = await sdk.buy(stableToken, parsed, minReturn, (hash: `0x${string}`) => {
console.log("buy tx sent:", hash)
})
setResult(`Buy succeeded. Tx: ${res.receipt.transactionHash}`)
} else {
const parsed = parseUnits(amountIn, gdDecimals)
const res = await sdk.sell(stableToken, parsed, minReturn, (hash: `0x${string}`) => {
console.log("sell tx sent:", hash)
})
setResult(`Sell succeeded. Tx: ${res.receipt.transactionHash}`)
}
setStatus("done")
} catch (err) {
setStatus("error")
setError(mapFriendlyError(err, "Transaction failed"))
}
},
[sdk, direction, stableToken, stableDecimals, gdDecimals],
)
return { execute, status, result, error }
}
```
**Usage in `ReserveSwap`:**
```tsx
const {
execute,
status: txStatus,
result: txResult,
error: txError,
} = useReserveSwapTx(sdk, {
direction,
stableToken,
stableDecimals,
gdDecimals,
})
const handleExecute = useCallback(async () => {
await execute(amountIn, quote)
if (txStatus === "done") {
setAmountIn("")
setQuote(null)
}
}, [execute, amountIn, quote, txStatus])
```
This removes transaction lifecycle logic from the JSX file and isolates it into a reusable hook.
---
Applying these two extractions reduces the size and responsibility of `ReserveSwap` substantially while keeping existing behavior. You can then optionally follow the same pattern for stats/balances and view-model formatting as needed.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+208
to
+217
| describe("buy", () => { | ||
| it("throws when no wallet client is provided", async () => { | ||
| await expect( | ||
| new GoodReserveSDK(makeMockClient()).buy(CELO_PROD_STABLE, 100n, 90n), | ||
| ).rejects.toThrow("wallet client is required") | ||
| }) | ||
|
|
||
| it("throws for zero amountIn", async () => { | ||
| await expect( | ||
| new GoodReserveSDK(makeMockClient(), {} as any).buy(CELO_PROD_STABLE, 0n, 0n), |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add tests for minReturn < 0n validation in buy and sell
Currently the tests only cover the amountIn <= 0n / gdAmount <= 0n validations. Please also add cases where buy and sell are called with a negative minReturn (e.g. -1n) and assert they throw with "minReturn cannot be negative", so these slippage checks remain enforced during future refactors.
Comment on lines
+221
to
+230
| it("approves then calls swapIn on Mento broker", async () => { | ||
| const rc = makeMentoReadContract(CELO_PROD_STABLE, CELO_PROD_GD) | ||
| const simulateContract = vi.fn().mockResolvedValue({ request: {} }) | ||
| const wc = makeMockWallet() | ||
| const publicClient = makeMockClient({ readContract: rc, simulateContract } as any) | ||
|
|
||
| const sdk = new GoodReserveSDK(publicClient, wc) | ||
| const result = await sdk.buy(CELO_PROD_STABLE, 100n, 90n) | ||
|
|
||
| expect(result.hash).toBe(MOCK_TX_HASH) |
Contributor
There was a problem hiding this comment.
suggestion (testing): Cover the ensureAllowance path where no approval is needed
Current buy/sell tests only cover the case where allowance is insufficient and an approval is sent before swapIn. Please also add a test where allowance >= amount so no approval is performed. For example, mock readContract for allowance to return a large value and assert that simulateContract is called once with functionName: "swapIn" and never with "approve", so we lock in the no-approval path and prevent regressions.
Suggested implementation:
const sdk = new GoodReserveSDK(publicClient, wc)
const result = await sdk.buy(CELO_PROD_STABLE, 100n, 90n)
expect(result.hash).toBe(MOCK_TX_HASH)
// Should fire twice: once for the approval, once for swapIn
})
it("does not approve when allowance is already sufficient", async () => {
const mentoReadContract = makeMentoReadContract(CELO_PROD_STABLE, CELO_PROD_GD)
const simulateContract = vi.fn().mockResolvedValue({ request: {} })
const readContract = vi.fn(async (args: any) => {
if (args.functionName === "allowance") {
// Large allowance so no approval should be sent
return 10_000n
}
// Delegate all other calls to the existing mento mock
return (mentoReadContract as any)(args)
})
const wc = makeMockWallet()
const publicClient = makeMockClient({ readContract, simulateContract } as any)
const sdk = new GoodReserveSDK(publicClient, wc)
const result = await sdk.buy(CELO_PROD_STABLE, 100n, 90n)
expect(result.hash).toBe(MOCK_TX_HASH)
expect(simulateContract).toHaveBeenCalledTimes(1)
expect(simulateContract).toHaveBeenCalledWith(
expect.objectContaining({ functionName: "swapIn" }),
)
// Ensure we never try to approve when allowance is already high
for (const [args] of simulateContract.mock.calls as any[]) {
expect(args.functionName).not.toBe("approve")
}- Make sure the braces and parentheses around the existing
"approves then calls swapIn on Mento broker"test are correctly matched so that the newit("does not approve when allowance is already sufficient", ...)is a sibling test in the samedescribeblock, not nested. - If
makeMentoReadContractexpects a different call signature or non-anyargs, adjust thereadContractmock typing to match your existing conventions (e.g. use yourPublicClient["readContract"]arg type instead ofany). - If your test suite uses a different pattern to inspect
simulateContractcalls (e.g.simulateContract.mock.calls.map(([args]) => args.functionName)), you can refactor the loop accordingly while preserving the core assertions: exactly oneswapIncall and noapprovecalls.
Comment on lines
+301
to
+310
| describe("getTransactionHistory", () => { | ||
| it("returns empty array when no events found", async () => { | ||
| const rc = makeMentoReadContract(CELO_PROD_STABLE, CELO_PROD_GD) | ||
| const sdk = new GoodReserveSDK(makeMockClient({ readContract: rc } as any)) | ||
| const result = await sdk.getTransactionHistory( | ||
| "0x0000000000000000000000000000000000000001", | ||
| ) | ||
| expect(result).toEqual([]) | ||
| }) | ||
|
|
Contributor
There was a problem hiding this comment.
suggestion (testing): Assert timestamp enrichment and event ordering in transaction history
Current tests cover event decoding but don’t exercise timestamp enrichment or ordering.
Please add a test that:
- Mocks
getContractEventsto return multiple events with differentblockNumbers andgetBlockto return distinct timestamps per block. - Asserts that the resulting
ReserveEvent[]is sorted byblockand that each event has the correcttimestamp.
This will validate the chunked getBlock behavior and ensure consumers can rely on chronological, timestamped history.
Suggested implementation:
it("returns empty array when no events found", async () => {
const rc = makeMentoReadContract(CELO_PROD_STABLE, CELO_PROD_GD)
const sdk = new GoodReserveSDK(makeMockClient({ readContract: rc } as any))
const result = await sdk.getTransactionHistory(
"0x0000000000000000000000000000000000000001",
)
expect(result).toEqual([])
})
it("enriches events with timestamps and sorts chronologically", async () => {
const getContractEvents = vi.fn().mockResolvedValue([
{
blockNumber: 12n,
transactionHash:
"0x0000000000000000000000000000000000000000000000000000000000000003",
logIndex: 0,
args: {} as any,
},
{
blockNumber: 10n,
transactionHash:
"0x0000000000000000000000000000000000000000000000000000000000000001",
logIndex: 0,
args: {} as any,
},
{
blockNumber: 11n,
transactionHash:
"0x0000000000000000000000000000000000000000000000000000000000000002",
logIndex: 0,
args: {} as any,
},
])
const getBlock = vi.fn(
async ({ blockNumber }: { blockNumber: bigint }) =>
({
number: blockNumber,
timestamp:
blockNumber === 10n
? 1000n
: blockNumber === 11n
? 2000n
: 3000n,
}) as any,
)
const client = makeMockClient({
getContractEvents,
getBlock,
} as any)
const sdk = new GoodReserveSDK(client)
const result = await sdk.getTransactionHistory(
"0x0000000000000000000000000000000000000001",
)
// Ensure events are sorted by block number (ascending)
expect(result.map((e) => e.block)).toEqual([10n, 11n, 12n])
// Ensure timestamps are enriched from getBlock and aligned with blockNumber
expect(result.map((e) => e.timestamp)).toEqual([1000n, 2000n, 3000n])
})
it("decodes XDC broker Swap events correctly", async () => {Depending on the existing GoodReserveSDK.getTransactionHistory implementation and the makeMockClient helper, you may need to:
- Extend the mocked event objects to include any additional fields the decoder relies on (e.g.
address,topics,data,removed,blockHash,transactionIndex), mirroring the shape used in the othergetTransactionHistorytests (like the "decodes XDC broker Swap events correctly" case). - Adjust the
getBlockmock signature if the real client calls it differently (e.g.getBlock({ blockNumber })vsgetBlock(blockNumber)); align the mock’s parameters to match the existing usage in other tests or in the SDK. - If
ReserveEventuses a different property name thanblockortimestamp, update the assertions accordingly to the actual shape (e.g.e.blockNumberore.blockTimestamp).
Comment on lines
+115
to
+124
| describe("constructor", () => { | ||
| it("throws on unsupported chain", () => { | ||
| const client = makeMockClient({ chain: { id: 1 } } as any) | ||
| expect(() => new GoodReserveSDK(client)).toThrow("Unsupported chain id") | ||
| }) | ||
|
|
||
| it("accepts a valid Celo public client", () => { | ||
| expect(() => new GoodReserveSDK(makeMockClient())).not.toThrow() | ||
| }) | ||
|
|
Contributor
There was a problem hiding this comment.
suggestion (testing): Add a constructor test for chains marked as unavailable in RESERVE_CONTRACT_ADDRESSES
You already cover unsupported chains and supported Celo/XDC cases, and isChainEnvSupported is tested. To fully exercise the RESERVE_CONTRACT_ADDRESSES behavior, please add a constructor test for a mode: "unavailable" chain/env (e.g. XDC_CHAIN_ID + "production") that asserts GoodReserveSDK throws with the configured reason string. This keeps the SDK error surface in sync with the contract map and catches future config changes that might otherwise alter the UX silently.
Suggested implementation:
// ── constructor ──────────────────────────────────────────────────────────────
describe("constructor", () => {
it("throws on unsupported chain", () => {
const client = makeMockClient({ chain: { id: 1 } } as any)
expect(() => new GoodReserveSDK(client)).toThrow("Unsupported chain id")
})
it("accepts a valid Celo public client", () => {
expect(() => new GoodReserveSDK(makeMockClient())).not.toThrow()
})
it("accepts a valid XDC public client", () => {
const client = makeMockClient({ chain: { id: XDC_CHAIN_ID } } as any)
expect(() =>
new GoodReserveSDK(client, undefined, "development"),
).not.toThrow()
})
it("throws with configured reason on unavailable chain/env", () => {
const client = makeMockClient({ chain: { id: XDC_CHAIN_ID } } as any)
const reason =
RESERVE_CONTRACT_ADDRESSES[XDC_CHAIN_ID].production.reason
expect(() =>
new GoodReserveSDK(client, undefined, "production"),
).toThrow(reason)
})
})- Ensure
RESERVE_CONTRACT_ADDRESSESis imported intopackages/good-reserve/test/reserve-sdk.test.tsfrom the same module whereXDC_CHAIN_ID(andGoodReserveSDK) are imported. For example, extend the existing import:import { GoodReserveSDK, XDC_CHAIN_ID } from "../src/..."toimport { GoodReserveSDK, XDC_CHAIN_ID, RESERVE_CONTRACT_ADDRESSES } from "../src/...".
- Confirm that
RESERVE_CONTRACT_ADDRESSES[XDC_CHAIN_ID].productionis indeed the entry withmode: "unavailable"and areasonstring. If the unavailable env is under a different key (e.g."mainnet"), update"production"in both the property access and the constructor call to match the actual env key.
Comment on lines
+93
to
+102
| /** Wires up a test SDK forced into exchange-helper mode. */ | ||
| const makeExchangeHelperSdk = ( | ||
| readContract: ReturnType<typeof vi.fn>, | ||
| overridePublicClient?: PublicClient, | ||
| ) => { | ||
| const client = overridePublicClient ?? makeMockClient({ readContract } as any) | ||
| const sdk = new GoodReserveSDK(client) | ||
| const anySDK = sdk as any | ||
| anySDK.contracts = { | ||
| mode: "exchange-helper", |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add write-path tests for exchange-helper buy and sell to complement quote/history coverage
makeExchangeHelperSdk nicely covers quote/history behavior in exchange-helper mode, but the buy/sell paths aren’t exercised.
Please add tests that:
- Invoke
sdk.buyandsdk.sellwithmode: "exchange-helper". - Assert
simulateContractis called withaddress: exchangeHelper, the correctfunctionName("buy" / "sell"), and properly orderedargs. - Optionally verify the approval targets
exchangeHelperas spender.
This will keep behavior consistent with broker mode and help catch regressions when the exchange-helper route is enabled on-chain.
Suggested implementation:
const CELO_PROD_STABLE = RESERVE_CONTRACT_ADDRESSES.production.celo.stableToken
const CELO_PROD_GD = RESERVE_CONTRACT_ADDRESSES.production.celo.goodDollar
describe("GoodReserveSDK (exchange-helper mode)", () => {
it("calls exchangeHelper.buy via simulateContract with correctly ordered args", async () => {
const readContract = vi.fn()
const simulateContract = vi.fn().mockResolvedValue({
// viem's simulateContract returns an object with a `request` that is passed
// to writeContract; we only need it to be truthy for the SDK to proceed.
request: {} as any,
})
const client = makeMockClient({
readContract,
simulateContract,
} as any)
const sdk = makeExchangeHelperSdk(readContract, client)
const amountIn = 100n
const minReturn = 90n
const account = "0x00000000000000000000000000000000000000b1" as const
await sdk.buy({
mode: "exchange-helper",
fromToken: MOCK_STABLE,
toToken: MOCK_GD,
amount: amountIn,
minReturn,
account,
} as any)
expect(simulateContract).toHaveBeenCalled()
const simulateCall = simulateContract.mock.calls[0]?.[0]
expect(simulateCall).toMatchObject({
address: MOCK_EXCHANGE_HELPER,
functionName: "buy",
})
// Ensure argument ordering matches the exchange-helper buy route expectations.
// Adjust these expectations if the underlying ExchangeHelper ABI differs.
expect(simulateCall.args).toEqual([
MOCK_STABLE,
MOCK_GD,
amountIn,
minReturn,
account,
])
})
it("calls exchangeHelper.sell via simulateContract with correctly ordered args", async () => {
const readContract = vi.fn()
const simulateContract = vi.fn().mockResolvedValue({
request: {} as any,
})
const client = makeMockClient({
readContract,
simulateContract,
} as any)
const sdk = makeExchangeHelperSdk(readContract, client)
const amountIn = 100n
const minReturn = 90n
const account = "0x00000000000000000000000000000000000000b2" as const
await sdk.sell({
mode: "exchange-helper",
fromToken: MOCK_GD,
toToken: MOCK_STABLE,
amount: amountIn,
minReturn,
account,
} as any)
expect(simulateContract).toHaveBeenCalled()
const simulateCall = simulateContract.mock.calls[0]?.[0]
expect(simulateCall).toMatchObject({
address: MOCK_EXCHANGE_HELPER,
functionName: "sell",
})
// Ensure argument ordering matches the exchange-helper sell route expectations.
// Adjust these expectations if the underlying ExchangeHelper ABI differs.
expect(simulateCall.args).toEqual([
MOCK_GD,
MOCK_STABLE,
amountIn,
minReturn,
account,
])
})
it("approves exchangeHelper as spender in exchange-helper mode", async () => {
const readContract = vi.fn()
const simulateContract = vi.fn().mockResolvedValue({
request: {} as any,
})
const client = makeMockClient({
readContract,
simulateContract,
} as any)
const sdk = makeExchangeHelperSdk(readContract, client)
const amountIn = 100n
const minReturn = 90n
const account = "0x00000000000000000000000000000000000000b3" as const
await sdk.buy({
mode: "exchange-helper",
fromToken: MOCK_STABLE,
toToken: MOCK_GD,
amount: amountIn,
minReturn,
account,
} as any)
// Look for an approval targeting the exchangeHelper as spender.
const approvalCall = readContract.mock.calls
.map((c) => c[0])
.find(
(call) =>
call?.functionName === "allowance" &&
// typical ERC20 allowance(owner, spender) shape
call?.args?.[1] === MOCK_EXCHANGE_HELPER,
)
expect(approvalCall).toBeDefined()
})
})These tests assume the following, which you should verify and adjust as needed:
makeMockClientaccepts a partial client object withreadContractandsimulateContractthat matches how other tests inject mocks.GoodReserveSDK.buy/sellsignatures accept an object with{ mode, fromToken, toToken, amount, minReturn, account }. If your API differs (e.g. different property names or positional args), update thesdk.buy/sdk.sellcall sites in the tests accordingly.- The ExchangeHelper ABI for
buy/sellmatches theargsexpectations used here. If the actual ABI takes different parameters or order, update theexpect(simulateCall.args).toEqual([...])assertions to match the real shape (you can mirror whatever arguments the production code passes intosimulateContractfor these methods). - If approvals are performed via a helper that uses
simulateContractorwriteContractinstead ofreadContract.allowance, update the final test to search the appropriate mock (simulateContract/writeContract) for a call whosespender/args[1]isMOCK_EXCHANGE_HELPER. - If there is an existing
describeblock forGoodReserveSDKtests you want these to live in, you may want to move thedescribe("GoodReserveSDK (exchange-helper mode)", ...)block into that section for consistency.
added 2 commits
March 10, 2026 14:25
…to prevent rate limits
- treat missing publicClient as loading state in useGoodReserve hook - add minReturn < 0n validation tests for buy and sell - add no-approval path test (sufficient allowance skips approve) - add timestamp enrichment and chronological sort test in getTransactionHistory - add constructor test for unavailable chain/env config - add exchange-helper buy/sell write-path tests - extract useReserveSwapQuote and useReserveSwapTx from ReserveSwap - replace hardcoded chain?.id === 50 with XDC_CHAIN_ID constant
Contributor
Author
|
Hi @L03TJ3 My PR is Ready |
blueogin
reviewed
Mar 20, 2026
| const [txResult, setTxResult] = useState<string | null>(null) | ||
| const [txError, setTxError] = useState<string | null>(null) | ||
|
|
||
| const execute = useCallback( |
Collaborator
There was a problem hiding this comment.
You can modify this function to return a boolean indicating whether the execution succeeded.
Contributor
Author
|
Hey @blueogin , pushed updates addressing your feedback:
Happy to adjust anything else, thanks for the 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.
Resolves #34
This pull request introduces
@goodsdks/good-reserve, a viem-based SDK for interacting with the GoodDollar Reserve to buy and sell G$. It includes React hook wrappers in@goodsdks/react-hooksand a complete demo application (demo-reserve-swap) demonstrating the full user flow.What was implemented
Core SDK (
@goodsdks/good-reserve)Implements
GoodReserveSDKwhich abstracts both Mento Broker routes (Celo production and XDC development) and Exchange Helper routes. Provides typed methods for quoting (getBuyQuote/getSellQuote), executing swaps (buy/sellwith automatic approvals), and retrieving reserve diagnostics and transaction history.Contract Mappings
Inlines required ABIs and defines canonical
RESERVE_CONTRACT_ADDRESSESforproduction,staging, anddevelopmentenvironments across supported chains.Addresses are pinned to GoodProtocol commit
04b5d250bc2802c7b04b7c4d87bc00222ea81e2b(fromreleases/deployment.json).ABIs are sourced from mento-core develop branch.
React Hooks Integration
Added
useGoodReservehook in@goodsdks/react-hooks, consistent with existing identity-related hooks and exported through the package barrel.Demo Application (
demo-reserve-swap)Provides a working demonstration of the flow including real-time quotes, dynamic approval handling, G$ ↔ stable token swaps (cUSD on Celo, USDC on XDC dev), and decoded transaction history display.
Refactoring & Improvements
getMentoExchangeIdnow usesPromise.allto query multiple pools concurrently instead of sequentially.extractPoolAddressesandextractPoolStats) that align with viem's tuple/struct return shapes.{ exactApproval: boolean }setting inGoodReserveSDKOptions. Defaults to approvingmaxUint256for gas efficiency on repeated swaps; can be set totrueto enforce exact-amount approvals.Checklist (DoD Verified)
@goodsdks/good-reserveworkspace builds and exports public typed APIs fromsrc/index.ts.@goodsdks/react-hooksand exported through package barrels.How to Test