replacing abci protobuf types with plain golang types (part 1)#2935
replacing abci protobuf types with plain golang types (part 1)#2935
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2935 +/- ##
=======================================
Coverage 68.42% 68.42%
=======================================
Files 5 5
Lines 456 456
=======================================
Hits 312 312
Misses 114 114
Partials 30 30
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
pompon0
changed the title
masih
left a comment
masih
left a comment
There was a problem hiding this comment.
One potential blocker on backward compatibility. Left a bunch of suggestions worth taking a look.
Loving the 3X code reduction diff 🙌
| ) | ||
|
|
||
| //go:generate ../../scripts/mockery_generate.sh Client | ||
|
|
| } | ||
|
|
||
| // RequestProcessProposal is the plain Go replacement for the legacy protobuf message. | ||
| type RequestProcessProposal struct { |
There was a problem hiding this comment.
You can extract the common fileds across this type and RequestFinalizeBlock into an unexported type, e.g. blockRequestBase to reduce boiler plating. Something like this:
type blockRequestBase struct {
Txs [][]byte
ByzantineValidators []Misbehavior
Hash []byte
Height int64
Time time.Time
NextValidatorsHash []byte
ProposerAddress []byte
AppHash []byte
ValidatorsHash []byte
ConsensusHash []byte
DataHash []byte
EvidenceHash []byte
LastBlockHash []byte
LastBlockPartSetTotal int64
LastBlockPartSetHash []byte
LastCommitHash []byte
LastResultsHash []byte
}
Then these types would become:
type RequestProcessProposal struct {
blockRequestBase
ProposedLastCommit CommitInfo
}
type RequestFinalizeBlock struct {
blockRequestBase
DecidedLastCommit CommitInfo
}
Which makes semantic differences obvious/easy to see.
There was a problem hiding this comment.
the plan is to use block header, because that's where all these fields come from, and it doesn't seem feasible to make block header an implementation detail any time soon (there are APIs and storage depending on it). Refactoring this will be the next step.
| LastBlockPartSetHash []byte | ||
| LastCommitHash []byte | ||
| LastResultsHash []byte | ||
| } |
There was a problem hiding this comment.
Thinking out loud: Do we need validator for these?
There was a problem hiding this comment.
I mean: do any of the types here need validation? We might have validation already somewhere. Checking that we do validate fields if they come off the wire.
There was a problem hiding this comment.
these are passed from consensus to application. Application does not interpret most of these, doesn't even access many of those, but still persists it at places. Presumably so that the header hash can be computed by light clients or sth.
| tmproto "github.com/sei-protocol/sei-chain/sei-tendermint/proto/tendermint/types" | ||
| ) | ||
|
|
||
| // RequestLoadLatest is the plain Go replacement for the legacy protobuf message. |
There was a problem hiding this comment.
Would be nice to also document the purpose of the type as well as its origin. Ditto for the rest.
| LastResultsHash []byte | ||
| } | ||
|
|
||
| func (m *RequestProcessProposal) GetTxs() [][]byte { |
There was a problem hiding this comment.
Beyond the scope of this PR:
Do we still need these getters? Aside from >400 lies of bilerplate they add, these have a hidden cost in that they encourage pointer passing. They make sense for a one-size-cut-all Proto generator but for hand-rolled types we could be more concise/robust.
I understand that removing these needs a careful review to make sure we are not missing any nil checks. But we all will be better off for it: in that it forces us to validate earlier, and interrupt the execution flow when it makes no sense to proceed. e.g. when *RequestProcessProposal is nil.
There was a problem hiding this comment.
I don't like the getters either, although they do not block my goal of reducing the api surface. +1 for getting rid of them though
masih
left a comment
masih
left a comment
There was a problem hiding this comment.
The one blocker comment discussed/resolved. Approving
3 participants
I'm working on reducing the API surface between consensus and Application for autobahn migration. This PR replaces protobuf types with plain golang types to reduce compatibility requirements. Will continue in the next pr.