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

ABCI++: Update new protos to use enum instead of bool #8158

Merged
merged 11 commits into from
Mar 21, 2022
Merged

Conversation

williambanfield
Copy link
Contributor

closes: #8039

This pull request updates the new ABCI++ protos to use enums in place of bools. enums may be preferred over bool because an enum can be udpated to include new statuses in the future, whereas a bool cannot and is fixed as just true or false over the whole lifecycle of the API.

Copy link
Contributor

@creachadair creachadair left a comment

Choose a reason for hiding this comment

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

LGTM, modulo the lint scribble.

Copy link
Contributor

@sergio-mena sergio-mena left a comment

Choose a reason for hiding this comment

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

Thanks a lot for taking care of this!

abci/types/types.go Outdated Show resolved Hide resolved
proto/tendermint/abci/types.proto.intermediate Outdated Show resolved Hide resolved
spec/abci++/abci++_methods_002_draft.md Show resolved Hide resolved
spec/abci++/abci++_methods_002_draft.md Show resolved Hide resolved
spec/abci++/abci++_methods_002_draft.md Outdated Show resolved Hide resolved
spec/abci++/abci++_methods_002_draft.md Outdated Show resolved Hide resolved
@sergio-mena sergio-mena added this to In progress in ABCI++ via automation Mar 21, 2022
@williambanfield williambanfield added the S:automerge Automatically merge PR when requirements pass label Mar 21, 2022
@williambanfield
Copy link
Contributor Author

@thanethomson heads up, this change now re-orders the .proto.intermediate file to match the .proto file to make inspecting the diff between the two a bit easier.

@mergify mergify bot merged commit cc838a5 into master Mar 21, 2022
ABCI++ automation moved this from In progress to Done Mar 21, 2022
@mergify mergify bot deleted the wb/issue-8039 branch March 21, 2022 16:57
@thanethomson
Copy link
Contributor

@thanethomson heads up, this change now re-orders the .proto.intermediate file to match the .proto file to make inspecting the diff between the two a bit easier.

Great stuff! 👍

@sergio-mena sergio-mena mentioned this pull request Jul 22, 2022
35 tasks
williambanfield added a commit that referenced this pull request Sep 9, 2022
closes: #8039

This pull request updates the new ABCI++ protos to use `enum`s in place of `bool`s. `enums` may be preferred over `bool` because an `enum` can be udpated to include new statuses in the future, whereas a `bool` cannot and is fixed as just `true` or `false` over the whole lifecycle of the API.
@williambanfield williambanfield restored the wb/issue-8039 branch September 9, 2022 16:11
faddat pushed a commit to notional-labs/tendermint that referenced this pull request Apr 3, 2023
* [cherry-picked] ABCI++: Update new protos to use enum instead of bool (tendermint#8158)

This pull request updates the new ABCI++ protos to use `enum`s in place of `bool`s. `enums` may be preferred over `bool` because an `enum` can be udpated to include new statuses in the future, whereas a `bool` cannot and is fixed as just `true` or `false` over the whole lifecycle of the API.

* Detect and handle UNKNOWN in `ResponseVerifyVoteExtension`

Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S:automerge Automatically merge PR when requirements pass
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

ABCI++: change "accept/reject" booleans by enum all over the API
4 participants