Skip to content
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

Consider narrowing the strings allowed for condition records in ConditionExpressionJSON #230

Closed
derekpierre opened this issue Jun 20, 2023 · 4 comments

Comments

@derekpierre
Copy link
Member

derekpierre commented Jun 20, 2023

See conversation from PR review here - #229 (comment).

Maybe we can use the schema fields defined for underlying conditions to limit the record keys that can be used with ConditionExpressionJSON object.

@derekpierre derekpierre mentioned this issue Jun 20, 2023
8 tasks
@piotr-roslaniec
Copy link
Contributor

Someone recommended using Zod, which supports static typing of object schemas: #94

It would solve the ambiguity of keys here, and the ambiguity of types in a bunch of other places.

@derekpierre
Copy link
Member Author

Could be interesting. We probably would need to push out any possible changes to Zod to a post-7.0.0 release. Also, it remains to be seen what ripple effects moving condition code to nucypher-core post 7.0.0 would have in this area. Not sure the level of work, but adding a whole new library at this later stage for v7.0.0 may not be worth it.

Generally, I think this specification:

type ConditionProps = 'operator' | 'method' | 'contractAddress';

export type ConditionExpressionJSON = {
  version: string;
  condition: Record<ConditionProps, unknown>;
};

should not try to specify ConditionProps since it is too low level, but rather should be something like this (and I don't know if this is allowed or not but):

type ConditionRecord = TimeConditionRecord | RPCConditionRecord | ContractConditionRecord | ...

export type ConditionExpressionJSON = {
  version: string;
  condition: ConditionRecord
};

That way you can be a lot more specific, and reuse the schema information from the various conditions themselves, instead of trying to define something separate.

In any case, this is not meant to be a comment on Zod, but rather a comment on what that expression should really be doing.

@piotr-roslaniec
Copy link
Contributor

Because of type ambiguity, the resulting ConditionRecord simplifies to Record<string, Joi.schema>

type ConditionRecord = TimeConditionRecord | RPCConditionRecord | ContractConditionRecord

I agree that issue could be reviewed post-7.0.0 and after reviewing possible changes to nucypher-core. I just wanted to highlight the subject of object schema typing.

@derekpierre
Copy link
Member Author

Closed via #267.

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

No branches or pull requests

2 participants