refactor: pdp with viem/core #555
Conversation
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
synapse-dev | 0a20df0 | Commit Preview URL Branch Preview URL |
Jan 20 2026, 02:33 PM |
|
@hugomrdias this seems to be the fix for multiple issues in our backlog, can you add which this PR will close, just so that we keep track of which issue tickets are current in progress |
hugomrdias
force-pushed
the
hugomrdias/move-tests
branch
from
064e600 to
c04ee29
Compare
There was a problem hiding this comment.
how were the fixtures generated here? the originals, which have been removed in here, were from https://github.com/FilOzone/filecoin-services/blob/main/service_contracts/test/SignatureFixtureTest.t.sol and should match https://github.com/FilOzone/filecoin-services/blob/main/service_contracts/test/external_signatures.json
Are we testing the impl against itself in here?
| private _session: SessionKey | null = null | ||
| private readonly _multicall3Address: string | ||
|
|
||
| connectorClient: Client<Transport, Chain, Account> |
There was a problem hiding this comment.
private? or are we using this publicly?
There was a problem hiding this comment.
also can we have a comment for this? what does "connector" mean in this context? why have you chosen this name
There was a problem hiding this comment.
"walletClient", "signerClient", "writeClient"?
There was a problem hiding this comment.
re public/private, I see a couple of synapse.connectorClient uses so I suppose that's intentional, is there any reason to make this read-only, or put it behind a getter?
There was a problem hiding this comment.
its the viem/wagmi naming, if it connects to an account/wallet its a connector client if its a public client (just reads) its just client or publicClient.
connectClient -> Client<Transport, Chain, Account>
client -> Client<Transport, Chain>
whats your preference here? getter? AFAIK readonly and even private are just hints to TS they dont transpile to anything enforcing it.
i normally write js with ts jsdocs, so its private fields and accessors for private and read-only but in native typescript we should set a recomendation.
There was a problem hiding this comment.
👍 if this is standard naming then it's fine by me, I'm mostly happy to defer to whatever is idiomatic
wrt private - yes, in JS it (mostly) doesn't matter, but since we're seeing more people use TS these days these are good triggers for discouraging bad patterns, when you have to (thing as any).foo that should either be in tests or be a warning that maybe you're not using it right, so with that lens, what's the right thing here? should devs be expected to fish around in a synapse instance to figure out what it's using and then reuse that themselves or even mutate it without consequence?
I think I'd lean to being more restrictive until someone comes and makes a case for lessening the restriction or we find that we need it, if we're accessing it internally then I guess it needs to be public.
There was a problem hiding this comment.
sorry, not a real answer, just thoughts, I don't feel too strongly here and don't want to make a big deal of it, use your judgement
There was a problem hiding this comment.
im keeping the TS private and readonly for now to reduce changes, theres a lot already.
as for the client naming im gonna go with the viem naming
- publicClient for just reads
- walletClient for reads and write
connectorClient is just used in wagmi ( which is higher level and react ) because it comes from the "connector" but its just a walletClient as far viem is concerned
i will change it in a follow up PR.
|
First pass review done, comments left in line, no major objections from me |
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
- Added `datasets-create` command for creating datasets. - Introduced `datasets-terminate` command for terminating datasets. - Implemented `pieces-upload` command for uploading files to datasets. - Updated `datasets` command to include block number information. - Added `debug` flag to global flags for enabling debug mode. - Introduced utility functions for handling cancellation and generating hash links. - Removed deprecated `dataset-terminate` command. - Updated package dependencies to include new libraries.
- Added `asChain` function to convert viem chains to Filecoin chains. - Introduced `DownloadPieceError` for improved error handling during piece downloads. - Updated `createDataSetAndAddPieces` to streamline dataset creation and piece addition. - Refactored `signAddPieces` and `signCreateDataSet` to support optional metadata. - Added utility functions for handling streams and async iterables. - Updated package dependencies, including adding `chai` and `@types/chai` for testing. - Introduced new tests for piece utilities and random functions to ensure reliability.
- Updated `pollForDataSetCreationStatus` to `waitForDataSetCreationStatus` for better readability. - Changed `pollForAddPiecesStatus` to `waitForAddPiecesStatus` to maintain consistency in naming conventions.
- Replaced `Piece.MAX_UPLOAD_SIZE` with `SIZE_CONSTANTS.MAX_UPLOAD_SIZE` for consistency. - Updated function calls from `pollForAddPiecesStatus` to `waitForAddPiecesStatus` in `StorageContext`. - Changed `pollForDataSetCreationStatus` to `waitForDataSetCreationStatus` for clarity. - Removed outdated test files for `piece` and `rand` utilities to streamline the codebase.
- Added `AtLeastOnePieceRequiredError` for better error management when no pieces are provided. - Updated `CreateDataSetOptions` and `AddPiecesOptions` to include new optional fields: `clientDataSetId` and `recordKeeper`. - Refactored `createDataSet` and `addPieces` functions to utilize the new options and improve signature handling. - Renamed `pollForDeletePieceStatus` to `waitForDeletePieceStatus` for consistency with naming conventions. - Enhanced tests for typed data to cover new functionalities and ensure robustness.
- Removed the `PDPAuthHelper` class to simplify authentication processes. - Updated `PDPServer` to directly utilize the `Client` from `viem` for improved integration. - Refactored `StorageContext` to eliminate dependencies on the removed `PDPAuthHelper`, enhancing clarity and maintainability. - Adjusted metadata handling to use `MetadataObject` for consistency across the codebase. - Removed outdated tests related to `PDPAuthHelper` and updated existing tests to reflect the new structure.
- Updated `pollForDeletePieceStatus` to `waitForDeletePieceStatus` to align with naming conventions established in previous commits.
- Updated comments in `sign-create-dataset.ts` and `data-sets.ts` to specify that `clientDataSetId` refers to a nonce. - Removed commented-out console log in `datasets.ts` for cleaner code.
- Bumped versions of `@astrojs/starlight` and `astro` in `package.json` for improved functionality and security. - Removed outdated sections from `components.mdx` related to `PDPAuthHelper` and `PDPServer` to streamline documentation and focus on current usage.
hugomrdias
force-pushed
the
hugomrdias/move-tests
branch
from
7acbefa to
0a20df0
Compare
-Removed the PDPAuthHelper class to simplify authentication processes.
-Updated PDPServer to directly utilize the Client from viem for improved integration.
-Refactored StorageContext to eliminate dependencies on the removed PDPAuthHelper, enhancing clarity and maintainability.
-Adjusted metadata handling to use MetadataObject for consistency across the codebase.
-Removed outdated tests related to PDPAuthHelper and updated existing tests to reflect the new structure.
closes #516