btccheckpoint: Add information about transactions to BTCCheckpointInfo #312
Conversation
There was a problem hiding this comment.
This could be a great optimisation for API backend! I'm fine about this change, but it would be good to wait for feedbacks from @KonradStaniec
| @@ -16,10 +17,10 @@ import ( | |||
|
|
|||
| var _ types.QueryServer = Keeper{} | |||
|
|
|||
| func (k Keeper) lowestBtcHeightAndHash(ctx sdk.Context, subKey *types.SubmissionKey) (uint64, []byte, error) { | |||
| func (k Keeper) lowestBtcHeightAndHash(ctx sdk.Context, subKey *types.SubmissionKey) (uint64, *bbntypes.BTCHeaderHashBytes, error) { | |||
There was a problem hiding this comment.
Given that bbntypes.BTCHeaderHashBytes is basically []byte which is already a pointer, should *bbntypes.BTCHeaderHashBytes be bbntypes.BTCHeaderHashBytes?
There was a problem hiding this comment.
the tk.Hash attribute is a *bbntypes.BTCHeaderHashBytes and below we were doing a pointer dereference for it. I changed it to do a direct assignment to avoid the pointer dereference here.
| @@ -38,7 +39,7 @@ func (k Keeper) lowestBtcHeightAndHash(ctx sdk.Context, subKey *types.Submission | |||
|
|
|||
There was a problem hiding this comment.
as we are making changes here, maybe we can remove panic above ?
In summary, queries which are not routed through abci query method are not synchornized in any way with tendermint, so it is entirely possible that bettween CheckHeaderIsOnMainChain and GetBlockHeight there was new block processed which could cause btc re-org.
At some point we should possibly review all our api-s for panics, which could happen due to to concurreny between block processing and api server.
There was a problem hiding this comment.
Agreed, though maybe we can add this to another PR to have a cleaner history and be able to revert if needed?
There was a problem hiding this comment.
sure, then I will fix this in another pr 👍
| @@ -74,6 +75,7 @@ func (k Keeper) getCheckpointInfo(ctx sdk.Context, epochNum uint64, subKeys []*t | |||
| if headerNumber < info.EarliestBtcBlockNumber { | |||
| info.EarliestBtcBlockNumber = headerNumber | |||
| info.EarliestBtcBlockHash = headerHash | |||
| info.EarliestBtcBlockTxs = sd.TxsInfo | |||
There was a problem hiding this comment.
do you think we need all data from TxsInfo or all tx indexs would be enough ? Just asking as I usually prefer to return as little data as necessary, this way we do not constrain ourselves in the future i.e we can add/delete data from transaction info however we want, but as soon as other tooling starts depend on it, it becomes much harder.
There was a problem hiding this comment.
Transaction indexes would be enough I believe, if we go entirely by explorer needs. However, I think that transaction hashes would be useful as well, as we aim to give information about the checkpoint in total. Overall, I'm against only giving the required information for the explorer, but instead providing responses and results that "would make sense" to include.
There was a problem hiding this comment.
I am sympathetic to make sense approach but for different people usually different things makes sense.
In general imo this endpoint is not really well thought (I remember I was adding it in a bit of haste before demo to fit explorer so we can see something 😅 )
Imo:
- It should have different name as BtcCheckpointInfo does not really tell much, especially in context of btccheckpoint module which operates on submissions not checkpoints. When looking at it we really return btc height of current best submission. So maybe
EpochBestSubmissionBtcInfoor something like that would be better. - It should take additional query bool param to indicate if it should return full results (full header, full transactions and proof) or simplified results (only hash of header, only hashes and indexes of transactions, no proofs). Or maybe we can have two endpoints, one for simplified results and for full results.
- if checkpoints is composed of two headers it should return info about both of them, and have separate field for height which is assigned to this best subbmission.
so tldr., maybe lets leave it as it is for now, and create task to improving this endpoint in general ?
There was a problem hiding this comment.
Agreed with all the points. Will create a Jira ticket for this. Overall, I think we need to rethink a lot of our query structure to return all the information that is contained inside our modules in a structured manner as many endpoints were created 1 by 1 to fill different external needs. Now that we have a much better picture of the overall requirements from our queries we can design something more elegant.
There was a problem hiding this comment.
Totally agree on need to re-vist our exteral api design.
Also one thing which need improvement from code quality pov, is that in many cases in our apis we use data types which we use to store data in database. This creates strong coupling between our database representation and our api which is not good. Good example is, TransactionInfo which is used in btccheckpoint module database, but also in few apis. This is not great, as if btccheckpoint would need to add some additional fields there, those would automatically be exposed through api, which is not always desired.
In general, types/protos used in apis should be separate from types/protos used in database, and there should be small middleware layer translating between those types. This way we will never expose accidental data through api, and each module will have total freedom in case of database representation.
Given that the API provides information about a BTC checkpoint it should also provide information about the transactions corresponding to it. This will also be useful for the front-end to demonstrate the transaction index inside a BTC block corresponding to a Babylon checkpoint. Overall, this PR follows the philosophy of only returning stuff for the earliest submission.