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: Add bls pubkey set api #232

Merged
merged 1 commit into from
Dec 5, 2022
Merged

feat: Add bls pubkey set api #232

merged 1 commit into from
Dec 5, 2022

Conversation

gitferry
Copy link
Contributor

@gitferry gitferry commented Dec 2, 2022

This PR added BLS public key set query. Adding this is for the Verifier to verify BTC raw checkpoints.

Copy link
Member

@SebastianElvis SebastianElvis left a comment

Choose a reason for hiding this comment

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

Lgtm! Just some minor comments without blocker 💯

end = total
}
var copiedValBLSKeys []*types.ValidatorWithBlsKey
err = copier.Copy(&copiedValBLSKeys, valBLSKeys[start:end])
Copy link
Member

Choose a reason for hiding this comment

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

Why do we want to copy it rather than using the slice directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want a deep copy since we don't want the passed slice to be affected by the original one. But, in this specific case, we do not need to copy it

}

total := uint64(len(valBLSKeys))
start := req.Pagination.Offset
Copy link
Member

Choose a reason for hiding this comment

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

I remember Cosmos SDK provides functionalities for getting the paginated response given a pagination request, e.g., something like https://github.com/babylonchain/babylon/blob/dev/x/epoching/keeper/grpc_query.go#L67-L92

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, thanks for the pointer. Thing is that the set is obtained from epoching store and sorted. I'm not sure how to use this functionality in our case

Comment on lines +28 to +34
ek := helper.EpochingKeeper
ck := helper.App.CheckpointingKeeper
querier := checkpointingkeeper.Querier{Keeper: ck}
queryHelper := baseapp.NewQueryServerTestHelper(helper.Ctx, helper.App.InterfaceRegistry())
types.RegisterQueryServer(queryHelper, querier)
queryClient := types.NewQueryClient(queryHelper)
msgServer := checkpointingkeeper.NewMsgServerImpl(ck)
Copy link
Member

Choose a reason for hiding this comment

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

It seems that L28-34 will be used quite frequently. Maybe they can be a part of the response of testepoching.NewHelper?

Copy link
Contributor Author

@gitferry gitferry Dec 5, 2022

Choose a reason for hiding this comment

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

Would that be strange that we have checkpointing stuff inside an epoching test helper? I have thought about having a helper specifically for checkpointing, but I think that would be a little bit heavy for now

Comment on lines +108 to +110
Pagination: &query.PageRequest{
Offset: 1,
},
Copy link
Member

Choose a reason for hiding this comment

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

Great, you even have a test for the pagination 👍

Copy link
Member

@vitsalis vitsalis left a comment

Choose a reason for hiding this comment

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

Nice!

@gitferry gitferry merged commit e52cbae into dev Dec 5, 2022
@gitferry gitferry deleted the checkpointing/bls-key-set branch December 5, 2022 07:28
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.

3 participants