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

Fede+Fabo/Support ledger cosmos app v1.5 and older versions #2382

Merged
merged 30 commits into from
May 11, 2019

Conversation

fedekunze
Copy link
Contributor

Description:

Thank you! 🚀


For contributor:

  • Added entries in PENDING.md with issue # and GitHub username
  • Reviewed Files changed in the github PR explorer
  • Attach screenshots of the UI components on the PR description (if applicable)
  • Scope of work approved for big PRs

For reviewer:

  • Manually tested the changes on the UI

@fedekunze
Copy link
Contributor Author

this PR needs to be updated to support the newest 1.5.0 app version once it's released

@faboweb faboweb changed the title Support ledger v1.3.1 and older versions Fede+Fabo/Support ledger cosmos app v1.5 and older versions May 6, 2019
@faboweb faboweb marked this pull request as ready for review May 6, 2019 12:55
@jbibla
Copy link
Collaborator

jbibla commented May 6, 2019

i don't totally understand this PR.

what is confirmAddress?

also, i am getting an instruction not supported error message when I sign in with ledger. and i don't see any way to confirm address or see the address when creating txs if i sign in with address.

@faboweb
Copy link
Collaborator

faboweb commented May 7, 2019

I forgot: You need to install the Ledger firmware 1.5.0 to test the new address feature: https://github.com/cosmos/ledger-cosmos/blob/master/docs/BUILD.md

instruction not supported

I will investigate!

@faboweb
Copy link
Collaborator

faboweb commented May 7, 2019

Fixed compatibility with version 1.0.1

@jbibla
Copy link
Collaborator

jbibla commented May 7, 2019

You need to install the Ledger firmware 1.5.0 to test the new address feature:

I would expect to see a warning that says my version is old and should be upgraded.

@faboweb
Copy link
Collaborator

faboweb commented May 8, 2019

The warning is commented out as the version is not yet available on Ledger Live.

Signin in with the address
<span class="address">{{ address }}</span
>.<br />
Please confirm on your Ledger.
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there an action associated with this confirm message? if no, it should say "Please confirm that your address is correct" or something like that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes it is associated with an action on the Ledger. Please test these critical features on your Ledger if they come up.

} catch ({ message }) {
switch (message) {
case `Transaction rejected`:
this.connectionError = `Account address rejected`
Copy link
Collaborator

Choose a reason for hiding this comment

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

why would an address get rejected?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The address approval is associated with an action, where you can "reject" the address.

src/vuex/modules/ledger.js Outdated Show resolved Hide resolved
src/vuex/modules/ledger.js Outdated Show resolved Hide resolved
src/vuex/modules/ledger.js Outdated Show resolved Hide resolved
await dispatch(`createLedgerAppInstance`)
const address = await dispatch(`getLedgerAddressAndPubKey`)
commit(`setLedgerConnection`, true)
// DEPRECATION enable once 1.5.0 is available on Ledger Live
Copy link
Collaborator

Choose a reason for hiding this comment

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

1.5 was released. still in dev mode.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok I enable this warning then.

src/vuex/modules/ledger.js Outdated Show resolved Hide resolved
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