Update ADR 020 to address client-side tx UX #6031

aaronc · 2020-04-20T18:28:14Z

ref: #6030

For contributor use:

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer

For admin use:

Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
Reviewers assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

codecov · 2020-04-20T18:43:15Z

Codecov Report

Merging #6031 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #6031   +/-   ##
=======================================
  Coverage   54.69%   54.69%           
=======================================
  Files         424      424           
  Lines       25790    25790           
=======================================
  Hits        14107    14107           
  Misses      10708    10708           
  Partials      975      975

docs/architecture/adr-020-protobuf-transaction-encoding.md

Co-Authored-By: Alexander Bezobchuk <[email protected]>

alexanderbez · 2020-04-21T18:20:13Z

A good stress test, have as many people review this as possible and see if the proposed changes make sense 👍

docs/architecture/adr-020-protobuf-transaction-encoding.md

alexanderbez · 2020-04-21T20:02:56Z

docs/architecture/adr-020-protobuf-transaction-encoding.md

-```Protobuf
-// app/codec/codec.proto
+Because signing and encoding are separated and apps will know how to encode
+a signing messages, we can provide a gRPC/REST endpoint that allows for generic


we can provide a gRPC/REST endpoint that allows for generic app-independent transaction submission.

I don't understand what this means. Are you referring to a type to be used to generate a JSON blob which when provided to /tx/broadcast will be JSON decoded and then re-encoded using the app-specific tx encoding scheme?

It's not json, it's protobuf via gRPC. Basically this AnyTransaction type which is not app dependent

And yes, the rpc endpoint would re-encode this in the app level transaction that uses a oneof

It's not json, it's protobuf via gRPC

This makes sense. But in this case, isn't the term "REST" in "we can provide a gRPC/REST endpoint" misleading in this paragraph? I mean RPC and REST are entirely different API concepts and when creating a bridge from REST to gRPC, it would require more explanation.

I still don't follow this @aaronc. I think it can be made more clear or explicit.

I will remove the part about REST here and just say gRPC because REST refers to grpc-gateway and that's a separate topic.

alexanderbez · 2020-04-21T20:05:18Z

docs/architecture/adr-020-protobuf-transaction-encoding.md

-  StdSignDocBase base = 1;
-  repeated Message msgs = 2;
+```proto
+message AnyTransaction {


It's not clear to me what purpose AnyTransaction serves apart from being used during tx signing. How does this fit into tx generation and broadcasting?

Are you stating that module tx endpoints will generate tx JSON blobs using AnyTransaction. Will these JSON blobs then be fed to /tx/broadcast, where they're JSON decoded and then Proto re-encoded?

Please make this very clear.

This should be SignDoc+signatures, i.e. a signed transaction in an application agnostic container.

Yes AnyTransaction can be used for tx broadcasting. It is related to SignDoc but not quite the same. This is basically the same exact design that we currently have with amino. It just involves a re-encoding step to translate Any to the oneof's

docs/architecture/adr-020-protobuf-transaction-encoding.md

webmaster128 · 2020-04-21T20:58:08Z

docs/architecture/adr-020-protobuf-transaction-encoding.md

@@ -73,27 +74,57 @@ and includes all the core field members that are common across all transaction t
 Developers do not have to include `StdTxBase` if they wish, so it is meant to be
 used as an auxiliary type.


What would be the alternative to using StdTxBase? Signatures need to go somewhere. Even slightly changing this type probably has a lot of consequences. Right now the sentence sounds like you can just drop StdTxBase.

I think this is intended to imply that apps can override the std behavior

webmaster128 · 2020-04-21T21:24:42Z

docs/architecture/adr-020-protobuf-transaction-encoding.md

-  StdSignDocBase base = 1;
-  repeated Message msgs = 2;
+```proto
+message AnyTransaction {


This should be SignDoc+signatures, i.e. a signed transaction in an application agnostic container.

docs/architecture/adr-020-protobuf-transaction-encoding.md

alexanderbez

conceptACK

aaronc · 2020-04-27T15:56:15Z

Closed in favor of #6081

Update ADR 020

0445e8a

aaronc requested review from alessio, alexanderbez, fedekunze, hschoenburg and nylira as code owners April 20, 2020 18:28

Update ADR 020

a35f824

alexanderbez reviewed Apr 20, 2020

View reviewed changes

aaronc and others added 5 commits April 20, 2020 14:55

Fix ADR

57eee2e

Apply suggestions from code review

11fc3f5

Co-Authored-By: Alexander Bezobchuk <[email protected]>

Describe TxService

370c20f

Grammar editing

2d0efb4

Merge branch 'master' into aaronc/6030-update-adr

99a37c4

alexanderbez added T: ADR An issue or PR relating to an architectural decision record R4R labels Apr 21, 2020

alexanderbez assigned aaronc Apr 21, 2020

alexanderbez reviewed Apr 21, 2020

View reviewed changes

Merge branch 'master' into aaronc/6030-update-adr

21f30c9

webmaster128 reviewed Apr 21, 2020

View reviewed changes

docs/architecture/adr-020-protobuf-transaction-encoding.md Outdated Show resolved Hide resolved

alexanderbez approved these changes Apr 22, 2020

View reviewed changes

Merge branch 'master' into aaronc/6030-update-adr

af29fce

aaronc mentioned this pull request Apr 22, 2020

Protobuf tx UX is bad for multi-chain wallets/block explorers #6030

Closed

4 tasks

webmaster128 mentioned this pull request Apr 24, 2020

Define supported mappings (proto->JSON or JSON->proto) regen-network/canonical-proto3#2

Open

aaronc marked this pull request as draft April 27, 2020 14:41

aaronc changed the title ~~Update ADR 020 to address client-side tx UX~~ Update ADR 019 and 020 to reflect usage of Any Apr 27, 2020

aaronc changed the title ~~Update ADR 019 and 020 to reflect usage of Any~~ Update ADRs 019 and 020 to reflect usage of Any Apr 27, 2020

Update ADR 019 to reflect usage of Any

c02323f

aaronc added 4 commits April 27, 2020 11:43

Update ADR 019

ca9441f

Update ADR 019

64b72b3

Update ADR 019

717ab43

Revert ADR 020 changes

1359459

aaronc changed the title ~~Update ADRs 019 and 020 to reflect usage of Any~~ Update ADR 019 to reflect usage of Any Apr 27, 2020

aaronc changed the title ~~Update ADR 019 to reflect usage of Any~~ Update ADR 020 to reflect usage of Any Apr 27, 2020

aaronc changed the title ~~Update ADR 020 to reflect usage of Any~~ Update ADR 020 to address client-side tx UX Apr 27, 2020

aaronc closed this Apr 27, 2020

alessio deleted the aaronc/6030-update-adr branch March 14, 2021 01:45

Itchibon777 mentioned this pull request Oct 27, 2023

[Snyk] Security upgrade axios from 0.19.0 to 1.6.0 Itchibon777/cosmos-sdk#6

Open

Itchibon777 mentioned this pull request Dec 27, 2023

[Snyk] Security upgrade axios from 0.19.0 to 1.6.3 Itchibon777/cosmos-sdk#7

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update ADR 020 to address client-side tx UX #6031

Update ADR 020 to address client-side tx UX #6031

aaronc commented Apr 20, 2020 •

edited by alexanderbez

Loading

codecov bot commented Apr 20, 2020 •

edited

Loading

alexanderbez commented Apr 21, 2020

alexanderbez Apr 21, 2020

aaronc Apr 21, 2020

aaronc Apr 21, 2020

webmaster128 Apr 22, 2020

alexanderbez Apr 22, 2020

aaronc Apr 22, 2020

alexanderbez Apr 21, 2020

webmaster128 Apr 21, 2020

aaronc Apr 21, 2020

webmaster128 Apr 21, 2020

aaronc Apr 21, 2020

webmaster128 Apr 21, 2020

alexanderbez left a comment

aaronc commented Apr 27, 2020

		@@ -73,27 +74,57 @@ and includes all the core field members that are common across all transaction t
		Developers do not have to include `StdTxBase` if they wish, so it is meant to be
		used as an auxiliary type.

Update ADR 020 to address client-side tx UX #6031

Update ADR 020 to address client-side tx UX #6031

Conversation

aaronc commented Apr 20, 2020 • edited by alexanderbez Loading

codecov bot commented Apr 20, 2020 • edited Loading

Codecov Report

alexanderbez commented Apr 21, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

aaronc commented Apr 27, 2020

aaronc commented Apr 20, 2020 •

edited by alexanderbez

Loading

codecov bot commented Apr 20, 2020 •

edited

Loading