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

[Feature]: Remove DecProto #15949

Closed
robert-zaremba opened this issue Apr 25, 2023 · 4 comments · Fixed by #16918
Closed

[Feature]: Remove DecProto #15949

robert-zaremba opened this issue Apr 25, 2023 · 4 comments · Fixed by #16918
Labels
C: Proto Proto definition and proto release T:feature-request

Comments

@robert-zaremba
Copy link
Collaborator

robert-zaremba commented Apr 25, 2023

Problem Definition

DecProto is not used anywhere, and I've seen some project using it instead of Dec type (which correctly implements Protobuf).

Proposal

To avoid confusions, we can simply remove DecProto.

Considerations

Advantage of having DecProto is the ability to write in the proto files:

cosmos.base.v1beta1.DecProto dec_amount = 1;

Disadvantages: more code and confusions in Go side of having Dec and DecProto.

@robert-zaremba
Copy link
Collaborator Author

Happy to handle it if others agree with the proposal.

@julienrbrt
Copy link
Member

As it is not used, and it allows us to remove more things in types/math.go (#14405 (comment)), personally I do not see why not.
cc @aaronc.

@julienrbrt julienrbrt added the C: Proto Proto definition and proto release label May 9, 2023
@julienrbrt
Copy link
Member

Happy to handle it if others agree with the proposal.

Do you still want to submit a PR? (ref #16798 (comment))

@robert-zaremba
Copy link
Collaborator Author

robert-zaremba commented Jul 3, 2023

Sure, I can remove IntProto and DecProto. So it's confirmed that we are good to do it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: Proto Proto definition and proto release T:feature-request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants