-
Notifications
You must be signed in to change notification settings - Fork 15
WIP refactor(ponder-sdk): define a package to consolidate Ponder-related ideas and functionality #1416
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
…ode/ponder-sdk`.
…s that are currently utilised
🦋 Changeset detectedLatest commit: 3cc7ea4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Awaits for Ponder application to be healthy before building the Indexing Status object
lightwalker-eth
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.
@tk-o Hey thanks for the updates here. Appreciate that this PR is still a draft. Reviewed and shared some early feedback 👍
| @@ -0,0 +1,3 @@ | |||
| # Ponder SDK | |||
|
|
|||
| This package is a set of libraries enabling smooth interaction with Ponder application and data, including shared types, data processing (such as validating data and enforcing invariants), and Ponder-oriented helper functions. | |||
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 package is a set of libraries enabling smooth interaction with Ponder application and data, including shared types, data processing (such as validating data and enforcing invariants), and Ponder-oriented helper functions. | |
| This package is a set of utilities for interacting with a Ponder app. |
| "@ensnode/ponder-sdk": minor | ||
| --- | ||
|
|
||
| Renames `@ensnode/ponder-metadata` package to `@ensnode/ponder-sdk`. |
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.
Do we need to make any changes to any of our devops scripts related to publishing packages to NPM or generating release notes?
Additionally, I imagine that we should include a reference to ponder-sdk in the root readme file of the monorepo the way we reference other packages 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.
There's no need to change any of our devops scripts.
On the second question, yes, that's a good point 👍
| (typeof PonderHealthCheckResults)[keyof typeof PonderHealthCheckResults]; | ||
|
|
||
| export class PonderClient { | ||
| #healthCheckResult: PonderHealthCheckResult | undefined; |
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.
Please note feedback I shared in another open PR for how all our clients should be stateless.
| @@ -1,5 +1,5 @@ | |||
| /** | |||
| * Ponder Metadata: RPC | |||
| * Ponder SDK: RPC | |||
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 don't feel good about the abstractions we're building here. It appears that we aren't building clean layers of abstractions and instead are breaking one layer into others in a way that breaks clear lines of responsibility and clean abstractions.
For example:
- Why should this package know anything about ENSIndexer?
- Where is the public client here coming from? I thought that we were just building a client library that wraps Ponder's APIs?
- Why is there any reference to
ponder:apihere?
For next steps, suggest creating a diagram showing each conceptual layer involved here. How are the layers of responsibility currently defined? How do you suggest we modify the layers of responsibility through this PR?
Suggest to create these diagrams and share with me before advancing other next steps to confirm alignment. Thanks 👍
| ): Promise<OmnichainIndexingStatusSnapshot> { | ||
| let metrics: PrometheusMetrics; | ||
| let status: PonderStatus; | ||
| await waitForPonderApplicationToBecomeHealthy; |
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.
Can you please document why this action is suggested to be taken here? Why is it important? What would happen if we didn't do this here?
| "description": "A utility library for interacting with Ponder application and data", | ||
| "license": "MIT", | ||
| "repository": { | ||
| "type": "git", |
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 should update the directory and homepage below.
| "viem": "catalog:" | ||
| }, | ||
| "devDependencies": { | ||
| "@ensnode/ensnode-sdk": "workspace:*", |
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.
Part of what's confusing me is how the ponder-sdk package would have a dependency on the ensnode-sdk package. Potentially ok with it but just struggling to get a clear mental model of the proposed strategy.
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 need to take define:
- types like
BlockRef,ChainId,UnixTimestamp, etc. - deserialization functionality, such that
deserializeBlockRef, to parse data coming from Ponder. - zod schema definitions, such that
makeChainIdStringSchemato build deserialization logic for Ponder response data model.
We can either import them from @ensnode/ensnode-sdk, or re-define them locally in the @ensnode/ponder-sdk package.
The @ensnode/ponder-sdk is only integrated in the ensindexer application. For that reason, it make sense to import listed dependencies from @ensnode/ensnode-sdk. We can make the @ensnode/ensnode-sdk to be a peer dependency for @ensnode/ponder-sdk. This way we'll ensure that @ensnode/ponder-sdk package remains minimal, but it requires @ensnode/ensnode-sdk package to be provided as well at the integration point (which in our case is ensindexer application).
packages/ponder-sdk/package.json
Outdated
| }, | ||
| "dependencies": { | ||
| "@ensnode/ensrainbow-sdk": "workspace:*", | ||
| "drizzle-orm": "catalog:", |
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.
Surprised to see drizzle as a dependency of the ponder-sdk package?
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.
Great catch! This must be a legacy config, I don't think it's needed now.
packages/ponder-sdk/package.json
Outdated
| "@ensnode/ensrainbow-sdk": "workspace:*", | ||
| "drizzle-orm": "catalog:", | ||
| "parse-prometheus-text-format": "^1.1.1", | ||
| "viem": "catalog:" |
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.
Should we make viem a peer dependency too?
| * ENSNode makes no use of that endpoint. | ||
| */ | ||
| public ready(): never { | ||
| throw new Error("Ponder `/ready` endpoint is not supported by this client."); |
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'm struggling to understand.
Suggested review order: commit-by-commit.
This PR is a substantial change to ENSNode with the following updates:
@ensnode/ponder-metadatapackage role has been renamed to@ensnode/ponder-sdkwith the goal of consolidating Ponder-related functionalities. This is a step towards achieving clean boundary between Ponder framework and the codebase of ENSNode components.Resolves #908