-
Notifications
You must be signed in to change notification settings - Fork 16
refactor(ensapi): apply operation result pattern #1545
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: feat/ensnode-result-type
Are you sure you want to change the base?
Changes from 20 commits
e80b9f4
56aa797
902d556
412e722
02fbeb3
ac5bb11
1cd4b10
a649edd
d6575b5
1246ce7
2d86cc2
6565097
159d210
fe3ffcc
8ea9155
229d8d8
35fa0b7
50b5e18
f5d75f7
d4261da
59f3700
9b46b51
3c4b632
13d5336
ad74cb7
8526bfd
a01108d
b64544c
aef406a
4f1302e
6395275
5768d6b
a4cb033
860d697
42c6d24
0cb64fd
bf40907
3cb3cf6
3e58e07
1b33cad
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -153,19 +153,12 @@ export function DisplayRegistrarActionsPanel({ | |
| <CardContent className="max-sm:p-3 max-sm:pt-0 flex flex-col gap-4"> | ||
| <p>The Registrar Actions API on the connected ENSNode instance is not available yet.</p> | ||
| <p> | ||
| The Registrar Actions API will be available once the omnichain indexing status reaches | ||
| one of the following: | ||
| The Registrar Actions API will be available once the omnichain indexing status becomes{" "} | ||
| <Badge variant="secondary"> | ||
| {formatOmnichainIndexingStatus(registrarActions.supportedIndexingStatusId)} | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm worried that our client-code is interpreting this value as a strict enum, rather than as an arbitrary string. It's super important we put a high priority on these distinctions.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you please expand on that note? Looking at export function formatOmnichainIndexingStatus(status: OmnichainIndexingStatusId): string {
const [, formattedStatus] = status.split("-");
return formattedStatus;
}Did you mean its input parameter type should be export function formatOmnichainIndexingStatus(status: string): string {
const [, formattedStatus] = status.split("-");
return formattedStatus;
}
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tk-o Your question here is about UI-level concerns. However the main idea I'm trying to flag here is about data-model level concerns in relation to change management. Here's a principle that should influence all our work: avoid breaking changes to our APIs (as soon as we can achieve the first "stable" API release). Therefore, we must work hard now to design the first "stable" API release so that we can avoid breaking changes as we continue iterating in the future! For any enum, we must thoughtfully consider future change management:
Use of option 1 should be VERY selective and deliberate. It should never be an accidental oversight. Use of option 2 requires that we explicitly prepare clients for gracefully handling new enum values that they won't recognize because they come from a future server version. Do you have any questions on the above? What do you recommend for this
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lightwalker-eth Yes, it's likely some enums will have to be extended to cover new cases. This may lead to situation where client-side code does not recognize a newly added enum variant included in server response. I would go ahead with option 1. As I proposed during our last team call, I think the client should be able to pick which version of the API response schema (i.e. For example:
As for option 2, if we wanted to use it just for enums, we would need to make the client app to handle the "unrecognized" enum variants. In other words, the client app would have to know which variants of the given enum are "recognized", so all other variants could be considered "unrecognized". If we went down this way, we would have to wrap all enum values in some data wrapper type to allow expressing a generic "unrecognized" variant, so the client app could handle it accordingly. |
||
| </Badge> | ||
| . | ||
lightwalker-eth marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| </p> | ||
tk-o marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| <ul> | ||
| {registrarActions.supportedIndexingStatusIds.map((supportedStatusId) => ( | ||
| <li className="inline" key={supportedStatusId}> | ||
| <Badge variant="secondary"> | ||
| {formatOmnichainIndexingStatus(supportedStatusId)} | ||
| </Badge>{" "} | ||
| </li> | ||
| ))} | ||
| </ul> | ||
| </CardContent> | ||
| <CardFooter className="gap-6"> | ||
| <Button asChild> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| import { NamedRegistrarAction, OmnichainIndexingStatusId } from "@ensnode/ensnode-sdk"; | ||
| import { NamedRegistrarAction, OmnichainIndexingStatusIds } from "@ensnode/ensnode-sdk"; | ||
|
|
||
| export const StatefulFetchStatusIds = { | ||
| /** | ||
|
|
@@ -61,7 +61,9 @@ export interface StatefulFetchRegistrarActionsUnsupported { | |
| */ | ||
| export interface StatefulFetchRegistrarActionsNotReady { | ||
| fetchStatus: typeof StatefulFetchStatusIds.NotReady; | ||
| supportedIndexingStatusIds: ReadonlyArray<OmnichainIndexingStatusId>; | ||
| supportedIndexingStatusId: | ||
| | typeof OmnichainIndexingStatusIds.Completed | ||
| | typeof OmnichainIndexingStatusIds.Following; | ||
|
||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| import { useENSNodeConfig, useRegistrarActions } from "@ensnode/ensnode-react"; | ||
| import { | ||
| getChainIndexingConfigTypeId, | ||
| IndexingStatusResponseCodes, | ||
| RegistrarActionsOrders, | ||
| RegistrarActionsResponseCodes, | ||
|
|
@@ -27,7 +28,7 @@ const { | |
| hasEnsIndexerConfigSupport, | ||
| hasIndexingStatusSupport, | ||
| requiredPlugins, | ||
| supportedIndexingStatusIds, | ||
| getSupportedIndexingStatus, | ||
| } = registrarActionsPrerequisites; | ||
|
|
||
| /** | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A key goal of our work here is to fully remove the need for the whole "use stateful registrar actions" concept. We should just have a "use registrar actions" that makes a single stateless API request. The client can then appropriately handle errors associated with insufficient indexing status or a config that won't support the API request.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree on the goal, but this PR is not aiming to update the client code. It will be done in a follow-up PR to reduce the PR scope. |
||
|
|
@@ -51,10 +52,12 @@ export function useStatefulRegistrarActions({ | |
| ) { | ||
| const { ensIndexerPublicConfig } = ensNodeConfigQuery.data; | ||
| const { omnichainSnapshot } = indexingStatusQuery.data.realtimeProjection.snapshot; | ||
| const chains = Array.from(omnichainSnapshot.chains.values()); | ||
| const configTypeId = getChainIndexingConfigTypeId(chains); | ||
|
|
||
| isRegistrarActionsApiSupported = | ||
| hasEnsIndexerConfigSupport(ensIndexerPublicConfig) && | ||
| hasIndexingStatusSupport(omnichainSnapshot.omnichainStatus); | ||
| hasIndexingStatusSupport(configTypeId, omnichainSnapshot.omnichainStatus); | ||
| } | ||
|
|
||
| // Note: ENSNode Registrar Actions API is available only in certain cases. | ||
|
|
@@ -101,12 +104,14 @@ export function useStatefulRegistrarActions({ | |
| } | ||
|
|
||
| const { omnichainSnapshot } = indexingStatusQuery.data.realtimeProjection.snapshot; | ||
| const chains = Array.from(omnichainSnapshot.chains.values()); | ||
| const configTypeId = getChainIndexingConfigTypeId(chains); | ||
|
|
||
| // fetching is temporarily not possible due to indexing status being not advanced enough | ||
| if (!hasIndexingStatusSupport(omnichainSnapshot.omnichainStatus)) { | ||
| if (!hasIndexingStatusSupport(configTypeId, omnichainSnapshot.omnichainStatus)) { | ||
|
||
| return { | ||
| fetchStatus: StatefulFetchStatusIds.NotReady, | ||
| supportedIndexingStatusIds, | ||
| supportedIndexingStatusId: getSupportedIndexingStatus(configTypeId), | ||
lightwalker-eth marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } satisfies StatefulFetchRegistrarActionsNotReady; | ||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.