Constellation-Labs / tessellation

Monadic execution contexts for topology organization
Apache License 2.0
49 stars 28 forks source link

refactor!: restrict function scope of DataApp lifecycle methods #913

Closed scasplte2 closed 1 month ago

scasplte2 commented 2 months ago

Purpose

The previous definition of DataApplicationContextualOps was shared between L0 and L1 services which led to metagraph developers needing to provide implementations for functions that were not invoked by the underlying TessellationIOApp. For example, Data-L1 nodes were forced to provide definitions for combine and validateData but these methods are only invoked for L0 nodes. This led to dummy implementations being provided simply to satisfy inheritance constraints for all Data Applications such as Dor.

This PR introduces SharedContextualOps, L0ContextualOps, & L1ContextualOps to restrict the scope of contextual operations to the layer where they are needed. This allows for metagraph developers to avoid the dummy implementations that were previously needed.

Changes

Testing

scasplte2 commented 2 months ago

This is in draft until two questions can be answered

  1. What is the scope of the new validateFee, estimateFee, & extractFee functionalities? I made my best guess but if @AlexBrandes @ryle-goehausen or @sdavidp may be able to provide further clarity
  2. This may be a breaking change for some metagraphs (if they have used the override modifier) but it is not obvious to me how this should interact with the conventional-commits ADR we've been discussing recently.
marcinwadon commented 2 months ago

Re. 1 - I think you don't need to provide a scope for this. The scope is optional, so the current commit name looks okay for me. Re. 2 - If this is a breaking change, you should mark it by an exclamation mark. Anyway, worth checking (@IPadawans maybe?) if this is truly a breaking change. We change our internal representation only, but fns' definitions remain the same, so it should be backward compatible for metagraphs at first glance.

scasplte2 commented 2 months ago

Latest push follows the usage of the newest fee functionality to define the following

I've also completed the end-to-end example with the todo metagraph and verified the expected behavior of the data application state updates (i.e. state updates are still processed successfully)

IPadawans commented 2 months ago

I forgot to answer here but yes, this is a breaking change and we maybe should hold this until the hackathon ends

marcinwadon commented 1 month ago

@scasplte2 @IPadawans AFAIK we won't release anything new to intnet or mainnet, so I think it's safe to merge it to develop. We have fee transactions on the develop as well, that are not ready for intnet/mainnet release. WDYT?

scasplte2 commented 1 month ago

@marcinwadon @AlexBrandes This PR was built atop the fee-transaction changes to Data Apps so I've cherry picked the commit and updated the base of this PR to target feature/fee-transaction. It is simple enough to create another branch with the changes only applicable to develop if we would like to pull in this refactor before the fee-transaction branch.