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

Refactor the app, fix bugs and add tests. #43

Merged
merged 5 commits into from
Sep 21, 2022

Conversation

overcat
Copy link
Contributor

@overcat overcat commented Aug 21, 2022

Note

  • The Ledger App project started several years ago, and some things may seem outdated at the moment, so I refactored the app based on app-boilerplate.
  • This is a large PR, so it may take more effort to review it, thank you for your efforts. 🙏
  • While this update contains breaking changes, but it does not affect Ledger Live, StellarX, StellarTerm and Stellar Account Viewer, they still work well together.

Changelog

Updated

  • Added a Sequence Number setting: Displayed or NOT Displayed, default to NOT Displayed. (resolves Sequence Number setting #40)
  • Optimized the display of amount. (ex. 10000000 XLM -> 10,000,000 XLM) (resolves Use commas in amount formatting #39)
  • Optimized the display of offer price.
  • Optimize the display of Memo Text and Manage Data Value, if they are printable ASCII characters, they will be printed directly, otherwise display the base64 encoded summary.
  • In some common operations, the prompt for the operation type was removed.
  • Other UX improvements.
  • Other bugfixes.
  • Refactored this app based on app-boilerplate.
  • Refactored unit tests and added full e2e tests.
  • Added APDU documentation.

Breaking changes

  • Removed keypair validation in GET_PUBLIC_KEY command. If necessary, we recommend that you ask the user to confirm the address on the device.
  • Removed support for the KEEP_ALIVE command at the app layer.

@codecov-commenter
Copy link

codecov-commenter commented Aug 21, 2022

Codecov Report

❗ No coverage uploaded for pull request base (develop@0edc5c9). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             develop      #43   +/-   ##
==========================================
  Coverage           ?   77.72%           
==========================================
  Files              ?       12           
  Lines              ?     2352           
  Branches           ?        0           
==========================================
  Hits               ?     1828           
  Misses             ?      524           
  Partials           ?        0           
Flag Coverage Δ
unittests 77.72% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@overcat overcat marked this pull request as draft August 21, 2022 11:19
## Updated
- Added a `Sequence Number` setting: `Displayed` or `NOT Displayed`, default to `NOT Displayed`.
- Optimized the display of amount. (ex. `10000000 XLM` -> `10,000,000 XLM`)
- Optimized the display of offer price.
- Optimize the display of `Memo Text` and `Manage Data Value`, if they are printable ASCII characters, they will be printed directly, otherwise display the base64 encoded summary.
- In some common operations, the prompt for the operation type was removed.
- Other UX improvements.
- Other bugfixes.
- Refactored this app based on [app-boilerplate](https://github.com/ledgerhq/app-boilerplate).
- Refactored unit tests and added full e2e tests.
- Added APDU documentation.

## Breaking changes
- Removed keypair validation in `GET_PUBLIC_KEY` command. If necessary, we recommend that you ask the user to confirm the address on the device.
- Removed support for the `KEEP_ALIVE` command at the app layer.
@overcat overcat marked this pull request as ready for review August 22, 2022 16:19
@lpascal-ledger lpascal-ledger merged commit 8e016af into LedgerHQ:develop Sep 21, 2022
@overcat
Copy link
Contributor Author

overcat commented Sep 21, 2022

@lpascal-ledger It looks like the CI run failed after the merge, let me check.

@overcat
Copy link
Contributor Author

overcat commented Sep 21, 2022

@lpascal-ledger Looks like the swap-ci-workflow.yml is outdated, I'll make a separate PR to fix this, please wait a moment.

@lpascal-ledger
Copy link
Contributor

@overcat I'm on it, don't worry. Bitcoin application cutoff some legacy last week so it's expected.
Also refreshing branch protection rules in the same time.

Nice CI though.

@overcat
Copy link
Contributor Author

overcat commented Sep 21, 2022

@lpascal-ledger I made a duplicate PR #45 since I didn't notice that you made a PR

tdejoigny-ledger pushed a commit that referenced this pull request Jul 10, 2024
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.

Sequence Number setting Use commas in amount formatting
3 participants