Introduce update transaction API (PATCH /transactions/{transactionId}) #145
Conversation
Signed-off-by: Chengxuan Xing <[email protected]>
Signed-off-by: Chengxuan Xing <[email protected]>
Signed-off-by: Chengxuan Xing <[email protected]>
| if syncRequest.txUpdates.GasPrice != nil { | ||
| return nil, i18n.NewError(ctx, tmmsgs.MsgTxHandlerUnsupportedFieldForUpdate, "gasPrice") | ||
| } |
There was a problem hiding this comment.
In order for gasPrice field to be updatable, the process transaction logic needs to change to do more sophisticated gas price tracking and management. Therefore, returning an error for the simple tx handler for now.
This constraint could be lifted with a separate PR if further enhancement to this simple example handler is required.
There was a problem hiding this comment.
Noting (per above discussions) this is a reference implementation, and the expectation is that providers of enterprise versions of FFTM are likely extending this simple engine with a much more sophisticated engine
Signed-off-by: Chengxuan Xing <[email protected]>
There was a problem hiding this comment.
Some commentary for discussion on this great looking change before I'm confident it's ready to go in, plus just one explicitly request for a change where we had "new"/"old" strings being used rather than a Go structure... but more than that I didn't understand the purpose of that go structure.
| // ApplyExternalTxUpdates applies the external updates to the managed transaction | ||
| // and returns the updates that were applied, whether any update occurred, and a map | ||
| // of changed fields with their old and new values for easy JSON marshalling. | ||
| func (mtx *ManagedTX) ApplyExternalTxUpdates(txUpdate TXUpdatesExternal) (txUpdates TXUpdates, updated bool, valueChangeMap map[string]map[string]string) { |
There was a problem hiding this comment.
So the design I believe is that this is a helper function that policy implementations (which hold the complex threading model) can call back into, at the point the "task" of editing a transaction has made it to the right locked in-memory code that is safe to make the change.
Will validate this understanding through the rest of the review.
| valueChangeMap = make(map[string]map[string]string) | ||
|
|
||
| if txUpdate.To != nil && mtx.To != *txUpdate.To { | ||
| valueChangeMap["to"] = map[string]string{ |
There was a problem hiding this comment.
The old/new item needs to be an API structured type, rather than string fields please.
I haven't reached the point in understanding what this is used for:
... and a map
// of changed fields with their old and new values for easy JSON marshalling.
| Gas *fftypes.FFBigInt `json:"gas,omitempty"` | ||
| Value *fftypes.FFBigInt `json:"value,omitempty"` | ||
| GasPrice *fftypes.JSONAny `json:"gasPrice,omitempty"` | ||
| TransactionData *string `json:"transactionData,omitempty"` |
There was a problem hiding this comment.
Please can we ensure we have a very clear knowledge (documented would be great in field-level docs) for how you clear this field.
Noting that means distinguishing between:
- Not changing the
transactionData - Setting the
transactionDatato be empty (simple transfer transaction)
We must ensure this works - even with the reference OSS handler.
The most common edit for experienced users fixing transaction that is broken, will be to turn it into a null transaction:
- Leaving the
nonceunchanged - Bumping the gas price artificially to allow a replacement transaction to go in
- Changing the
valueto be zero (if it wasn't before) - Clearing the
transactionData
This type of inert transaction that can be mined at very minimal execution transaction, is the safest way to clear bubbles where a transaction is breaking the pipe.
Noting a really sticky derivation of this made possible by this PR is:
- Add a new transaction that is inert, but gets shoved at the end of the nonce list
- Edit that transaction to have a
noncethat fills a gap
| if txUpdate == nil { | ||
| return nil, i18n.NewError(ctx, tmmsgs.MsgInvalidSortDirection) | ||
| } | ||
| updatedTx, err := m.txHandler.HandleTransactionUpdate(ctx, txID, *txUpdate) |
There was a problem hiding this comment.
Ok - so the whole of the job of thread management etc. is delegated down into the policy engine implementation 👍
| if syncRequest.txUpdates.GasPrice != nil { | ||
| return nil, i18n.NewError(ctx, tmmsgs.MsgTxHandlerUnsupportedFieldForUpdate, "gasPrice") | ||
| } |
There was a problem hiding this comment.
Noting (per above discussions) this is a reference implementation, and the expectation is that providers of enterprise versions of FFTM are likely extending this simple engine with a much more sophisticated engine
There was a problem hiding this comment.
Thanks for this @Chengxuan - I would love to see a document explaining when a TX can be patched, what does it mean to resume/pause/suspend a transaction, overall what does the transaction lifecycle look like. We should put this in the main docs possibly under a new section.
|
@EnriqueL8 - couple of things to think about there with your ask to @Chengxuan :
|
|
Makes sense - it would be on the implementation and when we expose this in the FireFly API then we can write a doc/relase notes explaining the behavior |
The existing FFTM lacks the functionality of updating the values of transaction parameters. The values are completely controlled by the transaction handler to avoid unexpected outcomes due to user intervention.
However, for users who are familiar with the transaction lifecycle and have more advanced tuning requirements, an API allowing them to update the transaction parameter values directly is valuable, especially when TM is stuck in a situation that requires manual resolution.
This PR introduces:
HandleTransactionUpdatein the Transaction Handler interface that needs to be implemented.