feat: api updates required for aminojson encoder #15019

kocubinski · 2023-02-13T21:05:33Z

Description

These are all the protobuf changes required to ship #10993. This patch is a subset of the feature complete PR #14877, and an attempt to make it more reviewable.

Changes

Introduce amino options for handling oneof types.
Specify numerous message and field encoders with amino.message_encoding for whole messages and amino.encoding for fields.
Add amino.dont_omityempty usages where needed.
Update amino.name usages to correlate with legacy amino type registration.

More nuanced descriptions are provided as comments closer to the source location in this PR.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

kocubinski · 2023-02-13T21:26:01Z

proto/amino/amino.proto

@@ -76,4 +76,10 @@ extend google.protobuf.FieldOptions {
  // out := AminoJSONEncoder(&f);
  // out == {"baz":""}
  bool dont_omitempty = 11110005;
+


These options were added to achieve marshal parity for messages with a oneof field, e.g. StakeAuthorization:

cosmos-sdk/proto/cosmos/staking/v1beta1/authz.proto

Lines 22 to 29 in 5946320

oneof validators {

option (amino.oneof_field_name) = "Validators";

// allow_list specifies list of validator addresses to whom grantee can delegate tokens on behalf of granter's

// account.

Validators allow_list = 2 [(amino.oneof_type_name) = "cosmos-sdk/StakeAuthorization/AllowList"];

// deny_list specifies list of validator addresses to whom grantee can not delegate tokens.

Validators deny_list = 3 [(amino.oneof_type_name) = "cosmos-sdk/StakeAuthorization/DenyList"];

}

Since the legacy encoder uses golang reflection it produces JSON shaped like below.

{"type":"cosmos-sdk/StakeAuthorization","value":{"Validators":{"type":"cosmos-sdk/StakeAuthorization/AllowList","value":{"allow_list":{"address":["foo"]}}}}}

The protoreflect default field traversal differs:

{"type":"cosmos-sdk/StakeAuthorization","value":{"allow_list":{"address":["foo"]}}}

The options are introspected and used in the encoder to bring the JSON output into equivalence.

cosmos-sdk/x/tx/aminojson/json_marshal.go

Lines 160 to 162 in 732bcaf

oneof := f.ContainingOneof()

isOneOf := oneof != nil

oneofFieldName, oneofTypeName, err := getOneOfNames(f)

kocubinski · 2023-02-13T21:41:04Z

proto/cosmos/auth/v1beta1/auth.proto

  uint64              account_number = 3;
  uint64              sequence       = 4;
 }

 // ModuleAccount defines an account for modules that holds coins on a pool.
 message ModuleAccount {
  option (amino.name)                        = "cosmos-sdk/ModuleAccount";
+  option (amino.message_encoding)            = "module_account";


The legacy encoder features a custom encoding for ModuleAccount

cosmos-sdk/x/auth/types/account.go

Lines 240 to 254 in 41a3dfe

func (ma ModuleAccount) MarshalJSON() ([]byte, error) {

accAddr, err := sdk.AccAddressFromBech32(ma.Address)

if err != nil {

return nil, err

}

return json.Marshal(moduleAccountPretty{

Address: accAddr,

PubKey: "",

AccountNumber: ma.AccountNumber,

Sequence: ma.Sequence,

Name: ma.Name,

Permissions: ma.Permissions,

})

}

The protoreflect encoder provides this by registering message encoding overrides by key. This encoding corresponds to this registration:

cosmos-sdk/x/tx/aminojson/json_marshal.go

Line 41 in 732bcaf

"module_account": moduleAccountEncoder,

and this implementation:

cosmos-sdk/x/tx/aminojson/encoder.go

Lines 101 to 124 in 583181c

func moduleAccountEncoder(msg protoreflect.Message, w io.Writer) error {

ma := msg.Interface().(*authapi.ModuleAccount)

pretty := moduleAccountPretty{

PubKey: "",

Name: ma.Name,

Permissions: ma.Permissions,

}

if ma.BaseAccount != nil {

pretty.Address = ma.BaseAccount.Address

pretty.AccountNumber = ma.BaseAccount.AccountNumber

pretty.Sequence = ma.BaseAccount.Sequence

} else {

pretty.Address = ""

pretty.AccountNumber = 0

pretty.Sequence = 0

}

bz, err := json.Marshal(pretty)

if err != nil {

return err

}

_, err = w.Write(bz)

return err

}

proto/cosmos/bank/v1beta1/authz.proto

kocubinski · 2023-02-13T22:28:53Z

proto/cosmos/slashing/v1beta1/slashing.proto

@@ -41,18 +41,21 @@ message Params {
  bytes min_signed_per_window = 2 [
    (gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Dec",
    (gogoproto.nullable)   = false,
+    (amino.encoding)       = "cosmos_dec_bytes",


encoder implementation:

cosmos-sdk/x/tx/aminojson/encoder.go

Lines 14 to 29 in 583181c

func cosmosDecBytesEncoder(_ AminoJSON, v protoreflect.Value, w io.Writer) error {

switch bz := v.Interface().(type) {

case []byte:

if len(bz) == 0 {

return jsonMarshal(w, "0")

}

var dec math.LegacyDec

err := dec.Unmarshal(bz)

if err != nil {

return err

}

return jsonMarshal(w, dec.String())

default:

return fmt.Errorf("unsupported type %T", bz)

}

}

tac0turtle

Generally looks good. Worried about the null_as_empty stuff. This looks really easy to forget.

kocubinski · 2023-02-14T00:48:22Z

Generally looks good. Worried about the null_as_empty stuff. This looks really easy to forget.

It's a fair point, we could also special case on all usages of []v1beta1.Coin in protoreflect messages if you think that'd be a better option. I'm generally in in favor of configuration over convention but maybe that's not the right thought here.

tac0turtle · 2023-02-14T01:25:20Z

Lets keep it as is. Sorry it wasn't a suggestion to change anything. Don't want to over engineer it since hopefully amino will die soon.

kocubinski · 2023-02-14T02:13:42Z

Lets keep it as is. Sorry it wasn't a suggestion to change anything. Don't want to over engineer it since hopefully amino will die soon.

If this will make upgrading easier I'm not sure its over engineering. Otherwise developers will need to remember to tag any usage of repeated Coin with (amino.encoding) = "null_slice_as_empty", right?

kocubinski · 2023-02-14T02:14:26Z

@AmauryM or @aaronc Any insight here on how this will impact adoption?

amaury1093

Since coins do have a special encoding, I prefer to add an annotation to all coins (instead of hard coding a convention). However, I see there's already a amino.dont_omit_empty, so I'm wondering if we need null_slice_as_empty.

proto/amino/amino.proto

proto/cosmos/bank/v1beta1/authz.proto

note 3 broken tests. could revert. ref: #15019 (comment)

…osmos-sdk into kocubinski/aminojson/just-proto

…minojson/just-proto

amaury1093

LGTM

protobuf changes from kocubinski/amino-json

2c9db9d

github-actions bot added C:x/auth C:x/bank C:x/consensus C:x/distribution distribution module related C:x/feegrant C:x/gov C:x/mint C:x/slashing C:x/staking labels Feb 13, 2023

kocubinski commented Feb 13, 2023

View reviewed changes

clean up moduleAccount proto

8025c10

kocubinski commented Feb 13, 2023

View reviewed changes

proto/cosmos/bank/v1beta1/authz.proto Outdated Show resolved Hide resolved

kocubinski commented Feb 13, 2023

View reviewed changes

kocubinski added 2 commits February 13, 2023 15:29

Merge branch 'main' into kocubinski/aminojson/just-proto

d9b8aef

add comments

295648e

kocubinski marked this pull request as ready for review February 13, 2023 22:41

kocubinski requested a review from a team as a code owner February 13, 2023 22:41

tac0turtle approved these changes Feb 14, 2023

View reviewed changes

Merge branch 'main' into kocubinski/aminojson/just-proto

19b636d

amaury1093 reviewed Feb 14, 2023

View reviewed changes

proto/amino/amino.proto Outdated Show resolved Hide resolved

proto/cosmos/bank/v1beta1/authz.proto Outdated Show resolved Hide resolved

Merge branch 'main' into kocubinski/aminojson/just-proto

71a4fda

kocubinski added a commit that referenced this pull request Feb 14, 2023

coins by convention.

ebf3283

note 3 broken tests. could revert. ref: #15019 (comment)

amaury1093 self-assigned this Feb 15, 2023

use name "legacy_coins" instead of "null_slice_as_empty"

541ad72

kocubinski added 3 commits February 15, 2023 09:38

Merge branch 'kocubinski/aminojson/just-proto' of github.com:cosmos/c…

8c5a3af

…osmos-sdk into kocubinski/aminojson/just-proto

Merge branch 'main' of github.com:cosmos/cosmos-sdk into kocubinski/a…

1f7d1bd

…minojson/just-proto

use convention for oneof field names

f4e4ef8

kocubinski changed the title ~~feat: protobuf changes required for aminojson encoder~~ feat: api updates required for aminojson encoder Feb 15, 2023

Merge branch 'main' into kocubinski/aminojson/just-proto

cf9bde5

amaury1093 approved these changes Feb 15, 2023

View reviewed changes

kocubinski merged commit 407c757 into main Feb 16, 2023

kocubinski deleted the kocubinski/aminojson/just-proto branch February 16, 2023 03:15

kocubinski mentioned this pull request Feb 16, 2023

feat: protoreflect based amino json encoder #14877

Merged

19 tasks

faddat mentioned this pull request Mar 28, 2023

update linter to 1521 ChihuahuaChain/cosmos-sdk#6

Closed

19 tasks

julienrbrt mentioned this pull request Jul 13, 2023

feat(client/v2): stringify bytes cosmos.Dec #16985

Merged

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: api updates required for aminojson encoder #15019

feat: api updates required for aminojson encoder #15019

kocubinski commented Feb 13, 2023 •

edited

Loading

kocubinski Feb 13, 2023 •

edited

Loading

kocubinski Feb 13, 2023 •

edited

Loading

kocubinski Feb 13, 2023

tac0turtle left a comment

kocubinski commented Feb 14, 2023

tac0turtle commented Feb 14, 2023

kocubinski commented Feb 14, 2023

kocubinski commented Feb 14, 2023

amaury1093 left a comment

amaury1093 left a comment

	oneof validators {
	option (amino.oneof_field_name) = "Validators";
	// allow_list specifies list of validator addresses to whom grantee can delegate tokens on behalf of granter's
	// account.
	Validators allow_list = 2 [(amino.oneof_type_name) = "cosmos-sdk/StakeAuthorization/AllowList"];
	// deny_list specifies list of validator addresses to whom grantee can not delegate tokens.
	Validators deny_list = 3 [(amino.oneof_type_name) = "cosmos-sdk/StakeAuthorization/DenyList"];
	}

	oneof := f.ContainingOneof()
	isOneOf := oneof != nil
	oneofFieldName, oneofTypeName, err := getOneOfNames(f)

	func (ma ModuleAccount) MarshalJSON() ([]byte, error) {
	accAddr, err := sdk.AccAddressFromBech32(ma.Address)
	if err != nil {
	return nil, err
	}

	return json.Marshal(moduleAccountPretty{
	Address: accAddr,
	PubKey: "",
	AccountNumber: ma.AccountNumber,
	Sequence: ma.Sequence,
	Name: ma.Name,
	Permissions: ma.Permissions,
	})
	}

	func moduleAccountEncoder(msg protoreflect.Message, w io.Writer) error {
	ma := msg.Interface().(*authapi.ModuleAccount)
	pretty := moduleAccountPretty{
	PubKey: "",
	Name: ma.Name,
	Permissions: ma.Permissions,
	}
	if ma.BaseAccount != nil {
	pretty.Address = ma.BaseAccount.Address
	pretty.AccountNumber = ma.BaseAccount.AccountNumber
	pretty.Sequence = ma.BaseAccount.Sequence
	} else {
	pretty.Address = ""
	pretty.AccountNumber = 0
	pretty.Sequence = 0
	}

	bz, err := json.Marshal(pretty)
	if err != nil {
	return err
	}
	_, err = w.Write(bz)
	return err
	}

	func cosmosDecBytesEncoder(_ AminoJSON, v protoreflect.Value, w io.Writer) error {
	switch bz := v.Interface().(type) {
	case []byte:
	if len(bz) == 0 {
	return jsonMarshal(w, "0")
	}
	var dec math.LegacyDec
	err := dec.Unmarshal(bz)
	if err != nil {
	return err
	}
	return jsonMarshal(w, dec.String())
	default:
	return fmt.Errorf("unsupported type %T", bz)
	}
	}

feat: api updates required for aminojson encoder #15019

feat: api updates required for aminojson encoder #15019

Conversation

kocubinski commented Feb 13, 2023 • edited Loading

Description

Changes

Author Checklist

Reviewers Checklist

kocubinski Feb 13, 2023 • edited Loading

Choose a reason for hiding this comment

kocubinski Feb 13, 2023 • edited Loading

Choose a reason for hiding this comment

kocubinski Feb 13, 2023

Choose a reason for hiding this comment

tac0turtle left a comment

Choose a reason for hiding this comment

kocubinski commented Feb 14, 2023

tac0turtle commented Feb 14, 2023

kocubinski commented Feb 14, 2023

kocubinski commented Feb 14, 2023

amaury1093 left a comment

Choose a reason for hiding this comment

amaury1093 left a comment

Choose a reason for hiding this comment

kocubinski commented Feb 13, 2023 •

edited

Loading

kocubinski Feb 13, 2023 •

edited

Loading

kocubinski Feb 13, 2023 •

edited

Loading