-
Notifications
You must be signed in to change notification settings - Fork 15
feat: add synapse-core and change sdk to viem client #289
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 | 16f7b12 | Commit Preview URL Branch Preview URL |
Oct 10 2025, 01:51 PM |
34def1c
to
34b117f
Compare
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
…ve deprecated files - Updated package.json to use wireit for build, lint, and test scripts. - Added references to synapse-core in tsconfig.json. - Removed wagmi.config.ts and related ABI generation files. - Refactored Synapse class to utilize viem's Client for wallet and public client management. - Updated PaymentsService to remove ERC20Permit ABI usage and replace it with ERC20 ABI. - Enhanced test cases to align with new client structure and removed deprecated authentication methods. - Added new methods to access wallet and public clients in Synapse class.
- Added .wireit/ to .gitignore to exclude Wireit configuration files. - Updated biome.json to ignore packages/synapse-core/src/gen.ts for better build management. - Added wireit as a dependency in package.json for improved build, lint, and test scripts.
- Removed the line setting the script shell to bash in the GitHub Actions workflow for the Synapse SDK, streamlining the test execution process.
- Modified test commands in package.json to use a consistent pattern for test file matching in both node and browser environments.
…rkflow - Set the script shell to bash in the GitHub Actions workflow for the Synapse SDK to ensure compatibility during test runs.
- Modified test commands in package.json to use a more inclusive pattern for test file matching in both node and browser environments, ensuring all relevant tests are executed.
- Refactored the test command in package.json to utilize wireit for streamlined execution, while ensuring that both node and browser tests are run sequentially. This change enhances the consistency and efficiency of the testing process.
- Updated the GitHub Actions workflow for the Synapse SDK to set the script shell to bash directly in the test command, enhancing compatibility and simplifying the execution process.
- Modified test commands in package.json to use double quotes for file patterns, ensuring consistency and compatibility across different environments.
- Rearranged peerDependencies in synapse-core's package.json for clarity. - Removed unnecessary dependencies from synapse-sdk's package.json while maintaining required ones.
- Temporarily disabled the test execution step in the GitHub Actions workflow for the Synapse SDK to prevent tests from running during CI builds.
d93d285
to
16f7b12
Compare
} | ||
] | ||
}, | ||
"packages/synapse-core": { |
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.
needs the rest of the config copied down
- name: Install Dependencies | ||
run: pnpm install | ||
# - name: Run tests | ||
# run: pnpm -r --filter @filoz/synapse-core run test |
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.
? not ready yet?
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.
no tests in core yet
}, | ||
Glif: { | ||
name: 'Glif', | ||
url: 'https://www.glif.io/en', |
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.
this is not a block explorer, or did I miss a memo?
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.
it still is and imo one of the best for pending txs
*/ | ||
|
||
/** | ||
* Synapse SDK main entry point |
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.
maybe we should not do this but just use named exports exclusively here? we fell into a trap in current Synapse by making the main export a bucket-of-all-things which is terrible and I'd like to undo that, I don't think there's a good reason to do the same here is there?
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.
what i normally do is namespace export in index.ts all the logical groups and teach users in docs theres named entrypoints if they want to cherrypick.
In the SDK case is different cause if you use the Synapse class you already pulling most of the other modules but yes we need to improve.
Here i think you right it we should only have named exports for logical groups.
Would you remove the default entrypoint completely ? if not what would you put there ?
return new FallbackProvider(providers) | ||
} | ||
|
||
return new JsonRpcProvider(transport.url, network) |
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.
shouldn't we support websocket urls here?
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.
yes
abi: ERC20_WITH_PERMIT_ABI, | ||
}, | ||
payments: { | ||
address: generated.paymentsAddress['314'], |
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.
What are the implications of exporting this here? what if I want to run the stack with my custom suite of contracts for deployment?
I'm not against baking in official addresses, I just don't want to make it even more onerous to test against new deployments for some subset of these contracts.
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 think that since we still pull the dependency contract addresses off warmstorage that it still works by supplying the option; why why do the addresses help us in here? is there some adjacent functionality that viem gives us that makes this a good thing to do?
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.
this gives us simplicity, you just need chainId and with getChain
below you get everything you need addresses, abi, etc
its not onerous to test against any deployment:
- open wagmi.config.ts
- change address and git tag/commit to the specific contract
- save and
pnpm run generate-abi
- workerd | ||
|
||
patchedDependencies: | ||
viem: patches/viem.patch |
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.
funky, do we really need this? is there an attempt to upstream this so we're not stuck with jank?
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.
this only works locally, we cant have this, only here to validate that everything works.
we will need a custom privateKeyToAccount function and i will remove this
// If it's a JsonRpcProvider or WebSocketProvider, it's not a browser provider | ||
// These can sign locally with a wallet | ||
if (provider instanceof ethers.JsonRpcProvider || provider instanceof ethers.WebSocketProvider) { | ||
if (actualSigner.provider instanceof JsonRpcProvider) { |
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.
needs websocket check too?
I think I also might need some more help being convinced that this new logic is enough to get the answe we need in here
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 need help understanding the special case for metamask.
BrowserProvider is always a eip1193 provider and always has a request function, the problem here may be old school interfaces from ethers before eip1193 existed.
throw new Error(`Invalid network: ${String(network)}. Only 'mainnet' and 'calibration' are supported.`) | ||
} | ||
const chain = getChain(options.client.chain.id) | ||
const network = chain.id === mainnet.id ? 'mainnet' : 'calibration' |
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.
we no longer do a getFilecoinNetworkType
check here, do we have any confidence other than the config that we've been passed that we are actually talking to that network type? or are we choosing to not care and it's caveat emptor?
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.
devs must define the chains first and should use our chain definitions or viem's if they try to customize and mess up rpc urls its there problem, anyway viem/wagmi already checks the chain definition against the RPC so devs really need to work hard and customize chain definition and RPC url to have problems.
@rvagg any way after many attempts to convert ethers to viem theres no clean way at least for the local private key account to do it. The conversion methods in this PR are already going into internals and for PKs we would need a custom privateKeyToAccount to expose the PK and convert to ethers. Also internal local sign methods for ether are sync and viems are async its a mess. i think its best to park this for this week to avoid disruption. |
this will probably fix the metamask problems but need to confirm.