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

Implement SIGN_MODE_DIRECT #6216

Closed
wants to merge 69 commits into from
Closed

Implement SIGN_MODE_DIRECT #6216

wants to merge 69 commits into from

Conversation

aaronc
Copy link
Member

@aaronc aaronc commented May 13, 2020

ref: #6213

@auto-comment
Copy link

auto-comment bot commented May 13, 2020

👋 Thanks for creating a PR!

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.


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)

Thank you for your contribution to the Cosmos-SDK! 🚀

@aaronc aaronc mentioned this pull request May 13, 2020
27 tasks
@aaronc aaronc added the WIP label May 13, 2020
@aaronc aaronc mentioned this pull request May 21, 2020
11 tasks
@jaekwon
Copy link
Contributor

jaekwon commented Jun 3, 2020

Is this signing proto3 encoded bytes? Signing should be on json bytes, proto bytes don't let you inspect data before signing them, as you can in the ledger app. The information encoded in fields is ambiguous when protobuf binary encoded. You can't even tell if a number you're signing is negative or positive. Enforcing a canonical JSON format as the blockchain signing standard helps signing tools (like ledger apps) stay intelligent. Even if say the ledger app doesn't know what the tx means, it can show all the field names and values for the signer to inspect, as an advanced option.

Signing options introduces more complexity in the SDK, and more surface area for attack.

Another example... batch-signing tools should operate on lines of JSON txs (or sign-docs) in a file. You want the file of batch of txs to be inspected and reviewed as a static representation, then you want the tool to sign each line.

Is there a discussion on why this is needed?

@aaronc
Copy link
Member Author

aaronc commented Jun 3, 2020

Is this signing proto3 encoded bytes? Signing should be on json bytes, proto bytes don't let you inspect data before signing them, as you can in the ledger app. The information encoded in fields is ambiguous when protobuf binary encoded. You can't even tell if a number you're signing is negative or positive. Enforcing a canonical JSON format as the blockchain signing standard helps signing tools (like ledger apps) stay intelligent. Even if say the ledger app doesn't know what the tx means, it can show all the field names and values for the signer to inspect, as an advanced option.

Signing options introduces more complexity in the SDK, and more surface area for attack.

Another example... batch-signing tools should operate on lines of JSON txs (or sign-docs) in a file. You want the file of batch of txs to be inspected and reviewed as a static representation, then you want the tool to sign each line.

Is there a discussion on why this is needed?

We had extensive, extensive discussions on github, discord and zoom to arrive at the current design. Please read through #6078 for context.

@aaronc aaronc mentioned this pull request Jun 8, 2020
8 tasks
@github-actions
Copy link
Contributor

github-actions bot commented Jul 4, 2020

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.

types/result.go Show resolved Hide resolved
types/result.go Show resolved Hide resolved
@sahith-narahari sahith-narahari mentioned this pull request Jul 14, 2020
9 tasks
mergify bot pushed a commit that referenced this pull request Jul 17, 2020
* refactor auths broadcast cmd in alignment with #6216

* add TxResponse proto definition, and related refactoring

* re-run make proto-gen, updating staking.pb.go

* cleanup TxResponse tests to handle nil return values

* properly handle nil Tx value in TxResponse's implementation of UnpackInterfaces

Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
chengwenxi pushed a commit to irisnet/cosmos-sdk that referenced this pull request Jul 22, 2020
* refactor auths broadcast cmd in alignment with cosmos#6216

* add TxResponse proto definition, and related refactoring

* re-run make proto-gen, updating staking.pb.go

* cleanup TxResponse tests to handle nil return values

* properly handle nil Tx value in TxResponse's implementation of UnpackInterfaces

Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
@alessio alessio deleted the aaronc/6213-sign-mode branch March 14, 2021 01:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). T: Proto Breaking Protobuf breaking changes: never don't do this!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants