feat: checkpointing/implement cli #78
Conversation
crypto/bls12381/types.go
Outdated
| @@ -69,6 +70,14 @@ func (sig Signature) Equal(s Signature) bool { | |||
| return string(sig) == string(s) | |||
| } | |||
|
|
|||
| func NewBLSSigFromHex(s string) (Signature, error) { | |||
| return hex.DecodeString(s) | |||
There was a problem hiding this comment.
This way, you do not perform the checks that unmarshal does. Overall, in order to ensure consistency, you should try to use only one entry point. Something like this:
| return hex.DecodeString(s) | |
| bz, err := hex.DecodeString(s) | |
| if err != nil { | |
| return nil, err | |
| } | |
| var sig Signature | |
| sig.Unmarshal(bz) | |
| return sig, nil |
crypto/bls12381/types.go
Outdated
| } | ||
|
|
||
| func (sig Signature) String() string { | ||
| return hex.EncodeToString(sig) |
There was a problem hiding this comment.
| return hex.EncodeToString(sig) | |
| return hex.EncodeToString(sig.MustMarshal()) |
x/checkpointing/types/types.go
Outdated
| @@ -86,6 +87,10 @@ func (cm *RawCheckpointWithMeta) Accumulate( | |||
| return true, nil | |||
| } | |||
|
|
|||
| func NewLastCommitHashFromHex(s string) (LastCommitHash, error) { | |||
| return hex.DecodeString(s) | |||
There was a problem hiding this comment.
Decode first and then use unmarshall so checks are performed
x/checkpointing/types/msgs.go
Outdated
|
|
||
| var ( | ||
| // Ensure that MsgInsertHeader implements all functions of the Msg interface | ||
| _ sdk.Msg = (*MsgAddBlsSig)(nil) | ||
| ) | ||
|
|
||
| func NewMsgAddBlsSig(epochNum uint64, lch LastCommitHash, sig bls12381.Signature, addr sdk.ValAddress) (*MsgAddBlsSig, error) { |
There was a problem hiding this comment.
There is no need for an error here, at least at this point.
| sdkCtx := sdk.UnwrapSDKContext(ctx) | ||
|
|
||
| store := k.CheckpointsState(sdkCtx).checkpoints | ||
| pageRes, err := query.FilteredPaginate(store, req.Pagination, func(key []byte, value []byte, accumulate bool) (bool, error) { |
There was a problem hiding this comment.
The key parameter is not used, maybe replace with _?
crypto/bls12381/types.go
Outdated
| } | ||
|
|
||
| func (sig Signature) String() (string, error) { | ||
| bz, err := sig.Marshal() |
There was a problem hiding this comment.
Maybe use MustMarshal here? If something succeeded in becoming a Signature, then if we cannot marshal it, this means that something went super wrong.
x/checkpointing/client/cli/query.go
Outdated
| // CmdRawCheckpointList defines the cobra command to query raw checkpoints by status | ||
| func CmdRawCheckpointList() *cobra.Command { | ||
| cmd := &cobra.Command{ | ||
| Use: "raw-checkpoint-list [epoch_number]", |
There was a problem hiding this comment.
The description says querying checkpoints by status, but the parameter is the epoch_number.
x/checkpointing/client/cli/query.go
Outdated
|
|
||
| status, exists := types.CheckpointStatus_value[args[0]] | ||
| if !exists { | ||
| return errors.New("checkpoint not found") |
There was a problem hiding this comment.
The message should denote that the status parameter does not correspond to a valid status.
| } | ||
|
|
||
| func (lch *LastCommitHash) Unmarshal(bz []byte) error { | ||
| *lch = bz |
There was a problem hiding this comment.
Do you need to perform any checks here?
There was a problem hiding this comment.
We don't need that because Cosmos is not aware of the content of the LastCommitHash, which comes from Tendermint. Thanks for pointing that out though.
There was a problem hiding this comment.
So we are not checking something like the length, because the canonic hash to be signed is in the checkpoint. If they send anything else, it will just be rejected.
|
|
||
| func CmdTxAddBlsSig() *cobra.Command { | ||
| cmd := &cobra.Command{ | ||
| Use: "submit [epoch_number] [last_commit_hash] [bls_sig] [signer address]", |
There was a problem hiding this comment.
Shouldn't it rather ask for the BLS private key file for example, or the name of the BLS key in the keyring, so that it can sign for you? Or maybe have a CLI command to sign, before you can call submit?
There was a problem hiding this comment.
Oh this CLI is just for submission. Signing will be in a future CLI
| return err | ||
| } | ||
|
|
||
| return clientCtx.PrintProto(res) |
There was a problem hiding this comment.
So this will print the raw checkpoint in JSON, which has a field that you should sign, right?
There was a problem hiding this comment.
Are you meaning signing LastCommitHash and the epoch number using the querier's BLS private key? If so, yes
This PR fixes BM-40.
In particular, this PR implements tx and query cli for the checkpointing module.
I also added @vitsalis as a reviewer since this PR is mostly influenced by btclightclient's cli.