context: store commit info #748
Conversation
context.go
Outdated
| @@ -72,6 +73,22 @@ func GetHeader(ctx Context) (abci.Header, bool) { | |||
| return val, ok | |||
| } | |||
|
|
|||
| // WithCommitInfo sets the info on who signed the block in this Context. | |||
| // Panics if already set. | |||
| func WithCommitInfo(ctx Context, info abci.LastCommitInfo) Context { | |||
There was a problem hiding this comment.
I try not to use any abci/tendermint data structures in the weave internals to provide a loose coupling, also so all structs are documented in our codebase (I need to check this out, and follow imports into tendermint to even see what is in abci.LastCommitInfo).
Can you just make an abci.LastCommitInfo -> weave.CommitInfo helper as in genesis file, and then we can discuss if all fields are needed? Or anything else?
There was a problem hiding this comment.
Sounds good to me, I wanted to validate the approach with you first.
I'm kind of ambivalent about the approach that should be taken, because our app is still pretty tightly coupled to tendermint in terms of interfaces/types it should support. One good reason to use tendermint structs is that when they change any of their types with tendermint upgrade - we get some if not all of that update for free.
In terms of digging to deps in order to see what they do, I guess I'm too spoiled using a fully fledged IDE to even notice, but it might definitely help a lot with IDEless PR reviews as well as means of documenting stuff. The downside is that we have to follow tendermints objects and duplicate the impl.
|
@ethanfrey please take a look |
Codecov Report
@@ Coverage Diff @@
## master #748 +/- ##
========================================
- Coverage 66.4% 66.3% -0.2%
========================================
Files 190 191 +1
Lines 10454 10473 +19
========================================
+ Hits 6948 6949 +1
- Misses 2680 2698 +18
Partials 826 826
Continue to review full report at Codecov.
|
|
|
||
| // CommitInfoFromABCI converts abci commit info to weave native type. | ||
| // This struct represents validator signatures on the current block. | ||
| func CommitInfoFromABCI(info abci.LastCommitInfo) CommitInfo { |
There was a problem hiding this comment.
Yeah, I am happy to have this a native weave type, even if we mirror their types.
Just to keep our API clean.
In the end, even if we do something like
type CommitInfo = abci.CommitInfo
and everyone imports weave.CommitInfo, then at least we decouple it there, so we could redefine CommitInfo in weave and then all packages use the new definition (complete with compile errors if there are inconsistent changes)
| @@ -10,3 +12,22 @@ package weave; | |||
| message Metadata { | |||
| uint32 schema = 1; | |||
| } | |||
|
|
|||
| // CommitInfo is basically a replica of tendermint commit info | |||
|
|
||
| // GetCommitInfo returns the info on validators that signed | ||
| // this block. Returns false if not present. | ||
| func GetCommitInfo(ctx Context) (CommitInfo, bool) { |
There was a problem hiding this comment.
It might be better to call it MustCommitInfo. There was similar issue with the BlockTime function that was renamed to MustBlockTime.
There was a problem hiding this comment.
sounds good to me.
There was a problem hiding this comment.
yeah, but I can't see this anywhere by the way, so leaving this as is for consistency. If we are about to change things up to Must we shall do it in one go I think.
Fixes #79
@ethanfrey Please validate that this is the right info.
I'm not sure if we want to customise any of that or use our own data structures.