nucypher/taco-web

Enhancements to Conditions API

Closed this issue · 9 comments

ghardin1314 commented

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.

  1. Add conditionType discriminating field to the condition schema itself

Right 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 be

{
  "conditionType": "evm" | "timelock" | ...,
  "method": "...",
  "parameters": [ ],
  ...
}

This also helps immensely on the ts side as you can use a discriminated union to parse from JSON

  1. Add types/interfaces to the constructor of each type of Condition

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:

interface RpcConditionProps {
  method: 'eth_getBalance' | 'balanceOf'
  parameters: [':userAddress']
  chain: number
  returnValueTest: ReturnValueTestProps
}

class RpcCondition extends Condition {
  constructor(value: RpcConditionProps) {
    super(value)
  }
}

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

ghardin1314 commented

related: #230

derekpierre commented

For 1) does this logic help you?

Conditions should be serialized as ConditionExpr due to versioning.

For 2):

  • I think we did some work to help here as well: 90f87ba - (#226) separating the schema and record. Does this split help in the short term with validation?

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 nucypher-core/Rust - although not sure if Zod has Rust implementation (maybe it does). The current situation without the condition being in the common shared library is that logic gets repeated between python and typescript. Unfortunately, from a timing perspective it's tough to do large changes for the 7.0.0-beta.

#230 is marked for consideration for 8.0.0 though.

ghardin1314 commented

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.

ghardin1314 commented

For 1) does this logic help you?

Conditions should be serialized as ConditionExpr due to versioning.

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 contractAddress field, but is not a ContractCondition? Then you have no way to tell them apart.

Also, if you add a new condition type it would be very easy to miss adding it to this if else statement. Whereas if you had a discriminating field you could easily do:

 switch (condition.conditionType) {
    case 'rpc': {
      return  new RpcCondition(condition)
    }
    case 'contract': {
      return new ContractCondition(condition)
    }
   ...
  default:
    throw new Error("Unsupported Condition")
  }
derekpierre commented

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.

👍

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.

ghardin1314 commented

Would the exported record or Joi schema objects help?

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

derekpierre commented

I think relying on this instead of a discriminating field backs you into a corner and make bugs or missed cases more likely.

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.

For instance, what if you have a new condition type with a contractAddress field, but is not a ContractCondition? Then you have no way to tell them apart.

Not sure I totally follow. At the very least it would probably be a sub-class of ContractCondition or else contractAddress value should not be present. On the other end of the spectrum it would be its own class. Perhaps you've thought of an example that I'm missing.

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 TimeCondition (timelock that uses block time) is just a subclass of RPCCondition that:

  • the method value is a constant =timelock
  • no parameters

I dont think this really helps because I am still not going to get a compile time typescript error if my schema is wrong

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 from_json() function...maybe...?

piotr-roslaniec commented

did you ever consider a non-json constructor for conditions and JSON optionally specified via a static from_json()

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.

piotr-roslaniec commented

Closed by #267