proto: Align TryFrom impls for Duration with prost-types

[romac]

No description provided.

[tony-iqlusion]

I think this probably makes more sense than supporting negative durations, at least unless there's an actual use for them in the Cosmos ecosystem somewhere

[tony-iqlusion]

@romac with this merged I can confirm I can upgrade cosmos-sdk-proto / cosmrs 🎉

[romac]

Glad to hear! Will do a release tomorrow then

[tony-iqlusion]

@romac it'd be good if you can get #1457 in as well

[romac]

Will do! Apologies for the oversight and thanks for all the PRs 🙏

[rnbguy]

Coming from the above ibc-rs issue. Because of this PR, we can't expect Protobuf<T: From<Self>>. So the Protobuf trait

tendermint-rs/proto/src/lib.rs

Lines 105 to 106 in 2f94e7f

	pub trait Protobuf<T: Message + From<Self> + Default>
	where

should be refactored to something like

pub trait Protobuf<T: Message + TryFrom<Self> + Default>
where

Should I open an issue for this? Or I missed anything from the latest releases?

[romac]

Ah that's an oversight from my part, sorry about that. Not sure what to do about that though? Do you think can make that conversion infallible for Duration?

[rnbguy]

I just explored the idea of trait Protobuf<T: Message + TryFrom<Self> + Default> and realized a lot of refactor needs to be done inside the tendermint domain types. Mainly because the encode_length or encode_vec -- these two Protobuf trait methods become fallible -- which in turn makes methods like hash, sign_bytes, into_signable_vec infallible too.

I am going to open an issue for this and continue the discussion there.

[erwanor]

Like Rano said, this broke a general ecosystem pattern of infallible domain type to wire conversion. To route around this, we are newtyping the core duration and making the conversion infallible: penumbra-zone/ibc-types#94. This is kind of ugly but works.

I don't think we should let this change contaminate the rest of the ecosystem types by changing the Protobuf marker trait. It's a core ecosystem pattern that is very entrenched, and good. If we change it, we generate work and headwinds for downstreams, with no upside in return.

Without commenting on this specific change, it would be awesome to have more heads up/consensus when we change domain type modeling or APIs. This is because the smallest change has dangerous ripple effects throughout. In most cases, downstream crates can navigate those change nimbly, but they are more perilous for deployed systems that need to plan around consensus/state compatibility. That's the case for Penumbra, Astria, Namada, and I'm sure many others as well.

[romac]

Agreed, we should probably revert the change and make the conversion infallible again. Maybe we can provide a newtype type that is fallible in case someone needs to make sure that the conversion does not saturate?