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

fix: restore ability to sign transaction with ledger firmware 2.2.2 #2668

Merged
merged 6 commits into from
Oct 13, 2023

Conversation

evan-forbes
Copy link
Member

@evan-forbes evan-forbes commented Oct 12, 2023

Overview

Closes #2650 by upgrading to celestiaorg/cosmos-sdk v1.18.2-sdk-v0.46.14 so that it is possible to sign transaction with a Ledger with firmware 2.2.2

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@evan-forbes evan-forbes added the bug Something isn't working label Oct 12, 2023
@evan-forbes evan-forbes self-assigned this Oct 12, 2023
@cmwaters
Copy link
Contributor

You don't want to just bump it on the SDK?

Ref: celestiaorg/cosmos-sdk#348

I'm fine with either. This is probably quicker because you don't need a second release

@rootulp rootulp self-assigned this Oct 12, 2023
@rootulp
Copy link
Collaborator

rootulp commented Oct 12, 2023

I think we may need to bump it in celestiaorg/cosmos-sdk and then here. Will start that now

$ make build && make install
--> Updating go.mod
# github.com/cosmos/cosmos-sdk/crypto/ledger
../../../../go/pkg/mod/github.com/celestiaorg/[email protected]/crypto/ledger/ledger_real.go:18:10: cannot use device (variable of type *ledger_cosmos_go.LedgerCosmos) as SECP256K1 value in return statement: *ledger_cosmos_go.LedgerCosmos does not implement SECP256K1 (wrong type for method SignSECP256K1)
		have SignSECP256K1([]uint32, []byte, byte) ([]byte, error)
		want SignSECP256K1([]uint32, []byte) ([]byte, error)
make: *** [build] Error 1

@rootulp rootulp changed the title fix: replace cosmos/ledger-cosmos-go with latest version fix: signing with ledger throws an empty error Oct 12, 2023
@rootulp rootulp added the backport:v1.x PR will be backported automatically to the v1.x branch upon merging label Oct 12, 2023
@@ -44,6 +44,7 @@ require (
github.com/bits-and-blooms/bitset v1.7.0 // indirect
github.com/consensys/bavard v0.1.13 // indirect
github.com/consensys/gnark-crypto v0.12.0 // indirect
github.com/cosmos/ledger-cosmos-go v0.13.1 // indirect
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@celestiaorg celestiaorg deleted a comment from codecov-commenter Oct 12, 2023
rootulp
rootulp previously approved these changes Oct 12, 2023
Copy link
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

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

LGTM if CI passes

@rootulp rootulp marked this pull request as ready for review October 12, 2023 18:11
cmwaters
cmwaters previously approved these changes Oct 13, 2023
@rootulp
Copy link
Collaborator

rootulp commented Oct 13, 2023

FYI we can merge this now or wait for a Cosmos SDK v0.46.x release (see celestiaorg/cosmos-sdk#349 (comment)) which is expected next week

@rootulp rootulp dismissed stale reviews from cmwaters and themself via dc332e5 October 13, 2023 16:20
@celestia-bot celestia-bot requested review from a team and rootulp and removed request for a team October 13, 2023 16:20
@rootulp rootulp requested a review from cmwaters October 13, 2023 16:21
@rootulp rootulp enabled auto-merge (squash) October 13, 2023 18:59
Copy link
Contributor

@staheri14 staheri14 left a comment

Choose a reason for hiding this comment

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

Looks good to me!
[optional] Regarding the PR title, is intended change to introduce the behavior of throwing an empty error, or preventing errors from being thrown? It might be more informative to emphasize the intended change rather than the initial bug (entirely optional though).

@rootulp rootulp merged commit c0e1c22 into main Oct 13, 2023
31 checks passed
@rootulp rootulp deleted the evan/bump-ledger-comsos branch October 13, 2023 20:04
mergify bot pushed a commit that referenced this pull request Oct 13, 2023
## Overview

Closes #2650 by upgrading to celestiaorg/cosmos-sdk
[v1.18.2-sdk-v0.46.14](https://github.com/celestiaorg/cosmos-sdk/releases/tag/v1.18.2-sdk-v0.46.14)

## Checklist

- [x] New and updated code has appropriate documentation
- [x] New and updated code has new and/or updated testing
- [x] Required CI checks are passing
- [x] Visual proof for any user facing features like CLI or
documentation updates
- [x] Linked issues closed with keywords

---------

Co-authored-by: Rootul Patel <[email protected]>
(cherry picked from commit c0e1c22)

# Conflicts:
#	go.mod
#	go.sum
@rootulp rootulp changed the title fix: signing with ledger throws an empty error fix: restore ability to sign transaction with ledger firmware 2.2.2 Oct 13, 2023
@rootulp
Copy link
Collaborator

rootulp commented Oct 13, 2023

Good point. This PR prevents an error from being thrown. I updated the PR description and PR title.

rootulp added a commit that referenced this pull request Oct 13, 2023
…backport #2668) (#2678)

This is an automatic backport of pull request #2668 done by
[Mergify](https://mergify.com).
Cherry-pick of c0e1c22 has failed:
```
On branch mergify/bp/v1.x/pr-2668
Your branch is up to date with 'origin/v1.x'.

You are currently cherry-picking commit c0e1c22.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   go.mod
	both modified:   go.sum

no changes added to commit (use "git add" and/or "git commit -a")
```


To fix up this pull request, you can check it out locally. See
documentation:
https://docs.github.com/en/github/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

---


<details>
<summary>Mergify commands and options</summary>

<br />

More conditions and actions can be found in the
[documentation](https://docs.mergify.com/).

You can also trigger Mergify actions by commenting on this pull request:

- `@Mergifyio refresh` will re-evaluate the rules
- `@Mergifyio rebase` will rebase this PR on its base branch
- `@Mergifyio update` will merge the base branch into this PR
- `@Mergifyio backport <destination>` will backport this PR on
`<destination>` branch

Additionally, on Mergify [dashboard](https://dashboard.mergify.com) you
can:

- look at your merge queues
- generate the Mergify configuration with the config editor.

Finally, you can contact us on https://mergify.com
</details>

---------

Co-authored-by: Evan Forbes <[email protected]>
Co-authored-by: Rootul Patel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:v1.x PR will be backported automatically to the v1.x branch upon merging bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Signing with ledger throws an empty error
4 participants