BCFR-934/remove-finality-depth-as-default-value-for-minConfirmation-and-fix-inconsistency #14509
Conversation
WF: CI Core#9c9678aNo errors found in this run. 🎉 |
There was a problem hiding this comment.
This is now passing existing tests, but I think we are missing some coverage. Consider if a user does not set a value. In the old path, a value would be obtained that may or may not be zero, which determines whether we wait for a callback or not. In the new path, we never wait for callback. It would seem that we need a means of getting a callback after finalization (regardless of the particular depth or tags being used).
There was a problem hiding this comment.
Is it valid to set only txRequest.PipelineTaskRunID without txRequest.MinConfirmations, to get a callback based on the configured finality? If that doesn't work already, it seems like a logical way to support this.
There was a problem hiding this comment.
Implemented. Now the only change in behavior should be the unnoticeable implementation detail of an "immediate" callback in the case of omitting a value for a chain with "instant" finality. So just a bit of latency in the worst case.
WF: CI Core#871214aNo errors found in this run. 🎉 |
jmank88
force-pushed
the
BCFR-934/remove-finality-depth-as-default-value-for-minConfirmation-and-fix-inconsistency
branch
from
7a49237
to
627a7e1
Compare
WF: CI Core#7a49237No errors found in this run. 🎉 |
jmank88
force-pushed
the
BCFR-934/remove-finality-depth-as-default-value-for-minConfirmation-and-fix-inconsistency
branch
from
627a7e1
to
4bf6083
Compare
WF: CI Core#627a7e1No errors found in this run. 🎉 |
jmank88
force-pushed
the
BCFR-934/remove-finality-depth-as-default-value-for-minConfirmation-and-fix-inconsistency
branch
from
4bf6083
to
3b899db
Compare
WF: CI Core#6312f63No errors found in this run. 🎉 |
|
| // Store the task run ID, so we can resume the pipeline when tx is confirmed | ||
| if !isMinConfirmationSet { | ||
| // Store the task run ID, so we can resume the pipeline when tx is finalized | ||
| txRequest.PipelineTaskRunID = &t.uuid |
There was a problem hiding this comment.
@jmank88 is PipelineTaskRunID only set during this specific functionality? I'm wondering if there is a case where it's being set for a different reason and the minConfirmations gets executed as a side-effect of that.
There was a problem hiding this comment.
is PipelineTaskRunID only set during this specific functionality?
Yes
and the minConfirmations gets executed as a side-effect of that.
What do you mean that it "gets executed"? AFAIK this parameter only affects the callback behavior, not confirmation.
There was a problem hiding this comment.
I'm referring to a scenario where PipelineTaskRunID is used by another task/process, but it seems it doesn't.
dimriou
deleted the
BCFR-934/remove-finality-depth-as-default-value-for-minConfirmation-and-fix-inconsistency
branch
Description
We shouldn't use FinalityDepth as the default value for minConfirmation when it's not provided by the user.
Update the sql query for fetching pending callback transactions:
if minConfirmation is not null, we check difference if the current block - tx block > minConfirmation,
else we check if the tx block is <= finalizedBlock
Tickets:
BCFR-934