refactor(recovery): return RecoveryClaim from ClaimPendingTransactions

[EvanYan1024]

Closes #1714

Follow-up to #1708.

Summary

ClaimPendingTransactions previously returned []*TransactionRecord, but the only consumer (recovery.Manager) reads just TxID and StoredAt — the other ten fields (action type, amounts, application/public metadata, status, ...) were always discarded.

Introduce a dedicated lightweight type in the driver layer, mirroring the existing RecoveryClaimParams input type:

type RecoveryClaim struct {
    TxID     string
    StoredAt time.Time
}

…and project only the two columns the recovery loop actually needs from SQL.

Concrete savings

• No more transactions JOIN. Both the postgres atomic claim and the common permissive query now read tx_id, stored_at directly from requests. The postgres CTE collapses to a single UPDATE ... RETURNING (tx_id, stored_at); the previous outer SELECT existed only to recover stored_at via a second join.
• Two JSON unmarshals per row removed (application_metadata, public_metadata).
• Eight unused columns dropped from the SELECT projection (action_type, sender_eid, recipient_eid, token_type, amount, status, application_metadata, public_metadata).
• Internal pendingTx struct in recovery.Manager removed — *RecoveryClaim is now used directly as the work-channel element, eliminating a redundant intermediate type.
• Dedupe-by-TxID block in Manager.recoverTransactions removed (see "Semantic change" below).

Semantic change — please confirm

olderThan is now compared against requests.stored_at (system insert time) instead of transactions.stored_at (the caller-supplied TransactionRecord.Timestamp).

Rationale: the recovery loop's notion of "how long has this row been stuck in pending" — which NotFoundGracePeriod in #1708 already relies on — is a system-time question, not a business-time one. The previous JOIN against transactions was using a caller-controlled field whose meaning depends on whoever calls AddTransaction, making the recovery clock implicitly dependent on the producer's choice of Timestamp. Anchoring on requests.stored_at makes the filter robust against that.

In typical usage AddTokenRequest and AddTransaction are committed in the same SQL transaction, so the two columns are within milliseconds of each other and observable behaviour is identical. The change matters only when a caller deliberately writes a backdated TransactionRecord.Timestamp — which the recovery loop should arguably not depend on.

Flagging this explicitly because it goes a bit beyond the literal description in #1714. Happy to revert this specific point if @adecaro / maintainers prefer to keep the historical behaviour.

Dedupe block removed

Manager.recoverTransactions used to dedupe by TxID because the previous SQL joined against transactions (which fans out to one row per movement/output for a single tx_id). The new query reads from requests where tx_id is the primary key, so each returned claim is unique by construction and the seen-map became dead weight.

Scope (5 layers)

1. db/driver/{ttx,audit}.go — new RecoveryClaim type + interface signature.
2. db/sql/common/transactions.go — query reads tx_id, stored_at from requests only; no join.
3. db/sql/postgres/transactions.go — CTE collapses to UPDATE ... RETURNING; dead unmarshalMetadata + encoding/json import removed.
4. {ttxdb,auditdb}/store.go — StoreService signature + re-exported type alias.
5. network/fabric/network.go + recovery/manager.go — adapter and recovery.Storage interface signatures; pendingTx and the dedupe-by-TxID block deleted, RecoveryClaim consumed directly by the worker goroutines.

Mocks regenerated via counterfeiter v6.12.2 (recovery/mock/storage.go, auditor/mock/audit_transaction_store.go).

Behaviour preserved

• The NotFoundGracePeriod orphan-promotion logic from fix(recovery): unblock queue head by marking NotFound orphans Deleted after grace period #1708 is unchanged (and now lines up with olderThan semantically — both are system-time anchored).

Test plan

• go build ./...
• go vet ./...
• go test ./token/services/storage/recovery/...
• go test ./token/services/storage/... — sqlite, ttxdb, auditdb, tokendb, walletdb all green
• go test ./token/services/auditor/... ./token/services/network/fabric/...
• make testing-docker-images && go test -race ./token/services/storage/db/sql/postgres -count=1 — full postgres suite green (this is the path that was red on the first CI run; tests now seed requests.stored_at via a new ageRequests helper so the filter actually matches)
• CI green

[adecaro]

Thanks a lot, @EvanYan1024 . I have left a few comments to the code to simplify it even further.

I would also probe your opinion on introducing a new state for this transactions that are kind of orphaned. This would allows us to find them more easily and give them a second chance.
What do you think?

[EvanYan1024]

I would also probe your opinion on introducing a new state for this transactions that are kind of orphaned. This would allows us to find them more easily and give them a second chance.

@adecaro to make sure I'm reading you right — is the proposal to change the SetStatus(ctx, txID, storage.Deleted, ...) call I added in #1708 (recovery/manager.go:331, on
the NotFoundGracePeriod path) so it sets a new dedicated Orphan state instead? That way Deleted stays reserved for explicit operator/user deletion, and "tried to
commit, never landed on chain within the grace window" gets its own unambiguous signal.

[adecaro]

I would also probe your opinion on introducing a new state for this transactions that are kind of orphaned. This would allows us to find them more easily and give them a second chance.

@adecaro to make sure I'm reading you right — is the proposal to change the SetStatus(ctx, txID, storage.Deleted, ...) call I added in #1708 (recovery/manager.go:331, on the NotFoundGracePeriod path) so it sets a new dedicated Orphan state instead? That way Deleted stays reserved for explicit operator/user deletion, and "tried to commit, never landed on chain within the grace window" gets its own unambiguous signal.

Yes, this is what I meant. We can introduce this new state in a new PR. That's also fine.

[EvanYan1024]

I would also probe your opinion on introducing a new state for this transactions that are kind of orphaned. This would allows us to find them more easily and give them a second chance.

@adecaro to make sure I'm reading you right — is the proposal to change the SetStatus(ctx, txID, storage.Deleted, ...) call I added in #1708 (recovery/manager.go:331, on the NotFoundGracePeriod path) so it sets a new dedicated Orphan state instead? That way Deleted stays reserved for explicit operator/user deletion, and "tried to commit, never landed on chain within the grace window" gets its own unambiguous signal.

Yes, this is what I meant. We can introduce this new state in a new PR. That's also fine.

Okay. I think that's fine.

[adecaro]

Hi @EvanYan1024 , please, double check the DCO and the unit-test to see if it is a flaky test or something else. Thanks 🙏

[adecaro]

LGTM. Thanks @EvanYan1024 🙏