-
Notifications
You must be signed in to change notification settings - Fork 216
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
Rebase ABCI domain types onto main #1203
Merged
Merged
Changes from 1 commit
Commits
Show all changes
29 commits
Select commit
Hold shift + click to select a range
3021889
split A.4.1, proto tooling changes
hdevalence 38a5de3
split A.2, changelog abci
hdevalence b7f3125
split A.1, tendermint and abci changes
hdevalence 8bc6a97
split A.3, abci-in-rpc
hdevalence 3096775
clean up imports in abci crate
hdevalence e403925
Return to 0.34 ABCI encoding
hdevalence 5de9425
Cherry-pick chrono changes
mzabaluev b9b6596
fixup! split A.3, abci-in-rpc
hdevalence c02739a
split A.4.1.1, check_event_attrs
hdevalence 2e429e7
split A.4.1.2, some slice change?
hdevalence 064fb82
split A.4.1.3, b.data.get(0).is_none()
hdevalence 39f7210
split A.4.1.4, remove test attrs
hdevalence 60dafb4
split A.4.1.5, someting optiony
hdevalence 61e6bf5
clippy fix
hdevalence 08a6a1e
Remove unnessarily generated files
mzabaluev ce30fe6
Fix deliver_tx.events KV tests
mzabaluev 35b75fc
Remove Option wart in net_info::Response
mzabaluev 9667148
rpc: Revert Event member events to a KV map
mzabaluev 8f45e28
Merge branch 'main' into mikhail/abci-domain-types
mzabaluev 3aba2d2
Revert tests to checking decoded KVs
mzabaluev 6c38131
rpc: Restore base64 serialization of event KV tags
mzabaluev 0ca7299
Post-merge fixes for rpc tests
mzabaluev 3a92d77
rpc: Use the re-exported serializers mod
mzabaluev 8d31c4e
Remove duplicate changelog entries
mzabaluev f9ae6a4
Fix kvstore-test build
mzabaluev 308d060
Changelog entry for switching to Bytes
mzabaluev 35e3fe5
Remove a commented out line
mzabaluev 2a89abe
abci: Restore Client::set_option as deprecated
mzabaluev b21d86b
Update abci/src/client.rs
thanethomson File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
2 changes: 2 additions & 0 deletions
2
.changelog/unreleased/breaking-changes/1203-bytes-for-abci-proto.md
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| - `[tendermint-proto]` Use `Bytes` for byte array fields of ABCI protobuf types. | ||
| ([#1203](https://github.com/informalsystems/tendermint-rs/pull/1203)) | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thanethomson I've added a notice about this change, but as stated already, I'm not sure we need to go ahead with it for 0.26. This is certainly unrelated to domain types, it's applied piecemeal to a single proto module not as a consistent change throughout tendermint-proto, and has the overall smell of a premature optimization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is related to the domain types, because the ABCI domain types use
Bytesinternally, so unwinding this part of the ABCI domain types will break existing users of the ABCI domain types. This is not meaningfully a breaking change for anyone else, because there weren't users of the ABCI proto types other than the stub ABCI demo application. It's also not a breaking change relative tomaster, because this code has been merged on that branch (albeit unreleased) for about a year.If this change is unwound out, you'll have to edit all of the ABCI domain types, and then all the users will have to edit all their uses of those domain types. I don't think it would be good to put this on the critical path for doing a semver release of the
tendermintcrates that contains ABCI domain types.If the decision, later, is that the domain types should not use the
Bytestype, it would be much better to do that as a later change that can be modeled with semver. (For the record, I don't think this is a good decision, but in any case I think it should be separate from getting this code backported as-is).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @hdevalence for the detailed exposition.
As a late-to-the-scene comment, if I were into micro-optimizing domain type conversions to protobuf, I'd start with the
Protobuftrait forcing a clone of the entire structure, because the domain-to-raw conversion is implemented viaFrom<Self>, but theencode*methods take&self. But this is a subject for a larger rework.