refactor: viem in pdp verifier and warm storage #566
Conversation
- Set "noNamespace" rule to "off" in biome.json for improved flexibility. - Removed biome-ignore comments from various TypeScript files to enhance code clarity and maintainability.
- Deleted `DataSetCreationStatusResponse`, `FindPieceResponse`, and `PieceAdditionStatusResponse` interfaces from `server.ts` to streamline the codebase. - Removed validation utilities related to the deleted types from `validation.ts`, enhancing maintainability. - Updated exports in `index.ts` to reflect the removal of these types and utilities.
- Removed the requirement for `.js` extensions in imports from the Biome Linting section of AGENTS.md to reflect updated practices. - Clarified other linting rules to enhance developer understanding and maintainability.
- Changed the type of `dataSetId` in the `upload` command from `Number` to `BigInt` for better precision. - Updated the `dataSetId` assignment in the `pieces` command to directly use `group.dataSetId`, removing unnecessary conversion to `Number`.
- Updated `dataSetId` type in `addPiecesWithMetadataCapture` from `number` to `bigint` for improved precision. - Added `eth_fillTransaction` method to JSON-RPC handler and defined its error handling. - Introduced `addApprovedProvider` and `removeApprovedProvider` methods in warm storage options, along with their respective error handling in the call handler. - Added `terminateService` and `topUpCDNPaymentRails` methods to warm storage options, enhancing functionality and error management.
- Introduced `dataSetLive`, `getActivePieceCount`, `getActivePieces`, `getDataSetLeafCount`, `getDataSetListener`, `getDataSetStorageProvider`, `getNextPieceId`, and `getScheduledRemovals` functions to enhance data set management capabilities. - Each function includes options for specifying `dataSetId`, `address`, and additional parameters for pagination where applicable. - Implemented corresponding call functions to facilitate contract interactions with the PDP Verifier. - Enhanced documentation with examples for better developer guidance.
…s and data set metadata - Added `addApprovedProvider` and `removeApprovedProvider` functions to manage approved providers, including transaction handling and error management. - Introduced `getApprovedProviders`, `getAllDataSetMetadata`, and `getAllPieceMetadata` functions for retrieving metadata, enhancing data management capabilities. - Implemented corresponding call functions for contract interactions and included detailed documentation with examples for improved developer guidance. - Refactored existing code to utilize new functions, ensuring consistency and maintainability across the warm storage module.
- Implemented `metadataArrayToObject` function to convert metadata arrays into objects, enhancing data handling capabilities. - Added comprehensive unit tests for `metadataArrayToObject`, `datasetMetadataObjectToEntry`, and `pieceMetadataObjectToEntry` to ensure functionality and reliability. - Included tests for edge cases, such as empty inputs and exceeding metadata limits, to improve robustness.
- Updated `package.json` to include PDP Verifier types and default entry points for improved module accessibility. - Modified `tsconfig.json` to include the PDP Verifier source file, ensuring it is compiled and available for use.
…hook - Changed the argument type for `getAllPieceMetadata` in `useDataSets` from `BigInt(piece.id)` to `piece.id` for consistency with expected input. - Refactored `useServicePrice` to replace `servicePrice` with `getServicePrice`, updating the import and ensuring the hook returns the correct service price result type.
- Changed the type of `dataSetId` from `number` to `bigint` in the `addPieces` and `getDataSet` methods of `PDPServer`. - Updated the `dataSetId` type in multiple methods of `PDPVerifier` to `bigint`, including `dataSetLive`, `getNextPieceId`, `getActivePieceCount`, `getDataSetListener`, `getDataSetStorageProvider`, `getDataSetLeafCount`, `getActivePieces`, and `getScheduledRemovals`. - Refactored method implementations to remove unnecessary conversions and ensure consistency across the codebase.
…ever modules - Changed the type of `client` and `providerAddress` parameters from `string` to `Address` in `ChainRetriever`, `FilBeamRetriever`, and `SubgraphRetriever` classes. - Updated relevant method signatures to ensure type consistency across the retriever modules.
…elated interfaces - Updated the type of `providerId` from `number` to `bigint` in multiple methods of `SPRegistryService`, including `getProvider`, `getPDPService`, `providerHasProduct`, and others for consistency and improved precision. - Adjusted the `ProviderInfo` and `ProviderRegistrationInfo` interfaces to reflect the new `bigint` type for `id` and `payee` fields. - Ensured all relevant method signatures and internal logic are updated accordingly to maintain functionality.
- Updated the type of the `address` parameter in the `getProviderByAddress` method from `string` to `Address` for improved type safety and consistency with other modules. - Ensured that the method signature aligns with recent changes in the codebase regarding address handling.
- Refactored the WarmStorageService class to utilize the Client and Chain types for improved type safety and consistency. - Changed various method signatures to accept `bigint` for dataSetId and providerId, aligning with recent updates in the codebase. - Removed deprecated code related to ethers.Provider and contract instances, streamlining the service's implementation. - Enhanced metadata handling by integrating utility functions for retrieving dataset and piece metadata. - Updated internal logic to leverage multicall for efficient contract interactions, improving performance and reliability.
…ed consistency - Changed the type of `providerAddress` in the `download` and `getProviderInfo` methods from `string` to `Address` for better type safety. - Updated the `WarmStorageService` instantiation to use `connectorClient` instead of directly passing the provider address, enhancing clarity and maintainability. - Ensured method signatures align with recent updates in the codebase regarding address handling.
- Changed the type of `dataSetId` and `pieceId` from `number` to `bigint` across multiple methods in `StorageContext` and `StorageManager` for improved precision and consistency. - Updated method signatures to reflect the new types, ensuring alignment with recent changes in the codebase. - Adjusted related logic to handle `bigint` values appropriately, enhancing type safety and maintainability.
…rfaces - Updated various types in `types.ts` to enhance consistency and type safety, changing `string` to `Address` for address-related fields and `number` to `bigint` for dataset identifiers. - Adjusted method signatures and interface definitions to reflect these changes, ensuring alignment with recent updates in the codebase. - Improved overall maintainability and clarity of the type definitions within the SDK.
…te viem client - Changed various instances of `dataSetId`, `pieceId`, and other identifiers from `number` to `bigint` across multiple test files for consistency and improved type safety. - Refactored the instantiation of services to utilize the `viem` client instead of `ethers.Provider`, enhancing clarity and maintainability. - Updated assertions and method calls to align with the new types, ensuring all tests reflect the latest changes in the codebase.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
synapse-dev | 979fd9f | Commit Preview URL Branch Preview URL |
Jan 22 2026, 04:28 PM |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
hugomrdias
changed the title
|
|
||
| const removalsDeduped = Array.from(new Set(removals)) | ||
| const [data, ids, hasMore] = activePiecesResult | ||
| const removals = Array.from(new Set(removalsResult)) |
There was a problem hiding this comment.
Unfortunate result of the func -> funcCall pattern is that you don't get the nice post-processing, so you've now duplicated this de-dupe step, and I guess in other places you're having to do similar things. I'm not really sure how you'd solve for that without yet more functions but it does seem like a flaw for when we have nontrivial logic to follow. Here, for instance, you're saying that it could contain duplicates (can it?), that's important knowledge that a user of getScheduledRemovalsCall should probably know and ideally not have to think about.
Maybe at a minimum we should be documenting these details?
There was a problem hiding this comment.
they are documented, tells the user to do exactly this.
the pattern that im playing with is:
- full method call
getServicePrice - parsing output
parseGetServicePriceused in getServicePrice - multicall helper
getServicePriceCall, docs explains that the result will be unparsed and that he should callparseGetServicePrice
There was a problem hiding this comment.
parseGetServicePrice this is the extra function I was imagining for this, but it gets pretty tedious where that parsing is trivial; and will be you so consistent as to have one of these even where they are not needed? so the user has to know that one exists if they use a Call that needs post-processing?
|
|
||
| getViewContractAddress(): string { | ||
| return this._addresses.viewContract | ||
| return this._chain.contracts.storageView.address |
There was a problem hiding this comment.
I don't love this tbh, it doesn't give us the upgrade flexibility - it's a read-only contract that, when we change it it'll have to be updated in FWSS to point to a new address. Same with SessionKeyRegistry.
In theory it's not unreasonable the view contract is fixed because its tied to the API in here (see latest getPDPConfig breakage in the new one that's going to break Synapse), but it does remove a lot of flexibility. SessionKeyRegistry also if we ever want to point to a new one, will have to be updated in the SDK to interact with it. Perhaps that's OK and FWSS will have to use old + new, but it feels like we're limiting our options by hardwiring them.
There was a problem hiding this comment.
you only need to run the generate abi cmd to update, it will get all the addresses from FWSS.
whats you use case? when we want to change contract address even if theres no abi changes?
There was a problem hiding this comment.
yeah, for example, let's say we find a critical breaking change in sessionkeyregistry, it's not ugpradable, so we have to update the ref to it in FWSS, now we're all out of whack and the addresses are tied into the sdk version
The reverse case is also true though - change ABI in one of these and Synapse is now out of sync and is talking to something that it doesn't think it should be. But we could deal with that case on the contract side by just not doing breaking ABI when we do v1 and trying to stick to a non-breaking ABI rule. e.g. FilOzone/pdp#243 (comment) add a new method rather than change the rules of an old one.
I'm not going to block this PR on this btw, just opening up this discussion and registering this as a likely stack failure mode, or at least something that may cause problems for us in the future.
github-project-automation
bot
moved this from 📌 Triage
to ✔️ Approved by reviewer
in FOC
There was a problem hiding this comment.
noting that you're now missing out on all of this nice validation and just blindly accepting satisfies everywhere, perhaps we should still consider doing this, at least for SP communication
There was a problem hiding this comment.
i can put that back as zod schemas, but validating responses is something i normally dont do.
whats your reason for validation here?
There was a problem hiding this comment.
Cause trusting downstream is typically a bad idea, we really don't know what we're dealing with. Right now we control e2e but I expect that to change over time and for Curio to not be the only one we talk to, or be talking to different Curio versions, modified Curio instances.
Also, we have been caught with this a couple of times already, making assumptions that weren't quite right about Curio's response patterns.
(And generally I just think TypeScript makes JavaScript programmers lazy, we should be validating stuff much more often than this and TS gives you a false sense of things being right, in typed programming languages you'd always get an error when deserialising data into a well-typed struct).
…age modules - Removed commented-out code related to metadata handling in `metadata.ts` for clarity and maintainability. - Deleted deprecated methods in `WarmStorageService` that were no longer in use, streamlining the service's implementation. - Cleaned up unnecessary imports in `context.ts` to enhance code readability.
dataSetId,pieceId, and other identifiers fromnumbertobigintacross multiple test files for consistency and improved type safety.viemclient instead ofethers.Provider, enhancing clarity and maintainability.targeting #555