Conversation
There was a problem hiding this comment.
Hey - I've found 1 security issue, 5 other issues, and left some high level feedback:
Security issues:
- Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (link)
General comments:
- In
StreamingTestPage,OperationSectionis being used with adisabledprop but the component’s props interface doesn’t define it or pass it through to the underlying button, which will cause type errors and makes the disabled state ineffective—adddisabled?: booleanto the props and wire it to theButton’sdisabledprop. - The change in
VerifyButtonto callonVerificationSuccess()immediately after settingwindow.location.hrefwill mark verification as successful before the user actually completes the external flow; consider moving this callback to the point where the user returns and the verification result has been confirmed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `StreamingTestPage`, `OperationSection` is being used with a `disabled` prop but the component’s props interface doesn’t define it or pass it through to the underlying button, which will cause type errors and makes the disabled state ineffective—add `disabled?: boolean` to the props and wire it to the `Button`’s `disabled` prop.
- The change in `VerifyButton` to call `onVerificationSuccess()` immediately after setting `window.location.href` will mark verification as successful before the user actually completes the external flow; consider moving this callback to the point where the user returns and the verification result has been confirmed.
## Individual Comments
### Comment 1
<location> `apps/demo-identity-app/src/components/StreamingTestPage.tsx:62-70` </location>
<code_context>
+ </YStack>
+)
+
+const OperationSection: React.FC<{
+ title: string
+ buttonText: string
+ buttonColor: string
+ isLoading: boolean
+ onAction: (receiver: string, amount: string) => void
+ showAmount?: boolean
+ timeUnit?: string
+ setTimeUnit?: (unit: "hour" | "day" | "month") => void
+}> = ({
+ title,
</code_context>
<issue_to_address>
**issue (bug_risk):** OperationSection props don’t define a disabled flag, but the component is used with a disabled prop, which will cause a type error and the disabled state to be ignored.
To resolve this, add `disabled?: boolean` to `OperationSection`’s props and pass it through to the Button, e.g. `disabled={isLoading || disabled}`. That will satisfy TypeScript and ensure the Button is actually non-interactive when `!G$_TOKEN`.
</issue_to_address>
### Comment 2
<location> `apps/demo-identity-app/src/components/StreamingTestPage.tsx:273-282` </location>
<code_context>
+
+ const G$_TOKEN = chainId ? getG$Token(chainId, environment) : undefined
+
+ const handleAction = (
+ receiver: string,
+ amount: string,
+ // eslint-disable-next-line @typescript-eslint/no-explicit-any
+ mutation: (args: UseCreateStreamParams | UseUpdateStreamParams | UseDeleteStreamParams, options?: any) => void,
+ msg: string
+ ) => {
+ if (!receiver || (!amount && msg !== "Stream deleted!")) return alert("Please fill in all fields")
+ if (!G$_TOKEN) return alert('G$ token not configured for this chain/environment')
+ const flowRate = amount ? calculateFlowRate(parseEther(amount), timeUnit) : undefined
+
+ const args: UseCreateStreamParams & UseUpdateStreamParams & UseDeleteStreamParams = {
+ receiver: receiver as Address,
+ token: G$_TOKEN as Address,
</code_context>
<issue_to_address>
**issue (bug_risk):** handleAction uses SDK hook parameter types without importing them and combines them in a way that doesn’t match the actual shapes.
`UseCreateStreamParams`, `UseUpdateStreamParams`, and `UseDeleteStreamParams` aren’t imported, so this won’t compile. Also, typing `args` as `UseCreateStreamParams & UseUpdateStreamParams & UseDeleteStreamParams` is inaccurate, since those fields aren’t all present at once and the discriminator is `msg`. Either import the SDK types and use the appropriate one per action, or define a minimal local `args` type matching what the mutations actually need (receiver, token, environment, and optional flowRate/newFlowRate) instead of intersecting all three.
</issue_to_address>
### Comment 3
<location> `packages/react-hooks/src/streaming/index.ts:49-26` </location>
<code_context>
+ enabled?: boolean
+}
+
+export interface UseGDAPoolsParams {
+ enabled?: boolean
+}
+
+export interface UsePoolMembershipsParams {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** useGDAPools ignores environment selection, but callers pass an environment expecting behavior to change per env.
In `useGDAPools`, the params only accept `enabled`, and the hook constructs `GdaSDK` with `{ chainId: publicClient.chain?.id }` without using an environment. On `StreamingTestPage` you call `useGDAPools({ environment, enabled: !!address })`, so `environment` is effectively ignored and env buttons don’t change the data source. If the SDK’s subgraph or behavior is env-specific, this hook should accept an `environment?: Environment`, pass it into `GdaSDK`/`SubgraphClient`, and include it in the query key so caching and data align with the selected env.
Suggested implementation:
```typescript
export interface UseGDAPoolsParams {
environment?: Environment
enabled?: boolean
}
```
To fully implement the suggestion, also update the `useGDAPools` hook implementation (in this or the corresponding hooks file) to:
1. Accept `environment` in its params type (which will now be `UseGDAPoolsParams`).
2. Pass `environment` into the `GdaSDK` / `SubgraphClient` constructor instead of or in addition to `{ chainId: publicClient.chain?.id }`, according to how the SDK expects env configuration (e.g. `new GdaSDK({ chainId: publicClient.chain?.id, environment })`).
3. Include `environment` in the React Query key (e.g. `['gdaPools', environment, publicClient.chain?.id, ...]`) so results are cached per environment.
4. Ensure all existing callers that pass `environment` (like `StreamingTestPage`) now get env-specific behavior without type errors.
</issue_to_address>
### Comment 4
<location> `packages/streaming-sdk/src/streaming-sdk.ts:74-82` </location>
<code_context>
+ this.walletClient = walletClient
+ }
+
+ async createStream(params: CreateStreamParams): Promise<Hash> {
+ const { receiver, token, flowRate, userData = "0x", onHash } = params
+
+ if (flowRate <= BigInt(0)) {
+ throw new Error("Flow rate must be greater than zero")
+ }
+
+ return this.submitAndWait(
+ {
+ address: this.cfaForwarder,
</code_context>
<issue_to_address>
**suggestion:** The createStream API accepts userData but doesn’t pass it to the on-chain call, which is inconsistent with the type and the GdaSDK API.
`CreateStreamParams` exposes `userData`, but `setFlowrate` only sends `[token, receiver, flowRate]`. If the target CFA forwarder method supports `userData`, please forward it (to match `updateStream`/`deleteStream` and GdaSDK pool operations). If it doesn’t, consider removing `userData` from `CreateStreamParams` or clearly marking it as reserved so the API doesn’t imply unused functionality.
```suggestion
return this.submitAndWait(
{
address: this.cfaForwarder,
abi: cfaForwarderAbi,
functionName: "setFlowrate",
args: [token, receiver, flowRate, userData],
},
onHash,
)
```
</issue_to_address>
### Comment 5
<location> `packages/react-hooks/src/streaming/index.ts:82` </location>
<code_context>
+ const { data: walletClient } = useWalletClient()
+ const queryClient = useQueryClient()
+
+ const sdks = useMemo(() => {
+ if (!publicClient) return new Map<string, StreamingSDK>()
+ const envs = ["production", "staging", "development"] as const
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the streaming and GDA hooks to use small shared factory/helpers instead of memoized maps and duplicated mutation logic to simplify and unify SDK usage.
You can simplify and de‑duplicate the hooks without changing behavior by:
1. Removing the per‑environment `Map` and constructing the SDK lazily in the mutation/query.
2. Extracting a small shared factory for streaming mutations.
3. Using a consistent pattern for GDA mutations.
### 1. Replace `sdks` map with a small SDK factory
Instead of memoizing a `Map<string, StreamingSDK>`, you can create a helper that constructs the SDK on demand:
```ts
function getStreamingSdk(
publicClient: ReturnType<typeof usePublicClient>,
walletClient: ReturnType<typeof useWalletClient>["data"] | undefined,
environment: Environment
) {
if (!publicClient) throw new Error("Public client not available");
if (!walletClient) throw new Error("Wallet client not available");
return new StreamingSDK(publicClient, walletClient, { environment });
}
```
Then your mutations become simpler and don’t need `useMemo` or a `Map`:
```ts
export function useCreateStream() {
const publicClient = usePublicClient();
const { data: walletClient } = useWalletClient();
const queryClient = useQueryClient();
return useMutation({
mutationFn: async ({
receiver,
token,
flowRate,
userData = "0x",
environment = "production",
}: UseCreateStreamParams): Promise<Hash> => {
const sdk = getStreamingSdk(publicClient, walletClient, environment);
return sdk.createStream({ receiver, token, flowRate, userData });
},
onSuccess: () => {
queryClient.invalidateQueries({ queryKey: ["streams"] });
},
});
}
```
You can apply the same pattern to `useUpdateStream` and `useDeleteStream`:
```ts
export function useUpdateStream() {
const publicClient = usePublicClient();
const { data: walletClient } = useWalletClient();
const queryClient = useQueryClient();
return useMutation({
mutationFn: async ({
receiver,
token,
newFlowRate,
userData = "0x",
environment = "production",
}: UseUpdateStreamParams): Promise<Hash> => {
const sdk = getStreamingSdk(publicClient, walletClient, environment);
return sdk.updateStream({ receiver, token, newFlowRate, userData });
},
onSuccess: () => {
queryClient.invalidateQueries({ queryKey: ["streams"] });
},
});
}
export function useDeleteStream() {
const publicClient = usePublicClient();
const { data: walletClient } = useWalletClient();
const queryClient = useQueryClient();
return useMutation({
mutationFn: async ({
receiver,
token,
environment = "production",
}: UseDeleteStreamParams): Promise<Hash> => {
const sdk = getStreamingSdk(publicClient, walletClient, environment);
return sdk.deleteStream({ receiver, token });
},
onSuccess: () => {
queryClient.invalidateQueries({ queryKey: ["streams"] });
},
});
}
```
This removes the repeated `useMemo` + `Map` logic and eagerly created SDKs, while preserving all functionality and error semantics (and keeps the `environment` arg).
### 2. Optional: DRY up streaming mutations further
If you want to centralize the common streaming mutation pattern:
```ts
function createStreamingMutation<TArgs>(
mutate: (sdk: StreamingSDK, args: TArgs) => Promise<Hash>,
invalidateKey: unknown[]
) {
const publicClient = usePublicClient();
const { data: walletClient } = useWalletClient();
const queryClient = useQueryClient();
return useMutation({
mutationFn: async (args: TArgs & { environment?: Environment }) => {
const {
environment = "production",
...rest
} = args as TArgs & { environment?: Environment };
const sdk = getStreamingSdk(publicClient, walletClient, environment);
return mutate(sdk, rest as TArgs);
},
onSuccess: () => {
queryClient.invalidateQueries({ queryKey: invalidateKey });
},
});
}
```
Usage:
```ts
export function useCreateStream() {
return createStreamingMutation<UseCreateStreamParams>(
(sdk, { receiver, token, flowRate, userData = "0x" }) =>
sdk.createStream({ receiver, token, flowRate, userData }),
["streams"]
);
}
```
Same pattern for update/delete with their respective payload shapes.
### 3. Align GDA mutations on a similar pattern
`useConnectToPool` and `useDisconnectFromPool` are very similar. You can use a small helper to reduce duplication and make the instantiation pattern consistent:
```ts
function createGdaMutation(
mutate: (sdk: GdaSDK, args: UseConnectToPoolParams | UseDisconnectFromPoolParams) => Promise<Hash>
) {
const publicClient = usePublicClient();
const { data: walletClient } = useWalletClient();
const queryClient = useQueryClient();
return useMutation({
mutationFn: async (args): Promise<Hash> => {
if (!publicClient) throw new Error("Public client not available");
if (!walletClient) throw new Error("Wallet client not available");
const sdk = new GdaSDK(publicClient, walletClient as any);
return mutate(sdk, args);
},
onSuccess: () => {
queryClient.invalidateQueries({ queryKey: ["gda-pools"] });
queryClient.invalidateQueries({ queryKey: ["gda-memberships"] });
},
});
}
export function useConnectToPool() {
return createGdaMutation((sdk, { poolAddress, userData = "0x" }) =>
sdk.connectToPool({ poolAddress, userData })
);
}
export function useDisconnectFromPool() {
return createGdaMutation((sdk, { poolAddress, userData = "0x" }) =>
sdk.disconnectFromPool({ poolAddress, userData })
);
}
```
These changes should address the reviewer’s concerns by:
- Eliminating repeated `useMemo` + map logic.
- Making SDK instantiation consistent across hooks.
- Centralizing error handling and invalidation behavior while preserving the existing API.
</issue_to_address>
### Comment 6
<location> `packages/streaming-sdk/src/sdk.test.ts:30` </location>
<code_context>
0x62B8B11039FcfE5aB0C56E502b1C372A3d2a9c7A
</code_context>
<issue_to_address>
**security (generic-api-key):** Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
*Source: gitleaks*
</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 OperationSection: React.FC<{ | ||
| title: string | ||
| buttonText: string | ||
| buttonColor: string | ||
| isLoading: boolean | ||
| onAction: (receiver: string, amount: string) => void | ||
| showAmount?: boolean | ||
| timeUnit?: string | ||
| setTimeUnit?: (unit: "hour" | "day" | "month") => void |
There was a problem hiding this comment.
issue (bug_risk): OperationSection props don’t define a disabled flag, but the component is used with a disabled prop, which will cause a type error and the disabled state to be ignored.
To resolve this, add disabled?: boolean to OperationSection’s props and pass it through to the Button, e.g. disabled={isLoading || disabled}. That will satisfy TypeScript and ensure the Button is actually non-interactive when !G$_TOKEN.
| const handleAction = ( | ||
| receiver: string, | ||
| amount: string, | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| mutation: (args: UseCreateStreamParams | UseUpdateStreamParams | UseDeleteStreamParams, options?: any) => void, | ||
| msg: string | ||
| ) => { | ||
| if (!receiver || (!amount && msg !== "Stream deleted!")) return alert("Please fill in all fields") | ||
| if (!G$_TOKEN) return alert('G$ token not configured for this chain/environment') | ||
| const flowRate = amount ? calculateFlowRate(parseEther(amount), timeUnit) : undefined |
There was a problem hiding this comment.
issue (bug_risk): handleAction uses SDK hook parameter types without importing them and combines them in a way that doesn’t match the actual shapes.
UseCreateStreamParams, UseUpdateStreamParams, and UseDeleteStreamParams aren’t imported, so this won’t compile. Also, typing args as UseCreateStreamParams & UseUpdateStreamParams & UseDeleteStreamParams is inaccurate, since those fields aren’t all present at once and the discriminator is msg. Either import the SDK types and use the appropriate one per action, or define a minimal local args type matching what the mutations actually need (receiver, token, environment, and optional flowRate/newFlowRate) instead of intersecting all three.
| flowRate: bigint | ||
| userData?: `0x${string}` | ||
| environment?: Environment | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): useGDAPools ignores environment selection, but callers pass an environment expecting behavior to change per env.
In useGDAPools, the params only accept enabled, and the hook constructs GdaSDK with { chainId: publicClient.chain?.id } without using an environment. On StreamingTestPage you call useGDAPools({ environment, enabled: !!address }), so environment is effectively ignored and env buttons don’t change the data source. If the SDK’s subgraph or behavior is env-specific, this hook should accept an environment?: Environment, pass it into GdaSDK/SubgraphClient, and include it in the query key so caching and data align with the selected env.
Suggested implementation:
export interface UseGDAPoolsParams {
environment?: Environment
enabled?: boolean
}To fully implement the suggestion, also update the useGDAPools hook implementation (in this or the corresponding hooks file) to:
- Accept
environmentin its params type (which will now beUseGDAPoolsParams). - Pass
environmentinto theGdaSDK/SubgraphClientconstructor instead of or in addition to{ chainId: publicClient.chain?.id }, according to how the SDK expects env configuration (e.g.new GdaSDK({ chainId: publicClient.chain?.id, environment })). - Include
environmentin the React Query key (e.g.['gdaPools', environment, publicClient.chain?.id, ...]) so results are cached per environment. - Ensure all existing callers that pass
environment(likeStreamingTestPage) now get env-specific behavior without type errors.
| return this.submitAndWait( | ||
| { | ||
| address: this.cfaForwarder, | ||
| abi: cfaForwarderAbi, | ||
| functionName: "setFlowrate", | ||
| args: [token, receiver, flowRate], | ||
| }, | ||
| onHash, | ||
| ) |
There was a problem hiding this comment.
suggestion: The createStream API accepts userData but doesn’t pass it to the on-chain call, which is inconsistent with the type and the GdaSDK API.
CreateStreamParams exposes userData, but setFlowrate only sends [token, receiver, flowRate]. If the target CFA forwarder method supports userData, please forward it (to match updateStream/deleteStream and GdaSDK pool operations). If it doesn’t, consider removing userData from CreateStreamParams or clearly marking it as reserved so the API doesn’t imply unused functionality.
| return this.submitAndWait( | |
| { | |
| address: this.cfaForwarder, | |
| abi: cfaForwarderAbi, | |
| functionName: "setFlowrate", | |
| args: [token, receiver, flowRate], | |
| }, | |
| onHash, | |
| ) | |
| return this.submitAndWait( | |
| { | |
| address: this.cfaForwarder, | |
| abi: cfaForwarderAbi, | |
| functionName: "setFlowrate", | |
| args: [token, receiver, flowRate, userData], | |
| }, | |
| onHash, | |
| ) |
| const { data: walletClient } = useWalletClient() | ||
| const queryClient = useQueryClient() | ||
|
|
||
| const sdks = useMemo(() => { |
There was a problem hiding this comment.
issue (complexity): Consider refactoring the streaming and GDA hooks to use small shared factory/helpers instead of memoized maps and duplicated mutation logic to simplify and unify SDK usage.
You can simplify and de‑duplicate the hooks without changing behavior by:
- Removing the per‑environment
Mapand constructing the SDK lazily in the mutation/query. - Extracting a small shared factory for streaming mutations.
- Using a consistent pattern for GDA mutations.
1. Replace sdks map with a small SDK factory
Instead of memoizing a Map<string, StreamingSDK>, you can create a helper that constructs the SDK on demand:
function getStreamingSdk(
publicClient: ReturnType<typeof usePublicClient>,
walletClient: ReturnType<typeof useWalletClient>["data"] | undefined,
environment: Environment
) {
if (!publicClient) throw new Error("Public client not available");
if (!walletClient) throw new Error("Wallet client not available");
return new StreamingSDK(publicClient, walletClient, { environment });
}Then your mutations become simpler and don’t need useMemo or a Map:
export function useCreateStream() {
const publicClient = usePublicClient();
const { data: walletClient } = useWalletClient();
const queryClient = useQueryClient();
return useMutation({
mutationFn: async ({
receiver,
token,
flowRate,
userData = "0x",
environment = "production",
}: UseCreateStreamParams): Promise<Hash> => {
const sdk = getStreamingSdk(publicClient, walletClient, environment);
return sdk.createStream({ receiver, token, flowRate, userData });
},
onSuccess: () => {
queryClient.invalidateQueries({ queryKey: ["streams"] });
},
});
}You can apply the same pattern to useUpdateStream and useDeleteStream:
export function useUpdateStream() {
const publicClient = usePublicClient();
const { data: walletClient } = useWalletClient();
const queryClient = useQueryClient();
return useMutation({
mutationFn: async ({
receiver,
token,
newFlowRate,
userData = "0x",
environment = "production",
}: UseUpdateStreamParams): Promise<Hash> => {
const sdk = getStreamingSdk(publicClient, walletClient, environment);
return sdk.updateStream({ receiver, token, newFlowRate, userData });
},
onSuccess: () => {
queryClient.invalidateQueries({ queryKey: ["streams"] });
},
});
}
export function useDeleteStream() {
const publicClient = usePublicClient();
const { data: walletClient } = useWalletClient();
const queryClient = useQueryClient();
return useMutation({
mutationFn: async ({
receiver,
token,
environment = "production",
}: UseDeleteStreamParams): Promise<Hash> => {
const sdk = getStreamingSdk(publicClient, walletClient, environment);
return sdk.deleteStream({ receiver, token });
},
onSuccess: () => {
queryClient.invalidateQueries({ queryKey: ["streams"] });
},
});
}This removes the repeated useMemo + Map logic and eagerly created SDKs, while preserving all functionality and error semantics (and keeps the environment arg).
2. Optional: DRY up streaming mutations further
If you want to centralize the common streaming mutation pattern:
function createStreamingMutation<TArgs>(
mutate: (sdk: StreamingSDK, args: TArgs) => Promise<Hash>,
invalidateKey: unknown[]
) {
const publicClient = usePublicClient();
const { data: walletClient } = useWalletClient();
const queryClient = useQueryClient();
return useMutation({
mutationFn: async (args: TArgs & { environment?: Environment }) => {
const {
environment = "production",
...rest
} = args as TArgs & { environment?: Environment };
const sdk = getStreamingSdk(publicClient, walletClient, environment);
return mutate(sdk, rest as TArgs);
},
onSuccess: () => {
queryClient.invalidateQueries({ queryKey: invalidateKey });
},
});
}Usage:
export function useCreateStream() {
return createStreamingMutation<UseCreateStreamParams>(
(sdk, { receiver, token, flowRate, userData = "0x" }) =>
sdk.createStream({ receiver, token, flowRate, userData }),
["streams"]
);
}Same pattern for update/delete with their respective payload shapes.
3. Align GDA mutations on a similar pattern
useConnectToPool and useDisconnectFromPool are very similar. You can use a small helper to reduce duplication and make the instantiation pattern consistent:
function createGdaMutation(
mutate: (sdk: GdaSDK, args: UseConnectToPoolParams | UseDisconnectFromPoolParams) => Promise<Hash>
) {
const publicClient = usePublicClient();
const { data: walletClient } = useWalletClient();
const queryClient = useQueryClient();
return useMutation({
mutationFn: async (args): Promise<Hash> => {
if (!publicClient) throw new Error("Public client not available");
if (!walletClient) throw new Error("Wallet client not available");
const sdk = new GdaSDK(publicClient, walletClient as any);
return mutate(sdk, args);
},
onSuccess: () => {
queryClient.invalidateQueries({ queryKey: ["gda-pools"] });
queryClient.invalidateQueries({ queryKey: ["gda-memberships"] });
},
});
}
export function useConnectToPool() {
return createGdaMutation((sdk, { poolAddress, userData = "0x" }) =>
sdk.connectToPool({ poolAddress, userData })
);
}
export function useDisconnectFromPool() {
return createGdaMutation((sdk, { poolAddress, userData = "0x" }) =>
sdk.disconnectFromPool({ poolAddress, userData })
);
}These changes should address the reviewer’s concerns by:
- Eliminating repeated
useMemo+ map logic. - Making SDK instantiation consistent across hooks.
- Centralizing error handling and invalidation behavior while preserving the existing API.
| writeContract: vi.fn().mockResolvedValue("0xhash"), | ||
| }) | ||
|
|
||
| const TEST_SUPERTOKEN = "0x62B8B11039FcfE5aB0C56E502b1C372A3d2a9c7A" as Address |
There was a problem hiding this comment.
security (generic-api-key): Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
Source: gitleaks
|
Closing this PR to remove the frontend test page.The test page made the files bulky and caused Vercel preview failures. I’ll reopen a new clean PR with only the core SDK + hooks 25 |
This PR implements the Superfluid Streaming SDK along with its associated React hooks and an interactive test page for the GoodDollar ecosystem. The implementation provides a unified interface for managing money streams and GDA distribution pools on Celo and Base.
Key Technical Highlights:
.envsupport.How Has This Been Tested?
turbo buildandturbo lintchecks.Checklist:
fixed issue #23
Summary by Sourcery
Introduce a Superfluid-based streaming SDK with React hooks and an interactive streaming test page, and wire it into the GoodDollar demo app and CI.
New Features:
Enhancements:
Build:
CI:
Documentation: