BCFR-934/remove-finality-depth-as-default-value-for-minConfirmation-and-fix-inconsistency #14509
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| --- | ||
| "chainlink": patch | ||
| --- | ||
|
|
||
| Remove finality depth as the default value for minConfirmation for tx jobs. | ||
| 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 | ||
| #updated |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,11 +64,11 @@ func (t *ETHTxTask) getEvmChainID() string { | |
| return t.EVMChainID | ||
| } | ||
|
|
||
| func (t *ETHTxTask) Run(ctx context.Context, lggr logger.Logger, vars Vars, inputs []Result) (Result, RunInfo) { | ||
| var chainID StringParam | ||
| err := errors.Wrap(ResolveParam(&chainID, From(VarExpr(t.getEvmChainID(), vars), NonemptyString(t.getEvmChainID()), "")), "evmChainID") | ||
| if err != nil { | ||
| return Result{Error: err}, RunInfo{} | ||
| } | ||
|
|
||
| chain, err := t.legacyChains.Get(string(chainID)) | ||
|
|
@@ -81,7 +81,7 @@ func (t *ETHTxTask) Run(ctx context.Context, lggr logger.Logger, vars Vars, inpu | |
| txManager := chain.TxManager() | ||
| _, err = CheckInputs(inputs, -1, -1, 0) | ||
| if err != nil { | ||
| return Result{Error: errors.Wrap(err, "task inputs")}, RunInfo{} | ||
| } | ||
|
|
||
| maximumGasLimit := SelectGasLimit(cfg.GasEstimator(), t.jobType, t.specGasLimit) | ||
|
|
@@ -107,25 +107,20 @@ func (t *ETHTxTask) Run(ctx context.Context, lggr logger.Logger, vars Vars, inpu | |
| errors.Wrap(ResolveParam(&failOnRevert, From(NonemptyString(t.FailOnRevert), false)), "failOnRevert"), | ||
| ) | ||
| if err != nil { | ||
| return Result{Error: err}, RunInfo{} | ||
| } | ||
| minOutgoingConfirmations, isMinConfirmationSet := maybeMinConfirmations.Uint64() | ||
|
|
||
| txMeta, err := decodeMeta(txMetaMap) | ||
| if err != nil { | ||
| return Result{Error: err}, RunInfo{} | ||
| } | ||
| txMeta.FailOnRevert = null.BoolFrom(bool(failOnRevert)) | ||
| setJobIDOnMeta(lggr, vars, txMeta) | ||
|
|
||
| transmitChecker, err := decodeTransmitChecker(transmitCheckerMap) | ||
| if err != nil { | ||
| return Result{Error: err}, RunInfo{} | ||
| } | ||
|
|
||
| fromAddr, err := t.keyStore.GetRoundRobinAddress(ctx, chain.ID(), fromAddrs...) | ||
|
|
@@ -159,8 +154,11 @@ func (t *ETHTxTask) Run(ctx context.Context, lggr logger.Logger, vars Vars, inpu | |
| SignalCallback: true, | ||
| } | ||
|
|
||
| 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 There was a problem hiding this comment.
Yes
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 |
||
| } else if minOutgoingConfirmations > 0 { | ||
| // Store the task run ID, so we can resume the pipeline after minOutgoingConfirmations | ||
| txRequest.PipelineTaskRunID = &t.uuid | ||
| txRequest.MinConfirmations = clnull.Uint32From(uint32(minOutgoingConfirmations)) | ||
| } | ||
|
|
@@ -170,11 +168,11 @@ func (t *ETHTxTask) Run(ctx context.Context, lggr logger.Logger, vars Vars, inpu | |
| return Result{Error: errors.Wrapf(ErrTaskRunFailed, "while creating transaction: %v", err)}, retryableRunInfo() | ||
| } | ||
|
|
||
| if txRequest.PipelineTaskRunID != nil { | ||
| return Result{}, RunInfo{IsPending: true} | ||
| } | ||
|
|
||
| return Result{}, RunInfo{} | ||
| } | ||
|
|
||
| func decodeMeta(metaMap MapParam) (*txmgr.TxMeta, error) { | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it valid to set only
txRequest.PipelineTaskRunIDwithouttxRequest.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.