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

Add empty rust crates for protobufs #4

Merged
merged 3 commits into from
Feb 14, 2023
Merged

Add empty rust crates for protobufs #4

merged 3 commits into from
Feb 14, 2023

Conversation

nick-mobilecoin
Copy link
Collaborator

No description provided.

@nick-mobilecoin
Copy link
Collaborator Author

nick-mobilecoin commented Feb 10, 2023

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

crates/README.md Outdated Show resolved Hide resolved
crates/.gitignore Outdated Show resolved Hide resolved
crates/deny.toml Outdated Show resolved Hide resolved
@nick-mobilecoin nick-mobilecoin requested review from jcape, a team and NotGyro and removed request for a team February 10, 2023 18:04
Copy link
Contributor

@jcape jcape left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally there would be a different crate for each package. i.e. the crate would be, like, mc-attestation-types-grpcio or mc-protobufs-attestation-types-grpcio.

The thinking is: these crates are the place to host type conversions to "business logic" rust types, which means they will need to depend on the foo-types-style crates, and I'd love to avoid having a huge dependency tree behind using a subset of these protobufs.

Base automatically changed from nick/attest-proto to main February 11, 2023 00:11
@nick-mobilecoin
Copy link
Collaborator Author

nick-mobilecoin commented Feb 13, 2023

Ideally there would be a different crate for each package. i.e. the crate would be, like, mc-attestation-types-grpcio or mc-protobufs-attestation-types-grpcio.

The thinking is: these crates are the place to host type conversions to "business logic" rust types, which means they will need to depend on the foo-types-style crates, and I'd love to avoid having a huge dependency tree behind using a subset of these protobufs.

Ensuring we're on the same page

graph LR
    A[mc-attestation-tonic] --> B[mc-attestation-protobuf-types]
    C[mc-attestation-grpcio] --> B
    B --> D[mc-attestation-types]
  • The mc-attestation-tonic and mc-attestation-grpcio are the interfaces that implement grpc attestation. I kind of want to have grpc in the name but it's confusing with grpcio. Following their names to the respective crate implementations will hopefully be clear enough.
  • The mc-attestation-protobuf-types are the types used in the grpc traffic. Note, using prost and diffing the generated content it seems we only need one common protobuf type for both tonic and grpcio. We may consider mc-attestation-prost-types too.
  • The mc-attestation-types contain the raw rust attestation types. These types should be isolated by the chosen transport protocol an thus why this crate intentionally does not depend on mc-attestation-protobuf-types. i.e. if we decide to serialize in JSON or XML this crate should not need to change.

Previously the crates were named to contain all future protobuf types
and services, now the crates have been named to only support
`attestation` with the intent to create dedicated crates for other gRPC
communications.
The rust crates are interleaved in the protobuf directories. This
prevents mirroring the directory structure in another sub-directory
Copy link

@NotGyro NotGyro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@nick-mobilecoin nick-mobilecoin merged commit 54b7ce5 into main Feb 14, 2023
@nick-mobilecoin nick-mobilecoin deleted the nick/crates branch February 14, 2023 23:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Large PRs
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants