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 ledger/multisig detection in SignTx functions #9026

Merged
merged 6 commits into from
Mar 31, 2021
Merged

Conversation

amaury1093
Copy link
Contributor

@amaury1093 amaury1093 commented Mar 30, 2021

Description

Before:
Some individual commands (tx sign, tx sign-batch) hardcoded to use SIGN_MODE_LEGACY_AMINO_JSON in the CLI command handler.

We forgot to hardcode SIGN_MODE_LEGACY_AMINO_JSON in the gentx command, hence the bug #9023

After:
Instead of putting the code:
if txF.SignMode == nil {txF.WithSignMode(LEGACY_AMINO_JSON)}
in each individual CLI command, we factorize that in the upstream .SignTx and .SignTxWithAddress functions that the CLI calls.

I tested simd gentx with a ledger key

closes: #9023


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

Comment on lines +71 to +72
// This function should only be used when signing with a multisig. For
// normal keys, please use SignTx directly.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is called twice in the sdk, and both are with multisigs. As I understand, there's no use-case for calling this without a multisig, correct?

Copy link
Member

Choose a reason for hiding this comment

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

That's probably right

@amaury1093 amaury1093 mentioned this pull request Mar 30, 2021
4 tasks
@codecov
Copy link

codecov bot commented Mar 30, 2021

Codecov Report

Merging #9026 (51e8df8) into master (feed37d) will decrease coverage by 0.10%.
The diff coverage is 12.50%.

❗ Current head 51e8df8 differs from pull request most recent head 41499bc. Consider uploading reports for the commit 41499bc to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9026      +/-   ##
==========================================
- Coverage   58.96%   58.85%   -0.11%     
==========================================
  Files         575      573       -2     
  Lines       32159    32091      -68     
==========================================
- Hits        18963    18888      -75     
- Misses      10982    10992      +10     
+ Partials     2214     2211       -3     
Impacted Files Coverage Δ
x/auth/client/tx.go 36.06% <0.00%> (-3.23%) ⬇️
x/auth/client/cli/tx_sign.go 73.88% <100.00%> (-0.71%) ⬇️
store/mem/store.go 60.00% <0.00%> (-21.82%) ⬇️
store/dbadapter/store.go 91.66% <0.00%> (-8.34%) ⬇️
store/rootmulti/store.go 66.05% <0.00%> (-2.42%) ⬇️
store/iavl/store.go 70.91% <0.00%> (-1.17%) ⬇️
store/tracekv/store.go 88.88% <0.00%> (-0.25%) ⬇️
store/gaskv/store.go 100.00% <0.00%> (ø)
store/types/store.go 60.00% <0.00%> (ø)
store/listenkv/store.go
... and 4 more

CHANGELOG.md Outdated Show resolved Hide resolved
@amaury1093 amaury1093 marked this pull request as ready for review March 30, 2021 11:14
@amaury1093 amaury1093 added C:CLI C:x/genutil genutil module issues labels Mar 30, 2021
@amaury1093 amaury1093 requested a review from clevinson March 30, 2021 11:32
Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

ACK

@aaronc aaronc added the A:automerge Automatically merge PR once all prerequisites pass. label Mar 30, 2021
@mergify mergify bot merged commit 413938c into master Mar 31, 2021
@mergify mergify bot deleted the am/9023-gentx branch March 31, 2021 09:25
amaury1093 added a commit that referenced this pull request Apr 1, 2021
* Add ledger/multisig detection in SignTx functions

* Fix tests

* Update CHANGELOG.md

* update cl

Co-authored-by: Alessio Treglia <[email protected]>
alessio pushed a commit that referenced this pull request Apr 1, 2021
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:CLI C:x/genutil genutil module issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't create gentx using ledger
3 participants