diff --git a/docs/architecture/adr-050-sign-mode-textual-annex2.md b/docs/architecture/adr-050-sign-mode-textual-annex2.md new file mode 100644 index 0000000000..ad0d5e6fd8 --- /dev/null +++ b/docs/architecture/adr-050-sign-mode-textual-annex2.md @@ -0,0 +1,122 @@ +# ADR 050: SIGN_MODE_TEXTUAL: Annex 2 XXX + +## Changelog + +- Oct 3, 2022: Initial Draft + +## Status + +DRAFT + +## Abstract + +This annex provides normative guidance on how devices should render a +`SIGN_MODE_TEXTUAL` document. + +## Context + +`SIGN_MODE_TEXTUAL` allows a legible version of a transaction to be signed +on a hardware security devices, such as a Ledger. Early versions of the +design rendered transactions directly to lines of ASCII text, but this +proved awkward from its in-band signaling, and for the need to display +Unicode text within the transaction. + +## Decision + +`SIGN_MODE_TEXTUAL` renders to an abstract representation, leaving it +up to device-specific software how to present this representation given the +capabilities, limitations, and conventions of the deivce. + +We offer the following normative guidance: + +1. The presentation should be as legible as possible to the user, given +the capabilities of the device. If legibility could be sacrificed for other +properties, we would recommend just using some other signing mode. +Legibility should focus on the common case - it is okay for unusual cases +to be less legible. + +2. The presentation should be invertible if possible without substantial +sacrifice of legibility. Any change to the rendered data should result +in a visible change to the presentation. This extends the integrity of the +signing to user-visible presentation. + +3. The presentation should follow normal conventions of the device, +without sacrificing legibility or invertibility. + +As an illustration of these principles, here is an example algorithm +for presentation on a device which can display a single 80-character +line of printable ASCII characters: + +- The presentation is broken into lines, and each line is presented in +sequence, with user controls for going forward or backward a line. + +- Expert mode screens are only presented if the device is in expert mode. + +- Each line of the screen starts with a number of `>` characters equal +to the screen's indetation level, followed by a `+` character if this +isn't the first line of the screen, followed by a space if either a +`>` or a `+` has been emitted, +or if this header is followed by a `>`, `+`, or space. + +- If the line ends with whitespace or an `@` character, and `@` character +is appended to the line. + +- The following ASCII control characters or backslash (`\`) are converted +to a backslash followed by a letter code, in the manner of string literals +in many languages: + + - a: U+0007 alert or bell + - b: U+0008 backspace + - f: U+000C form feed + - n: U+000A line feed + - r: U+000D carriage return + - t: U+0009 horizontal tab + - v: U+000B vertical tab + - `\`: U+005C backslash + +- All other ASCII control characters, plus non-ASCII Unicode code points, +are shown as either: + + - `\u` followed by 4 uppercase hex chacters for code points + in the basic multilingual plane (BMP). + + - `\U` followed by 8 uppercase hex characters for other code points. + +- The screen will be broken into multiple lines to fit the 80-character +limit, considering the above transformations in a way that attempts to +minimize the number of lines generated. Expanded control or Unicode characters +are never split across lines. + +Example output: + +``` +An introductory line. +key1: 123456 +key2: a string that ends in whitespace @ +key3: a string that ends in a single ampersand - @@ + >tricky key4<: note the leading space in the presentation +introducing an aggregate +> key5: false +> key6: a very long line of text, please co\u00F6perate and break into +>+ multiple lines. +> Can we do further nesting? +>> You bet we can! +``` + +The inverse mapping gives us the only input which could have +generated this output (JSON notation for string data): + +``` +Indent Text +------ ---- +0 "An introductory line." +0 "key1: 123456" +0 "key2: a string that ends in whitespace " +0 "key3: a string that ends in a single ampersand - @" +0 ">tricky key4<: note the leading space in the presentation" +0 "introducing an aggregate" +1 "key5: false" +1 "key6: a very long line of text, please coöperate and break into multiple lines." +1 "Can we do further nesting?" +2 "You bet we can!" +``` diff --git a/docs/architecture/adr-050-sign-mode-textual.md b/docs/architecture/adr-050-sign-mode-textual.md index 2508ae87cd..5e769e5f0d 100644 --- a/docs/architecture/adr-050-sign-mode-textual.md +++ b/docs/architecture/adr-050-sign-mode-textual.md @@ -7,6 +7,7 @@ - May 16, 2022: Change status to Accepted. - Aug 11, 2022: Require signing over tx raw bytes. - Sep 07, 2022: Add custom `Msg`-renderers. +- Sep 18, 2022: Structured format instead of lines of text ## Status @@ -23,72 +24,119 @@ Protobuf-based SIGN_MODE_DIRECT was introduced in [ADR-020](./adr-020-protobuf-t - SIGN_MODE_DIRECT is binary-based and thus not suitable for display to end-users. Technically, hardware wallets could simply display the sign bytes to the user. But this would be considered as blind signing, and is a security concern. - hardware cannot decode the protobuf sign bytes due to memory constraints, as the Protobuf definitions would need to be embedded on the hardware device. -In an effort to remove Amino from the SDK, a new sign mode needs to be created for hardware devices. [Initial discussions](https://github.com/cosmos/cosmos-sdk/issues/6513) propose a string-based sign mode, which this ADR formally specifies. +In an effort to remove Amino from the SDK, a new sign mode needs to be created for hardware devices. [Initial discussions](https://github.com/cosmos/cosmos-sdk/issues/6513) propose a text-based sign mode, which this ADR formally specifies. ## Decision -We propose to have SIGN_MODE_TEXTUAL’s signing payload `SignDocTextual` to be an array of strings, encoded as a `\n`-delimited string (see point #9). Each string corresponds to one "screen" on the hardware wallet device, with no (or little) additional formatting done by the hardware wallet itself. +In SIGN_MODE_TEXTUAL, a transaction is rendered into a textual representation, +which is then sent to a secure device or subsystem for the user to review and sign. +Unlike `SIGN_MODE_DIRECT`, the transmitted data can be simply decoded into legible text +even on devices with limited processing and display. -```protobuf -message SignDocTextual { - repeated string screens = 1; +The textual representation is a sequence of _screens_. +Each screen is meant to be displayed in its entirety (if possible) even on a small device like a Ledger. +A screen is roughly equivalent to a short line of text. +Large screens can be displayed in several pieces, +much as long lines of text are wrapped, +so no hard guidance is given, though 40 characters is a good target. +A screen is used to display a single key/value pair for scalar values +(or composite values with a compact notation, such as `Coins`) +or to introduce or conclude a larger grouping. + +The text can contain the full range of Unicode code points, including control characters and nul. +The device is responsible for deciding how to display characters it cannot render natively. +See [annex 2](./adr-050-sign-mode-textual-annex2.md) for guidance. + +Screens have a non-negative indentation level to signal composite or nested structures. +Indentation level zero is the top level. +Indentation is displayed via some device-specific mechanism. +Message quotation notation is an appropriate model, such as +leading `>` characters or vertical bars on more capable displays. + +Some screens are marked as _expert_ screens, +meant to be displayed only if the viewer chooses to opt in for the extra detail. +Expert screens are meant for information that is rarely useful, +or needs to be present only for signature integrity (see below). + +### Invertible Rendering + +We require that the rendering of the transaction be invertible: +there must be a parsing function such that for every transaction, +when rendered to the textual representation, +parsing that representation yeilds a proto message equivalent +to the original under proto equality. + +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 +the composition of rendering and parsing. + +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. + +### Chain State + +The rendering function (and parsing function) may depend on the current chain state. +This is useful for reading parameters, such as coin display metadata, +or for reading user-specific preferences such as language or address aliases. +Note that if the observed state changes between signature generation +and the transaction's inclusion in a block, the delivery-time rendering +might differ. If so, the signature will be invalid and the transaction +will be rejected. + +### Signature and Security + +For security, transaction signatures should have three properties: + +1. Given the transaction, signatures, and chain state, it must be possible to validate that the signatures matches the transaction, +to verify that the signers must have known their respective secret keys. + +2. It must be computationally infeasible to find a substantially different transaction for which the given signatures are valid, given the same chain state. + +3. The user should be able to give informed consent to the signed data via a simple, secure device with limited display capabilities. + +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. + +### Transaction Hash Malleability + +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. +This is the `TxHash`, and is used by various services to track the submitted transaction through its lifecycle. +Various misbehavior is possible if one can generate a modified transaction with a different TxHash +but for which the signature still checks out. + +SIGN_MODE_TEXTUAL prevents this transaction malleability by including the TxHash as an expert screen +in the rendering. + +### SignDoc + +The SignDoc for `SIGN_MODE_TEXTUAL` is formed from a data structure like: + +``` +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 + Expert bool } + +type SignDocTextual = []Screen ``` -The string array MUST follow the specifications below. +We do not plan to use protobuf serialization to form the sequence of bytes +that will be tranmitted and signed, in order to keep the decoder simple. +We will use [CBOR](https://cbor.io) ([RFC 8949](https://www.rfc-editor.org/rfc/rfc8949.html)) instead. -### 1. Bijectivity with Protobuf transactions +TODO: specify the details of the CBOR encoding. -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. +## Details -We concede that bijectivity is not strictly needed. Avoiding transaction malleability only requires collision resistance on the encoding. Lossless encoding also does not require decodability. However, bijectivity assures both non-malleability and losslessness. +In the examples that follow, screens will be shown as lines of text, +indentation is indicated with a leading '>', +and expert screens are marked with a leading `*`. -Bijectivity will be tested in two ways: - -- 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. - -This also prevents users signing over any hashed transaction data (fee, transaction body, `Msg` content that might be hashed etc), which is a security concern for Ledger's security team. - -We propose to maintain functional tests using bijectivity in the SDK. - -### 2. Only ASCII 32-127 characters allowed - -Ledger devices have limited character display capabilities, so all strings MUST only contain ASCII characters in the 32-127 range. - -In particular, the newline `"\n"` (ASCII: 10) character is forbidden. - -### 3. Strings SHOULD have the `: ` format - -Given the Regex `/^(\* )?(>* )?(.*: )?(.*)$/`, all strings SHOULD match the Regex with capture groups 3 and 4 non-empty. This is helpful for UIs displaying SignDocTextual to users. - -- The initial `*` character is optional and denotes the Ledger Expert mode, see #5. -- Strings can also include a number of `>` character to denote nesting. -- In the case where the first Regex capture group is not empty, it represents an indicative key, whose associated value is given in the second capture group. This MAY be used in the Ledger app to perform custom on-screen formatting, for example to break long lines into multiple screens. - -This Regex is however not mandatory, to allow for some flexibility, for example to display an English sentence to denote end of sections. - -The `` itself can contain the `": "` characters. - -### 4. Values are encoded using Value Renderers - -Value Renderers describe how Protobuf types are encoded to and decoded from a string array. The full specification of Value Renderers can be found in [Annex 1](./adr-050-sign-mode-textual-annex1.md). - -### 5. Strings starting with `*` are only shown in Expert mode - -Ledger devices have the an Expert mode for advanced users. Expert mode needs to be manually activated by the device holder, inside the device settings. There is currently no official documentation on Expert mode, but according to the [@Ledger_Support twitter account](https://twitter.com/Ledger_Support/status/1364524431800950785), - -> Expert mode enables further, more sophisticated features. This could be useful for advanced users - -Strings starting with the `*` character will only be shown in Expert mode. These strings are either hardcoded in the transaction envelope (see point #7). - -For hardware wallets that don't have an expert mode, all strings MUST be shown on the device. - -### 6. Strings MAY contain `>` characters to denote nesting - -Protobuf objects can be arbitrarily complex, containing nested arrays and messages. In order to help the Ledger-signing users, we propose to use the `>` symbol in the beginning of strings to represent nested objects, where each additional `>` represents a new level of nesting. - -### 7. Encoding of the Transaction Envelope +### Encoding of the Transaction Envelope We define "transaction envelope" as all data in a transaction that is not in the `TxBody.Messages` field. Transaction envelope includes fee, signer infos and memo, but don't include `Msg`s. `//` denotes comments and are not shown on the Ledger device. @@ -120,7 +168,7 @@ Tip: *Hash of raw bytes: // Hex encoding of bytes defined in #10, to prevent tx hash malleability. ``` -### 8. Encoding of the Transaction Body +### Encoding of the Transaction Body Transaction Body is the `Tx.TxBody.Messages` field, which is an array of `Any`s, where each `Any` packs a `sdk.Msg`. Since `sdk.Msg`s are widely used, they have a slightly different encoding than usual array of `Any`s (Protobuf: `repeated google.protobuf.Any`) described in Annex 1. @@ -161,7 +209,7 @@ Grantee: cosmos1ghi...jkl End of transaction messages ``` -### 9. Custom `Msg` Renderers +### Custom `Msg` Renderers Application developers may choose to not follow default renderer value output for their own `Msg`s. In this case, they can implement their own custom `Msg` renderer. This is similar to [EIP4430](https://github.com/ethereum/EIPs/blob/master/EIPS/eip-4430.md), where the smart contract developer chooses the description string to be shown to the end user. @@ -181,25 +229,9 @@ When this option is set on a `Msg`, a registered function will transform the `Ms The `` is a string convention chosen by the application developer and is used to identify the custom `Msg` renderer. For example, the documentation or specification of this custom algorithm can reference this identifier. This identifier CAN have a versioned suffix (e.g. `_v1`) to adapt for future changes (which would be consensus-breaking). We also recommend adding Protobuf comments to describe in human language the custom logic used. -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. A simple bijectivity test looks like: +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. -``` -// for renderer, msg, and ctx of the right type -keyvals, err := renderer.Format(ctx, msg) -if err != nil { - fail_check() -} -msg2, err := renderer.Parse(ctx, keyvals) -if err != nil { - fail_check() -} -if !proto.Equal(msg, msg2) { - fail_check() -} -pass_check() -``` - -### 10. Require signing over the `TxBody` and `AuthInfo` raw bytes +### Require signing over the `TxBody` and `AuthInfo` raw bytes Recall that the transaction bytes merklelized on chain are the Protobuf binary serialization of [TxRaw](https://github.com/cosmos/cosmos-sdk/blob/v0.46.0/proto/cosmos/tx/v1beta1/tx.proto#L33), which contains the `body_bytes` and `auth_info_bytes`. Moreover, the transaction hash is defined as the SHA256 hash of the `TxRaw` bytes. We require that the user signs over these bytes in SIGN_MODE_TEXTUAL, more specifically over the following string: @@ -218,17 +250,9 @@ By including this hash in the SIGN_MODE_TEXTUAL signing payload, we keep the sam These bytes are only shown in expert mode, hence the leading `*`. -### 11. Signing Payload and Wire Format - -This string array is encoded as a single `\n`-delimited string before transmitted to the hardware device, and this long string is the signing payload signed by the hardware wallet. - ## Additional Formatting by the Hardware Device -Hardware devices differ in screen sizes and memory capacities. The above specifications are all verified on the protocol level, but we still allow the hardware device to add custom formatting rules that are specific to the device. Rules can include: - -- if a string is too long, show it on multiple screens, -- break line between the `key` and `value` from #3, -- perform line breaks on a number or a coin values only when necessary. For example, a `sdk.Coins` with multiple denoms would be better shown as one denom per line instead of an coin amount being cut in the middle. +See [annex 2](./adr-050-sign-mode-textual-annex2.md). ## Examples diff --git a/tx/textual/valuerenderer/bench_test.go b/tx/textual/valuerenderer/bench_test.go index 5ce072ef2e..b14cbc58a9 100644 --- a/tx/textual/valuerenderer/bench_test.go +++ b/tx/textual/valuerenderer/bench_test.go @@ -23,17 +23,15 @@ var intValues = []protoreflect.Value{ func BenchmarkIntValueRendererFormat(b *testing.B) { ctx := context.Background() ivr := new(intValueRenderer) - buf := new(bytes.Buffer) b.ResetTimer() b.ReportAllocs() for i := 0; i < b.N; i++ { for _, value := range intValues { - if err := ivr.Format(ctx, value, buf); err != nil { + if _, err := ivr.Format(ctx, value); err != nil { b.Fatal(err) } } - buf.Reset() } } @@ -52,17 +50,15 @@ var decimalValues = []protoreflect.Value{ func BenchmarkDecimalValueRendererFormat(b *testing.B) { ctx := context.Background() dvr := new(decValueRenderer) - buf := new(bytes.Buffer) b.ResetTimer() b.ReportAllocs() for i := 0; i < b.N; i++ { for _, value := range intValues { - if err := dvr.Format(ctx, value, buf); err != nil { + if _, err := dvr.Format(ctx, value); err != nil { b.Fatal(err) } } - buf.Reset() } } @@ -81,16 +77,14 @@ var byteValues = []protoreflect.Value{ func BenchmarkBytesValueRendererFormat(b *testing.B) { ctx := context.Background() bvr := new(bytesValueRenderer) - buf := new(bytes.Buffer) b.ResetTimer() b.ReportAllocs() for i := 0; i < b.N; i++ { for _, value := range byteValues { - if err := bvr.Format(ctx, value, buf); err != nil { + if _, err := bvr.Format(ctx, value); err != nil { b.Fatal(err) } } - buf.Reset() } } diff --git a/tx/textual/valuerenderer/bytes.go b/tx/textual/valuerenderer/bytes.go index 7e7eba8da9..cf42c7e938 100644 --- a/tx/textual/valuerenderer/bytes.go +++ b/tx/textual/valuerenderer/bytes.go @@ -3,7 +3,7 @@ package valuerenderer import ( "context" "encoding/hex" - "io" + "fmt" "strings" "google.golang.org/protobuf/reflect/protoreflect" @@ -19,16 +19,16 @@ type bytesValueRenderer struct{} var _ ValueRenderer = bytesValueRenderer{} -func (vr bytesValueRenderer) Format(ctx context.Context, v protoreflect.Value, w io.Writer) error { - _, err := io.WriteString(w, strings.ToUpper(hex.EncodeToString(v.Bytes()))) - return err +func (vr bytesValueRenderer) Format(ctx context.Context, v protoreflect.Value) ([]Screen, error) { + text := strings.ToUpper(hex.EncodeToString(v.Bytes())) + return []Screen{{Text: text}}, nil } -func (vr bytesValueRenderer) Parse(_ context.Context, r io.Reader) (protoreflect.Value, error) { - formatted, err := io.ReadAll(r) - if err != nil { - return protoreflect.ValueOfBytes([]byte{}), err +func (vr bytesValueRenderer) Parse(_ context.Context, screens []Screen) (protoreflect.Value, error) { + if len(screens) != 1 { + return protoreflect.ValueOfBytes([]byte{}), fmt.Errorf("expected single screen: %v", screens) } + formatted := screens[0].Text data, err := hex.DecodeString(string(formatted)) if err != nil { diff --git a/tx/textual/valuerenderer/bytes_test.go b/tx/textual/valuerenderer/bytes_test.go index 87b1c72465..f79dd407a4 100644 --- a/tx/textual/valuerenderer/bytes_test.go +++ b/tx/textual/valuerenderer/bytes_test.go @@ -5,7 +5,6 @@ import ( "encoding/base64" "encoding/json" "os" - "strings" "testing" "cosmossdk.io/tx/textual/valuerenderer" @@ -31,14 +30,13 @@ func TestBytesJsonTestCases(t *testing.T) { valrend, err := textual.GetValueRenderer(fieldDescriptorFromName("BYTES")) require.NoError(t, err) - b := new(strings.Builder) - err = valrend.Format(context.Background(), protoreflect.ValueOfBytes(data), b) + screens, err := valrend.Format(context.Background(), protoreflect.ValueOfBytes(data)) require.NoError(t, err) - require.Equal(t, tc.hex, b.String()) + require.Equal(t, 1, len(screens)) + require.Equal(t, tc.hex, screens[0].Text) // Round trip - r := strings.NewReader(tc.hex) - val, err := valrend.Parse(context.Background(), r) + val, err := valrend.Parse(context.Background(), screens) require.NoError(t, err) require.Equal(t, tc.base64, base64.StdEncoding.EncodeToString(val.Bytes())) } diff --git a/tx/textual/valuerenderer/coin_test.go b/tx/textual/valuerenderer/coin_test.go index f48509b38d..69e36a1d4a 100644 --- a/tx/textual/valuerenderer/coin_test.go +++ b/tx/textual/valuerenderer/coin_test.go @@ -5,7 +5,6 @@ import ( "encoding/json" "fmt" "os" - "strings" "testing" "github.com/stretchr/testify/require" @@ -33,13 +32,11 @@ func mockCoinMetadataQuerier(ctx context.Context, denom string) (*bankv1beta1.Me } func TestMetadataQuerier(t *testing.T) { - b := new(strings.Builder) - // Errors on nil metadata querier textual := valuerenderer.NewTextual(nil) vr, err := textual.GetValueRenderer(fieldDescriptorFromName("COIN")) require.NoError(t, err) - err = vr.Format(context.Background(), protoreflect.ValueOf((&basev1beta1.Coin{}).ProtoReflect()), b) + _, err = vr.Format(context.Background(), protoreflect.ValueOf((&basev1beta1.Coin{}).ProtoReflect())) require.Error(t, err) // Errors if metadata querier returns an error @@ -49,9 +46,9 @@ func TestMetadataQuerier(t *testing.T) { }) vr, err = textual.GetValueRenderer(fieldDescriptorFromName("COIN")) require.NoError(t, err) - err = vr.Format(context.Background(), protoreflect.ValueOf((&basev1beta1.Coin{}).ProtoReflect()), b) + _, err = vr.Format(context.Background(), protoreflect.ValueOf((&basev1beta1.Coin{}).ProtoReflect())) require.ErrorIs(t, err, expErr) - err = vr.Format(context.Background(), protoreflect.ValueOf(NewGenericList([]*basev1beta1.Coin{{}})), b) + _, err = vr.Format(context.Background(), protoreflect.ValueOf(NewGenericList([]*basev1beta1.Coin{{}}))) require.ErrorIs(t, err, expErr) } @@ -70,8 +67,7 @@ func TestCoinJsonTestcases(t *testing.T) { t.Run(tc.Text, func(t *testing.T) { if tc.Proto != nil { ctx := context.WithValue(context.Background(), mockCoinMetadataKey(tc.Proto.Denom), tc.Metadata) - b := new(strings.Builder) - err = vr.Format(ctx, protoreflect.ValueOf(tc.Proto.ProtoReflect()), b) + screens, err := vr.Format(ctx, protoreflect.ValueOf(tc.Proto.ProtoReflect())) if tc.Error { require.Error(t, err) @@ -79,7 +75,8 @@ func TestCoinJsonTestcases(t *testing.T) { } require.NoError(t, err) - require.Equal(t, tc.Text, b.String()) + require.Equal(t, 1, len(screens)) + require.Equal(t, tc.Text, screens[0].Text) } // TODO Add parsing tests diff --git a/tx/textual/valuerenderer/coins.go b/tx/textual/valuerenderer/coins.go index f369777bb3..594db57bbf 100644 --- a/tx/textual/valuerenderer/coins.go +++ b/tx/textual/valuerenderer/coins.go @@ -3,7 +3,6 @@ package valuerenderer import ( "context" "fmt" - "io" "sort" "strings" @@ -28,9 +27,9 @@ type coinsValueRenderer struct { var _ ValueRenderer = coinsValueRenderer{} -func (vr coinsValueRenderer) Format(ctx context.Context, v protoreflect.Value, w io.Writer) error { +func (vr coinsValueRenderer) Format(ctx context.Context, v protoreflect.Value) ([]Screen, error) { if vr.coinMetadataQuerier == nil { - return fmt.Errorf("expected non-nil coin metadata querier") + return nil, fmt.Errorf("expected non-nil coin metadata querier") } // Check whether we have a Coin or some Coins. @@ -45,17 +44,16 @@ func (vr coinsValueRenderer) Format(ctx context.Context, v protoreflect.Value, w coins[i] = coin metadatas[i], err = vr.coinMetadataQuerier(ctx, coin.Denom) if err != nil { - return err + return nil, err } } formatted, err := formatCoins(coins, metadatas) if err != nil { - return err + return nil, err } - _, err = w.Write([]byte(formatted)) - return err + return []Screen{{Text: formatted}}, nil } // If it's a single Coin: case protoreflect.Message: @@ -64,23 +62,22 @@ func (vr coinsValueRenderer) Format(ctx context.Context, v protoreflect.Value, w metadata, err := vr.coinMetadataQuerier(ctx, coin.Denom) if err != nil { - return err + return nil, err } formatted, err := formatCoin(coin, metadata) if err != nil { - return err + return nil, err } - _, err = w.Write([]byte(formatted)) - return err + return []Screen{{Text: formatted}}, nil } default: - return fmt.Errorf("got invalid type %t for coins", v.Interface()) + return nil, fmt.Errorf("got invalid type %t for coins", v.Interface()) } } -func (vr coinsValueRenderer) Parse(_ context.Context, r io.Reader) (protoreflect.Value, error) { +func (vr coinsValueRenderer) Parse(_ context.Context, screens []Screen) (protoreflect.Value, error) { // ref: https://github.com/cosmos/cosmos-sdk/issues/13153 panic("implement me, see #13153") } diff --git a/tx/textual/valuerenderer/coins_test.go b/tx/textual/valuerenderer/coins_test.go index b8f2abcc8b..9d3b2eea40 100644 --- a/tx/textual/valuerenderer/coins_test.go +++ b/tx/textual/valuerenderer/coins_test.go @@ -4,7 +4,6 @@ import ( "context" "encoding/json" "os" - "strings" "testing" bankv1beta1 "cosmossdk.io/api/cosmos/bank/v1beta1" @@ -35,9 +34,8 @@ func TestCoinsJsonTestcases(t *testing.T) { ctx = context.WithValue(ctx, mockCoinMetadataKey(coin.Denom), tc.Metadata[coin.Denom]) } - b := new(strings.Builder) listValue := NewGenericList(tc.Proto) - err = vr.Format(ctx, protoreflect.ValueOf(listValue), b) + screens, err := vr.Format(ctx, protoreflect.ValueOf(listValue)) if tc.Error { require.Error(t, err) @@ -45,7 +43,8 @@ func TestCoinsJsonTestcases(t *testing.T) { } require.NoError(t, err) - require.Equal(t, tc.Text, b.String()) + require.Equal(t, 1, len(screens)) + require.Equal(t, tc.Text, screens[0].Text) } // TODO Add parsing tests diff --git a/tx/textual/valuerenderer/dec.go b/tx/textual/valuerenderer/dec.go index a3ab675220..dd21bcc9db 100644 --- a/tx/textual/valuerenderer/dec.go +++ b/tx/textual/valuerenderer/dec.go @@ -3,7 +3,6 @@ package valuerenderer import ( "context" "fmt" - "io" "strings" "google.golang.org/protobuf/reflect/protoreflect" @@ -21,17 +20,15 @@ type decValueRenderer struct{} var _ ValueRenderer = decValueRenderer{} -func (vr decValueRenderer) Format(_ context.Context, v protoreflect.Value, w io.Writer) error { +func (vr decValueRenderer) Format(_ context.Context, v protoreflect.Value) ([]Screen, error) { formatted, err := formatDecimal(v.String()) if err != nil { - return err + return nil, err } - - _, err = io.WriteString(w, formatted) - return err + return []Screen{{Text: formatted}}, nil } -func (vr decValueRenderer) Parse(_ context.Context, r io.Reader) (protoreflect.Value, error) { +func (vr decValueRenderer) Parse(_ context.Context, screens []Screen) (protoreflect.Value, error) { panic("implement me") } diff --git a/tx/textual/valuerenderer/duration.go b/tx/textual/valuerenderer/duration.go index b0df96d321..d7c2a7b088 100644 --- a/tx/textual/valuerenderer/duration.go +++ b/tx/textual/valuerenderer/duration.go @@ -3,7 +3,6 @@ package valuerenderer import ( "context" "fmt" - "io" "regexp" "strconv" "strings" @@ -65,19 +64,19 @@ func formatSeconds(seconds int64, nanos int32) string { } // Format implements the ValueRenderer interface. -func (dr durationValueRenderer) Format(_ context.Context, v protoreflect.Value, w io.Writer) error { +func (dr durationValueRenderer) Format(_ context.Context, v protoreflect.Value) ([]Screen, error) { // Reify the reflected message as a proto Duration msg := v.Message().Interface() duration, ok := msg.(*dpb.Duration) if !ok { - return fmt.Errorf("expected Duration, got %T", msg) + return nil, fmt.Errorf("expected Duration, got %T", msg) } // Bypass use of time.Duration, as the range is more limited than that of dpb.Duration. // (Too bad the companies that produced both technologies didn't coordinate better!) if err := duration.CheckValid(); err != nil { - return err + return nil, err } negative := false @@ -115,26 +114,25 @@ func (dr durationValueRenderer) Format(_ context.Context, v protoreflect.Value, s = "-" + s } - _, err := w.Write([]byte(s)) - return err + return []Screen{{Text: s}}, nil } var durRegexp = regexp.MustCompile(`^(-)?(?:([0-9]+) days?)?(?:, )?(?:([0-9]+) hours?)?(?:, )?(?:([0-9]+) minutes?)?(?:, )?(?:([0-9]+)(?:\.([0-9]+))? seconds?)?$`) // Parse implements the ValueRenderer interface. -func (dr durationValueRenderer) Parse(_ context.Context, r io.Reader) (protoreflect.Value, error) { - bz, err := io.ReadAll(r) - if err != nil { - return protoreflect.Value{}, err +func (dr durationValueRenderer) Parse(_ context.Context, screens []Screen) (protoreflect.Value, error) { + if len(screens) != 1 { + return protoreflect.Value{}, fmt.Errorf("expected single screen: %v", screens) } - parts := durRegexp.FindStringSubmatch(string(bz)) + parts := durRegexp.FindStringSubmatch(screens[0].Text) if parts == nil { - return protoreflect.Value{}, fmt.Errorf("bad duration format: %s", string(bz)) + return protoreflect.Value{}, fmt.Errorf("bad duration format: %s", screens[0].Text) } negative := parts[1] != "" var days, hours, minutes, seconds, nanos int64 + var err error if parts[2] != "" { days, err = strconv.ParseInt(parts[2], 10, 64) diff --git a/tx/textual/valuerenderer/duration_test.go b/tx/textual/valuerenderer/duration_test.go index d670c17518..4ad2bb5c0b 100644 --- a/tx/textual/valuerenderer/duration_test.go +++ b/tx/textual/valuerenderer/duration_test.go @@ -5,7 +5,6 @@ import ( "encoding/json" "fmt" "os" - "strings" "testing" "cosmossdk.io/tx/textual/valuerenderer" @@ -34,19 +33,19 @@ func TestDurationJSON(t *testing.T) { t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { rend := valuerenderer.NewDurationValueRenderer() + var screens []valuerenderer.Screen if tc.Proto != nil { - wr := new(strings.Builder) - err = rend.Format(context.Background(), protoreflect.ValueOf(tc.Proto.ProtoReflect()), wr) + screens, err = rend.Format(context.Background(), protoreflect.ValueOf(tc.Proto.ProtoReflect())) if tc.Error { require.Error(t, err) return } require.NoError(t, err) - require.Equal(t, tc.Text, wr.String()) + require.Equal(t, 1, len(screens)) + require.Equal(t, tc.Text, screens[0].Text) } - rd := strings.NewReader(tc.Text) - val, err := rend.Parse(context.Background(), rd) + val, err := rend.Parse(context.Background(), screens) if tc.Error { require.Error(t, err) return diff --git a/tx/textual/valuerenderer/int.go b/tx/textual/valuerenderer/int.go index 6197c405e5..54a87ec7ed 100644 --- a/tx/textual/valuerenderer/int.go +++ b/tx/textual/valuerenderer/int.go @@ -3,7 +3,6 @@ package valuerenderer import ( "context" "fmt" - "io" "strings" "google.golang.org/protobuf/reflect/protoreflect" @@ -19,17 +18,15 @@ type intValueRenderer struct{} var _ ValueRenderer = intValueRenderer{} -func (vr intValueRenderer) Format(_ context.Context, v protoreflect.Value, w io.Writer) error { +func (vr intValueRenderer) Format(_ context.Context, v protoreflect.Value) ([]Screen, error) { formatted, err := formatInteger(v.String()) if err != nil { - return err + return nil, err } - - _, err = io.WriteString(w, formatted) - return err + return []Screen{{Text: formatted}}, nil } -func (vr intValueRenderer) Parse(_ context.Context, r io.Reader) (protoreflect.Value, error) { +func (vr intValueRenderer) Parse(_ context.Context, screens []Screen) (protoreflect.Value, error) { panic("implement me") } diff --git a/tx/textual/valuerenderer/timestamp.go b/tx/textual/valuerenderer/timestamp.go index a14dd7823b..eebe60296f 100644 --- a/tx/textual/valuerenderer/timestamp.go +++ b/tx/textual/valuerenderer/timestamp.go @@ -3,7 +3,6 @@ package valuerenderer import ( "context" "fmt" - "io" "time" "google.golang.org/protobuf/reflect/protoreflect" @@ -20,12 +19,12 @@ func NewTimestampValueRenderer() ValueRenderer { } // Format implements the ValueRenderer interface. -func (vr timestampValueRenderer) Format(_ context.Context, v protoreflect.Value, w io.Writer) error { +func (vr timestampValueRenderer) Format(_ context.Context, v protoreflect.Value) ([]Screen, error) { // Reify the reflected message as a proto Timestamp msg := v.Message().Interface() timestamp, ok := msg.(*tspb.Timestamp) if !ok { - return fmt.Errorf("expected Timestamp, got %T", msg) + return nil, fmt.Errorf("expected Timestamp, got %T", msg) } // Convert proto timestamp to a Go Time. @@ -33,18 +32,16 @@ func (vr timestampValueRenderer) Format(_ context.Context, v protoreflect.Value, // Format the Go Time as RFC 3339. s := t.Format(time.RFC3339Nano) - w.Write([]byte(s)) - return nil + return []Screen{{Text: s}}, nil } // Parse implements the ValueRenderer interface. -func (vr timestampValueRenderer) Parse(_ context.Context, r io.Reader) (protoreflect.Value, error) { +func (vr timestampValueRenderer) Parse(_ context.Context, screens []Screen) (protoreflect.Value, error) { // Parse the RFC 3339 input as a Go Time. - bz, err := io.ReadAll(r) - if err != nil { - return protoreflect.Value{}, err + if len(screens) != 1 { + return protoreflect.Value{}, fmt.Errorf("expected single screen: %v", screens) } - t, err := time.Parse(time.RFC3339Nano, string(bz)) + t, err := time.Parse(time.RFC3339Nano, screens[0].Text) if err != nil { return protoreflect.Value{}, err } diff --git a/tx/textual/valuerenderer/timestamp_test.go b/tx/textual/valuerenderer/timestamp_test.go index b4d955905a..c6e72e569e 100644 --- a/tx/textual/valuerenderer/timestamp_test.go +++ b/tx/textual/valuerenderer/timestamp_test.go @@ -4,9 +4,7 @@ import ( "context" "encoding/json" "fmt" - "io" "os" - "strings" "testing" "time" @@ -45,19 +43,19 @@ func TestTimestampJsonTestcases(t *testing.T) { t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { rend := valuerenderer.NewTimestampValueRenderer() + var screens []valuerenderer.Screen if tc.Proto != nil { - wr := new(strings.Builder) - err = rend.Format(context.Background(), protoreflect.ValueOf(tc.Proto.ProtoReflect()), wr) + screens, err = rend.Format(context.Background(), protoreflect.ValueOf(tc.Proto.ProtoReflect())) if tc.Error { require.Error(t, err) return } require.NoError(t, err) - require.Equal(t, tc.Text, wr.String()) + require.Equal(t, 1, len(screens)) + require.Equal(t, tc.Text, screens[0].Text) } - rd := strings.NewReader(tc.Text) - val, err := rend.Parse(context.Background(), rd) + val, err := rend.Parse(context.Background(), screens) if tc.Error { require.Error(t, err) return @@ -73,21 +71,6 @@ func TestTimestampJsonTestcases(t *testing.T) { func TestTimestampBadFormat(t *testing.T) { rend := valuerenderer.NewTimestampValueRenderer() - wr := new(strings.Builder) - err := rend.Format(context.Background(), protoreflect.ValueOf(dur.New(time.Hour).ProtoReflect()), wr) + _, err := rend.Format(context.Background(), protoreflect.ValueOf(dur.New(time.Hour).ProtoReflect())) require.Error(t, err) } - -type badReader struct{} - -var _ io.Reader = badReader{} - -func (br badReader) Read(p []byte) (int, error) { - return 0, fmt.Errorf("reader error") -} - -func TestTimestampBadParse_reader(t *testing.T) { - rend := valuerenderer.NewTimestampValueRenderer() - _, err := rend.Parse(context.Background(), badReader{}) - require.ErrorContains(t, err, "reader error") -} diff --git a/tx/textual/valuerenderer/types.go b/tx/textual/valuerenderer/types.go index 21ca059cb2..97383b816e 100644 --- a/tx/textual/valuerenderer/types.go +++ b/tx/textual/valuerenderer/types.go @@ -2,11 +2,24 @@ package valuerenderer import ( "context" - "io" "google.golang.org/protobuf/reflect/protoreflect" ) +// Screen is the abstract unit of Textual rendering. +type Screen struct { + // Text is the text to display - a sequence of Unicode code points. + Text string + + // Indent is the indentation level of the screen. + // Zero indicates top-level. Should be less than 16. + Indent int + + // Expert indicates that the screen should only be displayed + // via an opt-in from the user. + Expert bool +} + // ValueRenderer defines an interface to produce formatted output for all // protobuf types as well as parse a string into those protobuf types. // @@ -15,6 +28,9 @@ import ( // here, so that optionally more value renderers could be built, for example, a // separate one for a different language. type ValueRenderer interface { - Format(context.Context, protoreflect.Value, io.Writer) error - Parse(context.Context, io.Reader) (protoreflect.Value, error) + // Format should render the value to a text plus annotation. + Format(context.Context, protoreflect.Value) ([]Screen, error) + + // Parse should be the inverse of Format. + Parse(context.Context, []Screen) (protoreflect.Value, error) } diff --git a/tx/textual/valuerenderer/valuerenderer_test.go b/tx/textual/valuerenderer/valuerenderer_test.go index 894624299b..ebd1c2fc26 100644 --- a/tx/textual/valuerenderer/valuerenderer_test.go +++ b/tx/textual/valuerenderer/valuerenderer_test.go @@ -6,7 +6,6 @@ import ( "fmt" "os" "strconv" - "strings" "testing" "github.com/stretchr/testify/require" @@ -33,11 +32,10 @@ func TestFormatInteger(t *testing.T) { if err == nil { r, err := textual.GetValueRenderer(fieldDescriptorFromName("UINT64")) require.NoError(t, err) - b := new(strings.Builder) - err = r.Format(context.Background(), protoreflect.ValueOf(i), b) + screens, err := r.Format(context.Background(), protoreflect.ValueOf(i)) require.NoError(t, err) - - require.Equal(t, tc[1], b.String()) + require.Equal(t, 1, len(screens)) + require.Equal(t, tc[1], screens[0].Text) } // Parse test case strings as protobuf uint32 @@ -45,11 +43,10 @@ func TestFormatInteger(t *testing.T) { if err == nil { r, err := textual.GetValueRenderer(fieldDescriptorFromName("UINT32")) require.NoError(t, err) - b := new(strings.Builder) - err = r.Format(context.Background(), protoreflect.ValueOf(i), b) + screens, err := r.Format(context.Background(), protoreflect.ValueOf(i)) require.NoError(t, err) - - require.Equal(t, tc[1], b.String()) + require.Equal(t, 1, len(screens)) + require.Equal(t, tc[1], screens[0].Text) } // Parse test case strings as sdk.Ints @@ -57,11 +54,10 @@ func TestFormatInteger(t *testing.T) { if ok { r, err := textual.GetValueRenderer(fieldDescriptorFromName("SDKINT")) require.NoError(t, err) - b := new(strings.Builder) - err = r.Format(context.Background(), protoreflect.ValueOf(tc[0]), b) + screens, err := r.Format(context.Background(), protoreflect.ValueOf(tc[0])) require.NoError(t, err) - - require.Equal(t, tc[1], b.String()) + require.Equal(t, 1, len(screens)) + require.Equal(t, tc[1], screens[0].Text) } } } @@ -81,11 +77,10 @@ func TestFormatDecimal(t *testing.T) { t.Run(tc[0], func(t *testing.T) { r, err := textual.GetValueRenderer(fieldDescriptorFromName("SDKDEC")) require.NoError(t, err) - b := new(strings.Builder) - err = r.Format(context.Background(), protoreflect.ValueOf(tc[0]), b) + screens, err := r.Format(context.Background(), protoreflect.ValueOf(tc[0])) require.NoError(t, err) - - require.Equal(t, tc[1], b.String()) + require.Equal(t, 1, len(screens)) + require.Equal(t, tc[1], screens[0].Text) }) } }