Transaction substatus

[matthew1001]

Closes #49

A diagram and more detail about the changes under this PR are available in the FireFly Micro board at https://miro.com/app/board/uXjVOWHk_6s=/?moveToWidget=3458764544594470770&cot=14 and in the original issue #49

The PR introduces additional granularity to transaction progress by introducing sub-status types, and actions that can cause a change in sub-status:

These are incorporated into the ManagedTX data structure with a history field and a historySummary field (see Miro board for more detailed description).

[peterbroadhurst]

Hi @matthew1001,

I've made some suggestions on the detail of this new implementation.

I am expecting to see deletion of the old implementation in this one too - and probably we want this to get in before v1.2 cuts, so we don't have a double-migration of the data structures.

[peterbroadhurst]

There seems to be a split approach here that I can't reconcile.

The code below suggests we are aiming to limit the number of unique entries to 50 (I guess the most recent 50?). This fits with an approach where (like k8s describe) we de-duplicate when something is in exactly the same state, but try to provide an accurate a history as possible.

However, the comparison on this line only uses the TxSubStatus line for comparison bare - without all the interesting things that might make it unique, such as the TX Hash. So it's impossible to get more than 6 entries in the table. In fact the entries don't really contain any detail at all now.

I think we need to choose whether to give detailed information, or not. I believe we should give detailed information - as we do before this PR.

I assert the following are a must, which certainly does introduce the potential for more than 50 records:

• The current Hash of the transaction we are tracking
• The receipt ProtocolID

The number of confirmations is slightly interesting, as we could choose to update that on an existing record, rather than produce a new record.

I like the approach you've got of adding the raw information on the JSON, and allowing translation of that into human readable form by the consumer of the API (such as a UX, or application).

So I think I'm proposing:

type TxSubStatusEntry struct {
	Occurrences     int              `json:"occurrences"`        // updates every time we add a duplicate
	FirstOccurrence *fftypes.FFTime  `json:"first"`              // set when first added to array
	LastOccurrence  *fftypes.FFTime  `json:"last"`               // set when first added, and updated on each duplicate
	Status          TxSubStatus      `json:"status"`             // part of uniqueness
	TX              string           `json:"tx"`                 // part of uniqueness
	Receipt         string           `json:"receipt"`            // part of uniqueness
	GasPrice        *fftypes.JSONAny `json:"gasPrice,omitempty"` // part of uniqueness - only present on Status==RetrievedGasPrice
	Confirmations   int              `json:"confirmations"`      // updates as more are added
}

func (s *TxSubStatusEntry) Key() string {
	return fmt.Sprintf("%s:%s:%s:%s", s.Status, s.TX, s.Receipt, s.GasPrice.String())
}

Maybe a higher level point as to why I feel quite strongly about ^^^: if the system is looping through fundamentally different sub-step, then I think it's really important that the information shows this.

e.g. I think between (A) and (B) I feel like people need (B):

Information preservation A

• ✅ validated (1)
• ✅ estimatedgas (2)
• ✅ submitted (3)

Information preservation B

• ✅ validated (1)
• ✅ estimatedgas (1) GasPrice=12345
• ✅ submitted (1) TXHash=12345
• ✅ estimatedgas (1) GasPrice=12999
• ✅ submitted (2) TXHash=23456

[peterbroadhurst]

... there's a subtle point here too, that I just realized I'm making.

I think the list of sub-status's is a little inconsistent. For some steps we've got before+after states, for others we've just got after states. I think we should be very consistent on that.

The example above is we just "after" states. I didn't include any "before" states ... apart from one initial state which is actually something that happens before we accept the transaction - I was wondering if we might get stuck in that state and it would be useful to have a count there.

[peterbroadhurst]

oh, darn - and that suddenly reminded me there might be an error that we're getting preventing us moving to the next state, so I guess there's a decision as to whether we record errors in special "before" states (doubling the length of the array)... or by just updating the previous state with the error preventing us from progressing.

[peterbroadhurst]

... which maybe means we should only have "before" states, rather than only have "after" states:

Information preservation C

• ✅ estimatingGas (3) LastError="Node unavaialable", LastErrorTime=10:57
• ✅ submitting (1) TXHash=12345, GasPrice=12345
• ✅ estimatingGas (1)
• ✅ submitted (2) TXHash=23456, GasPrice=12999

[peterbroadhurst]

Assuming we retain a limit on the buffer, I think we need to:

1. Make the limit configurable
2. Discard the oldest record, not the newest

[matthew1001]

Thanks for the comments. So I think there are 3 broad categories for discussion perhaps in person if easier:

• How much information should be in a sub-status entry
  • I guess I wasn't sure of the benefit of reporting TXHash, gas price etc. on every status change. Today we report these elsewhere in the detail section so is there a benefit to showing e.g. the gas price at every sub-status change?
• Should a state change be recorded for a 'before' and 'after' step
  • My thinking here was that a discrete before and after status would make it easy to determine how long certain steps were taking while allowing a caller who didn't care about that detail to ignore either the before or after step.
• How are errors reported vs sub-status changes
  • The existing history mechanism gives a slightly different view on changes in the transaction, including errors at a given point. Not having 2 "history" sections does make sense, so are you thinking that TxSubStatusEntry should have an optional error field similar to the current history list?

[matthew1001]

Thanks for the comments & discussion @peterbroadhurst. The new commit I think addresses many of the concerns over how we want to structure the data, how much of it to keep, and how much detail to retain etc.

There's one last tidy-up thing I could do with a pointer on - passing the configurable maxHistory* values from fftm.manager down into the apitypes.managed_tx so that AddSubStatus(...) have access to them (note the // TODO comments). Probably just an obvious best way of doing it that I'm missing so any pointers let me know.

I'm also seeing struct fields serialise in alphabetical order, not the order defined in the struct. Suspect I'm missing something simple as it would be nice for most important fields to be near the top when serialising.

[peterbroadhurst]

I wonder if we really need this configuration parameter, as this should be bounded by the number of types of status we have.

[peterbroadhurst]

Noting this list can be extended by policy engine implementations.

[peterbroadhurst]

This needs to be FIFO I think too

[peterbroadhurst]

Thanks @matthew1001 - this seems like a great step forwards.

I note the PR does not contain a summary of the new architecture / data structure that is proposed, so we'll need to document that separately somewhere (or update this PR).

I do have detail proposals on the TODOs, and some detail thoughts on the code, but I suspect the best way for me to express these efficiently is with code. So I'm going to merge this as an excellent step forwards, then make my suggestions as a PR for you to react to.

[matthew1001]

Yes agreed re. the documentation of the structures. I'll add something to this PR to describe them. Thanks for the code suggestions via #58.