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

feat: Add SignDocJSON for proto JSON signing #8476

Merged
merged 6 commits into from
Jul 20, 2021

Conversation

aaronc
Copy link
Member

@aaronc aaronc commented Jan 29, 2021

ref: #9320

After our call today, it feels like this is may be the simplest way forward for now.

Canonical proto signing would be done using this spec: https://github.com/regen-network/canonical-proto3/blob/master/README.md#json

@jleni @gagbo the JSON would look like this:

{
  "account_number": {number},
  "chain_id": {string},
  "auth_info":{
    "fee": {
      "amount": [{"amount": {number}, "denom": {string}}, ...],
      "gas_limit": {number}
    },
    "signer_infos":[{
      "sequence":{number}
     }]
  },
 "body":{
    "memo": {string},
    "messages": [{arbitrary}]
  }
}

Also proto messages will use @type instead of type.


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • 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
  • Review Codecov Report in the comment section below once CI passes

@aaronc aaronc marked this pull request as ready for review January 29, 2021 17:17
@aaronc aaronc requested a review from zmanian January 29, 2021 17:18
@gagbo
Copy link
Contributor

gagbo commented Feb 1, 2021

Hello, I'll try to be short :)

  • at least from hardware device signing perspective it should be fine, since it's basically the same thing as AMINO JSON (signing human readable bytes that a small app can parse)

  • It seems it will add work on the wallet side regarding serialization (which is expected).

    • Before, we knew we could take any json payload, sort it lexicographically, and sign it without any external knowledge.
    • Now, wallets need to know the proto files that the network expects in order to produce a correct serialization (especially for the inner messages) as far as I understand

I didn't follow the proto development here a lot, is there a place where we can find the proto files currently used by the chain ? Or even an endpoint that serves proto files, so that as a wallet, I can double check my formats during "proto format negotiation" before broadcasting ? I guess those questions are more for cosmos/gaia than this, no idea how getting the correct proto should happen basically, so that wallets can fetch and/or ensure a source of truth for serialization (and avoiding sign verification error on broadcast)

Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

Nice.

I think it would be very helpful to see an example JSON dump of this, including one or two messages as well as the hash.

The only annoyance I see with this is that we are back to depending on protobuf field names, enforcing code generators to preserve the original casing even in camelCase dominated ecosystems like JavaScript or Swift. This can only be prevented if there are strict casing rules, like all proto message fields must use snake_case. Then we could let the code generator do its preferred transformation making the linters happy, and auto-convert to snake_case in a signing library when creating a SignDocTextual. Maybe https://github.com/regen-network/canonical-proto3/blob/master/README.md#do-not-use-lowercamelcase-names could be adjusted to enforce snake_case instead of disallowing lowerCamelCase.

proto/cosmos/tx/v1beta1/tx.proto Outdated Show resolved Hide resolved
proto/cosmos/tx/v1beta1/tx.proto Outdated Show resolved Hide resolved
proto/cosmos/tx/v1beta1/tx.proto Outdated Show resolved Hide resolved
proto/cosmos/tx/v1beta1/tx.proto Outdated Show resolved Hide resolved
proto/cosmos/tx/v1beta1/tx.proto Show resolved Hide resolved
@github-actions
Copy link
Contributor

github-actions bot commented May 1, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label May 1, 2021
fedekunze and others added 2 commits May 2, 2021 10:00
Co-authored-by: Simon Warta <2603011+webmaster128@users.noreply.github.com>
@fedekunze fedekunze added pinned and removed stale labels May 2, 2021
@fedekunze
Copy link
Collaborator

@aaronc is this targeted for v0.43?

@aaronc
Copy link
Member Author

aaronc commented May 3, 2021

@aaronc is this targeted for v0.43?

No unfortunately not. For 0.44 yes.

@aaronc aaronc mentioned this pull request May 12, 2021
3 tasks
@aaronc
Copy link
Member Author

aaronc commented Jul 12, 2021

@AmauryM @ruhatch could you review this PR at your convenience please?

amaury1093
amaury1093 previously approved these changes Jul 13, 2021
Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

SignDoc proto def looks good.

I only have questions about naming:

  • what did "DIRECT" mean in SIGN_MODE_DIRECT?
  • since this is JSON, why are we still calling it "Textual"?

proto/cosmos/tx/v1beta1/tx.proto Outdated Show resolved Hide resolved
proto/cosmos/tx/v1beta1/tx.proto Show resolved Hide resolved
@amaury1093
Copy link
Contributor

@aaronc Are you still pushing commits to this PR? Or would you prefer @ruhatch / me to take over?

@aaronc
Copy link
Member Author

aaronc commented Jul 19, 2021

@aaronc Are you still pushing commits to this PR? Or would you prefer @ruhatch / me to take over?

It would be great if one of you could take over. Thank you 🙏

@amaury1093
Copy link
Contributor

As discussed during our standup, @ruhatch will push commits to this PR to address the few remaining review comments.

@orijbot
Copy link

orijbot commented Jul 20, 2021

Visit https://dashboard.github.orijtech.com?pr=8476&repo=cosmos%2Fcosmos-sdk to see benchmark details.

@ruhatch ruhatch requested a review from amaury1093 July 20, 2021 13:27
@ruhatch ruhatch changed the title Add SignDocTextual for proto JSON signing feat: Add SignDocJSON for proto JSON signing Jul 20, 2021
Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

lgtm

proto/cosmos/tx/v1beta1/tx.proto Show resolved Hide resolved
@ruhatch ruhatch dismissed amaury1093’s stale review July 20, 2021 15:14

Would like a new one!

@ruhatch ruhatch requested a review from amaury1093 July 20, 2021 15:14
@ruhatch ruhatch merged commit 12e4be3 into master Jul 20, 2021
@ruhatch ruhatch deleted the aaronc/6513-textual-json-proto branch July 20, 2021 15:15
likhita-809 added a commit that referenced this pull request Sep 30, 2021
…-proto"

This reverts commit 12e4be3, reversing
changes made to 085ab06.
likhita-809 added a commit that referenced this pull request Oct 5, 2021
…-proto"

This reverts commit 12e4be3, reversing
changes made to 085ab06.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants