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

Check Ledger app version in gaiacli #1998

Closed
cwgoes opened this issue Aug 13, 2018 · 10 comments
Closed

Check Ledger app version in gaiacli #1998

cwgoes opened this issue Aug 13, 2018 · 10 comments
Labels
C:CLI C:Ledger Issues and features related Ledger integration and functionality T: UX

Comments

@cwgoes
Copy link
Contributor

cwgoes commented Aug 13, 2018

In case we later release incompatible versions, then we can warn the user.

@alessio
Copy link
Contributor

alessio commented Nov 29, 2018

How do we go about it? Do we want to create a new command for that or implement a version check in gaiacli keys sub-commands that support ledger devices?

@alessio alessio added the C:Ledger Issues and features related Ledger integration and functionality label Nov 29, 2018
@alexanderbez
Copy link
Contributor

I think gaiacli keys add ... --ledger should warn the user of incompatible version(s).

@cwgoes
Copy link
Contributor Author

cwgoes commented Nov 29, 2018

I think it should maybe even refuse to proceed in case of incompatible version, unless the user passes an opt-in flag to override the version check.

@alexanderbez
Copy link
Contributor

💯 agree. I think this is a good UX.

@alessio
Copy link
Contributor

alessio commented Nov 29, 2018

Currently, ledger client returns the following:

&{AppId:0 Major:1 Minor:0 Patch:0}

Is that acceptable to check only against Major == 1?

@cwgoes
Copy link
Contributor Author

cwgoes commented Nov 29, 2018

Since we choose the version, that seems fine for now.

@alessio
Copy link
Contributor

alessio commented Nov 30, 2018

I believe this #2870 would close this

@cwgoes
Copy link
Contributor Author

cwgoes commented Nov 30, 2018

Does #2870 check the version (in the updated library) and throw an error on mismatch?

@alessio
Copy link
Contributor

alessio commented Nov 30, 2018

@alessio
Copy link
Contributor

alessio commented Nov 30, 2018

It does what I proposed above. Checking whether the major version => a required one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:CLI C:Ledger Issues and features related Ledger integration and functionality T: UX
Projects
None yet
Development

No branches or pull requests

4 participants