refactor: viem in payments #569
Conversation
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ❌ Deployment failed View logs |
synapse-dev | 3f9f831 | Jan 27 2026, 01:13 PM |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
hugomrdias
force-pushed
the
hugomrdias/more-pdp
branch
from
979fd9f to
e2eb979
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.
…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.
- Added comprehensive documentation for the Synapse Core library, detailing interactions with Filecoin Onchain Cloud smart contracts. - Implemented new features in the payments module, including deposit, withdrawal, and operator approval functionalities. - Updated existing methods to utilize `contractAddress` instead of `address` for improved clarity and consistency. - Introduced new test cases for accounts, deposits, and operator approvals to ensure robust functionality and reliability. - Refactored various components to enhance maintainability and type safety, including the integration of `bigint` for identifiers.
…mprove allowance handling - Refactored PaymentsService to utilize the Client and Chain types for enhanced type safety and consistency. - Updated method signatures to accept `bigint` for identifiers, aligning with recent changes in the codebase. - Changed allowance handling to use `rateUsage` and `lockupUsage` instead of `rateUsed` and `lockupUsed` for clarity. - Simplified deposit and withdrawal methods to improve readability and maintainability. - Updated tests to reflect changes in method signatures and ensure robust functionality.
- Added "ignoreUnknown" option to biome configuration for improved handling of unknown files. - Simplified linting commands across multiple package.json files by removing unnecessary flags, ensuring a more streamlined linting process. - Updated pre-commit hooks to utilize the new linting command format for consistency.
- Removed unused AllowanceDialog and DepositDialog components from PaymentsAccount for improved clarity and maintainability. - Updated import statements to reflect the removal of unused hooks. - Changed variable names in StorageMenu to use `rateUsage` and `lockupUsage` for consistency. - Deleted the config.ts file as it was no longer needed. - Updated package.json to remove the p-retry dependency.
…erator approvals - Renamed `owner` parameter to `address` in `accounts` and `operatorApprovals` functions for consistency and clarity. - Updated related method calls and test cases to reflect the new parameter naming. - Introduced optional `blockNumber` parameter in `accounts` for enhanced functionality.
- Consolidated imports from `@filoz/synapse-core/pay` for improved clarity. - Updated `UseAccountInfoProps`, `UseOperatorApprovalsProps`, `UseDepositProps`, and `UseWithdrawProps` to use new types from the `accounts`, `operatorApprovals`, `deposit`, and `withdraw` modules. - Renamed parameters for consistency, changing `address` to `owner` in relevant functions. - Adjusted query keys and mutation functions to reflect the new parameter names and types, enhancing maintainability.
- Renamed `owner` to `address` in `PaymentsService` methods for consistency with recent changes. - Updated `chai` dependency to version `^6.2.1`. - Removed the `sdk-version.ts` file as it was no longer needed, streamlining the codebase.
- Added new ignore patterns in knip.jsonc for generated files and specific directories to streamline the build process. - Enhanced workspace configurations for synapse-sdk, synapse-react, synapse-core, and synapse-playground to improve entry points and ignore files. - Fixed formatting in devnet.md for better readability in the documentation.
- Introduced "sideEffects": false in package.json to optimize the build process by enabling tree-shaking for unused exports.
- Moved ERC20-related functions into a dedicated `erc20` directory for better organization. - Updated import paths across the codebase to reflect the new structure. - Added new `approve` functionality to handle ERC20 token approvals, enhancing the module's capabilities. - Improved documentation and examples for ERC20 operations to ensure clarity and usability.
- Replaced ERC20BalanceOptions and ERC20BalanceResult with balance.OptionsType and balance.OutputType for improved type consistency. - Updated UseApproveAllowanceVariables and UseApproveAllowanceProps to utilize new types from the erc20 module, enhancing clarity and maintainability. - Adjusted function implementations to align with the updated type definitions.
- Replaced approveAllowance method with approve for ERC20 token approvals to streamline the approval process. - Simplified approval transaction handling by using approveSync, enhancing clarity and reducing complexity. - Updated related callback options to ensure proper transaction confirmation handling.
- Reorganized entry points in knip.jsonc for the synapse-core package to specify individual source files instead of a wildcard pattern. - Improved clarity and maintainability by explicitly listing all relevant source files for better build process management.
- Added event encoding for ERC20 approval callbacks in PaymentsService tests to improve accuracy in simulating transaction receipts. - Introduced new utility functions for encoding ABI parameters and event topics, enhancing test clarity and maintainability. - Updated mock server responses to include transaction receipt details for better testing of deposit callbacks.
- Updated various properties and method return types from string to Address for better type safety and consistency. - Modified Synapse, SynapseOptions, StorageInfo interfaces, and WarmStorageService methods to utilize the Address type. - Enhanced clarity and maintainability by ensuring uniformity in address handling throughout the codebase.
- Updated JSDoc comments for the useDeposit function to enhance clarity by linking to the UseDepositProps interface. - Removed redundant parameter descriptions to streamline documentation and improve readability.
- Changed the type of tokenAddress from string to Address for improved type safety and consistency across the codebase. - This update aligns with recent standardization efforts for address types in the Synapse SDK.
…sistency - Refactored payment operations examples to utilize the `viem` library for wallet client creation and transaction handling, enhancing clarity and modernizing the codebase. - Changed variable types from `number` to `bigint` for `railId` in multiple instances to ensure type consistency. - Updated allowance calculations in storage costs documentation to use `approval.lockupUsage` and `approval.rateUsage` for improved clarity and accuracy. - Enhanced overall documentation readability and maintainability by standardizing code examples and improving type safety.
hugomrdias
force-pushed
the
hugomrdias/payments
branch
from
192658e to
703b40d
Compare
| "main": "src/index.ts", | ||
| "scripts": { | ||
| "lint": "tsc && biome check --no-errors-on-unmatched .", | ||
| "lint": "tsc && biome check --fix .", |
There was a problem hiding this comment.
you sure you want to auto-fix in these package.json changes? up to you for examples and playground, glad you're not proposing it on the root package.json tho
There was a problem hiding this comment.
yep im good with it, useful for llms output to be cleaner without warning or unordered imports.
why would you do it ?
There was a problem hiding this comment.
tell the LLM to lint:fix by default if it wants to do the double action, then we leave the plain "lint this code" option that reports back problems to be dealt with so both paths are retained. Then you can lint in CI, and lint:fix (or lint) in dev. I definitely don't want lint:fix in CI, accidentally or not because then it's going to miss lint errors because they'll get fixed. We currently use pnpm run lint in there, which is good, but now that becomes auto-fix, which is bad.
There was a problem hiding this comment.
Check in a symlink to this from CLAUDE.md, also maybe consider deduping some content with the parent AGENTS.md since there's overlap? The detail in this is good but some of the basics should already be in ../.
| const balance = await ERC20.balance(this._client, { | ||
| address: spender, | ||
| }) |
There was a problem hiding this comment.
I think this might be wrong? balance includes a multicall for allowance but that looks at:
{
address: token,
abi: erc20Abi,
functionName: 'allowance',
args: [options.address, chain.contracts.payments.address],
},
so spender doesn't get passed through. So we replace usdfcContract.allowance(signerAddress, spender) with usdfcContract.allowance(signerAddress, filecoinPayAddress) here I think.
There was a problem hiding this comment.
we are saying options.address = spender
allowance args are [options.address, payments.address] its correct
or spender here is Filecoin Pay? if yes we need to the action payments is hardcoded needs an option for it
There was a problem hiding this comment.
Yeah, filecoin pay is the default spender of ERC20 funds. Description of the method is "Check the current ERC20 token allowance for a spender".
Previous version:
const currentAllowance = await usdfcContract.allowance(signerAddress, spender)Says: How much allowance have you (wallet) granted the spender (filecoin pay by default) to spend on your behalf.
With your ERC20 balance() doing args: [options.address, chain.contracts.payments.address] and options.address = spender, we are saying "how much has this spender allowed Filecoin Pay to spend on its behalf.
However, this was all designed before synapse-core and when this was a more generic interface to ERC20. We could drop the spender argument and just make the description "Check the current ERC20 token allowance for Filecoin Pay" and if you want generic ERC20 stuff then DIY or use synapse-core. I'm fine with that.
|
|
||
| const depositTx = await paymentsContract.deposit(this._usdfcAddress, depositTo, depositAmountBigint, txOptions) | ||
| const depositTx = await Pay.deposit(this._client, { | ||
| amount, |
There was a problem hiding this comment.
options has a to, and Pay.deposit takes a to but we ignore it here, unintentional?
| /** | ||
| * Information about a payment rail | ||
| */ | ||
| export interface RailInfo { |
There was a problem hiding this comment.
should we just be reexporting types from synapse-core when they are duplicates like this one?
There was a problem hiding this comment.
yes i will clean that up in a follow up
| lockupUsed: bigint | ||
| maxLockupPeriod: bigint | ||
| }> { | ||
| async serviceApproval(service: Address, token: TokenIdentifier = TOKENS.USDFC) { |
There was a problem hiding this comment.
return type missing from here now, intentional?
| 'PaymentsService', | ||
| 'settle', | ||
| `Failed to settle rail ${railIdBigint.toString()} up to epoch ${untilEpochBigint.toString()}`, | ||
| `Failed to settle rail ${railId.toString()} up to epoch ${untilEpoch?.toString()}`, |
There was a problem hiding this comment.
bit of a problem here now you're deferring to Pay.settleRail to deal with the untilEpoch===undefined case, it's going to be empty here. How about you just duplicate that options.untilEpoch ?? in here as well, it'll still only be done once because settleRail will get a value, but we also get to have nice msgs here.
| const currentEpoch = await getBlockNumber(this._client, { | ||
| cacheTime: 0, | ||
| }) |
There was a problem hiding this comment.
now we always fetch this, even if it's not necessary, and we don't use it for the error msg below
packages/synapse-core/src/warm-storage/get-all-piece-metadata.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
fwiw I'm not really reviewing the synapse-react packages, maybe @nijoe1 would like to take a pass at reviewing the changes to synapse-react in the big viem PRs?
There was a problem hiding this comment.
or @juliangruber, do you have much react experience?
- Refactored the ThemeProvider component to include the useTheme hook, improving accessibility to theme context. - Updated documentation in accounts and getRailsForPayerAndToken functions to clarify output types and contract addresses. - Modified deposit function documentation to specify the Filecoin Pay contract for better accuracy. - Adjusted constants for lockup period to utilize DEFAULT_LOCKUP_DAYS for improved clarity and maintainability. - Enhanced getAllPieceMetadata documentation to specify the FilecoinWarmStorage contract address, ensuring consistency in terminology.
* feat: add github link to filecoin cloud (#525) * refactor: move FilBeam domain to chain configuration Replace hardcoded chainId check in createPieceUrl with chain configuration lookup. Add filbeam.retrievalDomainName to Chain interface and configure it for mainnet, calibration, and devnet. Co-Authored-By: Claude Code <noreply@anthropic.com> Signed-off-by: Miroslav Bajtoš <oss@bajtos.net> * feat(example): Devnet Endorsements (#568) add ENDORSEMENTS_ADDRESS env config to utils/example-storage-e2e.js * fix(example): ENDORSEMENTS_ADDRESS identifier (#572) chore: fix identifier * refactor: rename retrievalDomainName to retrievalDomain --------- Signed-off-by: Miroslav Bajtoš <oss@bajtos.net> Co-authored-by: Abhivansh <31AbhivanshJ@gmail.com> Co-authored-by: William Morriss <wjmelements@gmail.com>
|
@hugomrdias in 3f9f831 (#570), I have accidentally merged in the latest |
|
TypeScript checks are failing. Claude Code claims it's caused by having two versions of viem in the monorepo:
|
Uh oh!
There was an error while loading. Please reload this page.