feat: Value Renderer for repeated #13604

joeabbey · 2022-10-20T17:13:41Z

Next steps:

Repeated for additional types
Refining conformance to ADR50
Parsing of Lists
Parsing of custom value renderers (e.g. Coins)

codecov · 2022-10-21T14:14:46Z

Codecov Report

Merging #13604 (5d1cd9d) into main (2739f83) will increase coverage by 0.45%.
The diff coverage is 73.28%.

❗ Current head 5d1cd9d differs from pull request most recent head d771861. Consider uploading reports for the commit d771861 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13604      +/-   ##
==========================================
+ Coverage   56.25%   56.71%   +0.45%     
==========================================
  Files         667      647      -20     
  Lines       56576    55435    -1141     
==========================================
- Hits        31829    31441     -388     
+ Misses      22165    21423     -742     
+ Partials     2582     2571      -11

Impacted Files	Coverage Δ
tx/textual/valuerenderer/coins.go	`69.56% <40.00%> (ø)`
tx/textual/valuerenderer/message.go	`69.62% <73.72%> (ø)`
tx/textual/valuerenderer/valuerenderer.go	`76.47% <100.00%> (ø)`
x/distribution/simulation/operations.go	`80.64% <0.00%> (-9.68%)`	⬇️
x/gov/abci.go	`90.29% <0.00%> (-6.86%)`	⬇️
server/config/config.go	`69.87% <0.00%> (-2.65%)`	⬇️
x/group/internal/math/dec.go	`47.14% <0.00%> (-1.39%)`	⬇️
x/group/module/module.go	`48.05% <0.00%> (-0.10%)`	⬇️
server/mock/tx.go	`53.33% <0.00%> (ø)`
server/config/toml.go	`66.66% <0.00%> (ø)`
... and 46 more

tx/textual/internal/testdata/repeated.json

tx/textual/valuerenderer/repeated.go

amaury1093

Nice! Left some comments

tx/textual/valuerenderer/valuerenderer.go

tx/textual/internal/testdata/repeated.json

tx/textual/valuerenderer/valuerenderer.go

tx/textual/valuerenderer/repeated.go

amaury1093

Looking good! I left a comment about removing Kind() from the interface.

tx/textual/valuerenderer/types.go

tx/textual/internal/testpb/1.proto

tx/textual/valuerenderer/message.go

tx/textual/valuerenderer/message_test.go

tx/textual/internal/testpb/1.proto

tx/textual/valuerenderer/message.go

JimLarson

Good progress, and good to hear that you're working on the parse.

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

tx/textual/valuerenderer/message.go

tx/textual/valuerenderer/repeated_test.go

JimLarson · 2022-11-08T01:59:03Z

tx/textual/internal/testdata/repeated.json

@@ -0,0 +1,157 @@
+[


Add these test cases to message.json instead of using a separate file - allows us to reuse the same test driver code.

I'm not sure I agree with this one. I still think the reason we put repeated logic in messageValueRenderer is an implementation detail. Conceptually, I prefer them to be 2 different value renderers. I'd love to know how JS implements this.

tx/textual/valuerenderer/valuerenderer.go

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

amaury1093 · 2022-11-14T11:09:06Z

tx/textual/internal/testdata/repeated.json

@@ -0,0 +1,157 @@
+[


I'm not sure I agree with this one. I still think the reason we put repeated logic in messageValueRenderer is an implementation detail. Conceptually, I prefer them to be 2 different value renderers. I'd love to know how JS implements this.

amaury1093

This overall LGTM! Nice progress on this PR.

My only requested change is to think about the ParseRepeated function signature.

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

tx/textual/internal/testpb/1.proto

tx/textual/internal/testdata/repeated.json

tx/textual/valuerenderer/message.go

tx/textual/valuerenderer/types.go

tx/textual/valuerenderer/repeated_test.go

amaury1093

Approving, thanks @joeabbey!

Just asked a question if we should be more rigorous in Parse, but I'm fine with the current version

amaury1093 · 2022-11-17T19:03:06Z

tx/textual/valuerenderer/message.go

+func (mr *messageValueRenderer) parseRepeated(ctx context.Context, screens []Screen, l protoreflect.List, vr ValueRenderer) error {
+
+	// <int> <field_kind>
+	headerRegex := *regexp.MustCompile(`(\d+) .+`)


Should parse check that <field_kind> is correct too? We would need to pass fd protoreflect.FieldDescriptor as an arg too.

Maybe it's fine for Parse to not be that strict.

What's the criteria for <field_kind>? We should perhaps add it into the regex. Also we need to ensure that the regex begins with an integer or permit spaces but essentially perhaps this

headerRegex := regexp.MustCompile(`^\s*(\d+) (\w+|\d+)`)

and no need to dereference the result from regexp.MustCompile.

The field kind is documented in Annex 1 of ADR 50:

field_kind:

if the type of the repeated field is a message, field_kind is the message name

if the type of the repeated field is an enum, field_kind is the enum name

in any other case, field_kind is the protobuf primitive type (e.g. "string" or "bytes")

amaury1093 · 2022-11-23T16:32:58Z

@facundomedica I'm assigning you here too (if you have bandwidth), since you said you might be interested in looking into Textual.

odeke-em

Thank you for this change @joeabbey and great to bump into you here!

I've added some suggestions, please take a look.

tx/textual/valuerenderer/message.go

odeke-em · 2022-11-27T00:44:35Z

tx/textual/valuerenderer/message.go

+func (mr *messageValueRenderer) parseRepeated(ctx context.Context, screens []Screen, l protoreflect.List, vr ValueRenderer) error {
+
+	// <int> <field_kind>
+	headerRegex := *regexp.MustCompile(`(\d+) .+`)


What's the criteria for <field_kind>? We should perhaps add it into the regex. Also we need to ensure that the regex begins with an integer or permit spaces but essentially perhaps this

headerRegex := regexp.MustCompile(`^\s*(\d+) (\w+|\d+)`)

and no need to dereference the result from regexp.MustCompile.

tx/textual/valuerenderer/message.go

tx/textual/valuerenderer/valuerenderer.go

…render

facundomedica

LGTM, the test proto doesn't seem well formatted but we can solve that later

…render

julienrbrt · 2022-11-29T19:53:54Z

tx/textual/valuerenderer/valuerenderer.go

-		{
-			return NewIntValueRenderer(), nil
-		}
+		return NewIntValueRenderer(), nil

 	case fd.Kind() == protoreflect.StringKind:


This condition is duplicated: https://sonarcloud.io/project/issues?resolved=false&sinceLeakPeriod=true&types=BUG&id=cosmos_cosmos-sdk&open=AYTE5cUYJ1y5ta4n9bJ5

Thanks! I'll fix it in my PR then: #13600

joeabbey requested a review from a team as a code owner October 20, 2022 17:13

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

joeabbey force-pushed the joeabbey/textual-repeated-repeated-repeated-render branch from 4a39d19 to 4fcbfd3 Compare October 21, 2022 14:03

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

joeabbey changed the title ~~[STACKED] Value Renderer for repeated~~ feat: Value Renderer for repeated Oct 21, 2022

JimLarson suggested changes Oct 24, 2022

View reviewed changes

facundomedica assigned amaury1093 Oct 26, 2022

joeabbey self-assigned this Oct 26, 2022

github-actions bot added C:CLI C:Cosmovisor Issues and PR related to Cosmovisor C:orm C:Store C:x/auth C:x/bank C:x/feegrant C:x/gov C:x/params C:x/upgrade Type: ADR Type: Build Type: CI labels Oct 31, 2022

joeabbey force-pushed the joeabbey/textual-repeated-repeated-repeated-render branch from 5d7f0fa to 7e018a6 Compare October 31, 2022 15:03

github-actions bot removed C:CLI C:x/bank C:Store C:x/feegrant C:x/gov C:x/params Type: Build labels Oct 31, 2022

github-actions bot removed C:x/auth Type: ADR Type: CI C:orm C:x/upgrade C:Cosmovisor Issues and PR related to Cosmovisor labels Oct 31, 2022

amaury1093 reviewed Oct 31, 2022

View reviewed changes

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

amaury1093 marked this pull request as draft November 2, 2022 10:26

amaury1093 reviewed Nov 3, 2022

View reviewed changes

JimLarson reviewed Nov 11, 2022

View reviewed changes

amaury1093 reviewed Nov 14, 2022

View reviewed changes

amaury1093 reviewed Nov 17, 2022

View reviewed changes

amaury1093 approved these changes Nov 17, 2022

View reviewed changes

amaury1093 marked this pull request as ready for review November 17, 2022 19:04

amaury1093 assigned facundomedica Nov 23, 2022

odeke-em reviewed Nov 27, 2022

View reviewed changes

Support for repeated

14c5057

joeabbey force-pushed the joeabbey/textual-repeated-repeated-repeated-render branch from 5d1cd9d to 14c5057 Compare November 28, 2022 17:39

Merge branch 'main' into joeabbey/textual-repeated-repeated-repeated-…

ebec624

…render

joeabbey enabled auto-merge (squash) November 28, 2022 21:20

amaury1093 mentioned this pull request Nov 29, 2022

feat: implement parse method for Int,Dec,Coin #13696

Merged

19 tasks

Merge branch 'main' into joeabbey/textual-repeated-repeated-repeated-…

de879f1

…render

facundomedica approved these changes Nov 29, 2022

View reviewed changes

Merge branch 'main' into joeabbey/textual-repeated-repeated-repeated-…

a6ca57e

…render

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

Merge branch 'main' into joeabbey/textual-repeated-repeated-repeated-…

d771861

…render

joeabbey merged commit 2cf92ca into cosmos:main Nov 29, 2022

julienrbrt reviewed Nov 29, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Value Renderer for repeated #13604

feat: Value Renderer for repeated #13604

joeabbey commented Oct 20, 2022 •

edited

Loading

codecov bot commented Oct 21, 2022 •

edited

Loading

amaury1093 left a comment

amaury1093 left a comment

JimLarson left a comment

JimLarson Nov 8, 2022

amaury1093 Nov 14, 2022

amaury1093 Nov 14, 2022

amaury1093 left a comment

amaury1093 left a comment

amaury1093 Nov 17, 2022 •

edited

Loading

odeke-em Nov 27, 2022

joeabbey Nov 28, 2022

amaury1093 commented Nov 23, 2022

odeke-em left a comment

odeke-em Nov 27, 2022

facundomedica left a comment

julienrbrt Nov 29, 2022

amaury1093 Nov 30, 2022

feat: Value Renderer for repeated #13604

feat: Value Renderer for repeated #13604

Conversation

joeabbey commented Oct 20, 2022 • edited Loading

codecov bot commented Oct 21, 2022 • edited Loading

Codecov Report

amaury1093 left a comment

Choose a reason for hiding this comment

amaury1093 left a comment

Choose a reason for hiding this comment

JimLarson 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

amaury1093 left a comment

Choose a reason for hiding this comment

amaury1093 left a comment

Choose a reason for hiding this comment

amaury1093 Nov 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaury1093 commented Nov 23, 2022

odeke-em left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facundomedica left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeabbey commented Oct 20, 2022 •

edited

Loading

codecov bot commented Oct 21, 2022 •

edited

Loading

amaury1093 Nov 17, 2022 •

edited

Loading