babylonchain / bindings

Apache License 2.0
0 stars 0 forks source link

Add btc tip query #3

Closed KonradStaniec closed 1 year ago

KonradStaniec commented 1 year ago

Initial pr for returning data from our btc light client module.

There were few possibilities how return type could look like (BtcBlockHeaderInfo) :

  1. return whole BTCHeaderInfo from Babylon - main drawback is that contains header as bytes so it forces caller to use some btc library to deserialize it, also it is a bit internal representation and I think sharing it through ffi is a bit no-go
  2. return whole header as hex encoded bytes along with height - again it forces caller to use some btc library to deserialize it
  3. return deserialised header along with height - I think drawback here is that for fields merkle_root and prev_blockhash hex encoding format in a bit ambgious i.e it could be either standard hex encoding or btc hex encoding (reverse byte order). Imo it should be btc hex encoding for compatibility with every every btc infra there is.

Ultimately I have approach 3) as imo it is most easy for the callers, but if some one has other opinions this is pr to discuss it

SebastianElvis commented 1 year ago

Since this will always be a feature of smart contracts over Babylon, it might be better if we propose a systematic way for this. For example, we could make this project babylon-rust just like https://github.com/osmosis-labs/osmosis-rust, and move the babylon-proto project here. This way,

Wdyt?

KonradStaniec commented 1 year ago

I would be fine with either option, although I slightly prefer for us to be consistent across our types usage, so I would go with either (1) or (2) as I worry that the conversion might lead to inconsistencies. However, we can take care to implement it properly so no issue with this as well. However, certain properties are hidden such as the hash itself (although it can be computed) and the accumulated work of the header (although we only use it to make fork choice)

Hmm I kinda have a bit opposite view in regards to conversions, as I think each use case should have its own type i.e BTCHeaderInfo is our babylon internal representation and we should not expose it outside. So for me 1 is kinda the worst approach. 2 is debatable, but the fact that it is forcing users to somehow deserialize headers is a bit problematic.

As for other points, hash is hidden but can be indeed be recomputed if needed so it would be redundant to have it part of return type (If I was presented such an api, I would not be sure if I should use the hash in struct or should I compute it myself). And accumulated work imo is kinda implementation detail of btc light client.

Since this will always be a feature of smart contracts over Babylon, it might be better if we propose a systematic way for this. For example, we could make this project babylon-rust just like https://github.com/osmosis-labs/osmosis-rust, and move the babylon-proto project here. This way,

we don't need to create new or duplicated structs users can encode/decode the object with protobuf or json in any language NOTE: we will need to additionally use https://github.com/neoeinstein/protoc-gen-prost/tree/main/protoc-gen-prost-serde plugin for supporting json codec developers of smart contracts over Babylon have native access to the codec functionalities Babylon contract for phase 2/3 can also benefit from this babylon-rust library and does not need to maintain protobuf stuff on its own

So this maybe long term plan to move all rust related stuff into this separate babylon-rust library, although for now it does not solve any problems related with the bindings as:

In my perfect world cosmowasm bindings depends only on cosmowasm libraries, and have separate set of types designed for communication with vm (only requirement for those types is that they are json friendly)

KonradStaniec commented 1 year ago

I will merge this for for now, to unblock other queries, we can talk about it later in engineering meeting if necessary :)