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

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

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
Member

@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.

@@ -1,6 +1,6 @@
//! `/commit` endpoint JSONRPC wrapper

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

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 informalsystems:lite_impl_simple_merkle_merged Nov 13, 2019
@liamsi
Copy link
Member

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
Member

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