-
Notifications
You must be signed in to change notification settings - Fork 10
feat(packages/ensnode-react): connection manager #893
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: resolution-client
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
const addConnection = useMutation({ | ||
mutationFn: async ({ url }: AddConnectionVariables) => { | ||
// Validate the URL | ||
const validationResult = await defaultValidator.validate(url); |
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 if we took the validator object instance from the useConnections
hook params. This way, the client app, such as ENSAdmin, could precisely manage the ENSNode connection validation 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.
@tk-o I'll have a look at modifying this so you can pass in a validator.
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.
@notrab Appreciate this. Reviewed and shared priority feedback.
* ENSIndexer Public Configuration | ||
* Configuration data fetched from an ENSNode endpoint | ||
*/ | ||
export interface ENSIndexerPublicConfig { |
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.
Eeek! @tk-o @notrab Please several key issues here:
This type shouldn't be defined at a React-level (inside the ensnode-react
package). It should be defined inside ensnode-sdk
.
Appreciate your advice.
Also, @tk-o what's the status on getting everything ready with this interface and related validation logic?
*/ | ||
export interface ENSNodeValidator { | ||
/** | ||
* Validates an ENSNode endpoint URL and fetches its public configuration |
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 are we "validating an ENSNode endpoint URL"?
What matters is:
- Getting the public config of the ENSIndexer instance.
- Deserializing the returned public config (into a "rich" / non-JSON object) and validating all of its invariants using Zod.
I don't like the idea that this is called a "Validator" or has a "validate" function.
The responsibility for serializing / deserializing (which includes validating!) belongs in ensnode-sdk and not at the react level.
/** | ||
* Variables for adding a new connection | ||
*/ | ||
export interface AddConnectionVariables { |
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're mixing up too many ideas and responsibilities here.
There should be just a single connection. The idea of managing a list of multiple possible connections doesn't belong anywhere in ensnode-react. It belongs only in an app like ensadmin.
/** | ||
* Parameters for the useConnections hook | ||
*/ | ||
export interface UseConnectionsParameters { |
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 see my other comment above.
We're putting ideas in the wrong place. It seems many of the ideas here belong only in ENSAdmin and not in ensnode-react.
@@ -0,0 +1,120 @@ | |||
import type { ENSIndexerPublicConfig, ENSNodeValidator } from "../types"; | |||
|
|||
/** |
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're mixing up ideas and putting them in the wrong places.
- All API calls to ENSNode need to be in ensnode-sdk, not here in ensnode-react.
- All validation / serialization / deserialization of an ens indexer public config need to be in ensnode-sdk.
It's critical to get these details right.
``` | ||
|
||
## Connection Management Modes |
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.
Virtually no one (other than ourselves in ENSAdmin) will build "ENSNode connection picker" UX. We should focus on making it easy for the 99.999% who will want just a single connection for their app.
This PR adds the connection management we discussed that can be added to
ensnode-react
(#886) so we can replace what's in ENSAdmin with this library.