feat: string and message value renderers for textual #13510

JimLarson · 2022-10-11T20:54:42Z

Description

Sign mode textual value renderers for string and message.

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)

JimLarson · 2022-10-11T20:56:18Z

Draft checkin to illustrate issue in valuerenderer.go:GetValueRenderer() for message types. Can't get to the Go structure from descriptors.

codecov · 2022-10-11T21:04:54Z

Codecov Report

Merging #13510 (107ceac) into main (a1b8ec2) will increase coverage by 0.00%.
The diff coverage is 61.71%.

Additional details and impacted files

@@           Coverage Diff            @@
##             main   #13510    +/-   ##
========================================
  Coverage   54.81%   54.81%            
========================================
  Files         652      654     +2     
  Lines       56023    56150   +127     
========================================
+ Hits        30707    30780    +73     
- Misses      22810    22847    +37     
- Partials     2506     2523    +17

Impacted Files	Coverage Δ
tx/textual/valuerenderer/valuerenderer.go	`72.41% <30.00%> (-7.18%)`	⬇️
tx/textual/valuerenderer/message.go	`64.22% <64.22%> (ø)`
tx/textual/valuerenderer/string.go	`66.66% <66.66%> (ø)`
x/staking/simulation/operations.go	`74.54% <0.00%> (-1.38%)`	⬇️
x/group/keeper/keeper.go	`56.25% <0.00%> (-0.40%)`	⬇️
crypto/keys/internal/ecdsa/privkey.go	`83.01% <0.00%> (+1.88%)`	⬆️

peterbourgon · 2022-10-12T02:35:36Z

docs/architecture/adr-050-sign-mode-textual.md


-The encoding and decoding operations between a Protobuf transaction (whose definition can be found [here](https://github.com/cosmos/cosmos-sdk/blob/master/proto/cosmos/tx/v1beta1/tx.proto#L13)) and the string array MUST be bijective.
+### Invertible Rendering


What motivates the change in nomenclature away from "bijective"?

Note that this draft PR is based off of the not-yet-committed #13434, so you're commenting on changes that will go away once this PR is rebased. You comments might be more appropriate for #13434.

But to quickly answer, "bijective" is not exactly correct. That would require that every valid Tx round-trip back to an equal Tx (yes) and also that every possible string round-trip back to an equal string (no). Having just the first condition makes Format() an "injection" (i.e. one-to-one), but this was deemed to be too technical, hence the use of "invertible".

Should I move my comments to #13434, or is there enough visibility here?

yes that's preferable. Let's keep this PR as message value renderer only

Hmm - #13434 (structured screens) is just awaiting final approval, so I don't want to derail that. I'm not seeing anything in these comments that would make me want to add any late changes. This discussion seems more appropriate for #12910 - but as you can see that ship sailed a while ago. @peterbourgon it would be most appropriate if you could open an issue for discussion and raise you concerns there. I'm going to change the target for this draft to we can see the actual content of this PR better.

peterbourgon · 2022-10-12T02:53:38Z

docs/architecture/adr-050-sign-mode-textual.md

-}
-pass_check()
-```
+Moreover, the renderer must provide 2 functions: one for formatting from Protobuf to string, and one for parsing string to Protobuf. These 2 functions are provided by the application developer. To satisfy point #1, these 2 functions MUST be bijective with each other. Bijectivity of these 2 functions will not be checked by the SDK at runtime. However, we strongly recommend the application developer to include a comprehensive suite in their app repo to test bijectivity, as to not introduce security bugs.


If the application fails to ensure bijectivity, are any guarantees asserted by Cosmos and/or Tendermint violated?

peterbourgon · 2022-10-12T02:56:50Z

docs/architecture/adr-050-sign-mode-textual.md

+Note that the existence of an inverse function ensures that the
+rendered text contains the full information of the original transaction,
+not a hash or subset.


Well, it claims this property, but doesn't really ensure it, right?

peterbourgon · 2022-10-12T03:03:15Z

docs/architecture/adr-050-sign-mode-textual.md

+The correctness and security of `SIGN_MODE_TEXTUAL` is guaranteed by demonstrating an inverse function from the rendering to transaction protos.
+This means that it is impossible for a different protocol buffer message to render to the same text.


It appears that inversability is not guaranteed or even verified by the SDK, right? It's just assumed to be true. So if an implementation happens to have a bug that violates this assumption, and isn't fully inversible in some circumstance, what happens?

peterbourgon · 2022-10-12T03:04:35Z

docs/architecture/adr-050-sign-mode-textual.md

+When client software forms a transaction, the "raw" transaction (`TxRaw`) is serialized as a proto
+and a hash of the resulting byte sequence is computed.


Note that protobuf serialization is not deterministic.

By default, repeated invocations of serialization methods on the same protocol buffer message instance may not produce the same byte output. That is, the default serialization is not deterministic.

Deterministic serialization only guarantees the same byte output for a particular binary. The byte output may change across different versions of the binary.

tx/textual/valuerenderer/message.go

docs/architecture/adr-050-sign-mode-textual-annex2.md

joeabbey · 2022-10-17T16:09:57Z

docs/architecture/adr-050-sign-mode-textual.md

-Bijectivity will be tested in two ways:
+Note that this inverse function does not need to perform correct
+parsing or error signaling for the whole domain of textual data.
+Merely that the range of valid transactions be invertible under


Suggested change

Merely that the range of valid transactions be invertible under

Merely, the range of valid transactions be invertible under

Still considering this. Where'd I put my Strunk & White?

joeabbey · 2022-10-17T16:10:22Z

docs/architecture/adr-050-sign-mode-textual.md


- by providing a set of test fixtures between a transaction's Proto JSON representation and its TEXTUAL representation, and checking that encoding/decoding in both directions matches the fixtures,
- by using property testing on the proto transaction itself, and testing that the composition of encoding and decoding yields the original transaction itself.
+Note that the existence of an inverse function ensures that the


Suggested change

Note that the existence of an inverse function ensures that the

Note: the existence of an inverse function ensures that the

I prefer the original, but push back if you feel strongly.

JimLarson · 2022-10-20T19:20:36Z

@joeabbey Thanks for finding al those problems in the docs! Unfortunately, those aren't really part of this PR - the structured screens PR hadn't been merged yet, so I branched this off the end of it, and its contents were displayed here since there was no way to make it the target. In any case, I've created #13608 for minor documentation fixes and addressed your comments there.

amaury1093

Nice work on this PR @JimLarson 👏

I left 2 small comments. I'll approve and put automerge once they are addressed

amaury1093 · 2022-10-21T10:41:42Z

tx/textual/internal/testdata/message.json

+        "screens": [
+            {"text": "Foo object"},
+            {"text": "Full name: subfield", "indent": 1},
+            {"text": "Left: Foo object", "indent": 1},
+            {"text": "Right: Foo object", "indent": 1},
+            {"text": "Nickname: junior", "indent": 2},
+            {"text": "Bar: Bar object", "indent": 1}
+        ]


This is an interesting test case. I'm wondering if the indentation is enough to clearly denote for humans the beginning and end of a struct. From a bijectivity view, yes indent is enough, but from a human-readability one, it's almost as if we're expecting the next line to be Bar's fields.

When writing the spec initially, I was pondering on whether to add a End of Foo object (like we do in repeated) or not. Looking at this test case, I'm considering yes.

I'd weigh the utility of the footer screen against the disadvantage of additional clutter. I think it's important to optimize legibility for the common cases, and I doubt that present-but-empty messages will be common. This was mostly meant to explore the corner cases of the renderer than to represent an important test case.

amaury1093 · 2022-10-21T10:42:17Z

tx/textual/internal/testdata/message.json

+            {"text": "Bar id: quux", "indent": 3},
+            {"text": "Data: FFFE", "indent": 3}
+        ]
+    }


I would add a couple of parsing error cases:

[]

[{"text": "Bar object"}]

[{"text": "Foo object", "indent": 1}]

[{"text": "Foo object"}, {"text": "Full name: deep", indent: 2}]

[{"text": "Foo object"}, {"text": "Leftover screen"}]

I'm inclined to not care about the limits of Parse(). It must successfully invert any Screens produced by Format(), but other than that it can really do whatever it wants.

joeabbey · 2022-10-21T13:39:08Z

tx/textual/valuerenderer/valuerenderer.go

+	case fd.Kind() == protoreflect.StringKind:
+		return stringValueRenderer{}, nil


Do we need two StringKind cases?

why not incorporate in the above?

case fd.Kind() == protoreflect.StringKind: if proto.GetExtension(fd.Options(), cosmos_proto.E_Scalar) == "" { return stringValueRenderer{}, nil } else { scalar, ok := proto.GetExtension(fd.Options(), cosmos_proto.E_Scalar).(string) ...

JimLarson · 2022-10-24T18:41:59Z

Updated. @AmauryM please re-enable automerge.

This change adds value renderers for message and string types. I also added some message types to testpb with nested messages + snake-cased names and without repeated fields (which aren't implemented yet). Updates cosmos#12713

github-actions bot added the Type: ADR label Oct 11, 2022

peterbourgon reviewed Oct 12, 2022

View reviewed changes

amaury1093 reviewed Oct 12, 2022

View reviewed changes

tx/textual/valuerenderer/message.go Outdated Show resolved Hide resolved

amaury1093 mentioned this pull request Oct 13, 2022

feat(textual): add renderers for message and string #12878

Closed

19 tasks

joeabbey approved these changes Oct 17, 2022

View reviewed changes

joeabbey mentioned this pull request Oct 20, 2022

feat: Value Renderer for repeated #13604

Merged

4 tasks

JimLarson force-pushed the jimlarson/textual-protomessage-render branch from 26f3089 to 4898da0 Compare October 20, 2022 19:04

github-actions bot removed the Type: ADR label Oct 20, 2022

JimLarson marked this pull request as ready for review October 20, 2022 23:41

JimLarson requested a review from a team as a code owner October 20, 2022 23:41

amaury1093 reviewed Oct 21, 2022

View reviewed changes

joeabbey reviewed Oct 21, 2022

View reviewed changes

amaury1093 approved these changes Oct 24, 2022

View reviewed changes

amaury1093 enabled auto-merge (squash) October 24, 2022 10:11

auto-merge was automatically disabled October 24, 2022 18:40
Head branch was pushed to by a user without write access

JimLarson force-pushed the jimlarson/textual-protomessage-render branch from 60a3f1f to 53f738c Compare October 24, 2022 18:40

amaury1093 enabled auto-merge (squash) October 24, 2022 20:15

cool-develope approved these changes Oct 24, 2022

View reviewed changes

auto-merge was automatically disabled October 24, 2022 23:25
Head branch was pushed to by a user without write access

JimLarson force-pushed the jimlarson/textual-protomessage-render branch from 53f738c to 4801283 Compare October 24, 2022 23:25

amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Oct 25, 2022

julienrbrt approved these changes Oct 25, 2022

View reviewed changes

kirbyquerby and others added 11 commits October 25, 2022 10:05

tx/textual: add renderers for message and string

b495f34

This change adds value renderers for message and string types. I also added some message types to testpb with nested messages + snake-cased names and without repeated fields (which aren't implemented yet). Updates cosmos#12713

add logic to skip default-valued fields

8805e89

better test name :P

0edc540

address comments

ec4a4ef

address comments

86fe897

fix: adapt message and string renderers for structured screens

425e831

test: add string renderer test and testdata

a1b2438

feat: message parser and testcase file - not working yet

3a73c3c

fix: use proto registry in message parsing

a4479da

fix: search for specialized message handlers first

05769ad

test: more challenging tests

107ceac

JimLarson force-pushed the jimlarson/textual-protomessage-render branch from 4801283 to 107ceac Compare October 25, 2022 17:05

mergify bot merged commit dc3cf4a into cosmos:main Oct 25, 2022

JimLarson deleted the jimlarson/textual-protomessage-render branch October 25, 2022 17:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: string and message value renderers for textual #13510

feat: string and message value renderers for textual #13510

JimLarson commented Oct 11, 2022

JimLarson commented Oct 11, 2022

codecov bot commented Oct 11, 2022 •

edited

Loading

peterbourgon Oct 12, 2022

JimLarson Oct 12, 2022

peterbourgon Oct 12, 2022

amaury1093 Oct 12, 2022 •

edited

Loading

JimLarson Oct 12, 2022

peterbourgon Oct 12, 2022

peterbourgon Oct 12, 2022

peterbourgon Oct 12, 2022

peterbourgon Oct 12, 2022

joeabbey Oct 17, 2022

JimLarson Oct 20, 2022

joeabbey Oct 17, 2022

JimLarson Oct 20, 2022

JimLarson commented Oct 20, 2022

amaury1093 left a comment

amaury1093 Oct 21, 2022

JimLarson Oct 23, 2022

amaury1093 Oct 21, 2022

JimLarson Oct 23, 2022

joeabbey Oct 21, 2022

JimLarson commented Oct 24, 2022


		The encoding and decoding operations between a Protobuf transaction (whose definition can be found [here](https://github.com/cosmos/cosmos-sdk/blob/master/proto/cosmos/tx/v1beta1/tx.proto#L13)) and the string array MUST be bijective.
		### Invertible Rendering

		The correctness and security of `SIGN_MODE_TEXTUAL` is guaranteed by demonstrating an inverse function from the rendering to transaction protos.
		This means that it is impossible for a different protocol buffer message to render to the same text.

		When client software forms a transaction, the "raw" transaction (`TxRaw`) is serialized as a proto
		and a hash of the resulting byte sequence is computed.

	Merely that the range of valid transactions be invertible under
	Merely, the range of valid transactions be invertible under

	Note that the existence of an inverse function ensures that the
	Note: the existence of an inverse function ensures that the

		case fd.Kind() == protoreflect.StringKind:
		return stringValueRenderer{}, nil

feat: string and message value renderers for textual #13510

feat: string and message value renderers for textual #13510

Conversation

JimLarson commented Oct 11, 2022

Description

Author Checklist

Reviewers Checklist

JimLarson commented Oct 11, 2022

codecov bot commented Oct 11, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaury1093 Oct 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JimLarson commented Oct 20, 2022

amaury1093 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JimLarson commented Oct 24, 2022

codecov bot commented Oct 11, 2022 •

edited

Loading

amaury1093 Oct 12, 2022 •

edited

Loading