Rebase ABCI domain types onto main #1203
Conversation
Replace chrono with time 0.3 (#1030) * pbt-gen: Converted from chrono to time 0.3 chrono has soundness issues (see RUSTSEC-2020-0159) and does not seem to be maintained. * Replace dependency on chrono with time 0.3 Change Time implementation to crate time: chrono has soundness issues (see RUSTSEC-2020-0159) and does not seem to be actively maintained. * Add Time methods checked_add and checked_sub These should be used instead of the overloaded operators that broke the operator convention by returning a Result. * proto: Don't use formatting methods of time Drop the "formatting" feature of time, as this brings in std. * pbt-gen: Add arb_datetime_for_rfc3339 With crate time, the range of supported dates stretches to the ancient past beyond the common era, which is not representable in RFC 3339. Add a generator to produce `OffsetDateTime` values that are within the RFC 3339 spec. * Ensure Time can only have values good for RFC 3339 As the time values are meant for human-readable representation in the RFC 3339 format, make it not possible to construct Time with values that fall outside this standard representation. Conversion from time::OffsetDateTime is made fallible with a TryFrom impl replacing the From impl. Conversion from Unix timestamps is also affected. * rpc: Replaced chrono with time 0.3 * testgen: Replaced chrono with time 0.3 * Less allocatey ways of formatting date-times Provide and use another helper in proto mod serializers::timestamp, one that formats into a provided fmt::Write object rather than allocating a string. * light-client: port from chrono to time * Revert to Add/Sub returning Result * light-client: changed the MBTs from chrono to time * tendermint: Remove a comment referencing chrono We use ErrorDetail::DurationOutOfRange without the source error from the time library because that is uninformative in the context. Also move the DateOutOfRange close with DurationOutOfRange since they can occur in the same operations and are generally similar. * light-client: add std feature for time The clock needs OffsetDateTime::now_utc(). * tendermint: Simplify Time::duration_since * testgen: minor code cleanup * Restrict valid Time years to 1-9999 Exclude year 0 because the spec on Google protobuf Timestamp forbids it. Add more helpers in pbt-gen to produce protobuf-safe values in both OffsetDateTime and RFC 3339 strings. Restrict the property tests to only use the protobuf-safe values where expected. * Test Time::checked_add and Time::checked_sub * Changelog entries for #1030 * Improve documentation of tendermint::Time * proto: remove the chrono type conversions The From impls are panicky and do not enforce compliance with protobuf message value restrictions. * tendermint: remove direct uses of chrono types Replace with tendermint::Time. * Harden Timestamp conversions and serde Require the timestamp to be in the validity range (years 1-9999 in UTC) and the nanosecond member value to not exceed 999_999_999. Rename ErrorDetail::TimestampOverflow to TimestampNanosOutOfRange, because the old variant was not very informative (the chained TryFromIntError did not help) and we also use it for the above-range case now which is not an overflow. * Changelog entry about changed error variants * Restore nanosecond range check in Time::from_unix_timestamp Add a unit test to exercise the check. * proto: Improve timestamp::fmt_as_rfc3339_nanos - More ergonomic signature - A non-allocating implementation * Fix component name in changelog for 1030-remove-chrono Co-authored-by: Thane Thomson <[email protected]> * time: Use Self instead of the type name in methods Co-authored-by: Thane Thomson <[email protected]> * Comment on the inner representation of `Time` * Don't alias crate time in testgen * Document the Time::from_utc helper Co-authored-by: Thane Thomson <[email protected]>
The event list change did not make it into tendermint 0.37.
Codecov Report
@@ Coverage Diff @@
## main #1203 +/- ##
=======================================
- Coverage 67.0% 64.0% -3.1%
=======================================
Files 219 250 +31
Lines 20706 21551 +845
=======================================
- Hits 13892 13802 -90
- Misses 6814 7749 +935
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
@mzabaluev did you perhaps rerun the proto compiler tool to generate these protos?
I tried that, the differences were superficial. |
|
Since these are going to be breaking changes, I assume we'll be targeting a v0.26.0 release with them, right? I'd recommend capturing some more detailed changelog entries for where there are breakages, e.g. the transition from Also don't worry about the nightly coverage breakage - it seems codecov's having an outage of some kind. |
|
| - `[tendermint-proto]` Use `Bytes` for byte array fields of ABCI protobuf types. | ||
| ([#1203](https://github.com/informalsystems/tendermint-rs/pull/1203)) |
There was a problem hiding this comment.
@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.
This is related to the domain types, because the ABCI domain types use Bytes internally, 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 to master, 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 tendermint crates that contains ABCI domain types.
If the decision, later, is that the domain types should not use the Bytes type, 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.
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 Protobuf trait forcing a clone of the entire structure, because the domain-to-raw conversion is implemented via From<Self>, but the encode* methods take &self. But this is a subject for a larger rework.
Besides this (subject to discussion above) and the ABCI domain types rework that is the subject of this PR, I have removed any other breaking changes. |
abci/src/client.rs
Outdated
| /// Request that the application set an option to a particular value. | ||
| pub fn set_option(&mut self, req: RequestSetOption) -> Result<ResponseSetOption, Error> { | ||
| perform!(self, SetOption, req) | ||
| } | ||
|
|
There was a problem hiding this comment.
Not a big deal, since I don't think anyone really uses this crate, but why was set_option removed here? It's still present in Tendermint Core v0.34, and has only just been removed in v0.37. IIRC it was removed in v0.35.
There was a problem hiding this comment.
I think this slipped through backporting. We could add it back in, or leave it out. There's no documentation on what it's actually supposed to do, so I'm not sure how anyone would use it.
There was a problem hiding this comment.
I have restored the method, but added a [deprecated] annotation to it.
| //! The old types should be eliminated and | ||
| //! merged with the new ABCI domain types. Moving them here in the meantime | ||
| //! disentangles improving the ABCI domain modeling from changes to the RPC | ||
| //! interface. |
There was a problem hiding this comment.
Just as a heads-up here, I started trying to do this in #1092, but didn't have time to finish it, and then we dropped Tendermint Core v0.35.
@mzabaluev I'd recommend having this be the follow-up to this PR if you have the capacity for it. It's probably even more important than the multi-version support, because as of this PR, we effectively have some duplicate "domain type"-like structures for ABCI responses.
Builds on #1185 to revert some data type changes that targeted Tendermint RPC 0.35.