-
Notifications
You must be signed in to change notification settings - Fork 19
feat: Add API for querying remaining egress #430
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
synapse-dev | 8be35a1 | Commit Preview URL Branch Preview URL |
Nov 13 2025, 06:42 PM |
| ) | ||
| } | ||
|
|
||
| const data = (await response.json()) as { cdnEgressQuota: string; cacheMissEgressQuota: string } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, sure, but also maybe it's not this type?
elsewhere in the SDK we do actual validation, not just type assertion, it shouldn't be hard to add an object check and two string typeof's in here, even just make it a function that becomes a type guard and throws appropriately if it's not what you expect; cause you're doing some BigInt down below and those could fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 2c8d4f7
packages/synapse-sdk/src/synapse.ts
Outdated
| } | ||
|
|
||
| // Create FilBeamService | ||
| const filbeamService = await FilBeamService.create(provider) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should change this to just take network, not provider because that's all you're using it for. Then it doesn't need to be async, you can just use a constructor instead of the async factory, and you don't have yet another getNetworkType call in the Synapse initialisation path (it should be cached, but we don't have a guarantee of that). Nice and fast to get this set up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to keep the create signature similar to other services but since we're not using the provider for anything else I think you made a fair point. Fixed in 2c8d4f7
- Validate data set stats response from Filbeam stats API - Change create signature to accept network instead of provider
bajtos
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also update the relevant documentation, please. Show Synapse SDK users how to use this new API.
I am not sure if the current docs mention the withCDN option at all. If it does not, then let's create a follow-up issue to address the problem of the missing docs holistically.
Great point! I've generated some docs that you can preview here |
| * const service = FilBeamService.create('mainnet') | ||
| * ``` | ||
| */ | ||
| static create(network: FilecoinNetworkType, fetchImpl?: typeof fetch): FilBeamService { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't bother with this, the only reason we use a factory in other places is to deal with async initialisation, but where you only have sync you really just need a constructor, so you've now got two constructors here
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
…:FilOzone/synapse-sdk into feat/api-to-query-remaining-egress-stats
Extends existing Synapse SDK API with FilBeam service allowing for querying the remaining data set egress.
Closes #392