feat: value renderer for timestamp protos #12860

JimLarson · 2022-08-08T19:34:27Z

Description

Part of Sign Mode Textual (ADR 050) implementation.

Renders Timestamp messages as RFC 3339 (simplified ISO 8601).

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)

codecov · 2022-08-08T19:47:32Z

Codecov Report

Merging #12860 (48fd2d0) into main (4fe7797) will decrease coverage by 0.13%.
The diff coverage is 63.42%.

@@            Coverage Diff             @@
##             main   #12860      +/-   ##
==========================================
- Coverage   55.87%   55.74%   -0.14%     
==========================================
  Files         646      649       +3     
  Lines       54895    55022     +127     
==========================================
- Hits        30675    30671       -4     
- Misses      21762    21880     +118     
- Partials     2458     2471      +13

Impacted Files	Coverage Δ
baseapp/abci.go	`64.09% <0.00%> (-0.33%)`	⬇️
client/flags/flags.go	`19.35% <0.00%> (-0.32%)`	⬇️
client/rpc/status.go	`66.66% <ø> (ø)`
client/utils.go	`34.92% <0.00%> (ø)`
server/config/config.go	`38.00% <0.00%> (-1.59%)`	⬇️
server/rosetta/client_online.go	`1.35% <0.00%> (ø)`
server/swagger.go	`0.00% <0.00%> (ø)`
testutil/list.go	`0.00% <0.00%> (ø)`
types/result.go	`76.74% <ø> (ø)`
x/auth/tx/query.go	`0.00% <ø> (ø)`
... and 69 more

And other testing infrastructure to match.

JimLarson · 2022-08-10T02:47:44Z

tx/textual/valuerenderer/timestamp.go

+// Fractional seconds are only rendered if nonzero.
+func NewTimestampValueRenderer() ValueRenderer {
+	return timestampValueRenderer{}
+}


Note that I'm starting a convention of exporting functions to create the various default renderer types (and simplifying unit tests accordingly). This is to allow custom renderers to reuse standard components for portions of their representation rather than having to reinvent the wheel.

amaury1093

This PR LGTM. Just have a small nit about not using a switch statement, but a registry.

tx/textual/valuerenderer/timestamp_test.go

tx/textual/valuerenderer/valuerenderer.go

tx/textual/valuerenderer/valuerenderer_test.go

amaury1093 · 2022-08-10T16:23:23Z

tx/textual/valuerenderer/timestamp_test.go

+		{name: "empty", text: ""},
+		{name: "whitespace", text: "   "},
+		{name: "garbage", text: "garbage"},
+		{name: "silly_americans", text: "11/30/2007"},


One question I had for @webmaster128 was whether the test case JSON files should contain error cases too. I would say yes.

In the .json tests I created I made some cases where null values mean an error occurred without going into detail which error that is.

However, since I only implement message -> text and not the other way around, I don't have any parsing logic.

Am I missing something?

It would be good to have the portable test cases more comprehensive, but I don't think we can make everything JSON-driven, e.g. the parser test for a bad I/O reader.

Nevertheless, I think it would be good to have the JSON capture the core functionality. Does JS have a library for parsing the protobuf text format? If so, we could have a universal test case format with tests being JSON objects with the fields:

"proto": text representation of the proto. Would need to make a test proto message with fields for all the scalars.

"text": expected text rendering

"error": boolean indicating that an attempt to format the proto should result in an error, and there will be no "text" field for these cases.

I don't think we need to test error cases for parsing - we only need to ensure success of round trips from valid proto messages.

Each test case file would have an expectation of what sort of proto message the "proto" field should generate. Alternatively, we could use "A" as the universal test proto and name one of the fields to use. Then we'd have a single test driver for all types.

BTW, I've done this for the CEL Language
with data-driven test files.

Unfortunately, the protobuf support for JavaScript doesn't seem to implement textproto. I've found one project that provides such support, but it does not appear to be maintained.

So I added an "Error" field to the test cases for testing both bad Format and bad Parse cases.

I also find having JSON objects with field names better for readability (especially for newcomers) than arrays 👍

webmaster128

🐎

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

webmaster128 · 2022-08-10T16:29:51Z

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

+simplification of ISO 8601), which is the current recommendation for portable
+time values. The rendering always uses "Z" (UTC) as the timezone. It uses only
+the necessary fractional digits of a second, omitting the fractional part
+entirely if the timestamp has no fractional seconds.


In https://github.com/cosmos/cosmjs/pull/1147/files#diff-1aee49f4f44f595fc5e4c8a2f0e15fce0b30fab5ea6e9e8f1b2894a917b19fa5 I always used 9 digits.

I'm happy for that version. But please note this removed the ability for lexicographic sorting of timestamps.

I suggest that long unbroken strings of numbers hinder legibility (hence the reason for the int renderer). I would advocate for a position separator in the fractional part - but I expect that fractional seconds will be rare in practice, and even then probably limited to milliseconds. That's why I'm using as few digits as possible.

Perhaps we should consider truncating zero seconds too? But I think that would break RFC 3339.

Since the rendering is going to a ledger rather than a data file, I think the legibility is more important than the sortability.

+1 for truncating unnecessary fractional seconds, but let's not break RFC3339. It's a format that people are used to.

Edit: we can add here the rationale of truncating, i.e. favor legibility over sortability.

webmaster128 · 2022-08-10T16:31:04Z

tx/textual/internal/testdata/timestamp.json

+        {"seconds": "1136214245", "nanos": 123000000},
+        "2006-01-02T15:04:05.123Z"
+    ]
+]


Can you use the tests from https://github.com/cosmos/cosmjs/pull/1147/files#diff-1aee49f4f44f595fc5e4c8a2f0e15fce0b30fab5ea6e9e8f1b2894a917b19fa5?

Done. Dropped a duplicate and also dropped the out-of-range tests, as the straightforward Go implementation doesn't return errors for those cases. It looks like the underlying Go library is able to tolerate slightly out-of-spec inputs, and that shouldn't be an error, IMHO.

JimLarson · 2022-08-18T04:22:14Z

Reviewers PTAL - addressed all comments.

amaury1093

LGTM, thanks @JimLarson

amaury1093 · 2022-08-18T09:42:24Z

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

+simplification of ISO 8601), which is the current recommendation for portable
+time values. The rendering always uses "Z" (UTC) as the timezone. It uses only
+the necessary fractional digits of a second, omitting the fractional part
+entirely if the timestamp has no fractional seconds.


+1 for truncating unnecessary fractional seconds, but let's not break RFC3339. It's a format that people are used to.

Edit: we can add here the rationale of truncating, i.e. favor legibility over sortability.

JimLarson · 2022-08-19T17:00:12Z

Added the rationale for fractional second truncation vs sortability.

Also confirmed 100% test coverage for the added code (except for the TODO for handling default proto messages).

JimLarson · 2022-08-22T23:19:32Z

Core reviewer - ready for your approval.

Changelog update intentionally omitted - we'll have one entry for the whole functionality.

amaury1093 · 2022-08-23T14:05:23Z

I put automerge on.

This branch is out-of-date with the base branch

@JimLarson could you rebase/merge master into your branch? Or alternatively give write access to your branch, so that our bot can do it.

…estamp-renderer No merge conflicts.

## Description Closes: cosmos#12709 Part of Sign Mode Textual (ADR 050) implementation. Renders Timestamp messages as RFC 3339 (simplified ISO 8601). --- ### 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... - [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [x] added `!` to the type prefix if API or client breaking change - [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting)) - [x] provided a link to the relevant issue or specification - [x] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules) - [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [x] included comments for [documenting Go code](https://blog.golang.org/godoc) - [x] updated the relevant documentation or specification - [x] reviewed "Files changed" and left comments if necessary - [x] 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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)

feat: value renderer for timestamp protos

00ee180

JimLarson added 2 commits August 9, 2022 18:33

feat: dispatch for timestamp renderer, plus json testdata

9915821

And other testing infrastructure to match.

docs: update the ADR annex for timestamps

836d395

github-actions bot added the Type: ADR label Aug 10, 2022

JimLarson mentioned this pull request Aug 10, 2022

docs(textual): custom Msg renderers #12785

Merged

19 tasks

JimLarson marked this pull request as ready for review August 10, 2022 02:19

JimLarson requested a review from a team as a code owner August 10, 2022 02:19

JimLarson commented Aug 10, 2022

View reviewed changes

amaury1093 assigned amaury1093 and kocubinski Aug 10, 2022

amaury1093 reviewed Aug 10, 2022

View reviewed changes

webmaster128 reviewed Aug 10, 2022

View reviewed changes

JimLarson added 4 commits August 17, 2022 13:31

test: switch more timestamp tests to be data driven

c5b4f76

refactor: make message dispatch table-driven

4c1b5bb

docs: extra example for timestamps

469c4f8

tests: add cosmosjs timestamp test cases

b317220

amaury1093 approved these changes Aug 18, 2022

View reviewed changes

docs: add rationale for fractional second truncation

9501781

alexanderbez approved these changes Aug 23, 2022

View reviewed changes

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

kocubinski approved these changes Aug 23, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/main' into jimlarson/12709-tim…

48fd2d0

…estamp-renderer No merge conflicts.

JimLarson force-pushed the jimlarson/12709-timestamp-renderer branch from 9499677 to 48fd2d0 Compare August 23, 2022 18:02

mergify bot merged commit ef4ad67 into cosmos:main Aug 23, 2022

JimLarson deleted the jimlarson/12709-timestamp-renderer branch August 23, 2022 20:45

JimLarson mentioned this pull request Aug 30, 2022

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

Closed

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: value renderer for timestamp protos #12860

feat: value renderer for timestamp protos #12860

JimLarson commented Aug 8, 2022 •

edited

Loading

codecov bot commented Aug 8, 2022 •

edited

Loading

JimLarson Aug 10, 2022

amaury1093 left a comment

amaury1093 Aug 10, 2022

webmaster128 Aug 10, 2022

JimLarson Aug 17, 2022

JimLarson Aug 17, 2022

amaury1093 Aug 18, 2022

webmaster128 left a comment

webmaster128 Aug 10, 2022

JimLarson Aug 17, 2022

amaury1093 Aug 18, 2022 •

edited

Loading

webmaster128 Aug 10, 2022

JimLarson Aug 17, 2022

JimLarson commented Aug 18, 2022

amaury1093 left a comment

amaury1093 Aug 18, 2022 •

edited

Loading

JimLarson commented Aug 19, 2022

JimLarson commented Aug 22, 2022

amaury1093 commented Aug 23, 2022

feat: value renderer for timestamp protos #12860

feat: value renderer for timestamp protos #12860

Conversation

JimLarson commented Aug 8, 2022 • edited Loading

Description

Author Checklist

Reviewers Checklist

codecov bot commented Aug 8, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

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

webmaster128 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaury1093 Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JimLarson commented Aug 18, 2022

amaury1093 left a comment

Choose a reason for hiding this comment

amaury1093 Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

JimLarson commented Aug 19, 2022

JimLarson commented Aug 22, 2022

amaury1093 commented Aug 23, 2022

JimLarson commented Aug 8, 2022 •

edited

Loading

codecov bot commented Aug 8, 2022 •

edited

Loading

amaury1093 Aug 18, 2022 •

edited

Loading

amaury1093 Aug 18, 2022 •

edited

Loading