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

Make Msg* structs pub(crate) #324

Closed
plafer opened this issue Jan 5, 2023 · 4 comments
Closed

Make Msg* structs pub(crate) #324

plafer opened this issue Jan 5, 2023 · 4 comments
Assignees
Labels
O: maintainability Objective: cause to ease modification, fault corrections and improve code understanding

Comments

@plafer
Copy link
Contributor

plafer commented Jan 5, 2023

This will allow us to make changes to our message domain types on breaking change.

@plafer plafer added A: good-first-issue Admin: good for newcomers O: maintainability Objective: cause to ease modification, fault corrections and improve code understanding labels Jan 5, 2023
@plafer plafer self-assigned this Jan 9, 2023
@plafer plafer added the A: blocked Admin: blocked by another (internal/external) issue or PR label Jan 10, 2023
@plafer
Copy link
Contributor Author

plafer commented Jan 10, 2023

After further analysis, this is a pretty big change that will affect our core API. We currently expect the user to construct a MsgEnvelope and call dispatch() (AFAIK no project uses deliver()). So we're currently exposing all our domain Msg types so that users can construct one of them, wrap it in MsgEnvelope, and call dispatch().

A better API would be to ask the users to pass in the protobuf Message types (i.e. the "raw" types) directly; this way we can make all our domain types private and change them without any breaking change. Presumably, projects will be using the raw types directly in their transaction format anyways, so I don't expect any friction there. Concretely, I suggest we repurpose the Msg trait to be implemented on all raw Msg types, and make validate()/execute() take a generic type bounded by Msg.

This being said, we should wait until we have fully removed the legacy Readers and Keepers to avoid refactoring code that will be removed soon.

blocked: #279

@github-project-automation github-project-automation bot moved this to 📥 To Do in ibc-rs Feb 2, 2023
@Farhad-Shabani Farhad-Shabani moved this from 📥 To Do to 🏗️ In Progress in ibc-rs Feb 2, 2023
@plafer plafer removed A: blocked Admin: blocked by another (internal/external) issue or PR A: good-first-issue Admin: good for newcomers labels Feb 22, 2023
@Farhad-Shabani Farhad-Shabani moved this from 🏗️ In Progress to 📥 To Do in ibc-rs Mar 28, 2023
@plafer
Copy link
Contributor Author

plafer commented Mar 29, 2023

Note that the proto struct definitions can sometimes have a field that changes name from one version to the next, since protobuf's encoding is based on field numbers (i.e. the field names can change without changing what the struct serializes to).

This actually happened with MsgUpdateClient: from ibc-go v6 to v7, the header field was renamed to client_message.

One possible solution is to have a thin wrapper around these structs that only keeps the naming constant; whenever a field name changes in the protobuf struct, we map that new name to the analogous field with the "old name".

@plafer
Copy link
Contributor Author

plafer commented Apr 10, 2023

Another reason why I am in favor of making the domain Msg types private is that the representation we have at any one point in time is tied to other decisions specific to the your library.

For example, in MsgUpdateClient, we currently keep the header field as Any. This is because our current ClientState trait needs to be trait object safe, so ClientState::check_header_and_update_state cannot be generic; we therefore pass the header as Any. Given this context, it wouldn't make sense for our domain type to store the header as some client-specific header type (e.g. the tendermint client's header type), since we would need to convert it back to Any before using it in the ClientState methods.

On top of all this, in light of recent experiments, we might change the ClientState to use static dispatch instead (i.e. use generics instead of trait objects), which would most likely mean that we then change the MsgUpdateClient type to store the header as a client-specific type as opposed to Any.

Hopefully this example succeeds in conveying my intuition for why I believe the domain types we use internally should be private. That being said, we could also export some "convenience domain types" for users who just want a general-purpose representation of IBC's raw message types, and don't care about it being perfectly tailored to their use case, as we do in ibc-rs.

@plafer
Copy link
Contributor Author

plafer commented Sep 29, 2023

I for some time now no longer this to be a good idea, and forgot to update the issue. The idea as presented here (i.e. using raw messages types in the public API) doesn't work because API-breaking but protocol-backwards-compatible changes to the raw types are possible and do happen (e.g. adding a field to a struct). Then, we have to deal with breaking changes anyway, so might as well use the domain types in our public API.

@plafer plafer closed this as completed Sep 29, 2023
@github-project-automation github-project-automation bot moved this from 📥 To Do to ✅ Done in ibc-rs Sep 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O: maintainability Objective: cause to ease modification, fault corrections and improve code understanding
Projects
None yet
Development

No branches or pull requests

1 participant