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.

Sign up for GitHub

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

Jump to bottom

Use Makefile to generate manifest to use ledgerctl #161

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Use Makefile to generate manifest to use ledgerctl #161

wants to merge 1 commit into from

Conversation

mlafon-ledger
Copy link

Description

Modify Makefile to create a manifest to sideload app with ledgerctl

Changes include

  • Bugfix (non-breaking change that solves an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (change that is not backwards-compatible and/or changes current functionality)
  • Tests
  • Documentation
  • Other (for changes that might not fit in any category)

@mlafon-ledger mlafon-ledger marked this pull request as draft February 8, 2023 09:28
@mlafon-ledger mlafon-ledger marked this pull request as ready for review February 8, 2023 16:05
xchapron-ledger
xchapron-ledger reviewed Feb 10, 2023
View reviewed changes
Copy link
Contributor

@xchapron-ledger xchapron-ledger left a comment

Choose a reason for hiding this comment

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

Look good for me.
Now you need someone from the Os to review it too :)

create_manifest.py
return int(x, 0)

def get_argparser():
parser = argparse.ArgumentParser(description="Load an app onto the device from a hex file.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment explaining this is taken as is from loadApp.py parser which can expect quite a lot of params...

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you could even import get_argparser() from ledgerblue.loadApp? It would make things more obvious. What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

So it means that users will have to use ledgerblue but the point is to use ledgerctl this is why I did not want to add ledgerblue dependancies. But I can add a comment and copy/paste the arguments in the same order that there are in ledgerblue if you want

Copy link
Author

Choose a reason for hiding this comment

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

I know what I used this order is to split element used by ledgerctl and element that are here for ledgerblue compatibility :

ledger-secure-sdk/create_manifest.py

Line 44 in 82a7b01

# Not used - to be compatible with python3 ledgerblue

create_manifest.py Outdated Show resolved Hide resolved
@xchapron-ledger
Copy link
Contributor

I'm wondering, couldn't we use the already existing --offline parameter from ledgerblue/loadµApp.py. It's purpose is to generate an app.apdu which should contains everything. But maybe ledgerctl doesn't support loading app from app.apdu file?

@yhql yhql mentioned this pull request Apr 13, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cbouvet-ledger cbouvet-ledger cbouvet-ledger approved these changes

@xchapron-ledger xchapron-ledger Awaiting requested review from xchapron-ledger

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mlafon-ledger @xchapron-ledger @cbouvet-ledger