Proto Transaction Migration

[alexanderbez]

ref: #5444

/cc @aaronc

---

For contributor use:

• Targeted PR against correct branch (see CONTRIBUTING.md)
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Code follows the module structure standards.
• Wrote unit and integration tests
• Updated relevant documentation (docs/) or specification (x/<module>/spec/)
• Added relevant godoc comments.
• Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
• Re-reviewed Files changed in the Github PR explorer

---

For admin use:

• Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
• Reviewers assigned
• Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

[codecov]

Codecov Report

Merging #5795 into master will decrease coverage by 3.53%.
The diff coverage is 10.26%.

@@            Coverage Diff             @@
##           master    #5795      +/-   ##
==========================================
- Coverage   36.03%   32.49%   -3.54%     
==========================================
  Files         342      346       +4     
  Lines       34782    39012    +4230     
==========================================
+ Hits        12534    12678     +144     
- Misses      20998    25066    +4068     
- Partials     1250     1268      +18

Impacted Files	Coverage Δ
codec/std/codec.pb.go	2.13% <ø> (ø)
types/tx_msg.go	84.61% <ø> (ø)	⬆️
client/context/context.go	2.12% <0%> (-0.1%)	⬇️
types/codec.go	25% <0%> (-75%)	⬇️
x/auth/types/txbuilder.go	23.4% <0%> (ø)	⬆️
x/auth/types/types.pb.go	8.74% <0.5%> (-7.27%)	⬇️
x/auth/ante/basic.go	84.61% <100%> (ø)	⬆️
x/auth/types/test_utils.go	44.18% <33.33%> (ø)	⬆️
x/auth/types/stdtx.go	54.19% <41.42%> (-8.12%)	⬇️
codec/std/tx.go	59.45% <59.45%> (ø)
... and 4 more

[tac0turtle]

LGTM

[aaronc]

Looks pretty good! Just a couple of minor things.

[aaronc]

Should we maybe leave 0, “”, null, false, [], and {}. in here so that it's clear?

[alexanderbez]

Well, that wouldn't necessarily be true, correct? The output can still have [] and {} for example. I suppose we can clearly stipulate that default values should be omitted.

[aaronc]

Well the issue is consistency. Clients should know what to do and what not to do. If the app emits [] for signature verification sometimes and other times it doesn't, it's hard for clients to know how to implement signing. Knowing which cases to omit and not omit would be a lot of out of band knowledge. So the simplest is just to say omit them always, then everyone knows what to do.

[alexanderbez]

Consistency should be provided by Proto JSON.

I'm stating that its not omitted when explicitly provided, even when [] for example. In other words, only when the value is completely omitted, should the encoding output omit the default value.

e.g.

type Foo struct {
  A int
  B []string
}

protoJSON(&Foo{A: 5}) =/= protoJSON(&Foo{A: 5, B: []string{}})

idk...it seems like we're both stating the same thing.

[aaronc]

With proto 3, the idea is that empty and default are indistinguishable. So if we follow that logic, there is no correct way to distinguish empty and default.

Think about it this way, if I am a client and explicitly pass [] to my field in javascript, when I encode that message as protobuf binary it may or may not be omitted. When the app decodes the binary protobuf message, the field may be populated with an empty array or just null. We can't know from the protobuf binary whether the client intended to omit the field or pass [] because in proto 3 there is no real distinction and it may be implementation dependent. So there is no way to distinguish.

Thus I'm arguing that protoJSON(&Foo{A: 5}) should in fact be equal to protoJSON(&Foo{A: 5, B: []string{}}). If it isn't clients will have a horrible time signing messages...

[aaronc]

What I had been suggesting and the way I read this line in the ADR is that there are actually three passes. The first pass is proto json. The second pass is actually this stripping default values pass. Then the third pass is canonical json.

I think this second pass approach is preferable to requiring json:"omitempty" through documentation. It wouldn't necessarily need to be via reflection. It could be done by parsing json and reserializing without default values. Not super complex. Could be done in a separate PR from this one.

If that approach is okay, I would just revert this documentation line to make it clear for clients.

I know the second pass requires more upfront work initially, but seems better than a lot of manual coordination around json:"omitempty".

What do you think?

[alexanderbez]

Yeah, OK, then, in that case, we're on the same page as I agree omitempty is not preferable. I believe it should be done in this PR.

However, I don't think it's super trivial. You decode Proto JSON into a map[string][]interface{}. You have no idea whats an empty/default value and what isn't. So unless I'm missing something, you have to check each respective value (recursively).

How do you propose to "reserializing without default values"?

[aaronc]

Here's code that does it:

func StripDefaults(val interface{}) interface{} {
	switch val := val.(type) {
	case bool:
		if val == false {
			return nil
		}
	case string:
		if val == "" {
			return nil
		}
	case float64:
		if val == 0 {
			return nil
		}
	case []interface{}:
		n := len(val)
		if n == 0 {
			return nil
		}
		res := make([]interface{}, n)
		for i, x := range val {
			res[i] = StripDefaults(x)
		}
		return res
	case map[string]interface{}:
		res := make(map[string]interface{})
		for k, v := range val {
			v2 := StripDefaults(v)
			if v2 != nil {
				res[k] = v2
			}
		}
		if len(res) == 0 {
			return nil
		}
		return res
	}
	return val
}

Tested it and it works so far. I can open a PR against this branch.

[aaronc]

See #5815

[alexanderbez]

While this approach is similar to what I originally suggested, it's not complete or fully working (see here). Not to mention, it's not the most ideal code and flow to maintain.

Having tried to implement a clearer alternative using reflect (which works), you still end up with null values that are now the final JSON output (canonical JSON doesn't omit them).

I'm starting to consider going back to the omitempty requirement as that's an easier approach. Thoughts?