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

Impact assessment of removing chrono dependency #1012

Closed
thanethomson opened this issue Oct 25, 2021 · 4 comments · Fixed by #1030
Closed

Impact assessment of removing chrono dependency #1012

thanethomson opened this issue Oct 25, 2021 · 4 comments · Fixed by #1030
Assignees
Labels
dependencies Pull requests that update a dependency file security

Comments

@thanethomson
Copy link
Member

Following from #1007 (comment), it could possibly make sense to remove our dependency on chrono altogether in tendermint-rs.

The primary outcome of this issue should be a clear indication as to whether this is possible and what the impact of this will be.

@thanethomson thanethomson added dependencies Pull requests that update a dependency file security labels Oct 25, 2021
@mzabaluev
Copy link
Contributor

It should be possible to migrate from chrono to time 0.3 unless you use some exotic chrono APIs, outside of basic datetime manipulation and serde. I've done this recently on a small code base in input-output-hk/jortestkit#96

@thanethomson
Copy link
Member Author

Yeah that's what I'm hoping. It's not super high priority right now, but I was thinking that, if I had some time to kill in the next month or so, I'd do a little scouting through the codebase and write up an ADR to quantify the impact.

@mzabaluev you're welcome to take this on if you have the time to spare.

@mzabaluev
Copy link
Contributor

@thanethomson Will do, also as a practice in writing ADRs.

On the initial examination, exposing the chrono type wrapped by Time may not be such a good idea.

As a side observation, the serde impls for both Time and Timestamp only use the string format. This is going to be wasteful and embarrassing when somebody uses a non-human-readable format serializer on structures containing these types (outside of protobuf which does not use serde). Perhaps the serde impls should use field-wise serialization of the Timestamp struct with any binary serializers.

@soareschen
Copy link
Contributor

For your refactoring using time, please also refrain from using the std features by adding default-features = false to the time crate for the pure part of tendermint crates that are used by ibc. If the current time is needed, they should be passed as arguments by the impure parts of the tendermint crates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file security
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants