Without a triggerType in the triggerMetadata definition, caller has to guess what parameters are inside.

[chrisli30]

This issue is about writing code in sdk and its above level.

Without a triggerType parameter in the triggerMetadata definition, the caller cannot determine what trigger type the metadata is for. The reason is because the ambiguity of FIXED_TIME.epoch, and CRON.epoch. With an epoch variable in TriggerMetadata, we can’t dinstinctively define a trigger for either FIXED_TIME or CRON. This case remains true when we get the TriggerMeta from server, it has only an epoch variable, that we have to guess what the trigger type is.

That said, determining trigger type using the variable name such as epoch is not a good way. We should include a triggerType in the TriggerMetadata, that way, it’s easy to make sure we are using the correct type of TriggerMetadata to trigger a task.

[v9n]

hmm @chrisli30 do we need it? a task has a single trigger. we know what is it. it cannot be ambigous. unless we started to allow multiple triggers.

[chrisli30]

Yes, at the time of making this API call, we should already know workflow.id and workflow.trigger.type. However, the fact that the TriggerMetadata struct is not self-contained feels problematic. As you pointed out, it relies on a variable from two levels above: triggerMetadata -> execution -> workflow -> workflow.trigger.type.

There’s also a risk that, due to the aync call, we could lose the context of the request if the response is delayed. In such cases, this ambiguity could result in invalid data because we wouldn’t be able to distinguish between FIXED_TIME and CRON. This issue is already reflected in a test case in TriggerWorkflow.test.ts.

Since TriggerMetadata is provided by the client side and doesn’t require a search on the server, I feel it’s a minor tradeoff to include an extra variable for clarity and consistency. Is there a specific challenge in implementing this additional tracking?

[v9n]

Is there a specific challenge in implementing this additional tracking?

no, not a challenge at all.

I was asking the question because the code( in this repository at least) doesn't use it, so adding a new field where there is no where in is used in the code feel redundant.

We can absolutely implement it.

[chrisli30]

Okay, let’s add this one, and well, I think later on we should add data validation functions for this TriggerMetadata struct. It’s not a priority now, and we can rely on the validation from the sdk side.

[v9n]

Fixed in https://github.com/AvaProtocol/ava-sdk-js/pull/46/files#diff-69d50d249b3bf328b802a6d8cffdc6fd474a0fe1e02555dd3deee2d1f25c0202R155
You can see in the response we assert the type

AVS implement is here: https://github.com/AvaProtocol/EigenLayer-AVS/pull/84/files#diff-77494b7116e328f80878e42128327bd124ab4ef1d7394c1d51109a5a5e93c4bcR669-R680