Upgrade to synapse 0.39.0. Closes #170#186
Upgrade to synapse 0.39.0. Closes #170#186juliangruber wants to merge 73 commits intoFilOzone:mainfrom
Conversation
|
@juliangruber we should update filecoin-pin first for sure and then pull in both filecoin-pin and synapse-sdk here at the same time |
|
Assigning to @SgtPooki to take over this PR, as per Slack thread |
| } as any, | ||
| }, | ||
| isActive: storageProvider.isActive, | ||
| pdp: { |
There was a problem hiding this comment.
waitForIpniProviderResults() from filecoin-pin expects PDPProviders as input. We here only have StorageProviders, from the deal object. We can fake a PDPProvider, it doesn't matter what we use as .pdp (apparently). Any advice @hugomrdias @rvagg?
There was a problem hiding this comment.
@juliangruber filecoin-pin uses the pdp provider details to build the expected multiaddr and then uses name and some other fields for logging/debug output. We can fake it, but we might need to shore those types up in filecoin-pin and here in Dealbot eventually, but I wouldn't take that on in this PR unless we need to
| const supportsPDP = !!info.products?.PDP; | ||
| // In order to support ipniIpfs, the provider must have PDP product | ||
| const supportsIpniIpfs = !!info.products?.PDP?.data?.ipniIpfs; | ||
| const isDevTagged = info.products?.PDP?.capabilities?.service_status === DEV_TAG; |
There was a problem hiding this comment.
this tag isn't exported by the PDPprovider type 🤔 Any advice @hugomrdias @rvagg?
There was a problem hiding this comment.
i don't know if we need "isDevTagged" anymore honestly.. we shouldn't be testing against them, and isActive/isEndorsed (whatever the endorsed flag is) should be enough for dealbot.. as long as devTagged SPs are never "isActive" then we should be good
|
@rjan90 could you add back the copilot review request? I want to see if there's anything valuable in there |
There was a problem hiding this comment.
Pull request overview
This PR upgrades the backend to Synapse SDK 0.39.0 (and filecoin-pin ^0.18.0) and updates Dealbot’s integration to match the new provider types, storage APIs, and upload progress callback/event naming.
Changes:
- Update Synapse SDK imports/usage (chain/account setup via
viem, newsp-registry/storageAPIs, provider model changes). - Rename and propagate upload progress events (
onUploadComplete→onStored,onPieceAdded/Confirmed→onPiecesAdded/Confirmed) and align schema fields (piece_*→pieces_*,region→location, IDs →bigint). - Add new DB migrations and update tests/docs/metrics DTOs to reflect the renamed fields and BigInt IDs.
Reviewed changes
Copilot reviewed 34 out of 35 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/checks/events-and-metrics.md | Updates event documentation to reflect renamed upload completion handler/event. |
| docs/checks/data-storage.md | Updates docs for renamed callback (onStored). |
| apps/backend/test/migrations.e2e-spec.ts | Adds new migrations to the integration test migration list and adjusts rollback steps. |
| apps/backend/src/wallet-sdk/wallet-sdk.types.ts | Updates wallet/provider types to new Synapse/filecoin-pin models and BigInt-centric structures. |
| apps/backend/src/wallet-sdk/wallet-sdk.service.ts | Refactors wallet services initialization and provider syncing for Synapse 0.39 APIs and BigInt IDs. |
| apps/backend/src/wallet-sdk/wallet-sdk.service.spec.ts | Updates tests for BigInt provider IDs and updated provider shape. |
| apps/backend/src/retrieval-addons/strategies/ipfs-block.strategy.ts | Switches provider URL access to the new pdp.serviceURL shape. |
| apps/backend/src/retrieval-addons/strategies/ipfs-block.strategy.spec.ts | Updates retrieval strategy tests and asserts new per-block TTFB output shape. |
| apps/backend/src/metrics/utils/check-metric-labels.ts | Changes metric label input providerId to bigint (still normalized to string labels). |
| apps/backend/src/metrics/services/failed-deals.service.ts | Updates failed-deal mapping to renamed piecesAddedTime field. |
| apps/backend/src/metrics/dto/provider-performance.dto.ts | Renames API field region → location. |
| apps/backend/src/metrics/dto/failed-retrievals.dto.ts | Updates provider ID type to bigint. |
| apps/backend/src/metrics/dto/failed-deals.dto.ts | Updates dataset ID type to bigint and renames pieceAddedTime → piecesAddedTime. |
| apps/backend/src/jobs/jobs.service.spec.ts | Updates job context expectations for BigInt provider IDs. |
| apps/backend/src/ipni/ipni-verification.service.ts | Switches expected provider model to PDPProvider (filecoin-pin) for IPNI verification. |
| apps/backend/src/ipni/ipni-verification.service.spec.ts | Updates IPNI verification test data for location. |
| apps/backend/src/dev-tools/dev-tools.service.ts | Aligns “active” flag checks with new provider shape (isActive). |
| apps/backend/src/deal/deal.service.ts | Refactors deal creation/upload flow for new storage context API and renamed upload progress callbacks; updates timing fields. |
| apps/backend/src/deal/deal.service.spec.ts | Updates deal service tests for new event names, BigInt IDs, and provider shape. |
| apps/backend/src/deal-addons/strategies/ipni.strategy.ts | Renames addon hook to onStored. |
| apps/backend/src/deal-addons/strategies/ipni.strategy.spec.ts | Updates test fixtures for BigInt IDs and location. |
| apps/backend/src/deal-addons/interfaces/deal-addon.interface.ts | Renames addon interface hook onUploadComplete → onStored. |
| apps/backend/src/deal-addons/deal-addons.service.ts | Renames and rewires addon execution from upload-complete to stored hook. |
| apps/backend/src/database/migrations/1761500000006-DataSetIdBigInt.ts | Adds migration changing deals.data_set_id type. |
| apps/backend/src/database/migrations/1761500000005-ProviderIdBigInt.ts | Adds migration changing storage_providers.providerId type. |
| apps/backend/src/database/migrations/1761500000004-RenameRegionToLocation.ts | Adds migration renaming region → location in storage_providers. |
| apps/backend/src/database/migrations/1761500000003-RenameEvents.ts | Adds migration renaming piece_*_time columns to pieces_*_time in deals. |
| apps/backend/src/database/entities/storage-provider.entity.ts | Updates entity fields/indexes for location and providerId: bigint. |
| apps/backend/src/database/entities/deal.entity.ts | Updates entity dataSetId to bigint and renames piece_*_time fields to pieces_*_time. |
| apps/backend/src/data-retention/data-retention.service.ts | Updates provider type references and BigInt provider IDs for metrics/logging. |
| apps/backend/src/data-retention/data-retention.service.spec.ts | Updates tests to pass BigInt provider IDs to metric label builder. |
| apps/backend/src/config/app.config.ts | Tightens private key typing to 0x${string} and updates env casting. |
| apps/backend/src/common/logging.ts | Updates log-context provider ID typing to bigint. |
| apps/backend/package.json | Bumps @filoz/synapse-sdk to 0.39.0, updates filecoin-pin, removes ethers. |
Comments suppressed due to low confidence (1)
apps/backend/src/common/logging.ts:109
DataSetLogContext.dataSetIdis still typed asnumber, butSynapse/DB dataset IDs are now treated asbigintelsewhere in this PR (e.g.,Deal.dataSetId?: bigint). This mismatch makes it easy to accidentally log/pass the wrong type and can reintroduce BigInt JSON serialization issues. UpdatedataSetIdtobigint | string(whichever is canonical) to keep log-context typing consistent.
export type DataSetLogContext = {
jobId?: string;
providerAddress: string;
providerId?: bigint;
providerName?: string;
dataSetId?: number;
dataSetIndex?: number;
metadata?: Record<string, string>;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pdp: { | ||
| // TODO | ||
| serviceURL: storageProvider.serviceUrl, | ||
| minPieceSizeInBytes: 0n, | ||
| maxPieceSizeInBytes: 0n, |
SgtPooki
left a comment
There was a problem hiding this comment.
overall LGTM but I left some comments/questions.
I know we're waiting for some input from synapse maintainers too. Please re-request review when you need me again
| @@ -293,7 +293,7 @@ export function loadConfig(): IConfig { | |||
| blockchain: { | |||
| network: (process.env.NETWORK || "calibration") as Network, | |||
| walletAddress: process.env.WALLET_ADDRESS || "0x0000000000000000000000000000000000000000", | |||
| walletPrivateKey: process.env.WALLET_PRIVATE_KEY || "", | |||
| walletPrivateKey: process.env.WALLET_PRIVATE_KEY as "0x${string}", | |||
There was a problem hiding this comment.
just a note that before we merge this we need to swap our old private key formats in dealbot infra repo to 0x format
There was a problem hiding this comment.
Makes sense! Where is the dealbot infra repo?
There was a problem hiding this comment.
| @Column({ name: "pieces_added_time", type: "timestamp", nullable: true }) | ||
| piecesAddedTime: Date; | ||
| @Column({ name: "pieces_confirmed_time", type: "timestamp", nullable: true }) | ||
| piecesConfirmedTime: Date; |
There was a problem hiding this comment.
we are only ever adding a single piece, do we need to change this?
There was a problem hiding this comment.
We don't need to, it keeps it tidy to match the entity name to the event name, otherwise this easily leads to confusion. The SDK event has pieces plural even if just a single piece was confiemd.
| @@ -29,7 +29,7 @@ export class StorageProvider { | |||
| isApproved!: boolean; | |||
|
|
|||
| @Column() | |||
| region!: string; | |||
| location!: string; | |||
There was a problem hiding this comment.
do we need to change region to location, or can we just have a comment indicating that it comes from .location value of X type?
| } | ||
| } | ||
|
|
||
| private async cleanupSynapseInstance(synapse: Synapse): Promise<void> { |
There was a problem hiding this comment.
do we no longer need to cleanup the synapse instance? I know @rvagg had some comments about this with the filecoin-pin upgrade to latest synapse.
There was a problem hiding this comment.
@rvagg's comment filecoin-project/filecoin-pin#343 (comment) was about process exit handing.
We could add back the cleanup logic, and add integration tests for module#close() properly exiting. I think that should be safer for a long running process like this, than just ignoring any opened resources. Will fix.
There was a problem hiding this comment.
Filecoin-pin doesn't export any cleanup logic any more.
I started with closing the WS connection, if there is one: 0a37f33
There was a problem hiding this comment.
We should also clean up any watch()ers, but Filecoin-pin doesn't use those, so I think we should be good. @rvagg should be best to confirm
| const supportsPDP = !!info.products?.PDP; | ||
| // In order to support ipniIpfs, the provider must have PDP product | ||
| const supportsIpniIpfs = !!info.products?.PDP?.data?.ipniIpfs; | ||
| const isDevTagged = info.products?.PDP?.capabilities?.service_status === DEV_TAG; |
There was a problem hiding this comment.
i don't know if we need "isDevTagged" anymore honestly.. we shouldn't be testing against them, and isActive/isEndorsed (whatever the endorsed flag is) should be enough for dealbot.. as long as devTagged SPs are never "isActive" then we should be good
|
@juliangruber : What are the next steps here? |
Targets FilOzone/synapse-sdk#555 (not installed because currently failing tests)Closes #170