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

why custom proto type (types.Dec) use two return type?( string, bytes ) #11792

Closed
4 tasks
hea9549 opened this issue Apr 27, 2022 · 5 comments
Closed
4 tasks

Comments

@hea9549
Copy link

hea9549 commented Apr 27, 2022

Summary

Problem Definition

In gov.proto, weight use types.Dec as string like below

message WeightedVoteOption {
  VoteOption option = 1;
  string     weight = 2 [
    (gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Dec",
    (gogoproto.nullable)   = false,
    (gogoproto.moretags)   = "yaml:\"weight\""
  ];
}

but with TallyParams message in same file, quorum uses types.Dec as bytes.

// TallyParams defines the params for tallying votes on governance proposals.
message TallyParams {
  //  Minimum percentage of total stake needed to vote for a result to be
  //  considered valid.
  bytes quorum = 1 [
    (gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Dec",
    (gogoproto.nullable)   = false,
    (gogoproto.jsontag)    = "quorum,omitempty"
  ];

How about unifying it? For external use, it seems better to use a string. Externally, we do not know of a custom type for deserializing a byte array.

Proposal


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@hea9549
Copy link
Author

hea9549 commented Apr 27, 2022

oops, it just marshal big.int data.

func (d Dec) Marshal() ([]byte, error) {
	if d.i == nil {
		d.i = new(big.Int)
	}
	return d.i.MarshalText()
}

then how about unifying it to bytes?

@aaronc
Copy link
Member

aaronc commented Apr 27, 2022

The current approach is not ideal and is going to get replaced

@hea9549
Copy link
Author

hea9549 commented Apr 27, 2022

thx for replying! can i get some docs or proposal about this?

@aaronc
Copy link
Member

aaronc commented Apr 27, 2022

You can search issues for other discussions, but #11783 is one thing we'll be doing soon

@tac0turtle
Copy link
Member

this is fixed with v1 gov proto files. Closing this in favour of v1 gov proto files

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

No branches or pull requests

3 participants