Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Implement BTCHeaderBytes and BTCHeaderHashBytes types #29

Merged
merged 7 commits into from
Jun 27, 2022

Conversation

vitsalis
Copy link
Member

@vitsalis vitsalis commented Jun 24, 2022

After the discussion on #26 , we agreed that there is a need for custom types that wrap Header and Header Hash bytes objects so that we can have control over our input and output formats depending on the context (CLI vs. RPC etc).

The types implementations are put under a global directory (a practice also followed by cosmos) since they are probably going to be needed by other modules (e.g. btccheckpoint) and implement various marshalling and umarshalling operations, most notably from hex strings to bytes and vice versa.

@vitsalis
Copy link
Member Author

Regarding Header Height: While having a type for it would be useful (instead of having uint64 everywhere), I believe that adding such custom type into our proto files will lead to unnecessary complexity. More specifically, having something like:

message Height {
    uint64 header_height = 1 [ (gogoproto.customtype) = "HeaderHeight"]
}

and in another file:

type HeaderHeight uint64

would lead to the Height.HeaderHeight being a reference to uint64, meaning that we would have difficulty performing arithmetic operations etc.

@aakoshh
Copy link
Contributor

aakoshh commented Jun 25, 2022

would lead to the Height.HeaderHeight being a reference to uint64, meaning that we would have difficulty performing arithmetic operations etc.

Would it? At least addition seems to work fine: https://go.dev/play/p/0FZkJKYjlnC

@@ -42,14 +43,18 @@ message QueryHashesRequest {

// QueryHashesResponse is response type for the Query/Hashes RPC method.
message QueryHashesResponse {
repeated bytes hashes = 1;
repeated bytes hashes = 1 [
(gogoproto.castrepeated) = "github.com/babylonchain/babylon/types.BTCHeaderHashesBytes"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't it not be a repeated BTCHeaderHashBytes?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

castrepeated expects a slice of the corresponding type. Changed this to customtype pointing to types.BTCHeaderHashBytes and works as expected.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mainly used this because it is used extensively in Cosmos, but requires defining a new type.

types/btc_header_bytes.go Outdated Show resolved Hide resolved
types/btc_header_bytes.go Outdated Show resolved Hide resolved
types/btc_header_bytes.go Outdated Show resolved Hide resolved
types/btc_header_bytes.go Outdated Show resolved Hide resolved
Comment on lines 20 to 21
var headerBytes bbl.BTCHeaderBytes
err := headerBytes.UnmarshalHex(headerHex)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would make sense to have a dedicated package for these types, like decimal.go in the SDK, so you can do this in one line, like headerBytes, err := btc_header.NewFromHex(headerHex) and similarly for hashes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now I added these methods under the bbl namespace.

Copy link
Contributor

@aakoshh aakoshh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Smashing! I admit I got a bit lost in the many marshalling variants, and perhaps the ergonomics could be improved so a variable declaration doesn't need to be separate from assignment, but otherwise it's a great leap! 👨‍🚀

@vitsalis
Copy link
Member Author

Thanks @aakoshh for the comments! Updates and answers have been posted.

Copy link
Contributor

@aakoshh aakoshh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the changes, looks good 👍

@vitsalis vitsalis merged commit f9716e3 into main Jun 27, 2022
@vitsalis vitsalis deleted the btclightclient-types branch June 27, 2022 10:14
KonradStaniec pushed a commit that referenced this pull request Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants