Created dedicated classes for Workflow,Node,Edge and Execution

[chrisli30]

This PR serves serveral purposes to improve the handling of Node and Trigger in the following ways:

1. Create dedicated classes for Node and Trigger to address the inconvenience of the directly exported struct from avs_pb.js. The way protobuf uses oneOf to create data structure is unconventional to Typescript.
2. Use Typescript strong types to regulate user inputs at the top level, making the required types for createWorkflow() explicitly visible to users.
3. Encapsulates the construction logic for each NodeType and TriggerType into their respective classes, so it’s easier to handle changes in a divide-and-conquer approach.

Usage example of create a new workflow:

      const workflow = client.createWorkflow({
        smartWalletAddress,
        nodes: NodeFactory.createNodes(NodesTemplate),
        edges: _.map(EdgesTemplate, (edge) => new Edge(edge)),
        trigger: TriggerFactory.create({
          name: "blockTrigger",
          type: TriggerTypes.BLOCK,
          data: { interval: 5 },
        }),
        startAt: WorkflowTemplate.startAt,
        expiredAt: WorkflowTemplate.expiredAt,
        maxExecution: 1,
      });

TODOs:

1. Rebase the base branch sync-protobuf.
2. Fix broken tests in the tests/createTask.test.ts
3. Clean up unnecessary console.log

[socket-security]

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/@eslint/[email protected]	None	0	14.9 kB	eslintbot, openjsfoundation
npm/@typescript-eslint/[email protected]	Transitive: environment, filesystem	+37	7.02 MB	bradzacher, jameshenry
npm/@typescript-eslint/[email protected]	Transitive: environment, filesystem	+29	2.24 MB	bradzacher, jameshenry
npm/[email protected]	Transitive: environment, eval, filesystem, shell, unsafe	+82	10.6 MB	eslintbot
npm/[email protected]	None	0	176 kB	sindresorhus

View full report↗︎

[chrisli30]

@v9n this PR is ready for review. The major change is to convert builder.ts to TriggerFactory and NodeFactory to include strong typing for building input parameters.

The maxExecution parameter returned from AVS seems to be inconsistent with the input. Currently the tests are failing in local, but they are all due to this line:

expect(actual.maxExecution).toBe(expected.maxExecution); at the end of tests/utils.ts. Commenting that line will result in all green from tests.

[v9n]

hey @chrisli30 these type parameter isn't needed when sending to protobuf. we can remove them.

[chrisli30]

Understood, but these type is not sent to AVS currently, but as an intrinsic variable of the SDK’s Trigger.

Each Trigger has their own data struct, without a shared type variable, it‘s hard to use shared code, in other words, class inheritance.

[v9n]

we can juse use sth as above with .getType() method define in the parent Trigger class to get class name and parse out the type.

[chrisli30]

Yes, that will work. However, I’m trying to achieve something extra: to directly reference the code you defined in the protobuf, which is the below code.

export namespace TaskTrigger {
    export enum TriggerTypeCase {
        TRIGGER_TYPE_NOT_SET = 0,
        MANUAL = 2,
        FIXED_TIME = 3,
        CRON = 4,
        BLOCK = 5,
        EVENT = 6,
    }
}

Even if we getType() the classNames, they are defined by the sdk. If we change anything in the protobuf, the classNames will need manual update as well.

[v9n]

no need to pass type. you will notice that in toRequest we dont need to send type to server.

and when you're in BlockTrigger class, you definetely know its type. no need to set this redundant param

[chrisli30]

I agree, we can use a typeof BlockTrigger to tell the type, but that’s not ideal, due to the below two reasons:

1. The result of the typeof can’t directly map to the original type’s values, such as TriggerTypes.BLOCK.
2. The typeof is not conventional and a little hacky, and we rarely use it.

I think the reason protobuff doesn’t need a type variable is that it does not support class inheritance, and use oneOf instead. However, the fact that there are 5 optional parameters, epochs,cron, conditions, etc. , using oneOf on a single struct, confused me. As a caller, it’s hard to figure out what parameters to choose.

Additionally, no matter what trigger type a caller chooses, the data field is all called data. I also kept the inner variable original, so we know that the inner data is a cron, epochList, or conditionsList.

[v9n]

i mean is there a reason we couldn't do this?

class Trigger {
  getType(): string {
     // parse class name to get type
    return this.constructor.name.substr(0, this.constructor.name.length - 7);
  }
}


class FooTrigger extends Trigger {
}

class BarTrigger extends Trigger {
}

const a = new FooTrigger();
console.log("im", a.getType());

const b = new BarTrigger();
console.log("im", b.getType());

Result

 npx tsc a.ts
❯ node a.js
im Foo
im Bar

it save users the work of manually passing the type parameter. BlockTrigger is definetely a block, just by infering the class name as i show above

[chrisli30]

Same reason as above. There’s no FooTrigger can be or should be defined as a type, except for the original protobuf values.

export enum TriggerTypeCase {
        TRIGGER_TYPE_NOT_SET = 0,
        MANUAL = 2,
        FIXED_TIME = 3,
        CRON = 4,
        BLOCK = 5,
        EVENT = 6,
    }

[v9n]

do you think this just cause extra unncecessary step?

  trigger: TriggerFactory.create({
    name: "blockTrigger",
    type: TriggerTypes.BLOCK,
    data: { interval: 5 },
  }),

vs

trigger: new BlockTrigger({name: 'every 5 block trigger', interval: 5})

no need to pass type, and wrap everything into data

[chrisli30]

Due to class inheritance, all Trigger class needs to share the same variable name, which is data.

Also, it’s really hard to get the typescript type definition right, with some of the class input being number type, but others an object or array type, so a common structure data: object was needed.

We could do data: 5, but I found data: {interval: 5} is more descriptive.

[v9n]

no i mean just do

trigger: BlockTrigger.create({name: 'every 5 block trigger', interval: 5})

a user know exactlt what parameter a block trigger need based on the type def of the blcok trigger.

there is no value to adding the below:

TriggerFactory.create({
    name: "ever 5 block",
    type: TriggerTypes.BLOCK,
    data: { interval: 5 },
  }),

Unless we do other conversion and dynamic calculateion, right now we literally just switch type then invoke the underlying class directly.

[chrisli30]

Yes, we can. I chose to do this way

TriggerFactory.create({
    name: "ever 5 block",
    type: TriggerTypes.BLOCK,
    data: { interval: 5 },
  }),

It’s a little extra code to keep a unified format for all 5 triggers.

[v9n]

@v9n this PR is ready for review. The major change is to convert builder.ts to TriggerFactory and NodeFactory to include strong typing for building input parameters.

The maxExecution parameter returned from AVS seems to be inconsistent with the input. Currently the tests are failing in local, but they are all due to this line:

expect(actual.maxExecution).toBe(expected.maxExecution); at the end of tests/utils.ts. Commenting that line will result in all green from tests.

The max execution issue is fixed here AvaProtocol/EigenLayer-AVS#55 (comment) and the local dev test is fixed now.

[v9n]

we can move forward with the PR. but imho we cause extra work with the factory.

[v9n]

@chrisli30 i included multiple calls to prove that this call is idempotent regardless how many time we call it. you removed the call will make us lost that assertion.

[chrisli30]

oh I see, I recognized those typos ... just added them back.