-
Notifications
You must be signed in to change notification settings - Fork 63
refactor(web3): Extract read operations into composable reads module #4335
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: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
💡 Enable Vercel Agent with $100 free credit for automated AI reviews |
|
Deployment failed with the following error: |
bae1122 to
1a359c6
Compare
| }): SpaceFactoryReads { | ||
| const { spaceFactoryAddress, publicClient } = args | ||
| const makeInstance = <Abi_ extends Abi>(abi: Abi_) => | ||
| getContract({ |
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 tried using getContract in bot framework to expose bot app contract functions, however I got stuck.
I couldn't make it compose .
const buildBot = <abi extends Abi>(customAbi: abi) => {
const app = getContract({
address: appAddress,
abi: [...simpleAppAbi, ...customAbi]
client,
})
// app.read <-- all the types was gone
}After looking for what should I do in wevm community, I found this reply from jxom:
jxom: Honestly, not using getContract is better for complex projects.
jxom: Just pass a central stateless contract config around to Viems contract actions (readContract, writeContract, etc).
Not sure if that's relevant feedback, but would like to share my experience (could be skill issue tbh)
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.
jxom is the goat behind viem.
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.
You need as const ABI for type inference.
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.
interesting...he also said they only added getContract b/c people are used to ethers OO. Ha, that is what I'm used to
but yeah like Shuhui said you probably just need type inference on the abi to get it working
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 had as const
Probably bc the Abi wasn't known at compile time.. but I was expecting at least the SimpleApp functions to show
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.
But you spreaded them.
miguel-nascimento
left a comment
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.
sharing some thoughts!
| } => { | ||
| const cacheManager = new CacheManager() | ||
|
|
||
| const readClient = createReadClient({ |
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.
nit: let user pass viem client to createReadApp instead.
it will grant more options / reusability
|
|
||
| export type ReadApp = ReturnType<typeof createReadApp> | ||
|
|
||
| export const createReadApp = (args: { |
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 a pretty generic name for a pretty specific app... we already have 2 dapps (or is it 3 now with the app registry?) and will probably get more - so can we make a pattern & naming convention here so that we can do the same thing everywhere?
- it's unclear to me why we need to pass a read app to the space dapp, can the space dapp just make a read app in it's constructor? what are the pros/cons there?
Description
This PR introduces a new reads/ module that extracts and reorganizes all read-only blockchain operations from SpaceDapp into a composable foundation. This is Phase 1 of a multi-phase effort to both migrate to viem and separate read and write operations, ultimately replacing SpaceDapp.
Migration Path
This PR completes Phase 1 of a planned migration:
Benefits
Changes
No functional changes to existing behavior
Created 18 new files organized into a layered, composable architecture:
Breaking Change: SpaceDapp constructor now requires a ReadApp parameter:
Removed Properties:
Result: Significantly simplified SpaceDapp (~467+ lines reduced), net -704 lines across the repository.
Deleted files (logic moved to reads/):
WalletLink refactored:
Checklist
- All existing tests updated and passing
- Tests verify proper integration of readApp with SpaceDapp