feat(streaming): implement superfluid sdk and react hooks#31
feat(streaming): implement superfluid sdk and react hooks#31HushLuxe wants to merge 25 commits intoGoodDollar:mainfrom
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In the React streaming hooks you eagerly instantiate
StreamingSDKfor all environments on every hook usage (e.g.useCreateStream,useUpdateStream,useDeleteStream); consider lazily creating only the environment actually used and/or sharing a memoized SDK factory to avoid repeated construction and to reduce unnecessary try/catch noise. - The
userDataparameter is accepted in the Streaming/GDA SDK methods and hook mutation params but is not consistently forwarded into the underlying contract calls (e.g.createStreamusessetFlowratewithoutuserData), which may be confusing to consumers; either wire it through where supported or remove it from the public API where it has no effect. - When resolving
CFA_FORWARDER_ADDRESSES/GDA_FORWARDER_ADDRESSESbychainId, there is no guard for missing entries, so an unsupported or misconfigured chain could lead to anundefinedaddress being used; consider validating that a forwarder address exists and throwing a clear error if not.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the React streaming hooks you eagerly instantiate `StreamingSDK` for all environments on every hook usage (e.g. `useCreateStream`, `useUpdateStream`, `useDeleteStream`); consider lazily creating only the environment actually used and/or sharing a memoized SDK factory to avoid repeated construction and to reduce unnecessary try/catch noise.
- The `userData` parameter is accepted in the Streaming/GDA SDK methods and hook mutation params but is not consistently forwarded into the underlying contract calls (e.g. `createStream` uses `setFlowrate` without `userData`), which may be confusing to consumers; either wire it through where supported or remove it from the public API where it has no effect.
- When resolving `CFA_FORWARDER_ADDRESSES`/`GDA_FORWARDER_ADDRESSES` by `chainId`, there is no guard for missing entries, so an unsupported or misconfigured chain could lead to an `undefined` address being used; consider validating that a forwarder address exists and throwing a clear error if not.
## Individual Comments
### Comment 1
<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 extracting a shared helper hook to build the StreamingSDK instances by environment and reuse it across the stream hooks (and optionally the list hook) to centralize environment handling and SDK setup.
You can reduce duplication and make the hooks easier to reason about by extracting a shared SDK helper and a single env definition, then using that across the stream hooks (and optionally the list hook).
### 1. Extract a shared env list + SDK map helper
```ts
const STREAMING_ENVIRONMENTS = ["production", "staging", "development"] as const
function useStreamingSdksByEnvironment() {
const publicClient = usePublicClient()
const { data: walletClient } = useWalletClient()
return useMemo(() => {
if (!publicClient) return new Map<Environment, StreamingSDK>()
const m = new Map<Environment, StreamingSDK>()
for (const e of STREAMING_ENVIRONMENTS) {
// keep current swallow behavior if you need backward compatibility
try {
m.set(
e,
new StreamingSDK(
publicClient,
walletClient ? (walletClient as any) : undefined,
{ environment: e },
),
)
} catch {
// ignore
}
}
return m
}, [publicClient, walletClient])
}
```
### 2. Reuse in `useCreateStream`, `useUpdateStream`, `useDeleteStream`
```ts
export function useCreateStream() {
const sdks = useStreamingSdksByEnvironment()
const queryClient = useQueryClient()
return useMutation({
mutationFn: async ({
receiver,
token,
flowRate,
userData = "0x",
environment = "production",
}: UseCreateStreamParams): Promise<Hash> => {
const sdk = sdks.get(environment)
if (!sdk) throw new Error("SDK not available for selected environment")
return sdk.createStream({ receiver, token, flowRate, userData })
},
onSuccess: () => {
queryClient.invalidateQueries({ queryKey: ["streams"] })
},
})
}
export function useUpdateStream() {
const sdks = useStreamingSdksByEnvironment()
const queryClient = useQueryClient()
return useMutation({
mutationFn: async ({
receiver,
token,
newFlowRate,
userData = "0x",
environment = "production",
}: UseUpdateStreamParams): Promise<Hash> => {
const sdk = sdks.get(environment)
if (!sdk) throw new Error("SDK not available for selected environment")
return sdk.updateStream({ receiver, token, newFlowRate, userData })
},
onSuccess: () => {
queryClient.invalidateQueries({ queryKey: ["streams"] })
},
})
}
export function useDeleteStream() {
const sdks = useStreamingSdksByEnvironment()
const queryClient = useQueryClient()
return useMutation({
mutationFn: async ({
receiver,
token,
environment = "production",
}: UseDeleteStreamParams): Promise<Hash> => {
const sdk = sdks.get(environment)
if (!sdk) throw new Error("SDK not available for selected environment")
return sdk.deleteStream({ receiver, token })
},
onSuccess: () => {
queryClient.invalidateQueries({ queryKey: ["streams"] })
},
})
}
```
This:
- Removes three nearly-identical `useMemo` blocks.
- Centralizes environment handling in one place.
- Removes repeated `publicClient`/`walletClient` checks inside each mutation (they’re implicitly handled by the `sdks` map being empty when clients are missing).
### 3. Optionally reuse helper in `useStreamList`
To keep environment handling consistent:
```ts
export function useStreamList({
account,
direction = "all",
environment = "production",
enabled = true,
}: UseStreamListParams) {
const sdks = useStreamingSdksByEnvironment()
const publicClient = usePublicClient()
return useQuery<StreamInfo[]>({
queryKey: ["streams", account, direction, environment, publicClient?.chain?.id],
queryFn: async () => {
const sdk = sdks.get(environment)
if (!sdk) throw new Error("SDK not available for selected environment")
return sdk.getActiveStreams(account, direction)
},
enabled: enabled && !!account && !!publicClient,
})
}
```
This keeps all existing behavior but consolidates the SDK construction and environment logic so future changes only need to be made in one 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.
| - run: npm install --global turbo@latest | ||
| - run: npm install --global vite@latest | ||
| - run: yarn install --immutable | ||
| # Build dependencies first using turbo |
There was a problem hiding this comment.
for vercel deployment fixes, all reverted though
apps/engagement-app/.vercelignore
Outdated
| @@ -0,0 +1,10 @@ | |||
| node_modules | |||
There was a problem hiding this comment.
was trying to fix frontend vercel deployment issues, but i have reverted all the identity app changes and kept it core sdk only
There was a problem hiding this comment.
the vercel build reports that your lock file isnt in sync. verify it is
There was a problem hiding this comment.
also another option is to upgrade vercel version
apps/demo-identity-app/.env.example
Outdated
| @@ -0,0 +1 @@ | |||
| VITE_GRAPH_API_KEY= | |||
There was a problem hiding this comment.
ALL changes to the demo-identity-app should be removed.
Create a new demo app for streaming
There was a problem hiding this comment.
I have removed the frontend test files i pushed earlier as they were still causing vercel deployment failures
There was a problem hiding this comment.
its important to have some kind of UI demo
There was a problem hiding this comment.
I’ve added a frontend for the demo
HushLuxe
force-pushed
the
feat/streaming-sdk
branch
from
5f0c556 to
4ef249b
Compare
|
@sourcery-ai guide does it fullfill issue #23 |
Reviewer's GuideAdds a new Superfluid-based streaming SDK package for Celo/Base plus React hooks that wrap it, centralizing protocol constants, subgraph access, and flow-rate utilities, and wires everything into the monorepo with tests, build config, and docs. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
HushLuxe
force-pushed
the
feat/streaming-sdk
branch
from
abbcf2d to
ceafbca
Compare
|
@HushLuxe do not use force push. I can not review the changes. |
HushLuxe
force-pushed
the
feat/streaming-sdk
branch
from
ceafbca to
19e1946
Compare
|
@sirpy I have now restored the original commit history but still looking for a way to fix the scanner issue |
HushLuxe
force-pushed
the
feat/streaming-sdk
branch
from
fac09e6 to
f1f3bd4
Compare
|
@HushLuxe we can just ignore sourcery security issue if it is invalid |
@HushLuxe The sdk should default to G$ token, the dev should not need to specify the token each time. |
|
Done..... |
| enabled: !!address, | ||
| }) as { data: StreamInfo[] | undefined, isLoading: boolean, refetch: () => void } | ||
|
|
||
| const { data: pools, isLoading: poolsLoading } = useGDAPools({ |
There was a problem hiding this comment.
You are using here an environment param but its not defined as being a param for this hook?
There was a problem hiding this comment.
I have now added an environment as a declared param on the useGDAPools
There was a problem hiding this comment.
Update: Actually, I realized that adding the environment parameter wasn’t really necessary for the current production scope. I hv removed the environment param from useGDAPools (and the demo call site) to keep the API surface minimal and clean
| ) | ||
| } | ||
|
|
||
| return this.submitAndWait( |
There was a problem hiding this comment.
setFlowRate only has three arguments and would fail through simulateContract because 'args expected 3, but received 4'.
How has this been tested?
Reference: https://sdk.superfluid.pro/docs/use-cases/manage-cfa-flow#using-setflowrate-recommended
There was a problem hiding this comment.
Also, test script you can run with node from within sdks/streaming-sdk:
https://gist.github.com/L03TJ3/93debaa9db7aee5b74058fdea920fe55
There was a problem hiding this comment.
Thank you for the test script. it was very helpful. I ran it from within packages/streaming-sdk and all checks pass with the corrected args.
packages/streaming-sdk/README.md
Outdated
| #### `getActiveStreams(account, direction?)` | ||
| Returns active streams for an account. | ||
| ```typescript | ||
| const streams = await sdk.getActiveStreams('0x...', 'outgoing' | 'incoming' | 'all') |
There was a problem hiding this comment.
Docs seem out of sync. the implementation expects an options object.
in this example, '0x...' is treated as options and the rest will be ignored
There was a problem hiding this comment.
I have also updated this file
3 participants
I'm adding the @goodsdks/streaming-sdk and @goodsdks/react-hooks (streaming module) to the monorepo. This provides a unified, type-safe way to handle Superfluid operations on Celo and Base, specifically optimized for G$ and GDA pools.
Changes
Testing
I've verified the implementation with:
Checklist:
Summary by Sourcery
Add a new Superfluid-based streaming SDK for Celo/Base and expose React hooks for managing streams and GDA pool interactions.
New Features:
Enhancements:
Tests: