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

Domain types for protobuf structs #537

Merged
merged 4 commits into from
Sep 3, 2020
Merged

Conversation

greg-szabo
Copy link
Member

@greg-szabo greg-szabo commented Aug 26, 2020

Closes #535

  • domain types with TryFrom(protobuf struct) and From(domaintype) for the protobuf structs

  • DomainType trait for direct .encode/.decode functionality from the protobuf types

  • DomainType derive macro

  • Referenced an issue explaining the need for the change

  • Updated all relevant documentation in docs

  • Updated all code comments where relevant

  • Wrote tests

  • Updated CHANGELOG.md

@greg-szabo greg-szabo added this to the Go v0.34 Compatibility milestone Aug 26, 2020
@greg-szabo greg-szabo self-assigned this Aug 26, 2020
@tarcieri
Copy link
Contributor

possibly a trait should be added for direct .encode/.decode functionality from the Raw type for easier handling.

Would definitely recommend an "extension trait" with a blanket impl for T: prost::Message here.

prost-amino-derive = "0.6"
prost = "0.6"
prost-derive = "0.6"
prost-types = "0.6"
Copy link
Member

Choose a reason for hiding this comment

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

❤️

prost-amino-derive = "0.6"
prost = "0.6"
prost-derive = "0.6"
prost-types = "0.6"
Copy link
Member

Choose a reason for hiding this comment

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

Is this due to using the well known type for time?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure what you mean. If you refer to me removing TimeMsg, it's because its implementation was the same or very similar to the built-in prost::Timestamp implementation. We might reintroduce it as a domain-type, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I think I understand your question! You are asking why the prost-types crate was added!?

It's for Timestamp, but I think we might not need it. I'll double-check and remove before we merge this.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I was wondering why the Prost-types dependency was added. Thanks for the clarification.

@greg-szabo greg-szabo changed the base branch from master to greg/aminoff September 2, 2020 23:47
@greg-szabo greg-szabo mentioned this pull request Sep 3, 2020
5 tasks
@greg-szabo greg-szabo marked this pull request as ready for review September 3, 2020 15:19
@greg-szabo greg-szabo changed the title WIP: Domain types for protobuf structs Domain types for protobuf structs Sep 3, 2020
@greg-szabo greg-szabo merged commit 5e35487 into greg/aminoff Sep 3, 2020
@greg-szabo greg-szabo deleted the greg/proto-domain branch September 3, 2020 15:20
greg-szabo added a commit that referenced this pull request Sep 3, 2020
Domain types for protobuf structs (#537)

* Domain types
* DomainType derive macro
* DomainType added to all amino_types

As close as test_validator_set gets without issue #506
greg-szabo added a commit that referenced this pull request Sep 3, 2020
* Domain types
* DomainType derive macro
* DomainType added to all amino_types
brapse pushed a commit that referenced this pull request Sep 10, 2020
* Ripped out amino types and replaced them with protobuf types.

* Vote and proposal serialization fix

* fmt fix

* As close as test_validator_set gets without issue #506

* sad fmt noises

* Domain types for protobuf structs (#537)

* Domain types
* DomainType derive macro
* DomainType added to all amino_types

* Minor fixes and cleanup

* Fixed voting and proposals

* Updated CHANGELOG

* Documentation update
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement domain types for protobuf-encoded structs
3 participants