fix: implement Amino serialization for x/authz and x/feegrant

[RiccardoM]

Description

Closes: #11190
Superseeds: #11214

---

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...

• included the correct type prefix in the PR title
• added ! to the type prefix if API or client breaking change
• targeted the correct branch (see PR Targeting)
• provided a link to the relevant issue or specification
• followed the guidelines for building modules
• included the necessary unit and integration tests
• added a changelog entry to CHANGELOG.md
• included comments for documenting Go code
• updated the relevant documentation or specification
• reviewed "Files changed" and left comments if necessary
• confirmed all CI checks have passed

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)

[RiccardoM]

After implementing the serialization for all authorization types, the only one that ends up having a not-so-good serialization is the StakeAuthorization. Here is an example:

{
   "type":"cosmos-sdk/StakeAuthorization",
   "value":{
      "Validators":{
         "type":"cosmos-sdk/StakeAuthorization/AllowList",
         "value":{
            "allow_list":{
               "address":[
                  "cosmosvaloper1xcy3els9ua75kdm783c3qu0rfa2eples6eavqq"
               ]
            }
         }
      },
      "authorization_type":1,
      "max_tokens":{
         "amount":"1000",
         "denom":"stake"
      }
   }
}

As you can see the Validators field does not respect the snake_case naming convention. I honestly don't know if there is a way to fix this

EDIT
Looks like custom tags are not suppported for oneof fields: gogo/protobuf#623

[amaury1093]

We still have a problem here. GenericAuthorization allows any msg TypeURL, and then you can MsgExec with that arbitrary Msg. We would need to register authz.RegisterConcrete for every msg for every module.

[amaury1093]

I don't know how to solve this problem right now. We can try to think of something before merging.

[robert-zaremba]

OMG, and we need to use the right type name...

[robert-zaremba]

side note: this global codec will must bit us sooner or later.

[amaury1093]

What do you think of removing the individual ModuleCdc in each module, and just use one single one? #11232

[robert-zaremba]

definitely better, although don't know how it will work with multi module setup... Amino codec will need to e it's own module.
Ideally this comes top-down as the proto codec. I gues we didn't do it because we thought amino will go away, and it's not worth to make that efforts. but amino doesn't go away.

[RiccardoM]

@robert-zaremba It is not possible to have a top-down approach in this case as the sdk.Msg interface has been poorly designed and it does not allow the GetSignBytes method to accept an external codec to be used. So it just assumes (and allows) each message to get its signature bytes differently, which doesn't make any sense honestly.

The best approach would be to simply re-design how this entire process is handled, either requiring the GetSignBytes to accept an external codec or by finding any other way (maybe a custom Protobuf implementation that also deals with Amino correctly or something similar).

In the meanwhile, we need to deal with what we have to make sure Ledger users are able to use the authz and feegrant modules properly.

[RiccardoM]

I've changed from using a ModuleCdc to use the global legacy.Cdc instance and it appears to be a better and more sustainable solution in the long run (as long as Amino is still here)

[RiccardoM]

This is just to make sure the Cdc is initialized before running the various init methods. Since the initialization order is the following:

1. package variables (as declared)
2. init methods (based on the file structure)

It is safer to have the Cdc being initialized as a package variable rather than inside the init method.

[peterbourgon]

Initialization order is not necessarily defined in this way.

[RiccardoM]

Isn't it? I found this order inside the Go specs (here)

[peterbourgon]

You're right that vars are evaluated before init funcs, my mistake there. But this is very subtle behavior to depend on.

[alexanderbez]

This honestly wasn't necessary IMO. I actually think keeping it in the init is cleaner as that's also where we do the initialization.

[amaury1093]

Suggested change

	var (
	amino = codec.NewLegacyAmino()
	// ModuleCdc references the global x/feegrant module codec. Note, the codec should
	// ONLY be used in certain instances of tests and for JSON encoding as Amino is
	// still used for that purpose.
	//
	// The actual codec used for serialization should be provided to x/feegrant and
	// defined at the application level.
	ModuleCdc = codec.NewAminoCodec(amino)
	)

Is that correct? Come to think about it, we can remove ModuleCdc globals from all modules, right? (we can do in a separate PR, so that this one focused on authz/feegrant)

Edit: we can do this in #11232

[RiccardoM]

@marbar3778 @AmauryM Can we put an automerge on this as well please?

[amaury1093]

We need 2 approvals for automerge to merge this. @robert-zaremba or @marbar3778 ?

[zmanian]

This change changes the behavior of the SDK to register all the Amino types for cosmos sdk modules when the module is imported.

modules in x/ should be manipulating global runtime state like this so that chain devs can replace them.

[alexanderbez]

@RiccardoM can you please revert this PR in favor of requiring developers to explicitly register their authz codec with other modules?

[ironmandefi]

Hi team, just following up here to see if this fix was shipped in the recent .46 release :) I see it is merged but just want to confirm here. Thanks