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

Add support for protobuf TxGenerator and SIGN_MODE_DIRECT #6385

Merged
merged 57 commits into from
Jul 6, 2020

Conversation

aaronc
Copy link
Member

@aaronc aaronc commented Jun 10, 2020

ref: #6213

Summary

This PR adds support for a protobuf client.TxGenerator with signing via SIGN_MODE_DIRECT.

Details

  • Protobuf Tx additions/changes:
    • Add implementations of client.TxGenerator and client.TxBuilder for the new protobuf Tx in the new x/auth/tx package (the old StdTx will move to x/auth/tx/legacy in a future PR)
    • Add a SignModeHandler implementation for SIGN_MODE_DIRECT
    • Adds the TxRaw proto type and updates SignDoc as well as doc comments to reflect the latest state of ADR 020, updates SignerInfo to use PublicKey rather than Any.
  • Refactoring of existing code:
    • Move sdk.TestMsg to codec/testdata adding protobuf support
    • Move the x/auth/ante.SigVerifiableTx to x/auth/signing.SigVerifiableTx so that it can be used from other packages
    • Add the SigFeeMemoTx interface to x/auth/signing which wraps sdk.Tx, FeeTx, TxWithMemo and SigVerifiableTx in a single interface and use this in client.TxBuilder. (I'm not sure this is well named... but functionally it makes sense because this is what TxBuilder actually supports and is standard throughout the SDK.)
    • Rename CompactBitArray.Size() to Count() and re-enable gogoproto.sizer - this was causing a nasty bug and the proto Size method shouldn't have been overridden.

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

@codecov
Copy link

codecov bot commented Jun 10, 2020

Codecov Report

Merging #6385 into master will increase coverage by 0.26%.
The diff coverage is 72.62%.

@@            Coverage Diff             @@
##           master    #6385      +/-   ##
==========================================
+ Coverage   57.04%   57.30%   +0.26%     
==========================================
  Files         481      491      +10     
  Lines       28939    29470     +531     
==========================================
+ Hits        16507    16889     +382     
- Misses      11258    11372     +114     
- Partials     1174     1209      +35     

@clevinson clevinson added this to the v0.39 milestone Jun 11, 2020
aaronc and others added 12 commits June 11, 2020 11:16
* WIP: added test for direct mode handler

* updated code

* Add msg

* Update TxWrapper API

* Fix pubkey declaration

* Add pubkey for tests

* Fix SetFee

* Remove logs

* Avoid global var declaration for tests

* Add test for GetPubKeys

* Fix direct signing tests

* Add more test cases for GetSignBytes

* Revert SetFee API

* Remove logs

* Refactor tests

Co-authored-by: anilCSE <[email protected]>
Co-authored-by: sahith-narahari <[email protected]>
@anilcse anilcse mentioned this pull request Jun 26, 2020
8 tasks
@aaronc aaronc changed the title Add TxWrapper, encoder, decoder and DirectModeHandler Add support for protobuf TxGenerator and SIGN_MODE_DIRECT Jun 30, 2020
@aaronc aaronc requested a review from blushi July 2, 2020 11:19
x/auth/types/test_utils.go Outdated Show resolved Hide resolved
x/auth/tx/sigs.go Outdated Show resolved Hide resolved
x/auth/tx/sigs.go Outdated Show resolved Hide resolved
x/auth/tx/sigs.go Show resolved Hide resolved
x/auth/tx/encoder.go Outdated Show resolved Hide resolved
x/auth/tx/encoder.go Outdated Show resolved Hide resolved
x/auth/tx/builder.go Show resolved Hide resolved
@sahith-narahari sahith-narahari mentioned this pull request Jul 6, 2020
9 tasks
@aaronc aaronc added the C:x/auth label Jul 6, 2020
@aaronc
Copy link
Member Author

aaronc commented Jul 6, 2020

@fedekunze addressed all your review comments. The test coverage should be better now.

@alexanderbez the only pending thing I see is figuring out this naming thing. What do you think?

Would be good to get this merged soon to keep #6213 moving.

Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

LGTM

@aaronc aaronc added the A:automerge Automatically merge PR once all prerequisites pass. label Jul 6, 2020
@mergify mergify bot merged commit 2f44fbf into master Jul 6, 2020
@mergify mergify bot deleted the aaronc/6213-sign-mode-direct branch July 6, 2020 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:x/auth T: Proto Breaking Protobuf breaking changes: generally don't do this!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants