feat(streaming): implement superfluid sdk and react hooks#31
feat(streaming): implement superfluid sdk and react hooks#31HushLuxe wants to merge 45 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.
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. |
| } | ||
| } | ||
| ` | ||
|
|
There was a problem hiding this comment.
the fetch logic for reserves is not filtered to the connected or passed down address/account.
It does not really make sense to expose fetching reserves in general
There was a problem hiding this comment.
Hi @L03TJ3 , Thank you for getting back to me. I have scoped the SUP reserves query to the passed account (through the lockerOwner: $account) and made the account param mandatory, so it only returns the connected wallet’s lockers. I also removed the generic querySUPReserves from StreamingSDK so it only lives on SubgraphClient
| return pools.find((p) => p.id.toLowerCase() === poolId.toLowerCase()) ?? null | ||
| } | ||
|
|
||
| async querySUPReserves() { |
There was a problem hiding this comment.
why do have a querySUPreserves for both gda-sdk and streaming-sdk?
whats the difference?
There was a problem hiding this comment.
I have addressed too. I removed querySUPReserves from StreamingSDK so it now lives only on SubgraphClient to eliminate duplicates. The react hook now calls SubgraphClient directly
| id: p.id as Address, | ||
| token: p.token.id as Address, | ||
| totalUnits: BigInt(p.totalUnits), | ||
| totalAmountClaimed: BigInt(p.totalAmountDistributedUntilUpdatedAt), |
There was a problem hiding this comment.
Misleading, totalAmountDistributed I believe is the total that the pool distributed, not what someone has claimed.
should this not use totalAmountClaimed?
There was a problem hiding this comment.
Fixed the totalAmountClaimed semantic gap. It no longer pulls the pool-wide total distributions, and specifically maps the per-member amount directly.
…serves to account
…emberPools and querySUPReserves
This reverts commit b2bf646.
HushLuxe
force-pushed
the
feat/streaming-sdk
branch
from
64d5889 to
5418546
Compare
| @@ -0,0 +1,166 @@ | |||
| # @goodsdks/streaming-sdk | |||
There was a problem hiding this comment.
Missing items in the Readme:
-
getBalanceHistory
-
Sup Reserve handling (its also a very limited demonstration in the demo)
-
getActiveStreams does not include pagination example (And have a look at how pagination is handled as there are inconsistencies. What would happen if I provide {first: 20} and not define skip?)
-
address maps are not documented
There was a problem hiding this comment.
Demo-app does not demonstrate:
- Sup Balances
- getBalanceHistory
| ) | ||
| } | ||
|
|
||
| async getActiveStreams( |
There was a problem hiding this comment.
It is more efficient for historical/aggregated overviews to use subgraph.
However as per bounty request. we should also expose to read current live flow-rate. (using getFlowRate)
|
|
||
| /** | ||
| * Internal helper to manage SDK instances by environment efficiently | ||
| */ |
There was a problem hiding this comment.
Not all methods have corresponding hooks.
and the wrapper react-hook for the sdk is not exported.
- No
useSuperTokenBalancehook - No
useBalanceHistoryhook useStreamListparams do not exposefirst/skip
|
Hey, making sure you saw my latest comments @HushLuxe |
|
Thanks for the review@L03TJ3 |
|
Hi @L03TJ3... I have made an adjustment The SDK README now includes On the hooks/demo side, I added I also re-ran:
Happy to fix anything else quickly |
| data: balanceHistory, | ||
| isLoading: balanceHistoryLoading, | ||
| refetch: refetchBalanceHistory, | ||
| } = useBalanceHistory({ |
There was a problem hiding this comment.
there is no from/to timestamp supplied, and we don't seem to have support for a default range (maybe last 30 days)
So this demo will never show anyone balance (that is also true for sup balance)
There was a problem hiding this comment.
Alright @L03TJ3
Can I close this current pr and open another one?
There was a problem hiding this comment.
No, we need to have a pull-request where the history of discussion is clear.
re-opening new PR's makes it very hard to continue reviewing with having to cross-reference what was done/already reviewed commented on etc.
There was a problem hiding this comment.
I have updated the demo (App.tsx) to pass an explicit rolling 30-day window to useBalanceHistory()
Also:
- Updated the balance snapshot label to clearly say it’s showing the last 30 days.
- Improved the “Active Streams” section labels for clarity. The direction label now shows
From:when the connected wallet is receiving, andTo:when it’s sending
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: