Skip to content

impl lite::Commit for commit::SignedHeader#67

Merged
liamsi merged 1 commit intocometbft:lite_impl_simple_merkle_mergedfrom
yihuang:impl-lite-commit
Nov 13, 2019
Merged

impl lite::Commit for commit::SignedHeader#67
liamsi merged 1 commit intocometbft:lite_impl_simple_merkle_mergedfrom
yihuang:impl-lite-commit

Conversation

@yihuang
Copy link
Contributor

@yihuang yihuang commented Nov 13, 2019

Implements the lite::Commit trait on the commit::SignedHeader struct. This may sound a bit confusing as there also is a commit::Commit struct. The latter does not now about the chain-id and hence we use the SignedHeader's header to fill in the chain-ids into the votes (which implments SignedVote).

(Description added by @liamsi)

Copy link
Contributor

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

Cool! Thanks a lot @yihuang for implementing the lite::Commit trait. I left a comment.

We can still merge this as is into my branch (which is still a draft PR #63 anyways) and can see where the impl should live afterwards.

//! `/commit` endpoint JSONRPC wrapper

use crate::{block, rpc};
use crate::{amino_types::vote::CanonicalVote, block, lite, rpc, vote::SignedVote, Hash};
Copy link
Contributor

Choose a reason for hiding this comment

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

The rpc module currently does not depend on amino_types. I think, if possible, we should try to keep it that way. Even if that means introducing another type under tendermint/block (or similar).

@liamsi liamsi merged commit 3e987fe into cometbft:lite_impl_simple_merkle_merged Nov 13, 2019
@liamsi
Copy link
Contributor

liamsi commented Nov 13, 2019

Did the suggested changes here: e1a7560

@yihuang
Copy link
Contributor Author

yihuang commented Nov 13, 2019

Great, thanks.

let chain_id = self.header.chain_id.to_string();
let mut votes = self.commit.precommits.clone().into_vec();
votes
.drain(..)
Copy link
Contributor

Choose a reason for hiding this comment

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

@yihuang Why use drain here rather than iter ?

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, my fault, not familiar enough with idiomatic rust then. But I believe this code already modified by @liamsi in the final merged version ;D

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