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

Align Vote/Proposal fields with canonical order and fields #2730

Merged
merged 5 commits into from
Oct 30, 2018

Conversation

liamsi
Copy link
Contributor

@liamsi liamsi commented Oct 30, 2018

resolves #2727

  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG_PENDING.md

POLBlockID: polBlockID,
}
}

// String returns a string representation of the Proposal.
func (p *Proposal) String() string {
return fmt.Sprintf("Proposal{%v/%v %v (%v,%v) %X @ %s}",
p.Height, p.Round, p.BlockPartsHeader, p.POLRound,
return fmt.Sprintf("Proposal{%X(Proposal) %v/%v %v (%v,%v) %X @ %s}",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need the %X(Proposal) since we already have Proposal{

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thought I make it consistent with Vote but you are right. Removed.

@codecov-io
Copy link

Codecov Report

Merging #2730 into develop will decrease coverage by 0.05%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #2730      +/-   ##
===========================================
- Coverage     62.2%   62.15%   -0.06%     
===========================================
  Files          211      211              
  Lines        17056    17048       -8     
===========================================
- Hits         10610    10596      -14     
- Misses        5573     5576       +3     
- Partials       873      876       +3
Impacted Files Coverage Δ
privval/ipc_server.go 64.15% <0%> (-9.44%) ⬇️
libs/autofile/autofile.go 76.31% <0%> (-2.64%) ⬇️
consensus/reactor.go 70.83% <0%> (-0.39%) ⬇️
consensus/state.go 75.93% <0%> (-0.36%) ⬇️
privval/priv_validator.go 70.83% <0%> (-0.19%) ⬇️
p2p/pex/pex_reactor.go 74.33% <0%> (+0.33%) ⬆️
blockchain/pool.go 66.43% <0%> (+0.69%) ⬆️
privval/tcp_server.go 81.42% <0%> (+2.85%) ⬆️

@liamsi
Copy link
Contributor Author

liamsi commented Oct 30, 2018

Updated but there seem to be some problems with circleci pulling the docker image:

Error pulling image circleci/golang:1.10.3: Error response from daemon: received unexpected HTTP status: 503 Service Unavailable... retrying
  image is cached as circleci/golang:1.10.3, but refreshing...

@ebuchman
Copy link
Contributor

Still WIP?

@ebuchman
Copy link
Contributor

Oh we need to update spec

@liamsi liamsi requested a review from zramsay as a code owner October 30, 2018 15:47
@liamsi
Copy link
Contributor Author

liamsi commented Oct 30, 2018

updated the spec. Should I add an entry to the changelog, or changelog pending?

@liamsi liamsi changed the title WIP: Align Vote/Proposal fields with canonical order and fields Align Vote/Proposal fields with canonical order and fields Oct 30, 2018
@ebuchman
Copy link
Contributor

Should I add an entry to the changelog, or changelog pending?

We're in a twilight zone re changelog right now so don't worry about it, I'll handle it on the release front.

Thanks!

types/vote.go Outdated
cmn.Fingerprint(vote.ValidatorAddress),
vote.Height,
vote.Round,
return fmt.Sprintf("Vote{%v(%v) %v/%02d @ %s %X %v:%X %X}",
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I don't think we should change the order in the print statement. It's super convenient for humans to have the validator info at the front

@ebuchman ebuchman merged commit a530352 into develop Oct 30, 2018
@ebuchman ebuchman deleted the align_msgs_with_canoncial branch October 30, 2018 16:16
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