-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
abci: Synchronize FinalizeBlock with the updated specification #7983
Conversation
3124f6e
to
9e002b8
Compare
…ze' into wb/abci-finalize-block-synchronize
Before I had read the next paragraph, I was going to suggest just keeping the original JSON field tag. So yeah, let's plan on that. This might be a little tedious because of the way protobuf names map to JSON, but we can shim it in the RPC if we have to.
Right now, as far as I can tell, it's entirely manual. In |
TODO:
|
@sergio-mena, Before I go creating a type to wrap the I'm wondering if you feel strongly about keeping the internal type name as The field is within a |
My view is that, if we risk breaking IBC due to renaming "events" to "tx_events", it's not worth it. So I think I'm aligned with you on this. Nevertheless, if you rename it, please adapt the corresponding field name in the spec. On a somewhat related point. I decided on the |
Switched to the old name, made sure to update the spec as well. |
This change set implements the most recent version of `FinalizeBlock`. * This change set is rather large but fear not! The majority of the files touched and changes are renaming `ResponseDeliverTx` to `ExecTxResult`. This should be a pretty inoffensive change since they're effectively the same type but with a different name. * The `execBlockOnProxyApp` was totally removed since it served as just a wrapper around the logic that is now mostly encapsulated within `FinalizeBlock` * The `updateState` helper function has been made a public method on `State`. It was being exposed as a shim through the testing infrastructure, so this seemed innocuous. * Tests already existed to ensure that the application received the `ByzantineValidators` and the `ValidatorUpdates`, but one was fixed up to ensure that `LastCommitInfo` was being sent across. * Tests were removed from the `psql` indexer that seemed to search for an event in the indexer that was not being created. * We store this [ABCIResponses](https://github.com/tendermint/tendermint/blob/5721a13ab1f4479f9807f449f0bf5c536b9a05f2/proto/tendermint/state/types.pb.go#L37) type in the data base as the block results. This type has changed since v0.35 to contain the `FinalizeBlock` response. I'm wondering if we need to do any shimming to keep the old data retrieveable? * Similarly, this change is exposed via the RPC through [ResultBlockResults](https://github.com/tendermint/tendermint/blob/5721a13ab1f4479f9807f449f0bf5c536b9a05f2/rpc/coretypes/responses.go#L69) changing. Should we somehow shim or notify for this change? closes: #7658
This change set implements the most recent version of
FinalizeBlock
.What does this change actually contain?
ResponseDeliverTx
toExecTxResult
. This should be a pretty inoffensive change since they're effectively the same type but with a different name.execBlockOnProxyApp
was totally removed since it served as just a wrapper around the logic that is now mostly encapsulated withinFinalizeBlock
updateState
helper function has been made a public method onState
. It was being exposed as a shim through the testing infrastructure, so this seemed innocuous.ByzantineValidators
and theValidatorUpdates
, but one was fixed up to ensure thatLastCommitInfo
was being sent across.psql
indexer that seemed to search for an event in the indexer that was not being created.Questions for reviewers
FinalizeBlock
response. I'm wondering if we need to do any shimming to keep the old data retrieveable?closes: #7658