Decouple job pipeline tables from the TXM DB

[amit-momin]

• Added SignalCallback boolean flag to the TxRequest and the corresponding signal_callback column to the evm.txes table to allow the caller (job pipeline service) to signify it wants the pipeline resume callback to be called when the tx is finalized.
• Added a callback_completed column to the evm.txes table to indicate if the callback for the tx has been called.
• Added method to the TxStore to mark the callback_completed flag as true
• Updated the method name FindReceiptsPendingConfirmation to FindTxesPendingCallback to better describe the functionality
• Updated the query in the FindTxesPendingCallback method by replacing the pipeline_runs.state='suspended' condition with signal_callback=true AND callback_completed=false and removed the joins to the pipeline_runs table.
• Updated the ResumePendingTaskRuns method in the Confirmer to set the callback_completed flag to true after the callback has been called without error.

[github-actions]

I see that you haven't updated any README files. Would it make sense to do so?

[jmank88]

Why don't we add some context about the particular calls that failed? The caller of ResumePendingTaskRuns already knows the method failed generally based on the error returned.

Suggested change

	return fmt.Errorf("ResumePendingTaskRuns failed: %w", err)
	}
	// Mark tx as having completed callback
	if err := ec.txStore.UpdateTxCallbackCompleted(ctx, data.ID, ec.chainID); err != nil {
	return fmt.Errorf("ResumePendingTaskRuns failed: %w", err)
	return fmt.Errorf("failed to resume calback: %w", err)
	}
	// Mark tx as having completed callback
	if err := ec.txStore.UpdateTxCallbackCompleted(ctx, data.ID, ec.chainID); err != nil {
	return fmt.Errorf("failed to update callback complete: %w", err)

[amit-momin]

Since this is wrapping an error from a more inner function, I'm worried I might repeat the same error message. I was under the impression we had this pattern to wrap the error with the function name to create a sort of error stack when this bubbles up to the top.

[jmank88]

We are trying to create a stack, but it should be human readable and differentiate between the cases. IMHO including function names in any way is an anti-pattern. We should prefer to add context that the caller cannot already infer themselves (they already know they called ResumePendingTaskRuns), and we should also prefer not to encode implementation details inside of human facing errors (when someone refactors ResumePendingTaskRuns, they have to notice all of these strings and update them).

[amit-momin]

Ok think I agree we shouldn't use function names explicitly. But I feel the only context I should include here is something like resuming pending task runs failed since the more specific context would be taken care of the underlying error since we're just passing it up here.

[jmank88]

I don't think that message adds any value for calling code which is already well aware of what func was called. What the calling code doesn't know is which of these particular steps failed.

[amit-momin]

I might be unnecessarily worrying about redundancy in the error message. I was thinking the calling code would know the step it failed on based on the error returned by the inner method. Like if the UpdateTxCallbackCompleted method returned a wrapped error with a message such as failed to update callback complete, it would just duplicate the message if I wrapped the error with a similar message in ResumePendingTaskRuns. But I guess there could be any number of specific errors underneath so this is alright. Sorry for the back and forth on something that's so simple

[jmank88]

If we are calling something that returns a message that we don't have anything to add to, then it is OK to return the error raw. 🤷

Sorry for the back and forth on something that's so simple

No problem. It seems simpler than it is and there are definitely edge cases.

[amit-momin]

Good point. I've updated errors to include more context and returning them raw where there isn't any more to add.

[prashantkumar1982]

Looks pretty good!

[prashantkumar1982]

nit: mention that these are confirrmed, beyond the minConfirmations param. Indicates that these are kind of finalized.

[prashantkumar1982]

Thanks for renaming this fn!
The old name was just soo misleading.

[cl-sonarqube-production]

SonarQube Quality Gate

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

82.9% Coverage
0.0% Duplication