Skip to content

Conversation

@tk-o
Copy link
Contributor

@tk-o tk-o commented Jan 22, 2025

Related to #52

Goals

  • easy exporting for ponder schema to support the upcoming @ponder/client feature
    • ponder plugins to be split into packages
      • cannot be done as ponder app requires all handlers to be defined within ponder's rootDir
      • rootDir is a single location, hence, having plugins in separate directories won't work
    • ponder-shema package to host ponder.schema.ts
  • ensnode-common package to host all primitives that can be used outside of ponder app context, for example, on the client side
  • opt-in to using the Subgraph-compatible middleware
    • the Subgraph middleware will have its own package
  • single directory apps to host all runnable modules
    • ensnode ponder app will be the first module here
    • ensnode app will import ponder schema & config from the ponder-bootstrap package
      • the ponder app expects these files to be loaded from a single rootDir — having ponder plugins in play, also place in the rootDir, prevents extracting ponder.config.ts & ponder.schema.ts into a separate package

tk-o added 9 commits January 22, 2025 21:21
Use abstraction to detach the subgraphGraphQL factory function from ponder
Use abstraction to detach the helpers.ts functions from ponder
Use abstraction to detach the ids.ts functions from ponder
Move the indexer source code into its dedicated place in the monorepo workspace
@tk-o tk-o changed the title Feat/monorepo feat(monorepo): split indexer into apps and packages Jan 23, 2025
Copy link
Member

@lightwalker-eth lightwalker-eth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tk-o Exciting to see this taking shape 👍 A few very small comments.

Copy link
Member

@lightwalker-eth lightwalker-eth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tk-o Hey nice updates 👍 Shared a few more small ideas.

@tk-o tk-o marked this pull request as ready for review January 23, 2025 18:54
@tk-o tk-o requested a review from a team as a code owner January 23, 2025 18:54
@tk-o
Copy link
Contributor Author

tk-o commented Jan 23, 2025

@shrugs, do you think, for this PR, that we should also work towards:

easy exporting for ponder schema to support the upcoming @ponder/client feature

?

I wanted to export ponder.schema.ts from one package and import it in another, but it gives me some drizzle type resolution errors, for example:

DTS Build start
../../apps/ensnode/ponder.schema.ts(4,14): error TS2742: The inferred type of 'domain' cannot be named without a reference to '.pnpm/[email protected]_@[email protected]_@[email protected][email protected][email protected][email protected]/node_modules/drizzle-orm/pg-core'. This is likely not portable. A type annotation is necessary.

I've found an explanation to that drizzle problem, but didn't yet figure how to solve it. Not sure how to progress about that.

What do you think?

Copy link
Collaborator

@shrugs shrugs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great work on this with the catalog definitions and the readmes and whatnot. some minor stuff i've noted from afar, but i'll check this out and start running it now

@@ -0,0 +1,5 @@
export const uniq = <T>(arr: T[]): T[] => [...new Set(arr)];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this file can/should live in the indexer, it doesn't need to be shared. and the file name helpers really only makes sense in that context.

// produces `blocknumber-logIndex` or `blocknumber-logindex-transferindex`
export const makeEventId = (event: Event, transferIndex?: number) =>
[event.block.number.toString(), event.log.logIndex.toString(), transferIndex?.toString()]
export const makeEventId = (blockNumber: bigint, logIndex: number, transferIndex?: number) =>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would prefer this file live in the indexer as well — i'm not sure we'll need to generate any of these ids outside of that context, and these ids are indexer-specific. there was a mention of maybe resolverId being used outside, but let's move all of these back to indexer for now, and only extract that one iff we actually need to do so.

Copy link
Member

@lightwalker-eth lightwalker-eth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tk-o Nice updates 👍 Shared a few more ideas with feedback 😄

Copy link
Collaborator

@shrugs shrugs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may not need to include vite resolution in root package.json?

@shrugs
Copy link
Collaborator

shrugs commented Jan 23, 2025

just noting for myself: i'd really like for env variable management to be exclusively at the root — can likely configure this with dotenv executing the script before running ponder if necessary

@shrugs
Copy link
Collaborator

shrugs commented Jan 23, 2025

also note to self: i worry this is too package-happy but the plugin framework can be extracted into its own package, initially private but publishable in the future

@tk-o
Copy link
Contributor Author

tk-o commented Jan 24, 2025

Thanks you so much for your support here, @shrugs 🚀

Copy link
Member

@lightwalker-eth lightwalker-eth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tk-o Hey super updates! 😄 Shared a few more small ideas with feedback. I think we're very close now!

- (possible) continued backwards compatibility with subgraph
- support indexing subset of data, i.e. only domains under parent node

## next up
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tk-o Hey if I remember correctly I saw an earlier request from Matt to remove the ideas in this "next up" section and transfer them into GitHub issues. I think it's best we action that outside of this PR and have Matt take the lead on that transition of ideas, since he's the original author of these and he'll understand them best.

Let's revert this change so that we retain these ideas as they are for now.

Sorry for the back and forth on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted. FYI @shrugs

Copy link
Member

@lightwalker-eth lightwalker-eth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tk-o Great updates! Just one more set of open questions based on your proposed CI updates.

Copy link
Member

@lightwalker-eth lightwalker-eth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tk-o Super updates. Replied to your question and small comments 👍

Copy link
Member

@lightwalker-eth lightwalker-eth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tk-o Awesome! PR now looks good to me 🚀 😄

@shrugs Passing PR review over to you 🙌

@shrugs
Copy link
Collaborator

shrugs commented Jan 24, 2025

if ensnode-utils is going to be internal, let's add private: true to the package.json and we can remove the extra stuff like the repo and homepage sections.

do we need the typecheck script in any of these packages?

we should standardize on whether we're going to org-prefix package names or not. i've done so for shared-configs (it's package name is @namehash/shared-configs, as an example. the benefit is that for internal packages where the name might be relatively generic ('shared-configs') when we import it we import it like @namehash/shared-configs which communicates that it's a namehash-specific module. if it's just named shared-configs a reader might assume that it's some npm package called shared-configs. up to you if we want to do this or not for the other packages, or whether we want to remove the prefix for shared-configs and go un-prefixed only.

Copy link
Collaborator

@shrugs shrugs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ready to merge, up to you on package name stuff, or whether to tackle separately. well done!

RPC_URL_59144: https://linea.drpc.org
RUNTIME_CHECK_TIMEOUT_SECONDS: 10
run: |
pnpm dev -vv &
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

start command here, yeah?

@lightwalker-eth
Copy link
Member

@shrugs Agreed on your feedback about org prefixing all our package names here 👍

Let's keep all our packages private for now, including ensnode-utils, but I like the idea of keeping fields there that will be relevant once we change that to public and setup everything for automatically publishing to NPM, etc.. But let's handle that in some future separate PR. This one is already big enough!

@shrugs
Copy link
Collaborator

shrugs commented Jan 24, 2025

if ensnode-utils is intended to be public in the future, that's reasonable and i agree - i had understood it was meant to stay private forever

@tk-o
Copy link
Contributor Author

tk-o commented Jan 24, 2025

I'm going to update the Custom Start Command on Railway for the indexing service.

From:

pnpm start --schema $RAILWAY_DEPLOYMENT_ID

to:

pnpm --filter ensnode start --schema $RAILWAY_DEPLOYMENT_ID

@tk-o tk-o merged commit 0ea1fd2 into main Jan 24, 2025
2 checks passed
@tk-o tk-o deleted the feat/monorepo branch January 24, 2025 18:32
@lightwalker-eth
Copy link
Member

@tk-o Sounds good! Thanks!

This was referenced Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants