Object safe Protobuf

[hu55a1n1]

Partially addresses: #2335

Replaces tendermint_proto::Protobuf with an object safe variant.

Note: The target branch is a long-lived branch hu55a1n1/light-client-extraction. The plan is to merge PRs (addressing #2335) such as this into hu55a1n1/light-client-extraction and ultimately merge that into master.

---

PR author checklist:

• Added changelog entry, using unclog.
• Added tests: integration (for Hermes) or unit/mock tests (for modules).
• Linked to GitHub issue.
• Updated code comments and documentation (e.g., docs/).

Reviewer checklist:

• Reviewed Files changed in the GitHub PR explorer.
• Manually tested (in case integration/unit/mock tests are absent).

[seanchen1991]

I love the thoroughness of the documentation! Just have a small question on the docs for the Protobuf trait.

[plafer]

We can remove the sealed module entirely. It basically defines two "marker traits", and "marks" all types that satisfy the constraints:

• SealedInto<T> for U: Clone + CoreInto<T>
• SealedTryFrom<T> for U: CoreTryFrom<T>
  These markers are then used as supertraits to our Into and TryFrom.

We see the same constraints being applied to our blanket implementations. So, the supertraits to our Into and TryFrom can simply be removed; the blanket implementation takes care of the constraints.

TLDR: if we remove the sealed module, the sealed::SealedInto<T> supertrait on Into, and the sealed::SealedTryFrom<T> supertrait on TryFrom, everything compiles and we've reduced the complexity of this module.

[hu55a1n1]

The sealed traits signal that they are not supposed to be implemented by the user. I think it's nice to have them because we provide blanket impls for these traits.

[plafer]

But what does having them provide that my suggestion doesn't? I understand that they're not supposed to be implemented by the user; I just don't see why they're useful at all.

TLDR: if we remove the sealed module, the sealed::SealedInto supertrait on Into, and the sealed::SealedTryFrom supertrait on TryFrom, everything compiles and we've reduced the complexity of this module.

[plafer]

I think we should give a different name to into(), such as into_ref(). With the current version, I noticed weird behaviors in my editor. When hovering over a call to erased::Into::into() in one of the functions of our Protobuf implementation, rust-analyzer claims that the into() being referenced is the one in core::convert::Into. I even tried to explicitly write

let raw: Box<T> = self.into();

and rust-analyzer still thinks that I'm using core::convert::into. I haven't tested other language servers.

I then played around a bit in a rust playground here. I'm not sure why it's not working; perhaps it has something to do with core::convert::Into being in the prelude?

In any case, using another name for into() avoids these headaches.

[hu55a1n1]

This is an interesting problem. I used erased::Into because that is the convention that erased_serde uses and I thought that it was a common convention, but it seems we're seeing problems because we're using it with std lib traits (in the prelude). IMHO, into_ref() would be a method that takes self by value, so a better name would perhaps be to_boxed(). WDYT?

[plafer]

I agree that into_ref() suggests taking ownership. On the flip slide, for me, to_boxed() suggests boxing self (i.e. no conversion to another type), such as

fn to_boxed(&self) -> Box<Self> {
    Box::new(self.clone())
}

Brainstorming alternatives:

• into_boxed(): still into suggests taking ownership of self
• clone_boxed(): I don't like it, doesn't suggest a conversion either
• clone_into_boxed(): quite wordy, but perhaps captures best the conversion we're doing?
• as_boxed(): as_* in std take a &self, and suggest a conversion?
• ... ?