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

Upgrading dependencies #3962

Merged
merged 6 commits into from
Mar 26, 2019
Merged

Conversation

jleni
Copy link
Member

@jleni jleni commented Mar 21, 2019

When a ledger device tries to sign a transaction and considers the message invalid, it generates a more specific error message than what it is being shown in the CLI.
Expose this error message.

Fixes are applied in the corresponding dependencies:
Zondax/ledger-cosmos-go#26
Zondax/ledger-go#7

Closes #3959

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote tests
  • Updated relevant documentation (docs/)
  • Added a relevant changelog entry: sdkch add [section] [stanza] [message]
  • rereviewed Files changed in the github PR explorer

For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@jackzampolin
Copy link
Member

Do we have any way to test ledger integration in CI?

@jackzampolin
Copy link
Member

Also why aren't we using the cosmos/* repos?

Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

Can we use https://github.com/cosmos/ledger-go instead of the fork?

@jleni
Copy link
Member Author

jleni commented Mar 24, 2019

@cwgoes Yes, sure. I will push the missing changes to cosmos/ledger-go and update the go.mod tomorrow morning.

@cwgoes
Copy link
Contributor

cwgoes commented Mar 24, 2019

Thanks - just trying to keep our dependency graph as sane as possible.

@jleni
Copy link
Member Author

jleni commented Mar 25, 2019

Do we have any way to test ledger integration in CI?

make test_ledger in the Cosmos SDK runs tests that require a connected Ledger device.
These are not an end to end tests but confirm public keys, HD paths, signatures.

Maybe we could extend this to CLI use cases too.

@jleni jleni force-pushed the jleni/ledger_errors branch from 5584213 to 95fba6a Compare March 25, 2019 14:57
@jleni
Copy link
Member Author

jleni commented Mar 25, 2019

@cwgoes dependencies updated

Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

ACK, thanks @jleni

@jleni jleni force-pushed the jleni/ledger_errors branch from 95fba6a to 751787f Compare March 25, 2019 15:19
@jackzampolin jackzampolin merged commit 2dfba4e into cosmos:develop Mar 26, 2019
@jleni jleni deleted the jleni/ledger_errors branch March 26, 2019 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants