Migrate x/params JSON state

[aaronc]

x/params uses JSON encoding for ParamSetPair.Values and it was missed in the protobuf state migrations. It won't easily work with protobuf JSON or binary because some types are primitives like uint64 and would need to be wrapped in UInt64Value for protobuf.

Does anyone know why this was JSON and not binary btw? @alexanderbez ?

We have a few options:

1. Do nothing and just make this one case where we still use amino for state
2. Change ParamSetPair.Value from interface{} -> proto.Message and make everything use a proto-compatible type. Then we can use proto binary or JSON. We might run in to trouble, however, trying to make this backwards compatible with amino state encoding. Maybe that's a non-goal now??
3. Attempt to use the standard golang json.Marshal methods. This may work, but there might be issues with types like Dec. I'm not really sure...

[aaronc]

Any thoughts on what to do here @alexanderbez ?

[alexanderbez]

I wasn't involved in the discussion for use of Amino JSON, but I do recall being frustrated by this because its slow af and Jae wanted it for some reason.

That being said, I don't think option (1) is viable. (3) seems like we might run into issues if there are non-go-based SDK clients (correct me if I'm wrong)? So I think (2) is preferable. Nearly all params are primitive types with the exception of x/gov, which uses structs.

[aaronc]

FYI we decided to go with approach (2) in our dev call.

[clevinson]

@aaronc Does ParamSetPair.Value need to be migrated to codec.ProtoMarshaler instead of proto.Message? It looks like ProtoMarshaler is the interface we are primarily using internally elsewhere.

Also, codec.ProtoMarshaler is necessary for when passing the protobuf message to cdc.UnmarshalBinaryBare

[aaronc]

We should be preferring proto.Messge generally. We should probably update Marshaler to use that. But that's a separate refactor. You can ProtoMarshaler if needed.

[clevinson]

@alexanderbez Just hit another sticking point with this that we need to making a decision on. The subspace Update(ctx sdk.Context, key, value []byte) method allows for you pass in serialized JSON bytes that only update a portion of the given param value's type (assuming the param value is a struct). I don't think this is something that we can support with protobuf as proto3 does not distinguish between unset and default field values, so there's no valid way for a user to submit only a subset of fields of a proto message to update.

See this test for an example of where we currently expect this as functionality.

I don't see why this is necessary as a feature. If a Param value is a struct, I would expect that it can only be get/set/updated wholesale. If there's a desire to update individual subfields of a given value, shouldn't this just be a separate parameter? Maybe there's some context i'm missing.. Seems related to #4403

[alexanderbez]

I don't see why this is necessary as a feature. If a Param value is a struct, I would expect that it can only be get/set/updated wholesale.

So a few things. One, I personally believe struct-based params are an anti-pattern, and AFAIK is only used by x/gov. However, we are where we are.

That being said and in this context, it's actually valuable because say you only want to update one field in the struct. You, as a ParamChangeProposal author, don't want to have to worry about copying the entire struct in your proposal -- you want the governance stakeholders and voters to solely focus on the change at hand and not have to worry about whether or not every other non-relevant field makes sense too, hence the feature to update a specific field in a struct.

Now for the actual issue with the migration to Proto3, it's unfortunate and I'm not sure what to do, but here are some immediate thoughts/plans that come to mind:

1. AFAIK, this currently works with Amino because we use omitempty. Can we also use that in proto3? I don't see why Proto's JSON encoder would behave any different than say Amino or even the stdlib encoding/json. This seems like the most straightforward path.
2. Update x/gov to not use struct-based params and remove the anti-pattern entirely (i.e. also remove Update). More work but might be the only option if (1) isn't viable.

[clevinson]

1. AFAIK, this currently works with Amino because we use omitempty. Can we also use that in proto3? I don't see why Proto's JSON encoder would behave any different than say Amino or even the stdlib encoding/json. This seems like the most straightforward path.

I'm not certain that omitempty will simply do the trick. Maybe I'm misunderstanding the exptected behavior of Marshal() and Unmarshal() calls though...

In the current code, it looks like the value bytes get unmarshaled into a pointer that already contains the old fields, and legacyAmino.UnmarshalJSON performs some merge operation that keeps all of the old fields in the dest pointer intact?

It's not clear to me that we should be expecting the same behavior when calling cdc.UnmarshalBinaryBare(value, dest). My assumption would be that this function expects dest to be a pointer that will get fully overwritten by the value because proto3 makes quite clear that there isn't a distinction between empty fields and unset fields. From my understanding this is also the case regardless of whether we are using JSON or binary protoserialization, but maybe I'm misreading things?

2. Update x/gov to not use struct-based params and remove the anti-pattern entirely (i.e. also remove Update). More work but might be the only option if (1) isn't viable.

This makes sense to me. I'll keep it in mind as our backup option.

[alexanderbez]

I'm not certain that omitempty will simply do the trick. Maybe I'm misunderstanding the exptected behavior of Marshal() and Unmarshal() calls though...

We would be using MarshalJSON, not binary.

With binary encoding, I'm not sure we can support this.

[clevinson]

After spending some more time digging into this I've realized I'm not quite happy with any of the paths I see forward. Here's an articulation of where I've gotten. I'd appreciate feedback from anyone whose spent time with x/params to provide feedback on what makes the most sense.

Option 1: All parameter values must be valid protobuf messages

This is the first strategy I was pursuing, which involves requiring that for each Params message (for a given module), all fields must have values that themselves are protobuf messges. Primitive types must be wrapped accordingly.

The main pain point with this, is that modules when interacting with params have to unwrap them with .Value calls (see here). Similarly, on the client side, all params with primitive values need to have their .Value field set to a uint64, as opposed to just setting the param field itself to a uint64. This may make for poor UX when sending param change proposals over GRPC, as well as legacy endpoints (which would be an API breaking change).

Option 2: All parameter values must be either a whitelisted set of primitive types, or a valid protobuf message

I then started exploring a situation in which we try to keep everything as is, except for the actual serialization of param messages. This strategy involves Subspace's Get() and Set() methods continuing to accept arbitrary types (e.g. interface{}), and doing reflection & case switches to wrap a whitelisted set of primitives into proto types before serializing them in the subspace like below:

func (s Subspace) Get(ctx sdk.Context, key []byte, ptr interface{}) {
	s.checkType(key, ptr)

	store := s.kvStore(ctx)
	bz := store.Get(key)

	switch v := ptr.(type) {
	case uint64:
		var wrapper_ptr *pt.UInt64Value
		if err := s.cdc.UnmarshalBinaryBare(bz, wrapper_ptr); err != nil {
			panic(err)
		}
		v = wrapper_ptr.Value
	case string:
		var wrapper_ptr *pt.StringValue
		if err := s.cdc.UnmarshalBinaryBare(bz, wrapper_ptr); err != nil {
			panic(err)
		}
		v = wrapper_ptr.Value
...
...
	case codec.ProtoMarshaler:
		if err := s.cdc.UnmarshalBinaryBare(bz, v); err != nil {
			panic(err)
		}
	default:
		panic(fmt.Sprintf("Unsupported ParamSetPair Value type: %T", v))
	}
}

While I think this option does result in a cleaner UX for end users and chain developers, it still requires knowing & understanding what types can be handled as bare primitives, and which ones can't. For instance, this will break for repeated fields (the wrapper type for repeated fields ListValue), unless we do some sort of recursive calls to case switch on the ListValue's value type.

All to say that this approach also felt hacky and ugly to me. We would need to decide how to treat some existing parameters like time.Duration, sdk.Dec, etc. and then add then to a whitelist, or if not then convert the existing module parameters that use these types to wrap these parameters in proto messages.

Option 3: Store a full module's paramset as one serialized value in the Subspace's KV store.

This involves a larger refactor of the params module, which would do away with a lot of the ParamSetPair abstraction, in place of a params interface that stores the entire parameter set as one serialized protobuf message in a module specific params Subspace. In order to update individual parameters, update calls would most likely require an API that involves field masks (see section on "Field Masks in Update Operations").

Option 4: Leave as is and punt on this entire issue until we do a larger params refactor in conjunction with other store refactors

Just bringing this up as a possibility. As we're inching closer to Stargate release, I don't want this to the yet another issue that balloons in scope and slows down getting an RC out the door.

---

Please comment with any preferences, upgrades, or other alternatives I should be considering.

I'm considering this largely orthogonal to the previous issue above, which is why i haven't gone into details of x/gov's updating of struct fields in this, as I think a solution for that will be found independently of this larger conversation.

[alexanderbez]

Thanks for the detailed and excellent summary @clevinson!

Yeah, option (1) definitely seems like a no-go from what you've laid out, especially since it results in a very poor UX -- no gusta.

I really like options (2) and (3). The former seems like something we can tackle before stargate and honestly, having a limited set of types to support for params may not be that bad, but I can foresee it being a limiting factor for app devs, which may also lead to a poor dev-UX.

So really, option (3) seems like the best path to take, especially with field-masks as that allows us to keep the current behavior. However, this is unlikely to be completed before stargate. This is something I can take off of your plate though.

So tl;dr, I think punting on this for 0.41 is the best path to take. Can you layout any remaining blockers for stargate?

[aaronc]

I agree we should punt this to 0.41, and really simplify this when we do. It should just be primitive types. All the reflection and struct stuff really strikes me as anti patterns.

[clevinson]

Ok great. I'll do a last audit of the existing params tx pathways to make sure there isn't anything breaking or horrid in the current path (e.g. verifying how params update proposals get handled via the GRPC endpoints), and then move this issue to 0.41.

@alexanderbez Remaining stargate blockers are being tracked here

[tac0turtle]

superseded by #10514