Add route for /transactions/{txnid}/status

[awrichar]

No description provided.

[codecov-commenter]

Codecov Report

Merging #457 (26da4b3) into main (5526967) will decrease coverage by 0.07%.
The diff coverage is 95.56%.

@@             Coverage Diff             @@
##              main     #457      +/-   ##
===========================================
- Coverage   100.00%   99.92%   -0.08%     
===========================================
  Files          279      282       +3     
  Lines        15077    15171      +94     
===========================================
+ Hits         15077    15160      +83     
- Misses           0       11      +11     

Impacted Files	Coverage Δ
internal/database/sqlcommon/tokenpool_sql.go	100.00% <ø> (ø)
internal/orchestrator/orchestrator.go	100.00% <ø> (ø)
pkg/fftypes/event.go	100.00% <ø> (ø)
internal/txcommon/txcommon.go	77.55% <77.55%> (ø)
internal/apiserver/route_get_txn_status.go	100.00% <100.00%> (ø)
internal/assets/manager.go	100.00% <100.00%> (ø)
internal/assets/token_pool.go	100.00% <100.00%> (ø)
internal/assets/token_transfer.go	100.00% <100.00%> (ø)
internal/batch/batch_processor.go	100.00% <100.00%> (ø)
internal/batchpin/batchpin.go	100.00% <100.00%> (ø)
... and 9 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5526967...26da4b3. Read the comment docs.

[peterbroadhurst]

I know this is a bit of a pain, but I think one improvement would be to order the items in the list. My gut would be:

• Pending first
• Then sorted by descending timestamp for operations and blockchain events

I do think the timestamp for anything that's completed should be a top-level item in the status.

I um'd and ah'd about ideas for making the output more human+computer readable, but this was fundamentally the only idea I had.

[peterbroadhurst]

Found this case we're still attempting to update TX status:

firefly/internal/assets/token_transfer.go

Lines 314 to 316 in 5526967

	if err = s.mgr.database.UpdateTransaction(ctx, tx.ID, update); err != nil {
	l.Errorf("TX update failed: %s update=[ %s ]", err, update)
	}

[peterbroadhurst]

... considering submitting my PR on events to this branch, to avoid merge hell

[awrichar]

Thanks for looking this over. I agree timestamp is a useful field to add (anything that can be helpful for debugging).

On the ordering, I'll say that the benefit of the current order is that it should be stable between successive calls to the API, or when comparing two similar transactions. But I can see value in ordering by timestamp as well, from the angle of trying to plot when it got stuck. Will ponder some more...

[awrichar]

Another subtle point about timestamps - the time for BlockchainEvents comes from the chain, while the time for other objects comes from FireFly. For the most part they should line up; just noting it.

Also I just realized I may not be intelligently handling large queries (such as a large Batch with a ton of Operations). Another edge case to ponder...

[peterbroadhurst]

Also I just realized I may not be intelligently handling large queries

I feel like maybe this is ok. Feel like a restriction that your FF must be able to cope with returning small JSON snippets for the maximum number of operations you include in any transaction, within a single JSON payload, is a valid restriction.

[peterbroadhurst]

such as a large Batch with a ton of Operations

The size of the batch in terms of messages/data wouldn't affect the overall operation count.
It's just one write for the whole batch.

For private messages, it would be the number of different parties you sent to that would scale out the ops.

[peterbroadhurst]

kaleido-io#60 raised to merge my changes into this branch.

[awrichar]

Decided to go with the recommended sorting (descending by timestamp, with "pending" items at the top)

[peterbroadhurst]

Found an issue to sort in the merge. Fixing

time="2022-01-29T02:09:05Z" level=debug msg="New batch e37d7961-58e9-4667-ad8a-d0f1a17feabd" role="batchproc-ns1::"
time="2022-01-29T02:09:05Z" level=debug msg="Adding 1 entries to batch e37d7961-58e9-4667-ad8a-d0f1a17feabd. Size=1 Seal=true" role="batchproc-ns1::"
panic: 
assert: mock: I don't know what to return because the method call was unexpected.
	Either do Mock.On("InsertTransaction").Return(...) first, or remove the InsertTransaction() call.
	This method was unexpected:
		InsertTransaction(*context.cancelCtx,*fftypes.Transaction)
		0: &context.cancelCtx{Context:(*context.valueCtx)(0xc000458bd0), mu:sync.Mutex{state:0, sema:0x0}, done:(chan struct {})(nil), children:map[context.canceler]struct {}(nil), err:error(nil)}
		1: &fftypes.Transaction{ID:(*fftypes.UUID)(0xc000023360), Namespace:"ns1", Type:"batch_pin", Created:(*fftypes.FFTime)(nil), BlockchainIDs:fftypes.FFStringArray(nil)}
	at: [plugin.go:2190 txcommon.go:53 batch_processor.go:307 batch_manager_test.go:461 plugin.go:2232 batch_processor.go:286 retry.go:55 batch_processor.go:285 batch_processor.go:368 asm_amd64.s:1371]

[peterbroadhurst]

Sadly it's timing rather than merge 🙃