[R4R] Add proposer to NewRound event and proposal info to CompleteProposal event#2767
[R4R] Add proposer to NewRound event and proposal info to CompleteProposal event#2767ebuchman merged 14 commits intotendermint:developfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2767 +/- ##
===========================================
- Coverage 62.21% 62.15% -0.07%
===========================================
Files 212 212
Lines 17269 17212 -57
===========================================
- Hits 10744 10698 -46
+ Misses 5621 5608 -13
- Partials 904 906 +2
|
|
I was thinking that maybe we should also add the |
|
Hmm - can you briefly describe the use case here? We should try to think about the event msgs a bit more holistically before we start adding fields here and there. |
|
Sure. I am trying to build a live tendermint consensus visualizer with all of the critical steps for consensus essentially a live version of this flow chart https://tendermint.com/docs/assets/img/consensus_logic.e9f4ca6f.png Really the only thing I am missing is being able to highlight the proposer. Originally I was thinking it makes sense to include that in the |
|
Right of course. I think a NewRound msg makes sense. Currently we just use EventDataRoundState but I don't think we should add the proposer stuff to that. Instead we should probably have a new type, EventDataNewRound, that has the basic H/R/S info as well as a ProposerInfo field that contains the proposer's address and index. How's that sound? |
|
sounds good to me. will update |
ebuchman
left a comment
There was a problem hiding this comment.
Thanks! I'll address most of my comments and merge this :)
| Round int `json:"round"` | ||
| Step string `json:"step"` | ||
|
|
||
| BlockID BlockID `json:"block_id"` |
There was a problem hiding this comment.
Do we want to add the ValidatorInfo to this event as well? Seems it would make sense
There was a problem hiding this comment.
I had it there at first, but EventDataNewRound seems like the correct place to put it. Can add it back if you like.
There was a problem hiding this comment.
No worries. Let's leave it as is for now - can always add it later.
| } | ||
| } | ||
|
|
||
| func ensureProposal(proposalCh <-chan interface{}, height int64, round int, propId types.BlockID) { |
There was a problem hiding this comment.
What prompted the need for this if we already have ensureNewProposal?
There was a problem hiding this comment.
allows us to also assert details about the proposal (eg we want to assert this specific block is proposed vs a block is proposed). I took the pattern from ensureNewVote (though it isnt actually used at the moment) vs ensureVote
There was a problem hiding this comment.
Yep - we probably want to replace ensureNewProposal with this eventually so we're always asserting the correct value. Fine for now.
|
thanks! responded to leftover comments |
Uh oh!
There was an error while loading. Please reload this page.