-
Notifications
You must be signed in to change notification settings - Fork 22
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
Enhancements to Conditions API #258
Comments
related: #230 |
For 1) does this logic help you?
Conditions should be serialized as For 2):
Interested to better understand how these condition struggles play out for developers like yourself - bit of a blurr on my side. Are you providing bespoke condition creation to users on your side, and therefore validation is tricky? What's the user story for needing process json? Do you care about the types, or is it that validation is easier? Some of the sentiments are shared. Ultimately, the move to Zod could be a good one, but only if conditions remain out of #230 is marked for consideration for 8.0.0 though. |
The issue is mostly when creating new Condition sets during encryption time. Say I want to create a new Contract condition which would look something like: new conditions.base.ContractCondition({
contractAddress: 0x...,
method: 'isRevoked',
parameters: [':userAddress'],
functionAbi: {
inputs: [
{
name: 'addr',
type: 'address',
},
],
name: 'isRevoked',
outputs: [
{
name: 'addr',
type: 'bool',
},
],
stateMutability: 'view',
type: 'function',
},
chain: 80001,
returnValueTest: {
comparator: '==',
value: false,
},
}); To know the exact schema of what I need to pass to this object, I literally had to go read the source code of the nucypher-ts library and there is no compile time check from typescript that my condition schema is valid. |
I think relying on this instead of a discriminating field backs you into a corner and make bugs or missed cases more likely. For instance, what if you have a new condition type with a Also, if you add a new condition type it would be very easy to miss adding it to this switch (condition.conditionType) {
case 'rpc': {
return new RpcCondition(condition)
}
case 'contract': {
return new ContractCondition(condition)
}
...
default:
throw new Error("Unsupported Condition")
} |
👍 Would the exported record or Joi schema objects help?
Seems like functionABI should be its own and have exported record/schema as well - https://github.com/nucypher/nucypher-ts/blob/alpha/src/conditions/base/contract.ts#L10 - which it isn't right now 🤔 @piotr-roslaniec (currently OOO) is more familiar in this area. |
I dont think this really helps because I am still not going to get a compile time typescript error if my schema is wrong. This is pretty much the entire reason to use Typescript over Javascript |
Don't get me wrong, I wasn't trying to imply that it was a perfect solution - the solution would probably be part of #230 and may indeed use a discriminating field as you alluded to - but rather that there is existing validation work already done for you, and can be used right now for 7.0.0-beta, albeit not at compile time.
Not sure I totally follow. At the very least it would probably be a sub-class of Basically, it would either be its own separate class, or a subclass - but either way there will be unique defining characteristic(s) - otherwise it shouldn't be a separate class/sub-class at all and just use the existing condition class. For example
That's fair - we have that in Python. @piotr-roslaniec - did you ever consider a non-json constructor for conditions and JSON optionally specified via a static |
Yes, that was a consideration before we implemented PRE-based CBD, perhaps it's worthwhile to revisit it. Hi @ghardin1314, thank you for posting this issue. I generally agree with both of your points. Please feel free to post these changes in a PR for me to review if you have them on hand. If not, I will look into this issue (sooner than later_ and evaluate these changes. |
Closed by #267 |
Currently working with conditions is very difficult. There is no way to get type inference on what the schema of the condition should be based on the condition type (evm, timelock, etc). Here are my suggestions to improve this.
conditionType
discriminating field to the condition schema itselfRight now it is very hard to tell from a JSON condition, what type it is. Adding a new top level field to the
Condition
schema itself would make this much easier. Not sure exactly who owns the condition schema definition right now, as if I pass extra fields like this to Porter I get an error. Example would beThis also helps immensely on the ts side as you can use a discriminated union to parse from JSON
Right now the constructors of all types of conditions share the same interface as the base condition which is
value: Record<string, unknown>
. This provides very little feedback to the developer about what they are actually supposed to pass here and leads to a lot of trial/error and frustration.To fix this, the actual type of the schema should be inferred in the constructor. For example the type for RpcCondition should look something like:
These types can be written manually to match the Joi schema, or generated using something like (Joi to Typescript)[https://github.com/mrjono1/joi-to-typescript]
My real recommendation is to switch from Joi to Zod so the types can be inferred directly from the schema itself. You can see an example here
I think these 2 changes will massively improve DX for working with conditions. More than happy to submit a PR for refactoring, but @piotr-roslaniec mentioned there may be big changes to the condition schema so would like to wait until that is finalized
The text was updated successfully, but these errors were encountered: