Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(textual): Use Title/Content instead of Text #14730

Merged
merged 19 commits into from
Feb 2, 2023
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions docs/architecture/adr-050-sign-mode-textual.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
* Dec 01, 2022: Link to examples in separate JSON file.
* Dec 06, 2022: Re-ordering of envelope screens.
* Dec 14, 2022: Mention exceptions for invertability.
* Jan 23, 2022: Switch Screen.Text to Title+Content.

## Status

Expand Down Expand Up @@ -126,10 +127,11 @@ in the rendering.

The SignDoc for `SIGN_MODE_TEXTUAL` is formed from a data structure like:

```
```go
type Screen struct {
Text string text // possibly size limited to, e.g. 255 characters
Indent uint8 // size limited to something small like 16 or 32
Title string // possibly size limited to, e.g. 64 characters
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider "advised to X characters".

Content string // possibly size limited to, e.g. 255 characters
Indent uint8 // size limited to something small like 16 or 32
Expert bool
}

Expand All @@ -154,15 +156,17 @@ screens = [* screen]
;; Text defaults to the empty string, indent defaults to zero,
;; and expert defaults to false.
screen = {
? text_key: tstr,
? title_key: tstr,
? content_key: tstr,
? indent_key: uint,
? expert_key: bool,
}

;; Keys are small integers to keep the encoding small.
text_key = 1
indent_key = 2
expert_key = 3
title_key = 1
content_key = 2
indent_key = 3
expert_key = 4
```

## Details
Expand Down
4 changes: 2 additions & 2 deletions x/tx/textual/any.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func (ar anyValueRenderer) Format(ctx context.Context, v protoreflect.Value) ([]
}

screens := make([]Screen, 1+len(subscreens))
screens[0].Text = anymsg.GetTypeUrl()
screens[0].Content = anymsg.GetTypeUrl()
for i, subscreen := range subscreens {
subscreen.Indent++
screens[i+1] = subscreen
Expand All @@ -64,7 +64,7 @@ func (ar anyValueRenderer) Parse(ctx context.Context, screens []Screen) (protore
return nilValue, fmt.Errorf("bad indentation: want 0, got %d", screens[0].Indent)
}

msgType, err := protoregistry.GlobalTypes.FindMessageByURL(screens[0].Text)
msgType, err := protoregistry.GlobalTypes.FindMessageByURL(screens[0].Content)
if err != nil {
return nilValue, err
}
Expand Down
6 changes: 3 additions & 3 deletions x/tx/textual/bytes.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ func (vr bytesValueRenderer) Format(ctx context.Context, v protoreflect.Value) (

if len(bz) <= maxByteLen {
text := toHex(bz)
return []Screen{{Text: text}}, nil
return []Screen{{Content: text}}, nil
}

// For long bytes, we display the hash.
Expand All @@ -43,14 +43,14 @@ func (vr bytesValueRenderer) Format(ctx context.Context, v protoreflect.Value) (
h := hasher.Sum(nil)

text := fmt.Sprintf("%s%s", hashPrefix, toHex(h))
return []Screen{{Text: text}}, nil
return []Screen{{Content: text}}, nil
}

func (vr bytesValueRenderer) Parse(_ context.Context, screens []Screen) (protoreflect.Value, error) {
if len(screens) != 1 {
return nilValue, fmt.Errorf("expected single screen: %v", screens)
}
formatted := screens[0].Text
formatted := screens[0].Content

// If the formatted string starts with `SHA-256=`, then we can't actually
// invert to get the original bytes. In this case, we return empty bytes.
Expand Down
2 changes: 1 addition & 1 deletion x/tx/textual/bytes_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ func TestBytesJsonTestCases(t *testing.T) {
screens, err := valrend.Format(context.Background(), protoreflect.ValueOfBytes(tc.base64))
require.NoError(t, err)
require.Equal(t, 1, len(screens))
require.Equal(t, tc.hex, screens[0].Text)
require.Equal(t, tc.hex, screens[0].Content)

// Round trip
val, err := valrend.Parse(context.Background(), screens)
Expand Down
2 changes: 1 addition & 1 deletion x/tx/textual/coin_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ func TestCoinJsonTestcases(t *testing.T) {

require.NoError(t, err)
require.Equal(t, 1, len(screens))
require.Equal(t, tc.Text, screens[0].Text)
require.Equal(t, tc.Text, screens[0].Content)

// Round trip.
value, err := vr.Parse(ctx, screens)
Expand Down
12 changes: 6 additions & 6 deletions x/tx/textual/coins.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ func (vr coinsValueRenderer) Format(ctx context.Context, v protoreflect.Value) (
return nil, err
}

return []Screen{{Text: formatted}}, nil
return []Screen{{Content: formatted}}, nil
}

func (vr coinsValueRenderer) FormatRepeated(ctx context.Context, v protoreflect.Value) ([]Screen, error) {
Expand All @@ -74,19 +74,19 @@ func (vr coinsValueRenderer) FormatRepeated(ctx context.Context, v protoreflect.
return nil, err
}

return []Screen{{Text: formatted}}, nil
return []Screen{{Content: formatted}}, nil
}

func (vr coinsValueRenderer) Parse(ctx context.Context, screens []Screen) (protoreflect.Value, error) {
if len(screens) != 1 {
return nilValue, fmt.Errorf("expected single screen: %v", screens)
}

if screens[0].Text == emptyCoins {
if screens[0].Content == emptyCoins {
return protoreflect.ValueOfMessage((&basev1beta1.Coin{}).ProtoReflect()), nil
}

parsed, err := vr.parseCoins(ctx, screens[0].Text)
parsed, err := vr.parseCoins(ctx, screens[0].Content)
if err != nil {
return nilValue, err
}
Expand All @@ -99,11 +99,11 @@ func (vr coinsValueRenderer) ParseRepeated(ctx context.Context, screens []Screen
return fmt.Errorf("expected single screen: %v", screens)
}

if screens[0].Text == emptyCoins {
if screens[0].Content == emptyCoins {
return nil
}

parsed, err := vr.parseCoins(ctx, screens[0].Text)
parsed, err := vr.parseCoins(ctx, screens[0].Content)
if err != nil {
return err
}
Expand Down
2 changes: 1 addition & 1 deletion x/tx/textual/coins_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ func TestCoinsJsonTestcases(t *testing.T) {

require.NoError(t, err)
require.Equal(t, 1, len(screens))
require.Equal(t, tc.Text, screens[0].Text)
require.Equal(t, tc.Text, screens[0].Content)

// Round trip.
parsedValue := NewGenericList([]*basev1beta1.Coin{})
Expand Down
4 changes: 2 additions & 2 deletions x/tx/textual/dec.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,15 @@ func (vr decValueRenderer) Format(_ context.Context, v protoreflect.Value) ([]Sc
if err != nil {
return nil, err
}
return []Screen{{Text: formatted}}, nil
return []Screen{{Content: formatted}}, nil
}

func (vr decValueRenderer) Parse(_ context.Context, screens []Screen) (protoreflect.Value, error) {
if n := len(screens); n != 1 {
return nilValue, fmt.Errorf("expected 1 screen, got: %d", n)
}

parsed, err := parseDec(screens[0].Text)
parsed, err := parseDec(screens[0].Content)
if err != nil {
return nilValue, err
}
Expand Down
6 changes: 3 additions & 3 deletions x/tx/textual/duration.go
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ func (dr durationValueRenderer) Format(_ context.Context, v protoreflect.Value)
s = "-" + s
}

return []Screen{{Text: s}}, nil
return []Screen{{Content: s}}, nil
}

var durRegexp = regexp.MustCompile(`^(-)?(?:([0-9]+) days?)?(?:, )?(?:([0-9]+) hours?)?(?:, )?(?:([0-9]+) minutes?)?(?:, )?(?:([0-9]+)(?:\.([0-9]+))? seconds?)?$`)
Expand All @@ -125,9 +125,9 @@ func (dr durationValueRenderer) Parse(_ context.Context, screens []Screen) (prot
return protoreflect.Value{}, fmt.Errorf("expected single screen: %v", screens)
}

parts := durRegexp.FindStringSubmatch(screens[0].Text)
parts := durRegexp.FindStringSubmatch(screens[0].Content)
if parts == nil {
return protoreflect.Value{}, fmt.Errorf("bad duration format: %s", screens[0].Text)
return protoreflect.Value{}, fmt.Errorf("bad duration format: %s", screens[0].Content)
}

negative := parts[1] != ""
Expand Down
2 changes: 1 addition & 1 deletion x/tx/textual/duration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ func TestDurationJSON(t *testing.T) {
}
require.NoError(t, err)
require.Equal(t, 1, len(screens))
require.Equal(t, tc.Text, screens[0].Text)
require.Equal(t, tc.Text, screens[0].Content)
}

val, err := rend.Parse(context.Background(), screens)
Expand Down
22 changes: 14 additions & 8 deletions x/tx/textual/encode.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,22 +7,25 @@ import (
)

var (
textKey = cbor.NewUint(1)
indentKey = cbor.NewUint(2)
expertKey = cbor.NewUint(3)
titleKey = cbor.NewUint(1)
contentKey = cbor.NewUint(2)
indentKey = cbor.NewUint(3)
expertKey = cbor.NewUint(4)
)

// encode encodes an array of screens according to the CDDL:
//
// screens = [* screen]
// screen = {
// ? text_key: tstr,
// ? title_key: tstr,
// ? content_key: tstr,
// ? indent_key: uint,
// ? expert_key: bool,
// }
// text_key = 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to title_key = 1.

// indent_key = 2
// expert_key = 3
// content_key = 2
// indent_key = 3
// expert_key = 4
//
// with empty values ("", 0, false) omitted from the screen map.
func encode(screens []Screen, w io.Writer) error {
Expand All @@ -35,8 +38,11 @@ func encode(screens []Screen, w io.Writer) error {

func (s Screen) Cbor() cbor.Cbor {
m := cbor.NewMap()
if s.Text != "" {
m = m.Add(textKey, cbor.NewText(s.Text))
if s.Title != "" {
m = m.Add(titleKey, cbor.NewText(s.Title))
}
if s.Content != "" {
m = m.Add(contentKey, cbor.NewText(s.Content))
}
if s.Indent > 0 {
// #nosec G701
Expand Down
4 changes: 2 additions & 2 deletions x/tx/textual/enum.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ func (er enumValueRenderer) Format(_ context.Context, v protoreflect.Value) ([]S
return nil, fmt.Errorf("cannot get enum %s variant of number %d", er.ed.FullName(), v.Enum())
}

return []Screen{{Text: string(evd.Name())}}, nil
return []Screen{{Content: string(evd.Name())}}, nil

}

Expand All @@ -39,7 +39,7 @@ func (er enumValueRenderer) Parse(_ context.Context, screens []Screen) (protoref
return nilValue, fmt.Errorf("expected single screen: %v", screens)
}

formatted := screens[0].Text
formatted := screens[0].Content

// Loop through all enum variants until we find the one matching the
// formatted screen's one.
Expand Down
2 changes: 1 addition & 1 deletion x/tx/textual/enum_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ func TestEnumJsonTestcases(t *testing.T) {
screens, err := valrend.Format(context.Background(), val)
require.NoError(t, err)
require.Equal(t, 1, len(screens))
require.Equal(t, tc.Text, screens[0].Text)
require.Equal(t, tc.Text, screens[0].Content)

// Round trip
parsedVal, err := valrend.Parse(context.Background(), screens)
Expand Down
4 changes: 2 additions & 2 deletions x/tx/textual/int.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,15 @@ func (vr intValueRenderer) Format(_ context.Context, v protoreflect.Value) ([]Sc
if err != nil {
return nil, err
}
return []Screen{{Text: formatted}}, nil
return []Screen{{Content: formatted}}, nil
}

func (vr intValueRenderer) Parse(_ context.Context, screens []Screen) (protoreflect.Value, error) {
if n := len(screens); n != 1 {
return nilValue, fmt.Errorf("expected 1 screen, got: %d", n)
}

parsedInt, err := parseInt(screens[0].Text)
parsedInt, err := parseInt(screens[0].Content)
if err != nil {
return nilValue, err
}
Expand Down
2 changes: 1 addition & 1 deletion x/tx/textual/int_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ func checkNumberTest(t *testing.T, r textual.ValueRenderer, pv protoreflect.Valu
require.Equal(t, 0, screens[0].Indent)
require.Equal(t, false, screens[0].Expert)

require.Equal(t, expected, screens[0].Text)
require.Equal(t, expected, screens[0].Content)

// Round trip.
value, err := r.Parse(context.Background(), screens)
Expand Down
38 changes: 19 additions & 19 deletions x/tx/textual/internal/testdata/any.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
"@type": "/Foo"
},
"screens": [
{"text": "/Foo"},
{"text": "Foo object", "indent": 1}
{"content": "/Foo"},
{"content": "Foo object", "indent": 1}
]
},
{
Expand All @@ -14,9 +14,9 @@
"full_name": "testing"
},
"screens": [
{"text": "/Foo"},
{"text": "Foo object", "indent": 1},
{"text": "Full name: testing", "indent": 2}
{"content": "/Foo"},
{"content": "Foo object", "indent": 1},
{"title": "Full name", "content": "testing", "indent": 2}
]
},
{
Expand All @@ -25,8 +25,8 @@
"value": "2006-01-02T15:04:05Z"
},
"screens": [
{"text": "/google.protobuf.Timestamp"},
{"text": "2006-01-02T15:04:05Z", "indent": 1}
{"content": "/google.protobuf.Timestamp"},
{"content": "2006-01-02T15:04:05Z", "indent": 1}
]
},
{
Expand All @@ -37,9 +37,9 @@
}
},
"screens": [
{"text": "/google.protobuf.Any"},
{"text": "/Foo", "indent": 1},
{"text": "Foo object", "indent": 2}
{"content": "/google.protobuf.Any"},
{"content": "/Foo", "indent": 1},
{"content": "Foo object", "indent": 2}
]
},
{
Expand All @@ -51,10 +51,10 @@
}
},
"screens": [
{"text": "/google.protobuf.Any"},
{"text": "/Foo", "indent": 1},
{"text": "Foo object", "indent": 2},
{"text": "Full name: testing", "indent": 3}
{"content": "/google.protobuf.Any"},
{"content": "/Foo", "indent": 1},
{"content": "Foo object", "indent": 2},
{"title": "Full name", "content": "testing", "indent": 3}
]
},
{
Expand All @@ -66,11 +66,11 @@
}
},
"screens": [
{"text": "/A"},
{"text": "A object", "indent": 1},
{"text": "ANY: /Foo", "indent": 2},
{"text": "Foo object", "indent": 3},
{"text": "Full name: testing", "indent": 4}
{"content": "/A"},
{"content": "A object", "indent": 1},
{"title": "ANY", "content": "/Foo", "indent": 2},
{"content": "Foo object", "indent": 3},
{"title": "Full name", "content": "testing", "indent": 4}
]
}
]
Loading