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(x/auth/tx): propagate tx options to signing handlers #17996

Merged
merged 2 commits into from
Oct 11, 2023

Conversation

kocubinski
Copy link
Member

Description

Closes: #17975


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • run make lint and make test
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@julienrbrt
Copy link
Member

If this fixes the issue, can we have this target main and we backport?

@kocubinski
Copy link
Member Author

If this fixes the issue, can we have this target main and we backport?

Yea, once I get a confirmation from @damiannolan I'll rebase the rebase the branch from main.

Copy link
Contributor

@damiannolan damiannolan left a comment

Choose a reason for hiding this comment

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

Hey @kocubinski, I built simd using the code in this PR and can confirm it works. I successfully signed a ibc MsgTransfer using amino-json.
I also tried using signmode=textual and got the following:

err="[APDU_CODE_INVALID_P1P2] Wrong parameter(s) P1-P2"

Unsure what that's about right now. Happy to say that amino is working fine tho, maybe the textual signing error is related to something else but I would confirm its working before cutting a final.

LGTM. Could we add a test for this?

x/auth/tx/config.go Outdated Show resolved Hide resolved
@tac0turtle tac0turtle added backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release and removed backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release labels Oct 9, 2023
@tac0turtle
Copy link
Member

lets make another pr against main for this

@kocubinski
Copy link
Member Author

err="[APDU_CODE_INVALID_P1P2] Wrong parameter(s) P1-P2"

Unsure what that's about right now. Happy to say that amino is working fine tho, maybe the textual signing error is related to something else but I would confirm its working before cutting a final.

This seems unrelated to the current issue but I'm happy to follow up separately.

LGTM. Could we add a test for this?

A test was added but it's very basic and doesn't expose the current bug. That would require some structural refactoring to give access to unexported fields in tests. I'm inclined to skip that for now. What do you think?

@kocubinski kocubinski force-pushed the kocubinski/sign-17975 branch from 51c1bfe to 3ef834f Compare October 10, 2023 01:19
@kocubinski kocubinski force-pushed the kocubinski/sign-17975 branch from 3ef834f to 31247a1 Compare October 10, 2023 01:21
@kocubinski kocubinski changed the base branch from release/v0.50.x to main October 10, 2023 01:21
@julienrbrt julienrbrt added backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release and removed C:x/circuit C:Hubl Tool: Hubl C:log labels Oct 10, 2023
@raynaudoe
Copy link
Contributor

Hey @kocubinski, I built simd using the code in this PR and can confirm it works. I successfully signed a ibc MsgTransfer using amino-json. I also tried using signmode=textual and got the following:

err="[APDU_CODE_INVALID_P1P2] Wrong parameter(s) P1-P2"

Unsure what that's about right now. Happy to say that amino is working fine tho, maybe the textual signing error is related to something else but I would confirm its working before cutting a final.

LGTM. Could we add a test for this?

Hi! just curious about this bug. You're using a ledger device to sign the tx, right ? could you please describe the steps to reproduce this error ?

@damiannolan
Copy link
Contributor

Hi! just curious about this bug. You're using a ledger device to sign the tx, right ? could you please describe the steps to reproduce this error ?

Hey @raynaudoe. Yes I've been using a ledger nano X for tx signing. This error arose when trying to sign an ibc MsgTransfer using flags --ledger --sign-mode textual. In order to reproduce it you'll need an ibc enabled simapp setup as well as an ibc transfer channel with a counterparty chain (otherwise CheckTx fails, you can use a single node setup but you'd need to just generate the tx json file offline and then sign). You can use the repo I linked in the original issue of this PR in order to quickly setup a local environment: https://github.com/damiannolan/simd-scripts

There is a readme on the repository with useful commands but I can outline here what steps I took.

  1. Reset the ledger I was using, use ledger live, install cosmos app, create a new account address on it.
  2. Clone ibc-go and build the simd binary for chain nodes:
git clone git@github.com:cosmos/ibc-go.git && cd ibc-go

git checkout damian/ledger-testing # (use this branch - https://github.com/cosmos/ibc-go/pull/4812)

make install
  1. Follow the rest of the steps on the readme of simd-scripts repo for setting up two local chains with an ibc channel - hermes relayer is a prereq for this - there is some make targets:
make init # bootstrap the two networks with some test accounts etc 
make hermes-setup-connection # sets up an ibc connection between the two chains
make hermes-setup-channel # sets up an ibc transfer channel on top of the connection
  1. There is a section at the end of the readme - Ledger signing which has some sample cmds to fund the ledger account from one of the demo accounts and CLI's for sending the MsgTransfer. Use the last cmd and provide sign-mode textual as a flag:
simd tx ibc-transfer transfer transfer channel-0 cosmos17dtl0mjt3t77kpuhg2edqzjpszulwhgzuj9ljs 1000stake --from ledgerKey --ledger --sign-mode textual --node tcp://localhost:16657 --home ./data/test-1

Bit of an involved setup but fairly quick with the scripts in that repo - feel free to do generate and sign offline if you want.

@raynaudoe
Copy link
Contributor

Yes I've been using a ledger nano X for tx signing

@damiannolan Thanks for the description!
The official release of the cosmos app for nano X still doesn't support signmode textual, most likely that's why you're receiving that error. However, if you have a ledger nanoS/nanoS+ that feature is already supported on the latest app version.

@damiannolan
Copy link
Contributor

Yes I've been using a ledger nano X for tx signing

@damiannolan Thanks for the description! The official release of the cosmos app for nano X still doesn't support signmode textual, most likely that's why you're receiving that error. However, if you have a ledger nanoS/nanoS+ that feature is already supported on the latest app version.

No problem! Thanks a lot. I assume signmode textual support for the nano X will come later?

@julienrbrt julienrbrt added this pull request to the merge queue Oct 11, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 11, 2023
@julienrbrt julienrbrt added this pull request to the merge queue Oct 11, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 11, 2023
@facundomedica facundomedica added this pull request to the merge queue Oct 11, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 11, 2023
@julienrbrt julienrbrt added this pull request to the merge queue Oct 11, 2023
Merged via the queue into main with commit 758d3a1 Oct 11, 2023
@julienrbrt julienrbrt deleted the kocubinski/sign-17975 branch October 11, 2023 10:21
mergify bot pushed a commit that referenced this pull request Oct 11, 2023
Co-authored-by: Marko <marbar3778@yahoo.com>
(cherry picked from commit 758d3a1)
julienrbrt pushed a commit that referenced this pull request Oct 11, 2023
…7996) (#18067)

Co-authored-by: Matt Kocubinski <mkocubinski@gmail.com>
@raynaudoe
Copy link
Contributor

Yes I've been using a ledger nano X for tx signing

@damiannolan Thanks for the description! The official release of the cosmos app for nano X still doesn't support signmode textual, most likely that's why you're receiving that error. However, if you have a ledger nanoS/nanoS+ that feature is already supported on the latest app version.

No problem! Thanks a lot. I assume signmode textual support for the nano X will come later?

Yes, should be released very soon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release C:x/auth
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Legacy amino-json ledger signing fails for ibc MsgTransfer (protos outside of sdk)
5 participants