-
Notifications
You must be signed in to change notification settings - Fork 15
refactor(ensindexer): create a unified way to define a plugin #775
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
Conversation
…om function call params
Decouple handlers from plugins. At least for the eager type inference part.
This way, we decouple the plugin handlers definition from the plugin config definition. It allows breaking circular type inference.
🦋 Changeset detectedLatest commit: a1ce74d The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 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 Git ↗︎
|
…ildPlugin` function.
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 Thanks for the updates. I've shared many suggestions for improvement. I'm open to following up on these suggestions in other future PRs. Please make a special effort to ensure none of the suggestions shared are forgotten and all ultimately receive a response.
You're welcome to merge this PR when ready. Please decide for yourself if Matt or I should review this PR once more. Either option is ok from my perspective.
| /** | ||
| * Attach plugin's event handlers for indexing. | ||
| * | ||
| * Note: this function is called when the plugin is active. |
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 clarify the language here. It's not sufficiently clear or precise. There's too many ways to interpret this.
As I understand, this is what you're trying to say:
| * Note: this function is called when the plugin is active. | |
| * Note: this function is called if and only if the plugin is being activated. |
Is that right? Or are you trying to say something else?
| ...((chainId === 31337 || chainId === 1337) && { disableCache: true }), | ||
| } satisfies NetworkConfig, | ||
| }; | ||
| createPonderConfig( |
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.
Currently we're mixing together our plugin definitions with a lot of Ponder-specific details. It seems there should be a more simple design?
Why couldn't we completely remove explicit reference to Ponder things when we define a plugin, and instead define our own types for our plugins?
For example, imagine we completely separated the following into distinct layers:
- Defining an ENSIndexer plugin (using our own types).
- Taking an ENSIndexer plugin definition and converting it into a PonderConfig.
In layer (1) listed above, we might:
A. Completely remove the createPonderConfig function.
B. Add a function for getIndexedContracts that returns only the contracts value that is currently returned by createPonderConfig.
C. Add a function for getEventHandlers that returns the record type for mapping between (namespaced) contract names and event handler callback functions. Importantly: this wouldn't ever include an import or call to ponder. Please see my other more detailed feedback about this idea in another comment.
In layer (2) listed above, we might:
A. Take all the values returned by getIndexedContracts for all active plugins and..:
i. Identify the chains being requested for indexing, and then getting the rpcConfigs from the ENSIndexerConfig as needed to build the networks object needed by Ponder.
ii. Build the contracts object needed by Ponder.
Only layer (2) would then need all these complex looking type definitions that Ponder needs, right?
We could refer to Layer (1) as our "Plugin" layer and Layer (2) as our "PonderConfig" layer.
In other words, Layer (1) doesn't need to know about PonderConfigs, because Layer (2) takes the full responsibility for translating from our "Plugins" into the "PonderConfig" that we ultimately pass to Ponder.
| * | ||
| * @returns network configuration based on the contract | ||
| */ | ||
| export function networkConfigForContract<CONTRACT_CONFIG extends ContractConfig>( |
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.
Why do we need to use a type template here for CONTRACT_CONFIG? Seems we should be fine to remove the use of type templates here?
| * Builds a ponder#NetworksConfig for a single, specific chain. | ||
| * | ||
| * @param {number} chainId | ||
| * @param rpcConfigs a dictionary of RPC configurations, grouped by chainId |
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.
| * @param rpcConfigs a dictionary of RPC configurations, grouped by chainId | |
| * @param rpcConfigs a dictionary of RPC configurations, keyed by chainId |
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.
Sharing a few additional ideas for future follow up on enhancements to our plugin system.
| import { attachLineanamesPluginEventHandlers } from "@/plugins/lineanames/event-handlers"; | ||
| import { attachSubgraphPluginEventHandlers } from "@/plugins/subgraph/event-handlers"; | ||
| import { attachThreeDNSPluginEventHandlers } from "@/plugins/threedns/event-handlers"; | ||
| // Shared Resolver Handlers |
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 a future opportunity for us to strengthen the documentation for specifically what it means for handlers to be "shared". Ideally we could also connect this idea to why some handlers are are associated with namespaced contracts and others aren't.
| import { attachSharedResolverHandlers } from "@/plugins/shared/Resolver"; | ||
|
|
||
| // Subgraph Handlers | ||
| import { attachSubgraphNameWrapperEventHandlers } from "./subgraph/handlers/NameWrapper"; |
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 seems attractive to separate the following ideas:
- The definition of a mapping from an event name with a (namespaced) contract to an event handler function (just a named import for each event handler, not the implementation of those event handlers). This should never make any reference to
ponder.onor importponder. - The registration of the relationship defined in point (1) above with ponder using
ponder.on. This importsponderand callsponder.on. - The implementation of the event handlers defined in point (1) above. These don't import
ponder.
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 not sure an additional level of abstraction between plugins and ponder would help but we can explore what that would look like. also it's the usage of ponder.on that gives us the inferred types, so another level of abstraction will give us additional type wrangling to do that seems less productive
| * handlers. ponder.config.ts will call {@link attachPluginEventHandlers} to conditionally | ||
| * register a specific plugin's handlers with Ponder. | ||
| * | ||
| * NOTE: defined separate from plugin.ts to avoid possible circular dependencies |
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 would be nice if we could be more specific about exactly what is creating the circular dependencies. As I understand these are caused by our calls to ponder.on. Please see my other comment on this file suggesting to split related logic into 3 distinct layers.
| attachSubgraphRegistrarEventHandlers, | ||
| attachSubgraphRegistryEventHandlers, | ||
|
|
||
| // NOTE: shared Resolver handlers |
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 an additional comment on the desire to strengthen our docs and language related to the use of the word "shared" here.
Is it correct to assume that ideas here include how the "shared Resolver handlers" are fundamentally a per-chain concept rather than a per-registry / subregistry concept?
It would be great to better document these ideas.
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.
let's avoid documenting it in every possible location, perhaps we can write some page in the docs and just link to it at these callsites?
| ); | ||
|
|
||
| ponder.on(ns("BaseRegistrar:Transfer"), async ({ context, event }) => { | ||
| ponder.on(namespaceContract(pluginName, "BaseRegistrar:Transfer"), async ({ context, event }) => { |
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.
Hmm. it seems we're not actually namespacing contracts here. As I understand, we should rename namespaceContract to namespaceEvent. Is that fair?
One idea is to refine our strategy here so that we don't pass in event names such as "BaseRegistrar:Transfer" but instead separate params for "BaseRegistrar" (the contract name), and "Transfer" as the event name, and then pluginName as a 3rd param for the contract namespace.
Then this could be called namespaceEvent. Or perhaps better: make the contract namespace param optional and rename namespaceContract to something like buildEventName or something.
Advice appreciated.
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.
let's keep namespace to imply namespacing behavior, very important here
namespaceEvent or namespaceEventName seems fine
This PR addresses a challenge of defining ENSIndexer plugins with proper type inference.
Related issue:
Review suggestions:
Review order:
apps/ensindexer/src/lib/plugin-helpers.tsapps/ensindexer/src/lib/ponder-helpers.tsplugin-helpers.tsfileapps/ensindexer/src/plugins/index.tsapps/ensindexer/src/plugins/event-handlers.tsapps/ensindexer/src/plugins/basenames/plugin.ts&apps/ensindexer/src/plugins/basenames/event-handlers.tsapps/ensindexer/src/plugins/lineanames/plugin.ts&apps/ensindexer/src/plugins/lineanames/event-handlers.tsapps/ensindexer/src/plugins/subgraph/plugin.ts&apps/ensindexer/src/plugins/subgraph/event-handlers.tsapps/ensindexer/src/plugins/threedns/plugin.ts&apps/ensindexer/src/plugins/threedns/event-handlers.tsapps/ensindexer/ponder.config.tsapps/ensindexer/test/*docs/*Key changes:
basenamesplugin as an example: its handlers file must be present atapps/ensindexer/src/plugins/basenames/event-handlers.tsponder.config.tsfor each active pluginattachPluginEventHandlersbuildPluginfunction with automated type inferencebuildPluginfunctionponder.config.tsfile can access Ponder config object for each active plugin by calling itscreatePonderConfigmethod