helium
introduce ProcessedTransaction
Many of the endpoints that provide transactions (eg: /accounts/{ACCOUNT_PUBKEY}/activity, /v1/transactions/{TXN_HASH}) provide them with information about what block it was included in and at what time. This is different than the JSON RPC transaction endpoints, which do not include this information; it basically provides the protobuf definition of a transaction in JSON form.
This PR introduce ~~TransactionRecord~~ ProcessedTransaction which includes the height and time of the transaction.
Why not just put the height and time in the transaction result? This crate is not trying to be compatible with the jsonrpc api, right?
I believe @chrisabruce wanted to rely on the definitions here for JSON RPC too.
I personally like it too since one can use the Transaction type a little more interchangeably regardless of source.
I believe @chrisabruce wanted to rely on the definitions here for JSON RPC too.
I personally like it too since one can use the Transaction type a little more interchangeably regardless of source.
Perhaps I dislike the name since "Record" has no real meaning..
- Alternately we could enhance jsonrpc to include the height and time perhaps?
- We could make height and time optional in the Transaction type?
Perhaps I dislike the name since "Record" has no real meaning..
I am open to other naming. Record to me implies that it has been recorded on the blockchain, hence has an associated height and time. Or perhaps ProcessedTransaction would make you happier? Perhaps that provides some consistency with the PendingTransaction type.
- Alternately we could enhance jsonrpc to include the height and time perhaps?
This could work and I'm not strongly against a solution like that. However, I prefer the idea that enum Transaction correspond to the message blockchain_txn { oneof txn { } } definition. And so each struct would correspond to the underlying transaction; in other words, these are native Rust types for the proto definitions (we have omitted signature fields for example, but we should include those as Options if we pursue this philosophy). It would be nice to be able to implement From<Transaction> for the generated prost object BlockchainTxn.
- We could make height and time optional in the Transaction type?
I would argue against this one since we know a "processed" transaction should always have a height (and time). Adding Option seems like a way to make a single type catch-all when a solution of nested types provides flexibility AND a strong definition. In brief, I hate doing if let Some in a context when I know there's always something; I'd rather have a type that expresses that.
Revisiting this topic in #41 , I realized that there is in fact a transaction which has "height" in it: validator heartbeat making a nested struct (or JSON object) format even more important.
When ETL returns it, does it provide height that the validator wrote in the transaction or height of when the transaction was processed?